| Message ID | 20200930144602.10290-3-mailhol.vincent@wanadoo.fr |
|---|---|
| State | Superseded |
| Headers | show |
| Series | None | expand |
On 9/30/20 4:45 PM, Vincent Mailhol wrote: > In classical CAN, the length of the data (i.e. CAN payload) is not > always equal to the DLC! If the frame is a Remote Transmission Request > (RTR), data length is always zero regardless of DLC value and else, if > the DLC is greater than 8, the length is 8. Contrary to common belief, > ISO 11898-1 Chapter 8.4.2.3 (DLC field) do allow DLCs greater than 8 > for Classical Frames and specifies that those DLCs shall indicate that > the data field is 8 bytes long. > > Above facts are widely unknown and so many developpers uses the "len" > field of "struct canfd_frame" to get the length of classical CAN > frames: this is incorrect! > > This patch introduces function get_can_len() which can be used in > remediation. The function takes the SKB as an input in order to be > able to determine if the frame is classical or FD. > > Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr> > --- > include/linux/can/dev.h | 23 +++++++++++++++++++++++ > 1 file changed, 23 insertions(+) > > diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h > index 5e3d45525bd3..72a8a60c0094 100644 > --- a/include/linux/can/dev.h > +++ b/include/linux/can/dev.h > @@ -177,6 +177,29 @@ u8 can_dlc2len(u8 can_dlc); > /* map the sanitized data length to an appropriate data length code */ > u8 can_len2dlc(u8 len); > > +/* > + * get_can_len(skb) - get the length of the CAN payload. > + * > + * In classical CAN, the length of the data (i.e. CAN payload) is not > + * always equal to the DLC! If the frame is a Remote Transmission > + * Request (RTR), data length is always zero regardless of DLC value > + * and else, if the DLC is greater than 8, the length is 8. Contrary > + * to common belief, ISO 11898-1 Chapter 8.4.2.3 (DLC field) do allow > + * DLCs greater than 8 for Classical Frames and specifies that those > + * DLCs shall indicate that the data field is 8 bytes long. > + */ > +static inline int get_can_len(struct sk_buff *skb) make this return an u8 make the skb const > +{ > + struct canfd_frame *cf = (struct canfd_frame *)skb->data; const > + > + if (can_is_canfd_skb(skb)) > + return min_t(__u8, cf->len, CANFD_MAX_DLEN); u8 > + else if (cf->can_id & CAN_RTR_FLAG) > + return 0; > + else > + return min_t(__u8, cf->len, CAN_MAX_DLEN); u8 > +} > + > struct net_device *alloc_candev_mqs(int sizeof_priv, unsigned int echo_skb_max, > unsigned int txqs, unsigned int rxqs); > #define alloc_candev(sizeof_priv, echo_skb_max) \ > Marc
> > +static inline int get_can_len(struct sk_buff *skb) > > make this return an u8 > make the skb const > > > +{ > > + struct canfd_frame *cf =3D (struct canfd_frame *)skb->data; > > const > > > + > > + if (can_is_canfd_skb(skb)) > > + return min_t(__u8, cf->len, CANFD_MAX_DLEN); > > u8 > > > + else if (cf->can_id & CAN_RTR_FLAG) > > + return 0; > > + else > > + return min_t(__u8, cf->len, CAN_MAX_DLEN); > > u8 Noted. All those changes will be addressed in v3 series. Thank you. As a side note, macros get_can_dlc() and get_canfd_dlc of the same file (include/linux/can/dev.h) also use __u8 instead of u8. Do you want me to add a patch to change these as below? /* * get_can_dlc(value) - helper macro to cast a given data length code (dlc) - * to __u8 and ensure the dlc value to be max. 8 bytes. + * to u8 and ensure the dlc value to be max. 8 bytes. * * To be used in the CAN netdriver receive path to ensure conformance with * ISO 11898-1 Chapter 8.4.2.3 (DLC field) */ -#define get_can_dlc(i) (min_t(__u8, (i), CAN_MAX_DLC)) -#define get_canfd_dlc(i) (min_t(__u8, (i), CANFD_MAX_DLC)) +#define get_can_dlc(i) (min_t(u8, (i), CAN_MAX_DLC)) +#define get_canfd_dlc(i) (min_t(u8, (i), CANFD_MAX_DLC)) /* Check for outgoing skbs that have not been created by the CAN subsystem */ static inline bool can_skb_headroom_valid(struct net_device *dev,
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h index 5e3d45525bd3..72a8a60c0094 100644 --- a/include/linux/can/dev.h +++ b/include/linux/can/dev.h @@ -177,6 +177,29 @@ u8 can_dlc2len(u8 can_dlc); /* map the sanitized data length to an appropriate data length code */ u8 can_len2dlc(u8 len); +/* + * get_can_len(skb) - get the length of the CAN payload. + * + * In classical CAN, the length of the data (i.e. CAN payload) is not + * always equal to the DLC! If the frame is a Remote Transmission + * Request (RTR), data length is always zero regardless of DLC value + * and else, if the DLC is greater than 8, the length is 8. Contrary + * to common belief, ISO 11898-1 Chapter 8.4.2.3 (DLC field) do allow + * DLCs greater than 8 for Classical Frames and specifies that those + * DLCs shall indicate that the data field is 8 bytes long. + */ +static inline int get_can_len(struct sk_buff *skb) +{ + struct canfd_frame *cf = (struct canfd_frame *)skb->data; + + if (can_is_canfd_skb(skb)) + return min_t(__u8, cf->len, CANFD_MAX_DLEN); + else if (cf->can_id & CAN_RTR_FLAG) + return 0; + else + return min_t(__u8, cf->len, CAN_MAX_DLEN); +} + struct net_device *alloc_candev_mqs(int sizeof_priv, unsigned int echo_skb_max, unsigned int txqs, unsigned int rxqs); #define alloc_candev(sizeof_priv, echo_skb_max) \
In classical CAN, the length of the data (i.e. CAN payload) is not always equal to the DLC! If the frame is a Remote Transmission Request (RTR), data length is always zero regardless of DLC value and else, if the DLC is greater than 8, the length is 8. Contrary to common belief, ISO 11898-1 Chapter 8.4.2.3 (DLC field) do allow DLCs greater than 8 for Classical Frames and specifies that those DLCs shall indicate that the data field is 8 bytes long. Above facts are widely unknown and so many developpers uses the "len" field of "struct canfd_frame" to get the length of classical CAN frames: this is incorrect! This patch introduces function get_can_len() which can be used in remediation. The function takes the SKB as an input in order to be able to determine if the frame is classical or FD. Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr> --- include/linux/can/dev.h | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)