Re: [net-rfc 04/16] can: dev: can_get_len(): add a helper function to get the correct length of Classical frames

[Marc Kleine-Budde <mkl@pengutronix.de>]

[-- Attachment #1.1: Type: text/plain, Size: 3533 bytes --]

On 10/19/20 10:35 PM, Oliver Hartkopp wrote:
> 
> 
> On 19.10.20 21:05, Marc Kleine-Budde wrote:
>> From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
>>
>> 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>
>> Link: https://lore.kernel.org/r/20201002154219.4887-4-mailhol.vincent@wanadoo.fr
>> [mkl: renamed get_can_len() -> can_get_len()]
>> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
>> ---
>>   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 41ff31795320..2bb132fc6d88 100644
>> --- a/include/linux/can/dev.h
>> +++ b/include/linux/can/dev.h
>> @@ -192,6 +192,29 @@ u8 can_dlc2len(u8 can_dlc);
>>   /* map the sanitized data length to an appropriate data length code */
>>   u8 can_len2dlc(u8 len);
>>   
>> +/*
>> + * can_get_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 u8 can_get_len(const struct sk_buff *skb)
>> +{
>> +	const struct canfd_frame *cf = (const 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);
>> +}
> 
> The main idea behind this patch and patch 05/16 is to provide a correct 
> statistic in the tx bytes, right?
> 
> A simple test for the CAN_RTR_FLAG will do the job as all the length 
> sanitizing is already done in the tx path by can_dropped_invalid_skb() 
> in all known drivers right *before* the skb is stored in the echo skb array.
> 
> IMO there's no need for a separate helper function. Maybe a macro which 
> should have something with 'payload' in its name - to determine the tx 
> byte statistics based on CAN_RTR_FLAG ...

Good point!

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]