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

[Oliver Hartkopp <socketcan@hartkopp.net>]

Hi Vincent,

On 21.10.20 08:23, Vincent MAILHOL wrote:
>>>  From a first thought I would see a new flag CAN_CTRLMODE_RAW_DLC in the
>>> netlink interface of IFLA_CAN_CTRLMODE for the CAN controller driver.
>>>
>>> This could switch the sanitizing AND the CAN controller can properly
>>> expose its ability to support this mode.
>>
>> Absolutely yes. In my first message, I mentioned the idea of managing
>> that through socket option, glad that we now share the same idea.
> 
> Actually, I just realized that I replied to you too quickly. I was not
> exactly thinking of the same thing here so let me correct what I
> previously said.
> 
> IFLA_CAN_CTRLMODE is at the netlink level. My idea is to have it, in
> addition, at the socket level. Example: add CAN_RAW_RAW_DLC in
> include/uapi/linux/can/raw.h.

Yes. We need at least some different handling inside the driver level 
which could be switched with CAN_CTRLMODE_RAW_DLC.

> The reason is that if we only manage it at the netlink level, some
> application not aware of the RAW_DLC issue might run into some buffer
> overflow issue. Unless an application directly requests it, the current
> behaviour should be maintained (rationale: do not break userland).
> 
> So the full picture will be to have both the CAN_CTRLMODE_RAW_DLC at
> netlink level and CAN_RAW_RAW_DLC at the socket level (in the exact same
> way we have both CAN_CTRLMODE_FD and CAN_RAW_FD_FRAMES for
> CAN-FD).

Yes. That hits the point.

Btw. the impact on all protocols seems to be pretty heavy and to me it 
turned out that it would be a bad idea to use can_frame.can_dlc as 
transport vehicle for the raw dlc. Especially as this will contradict 
the rule that the can_dlc element is a plain length information today as 
canfd_frame.len and shares the same position in the struct.

I currently tend to only have a switch on driver level with 
CAN_CTRLMODE_RAW_DLC and make use of can_frame._res0 -> can_frame.raw_dlc

With CAN_CTRLMODE_RAW_DLC enabled the CAN driver would ...

- fill can_dlc and raw_dlc at reception time
- will use raw_dlc instead of can_dlc at tx time
- will use can_dlc if raw_dlc == 0 at tx time

This would have a minimal impact on the code and we only need to make 
sure that the raw_dlc is not killed at some stage in the network layer.

#define CAN_MAX_RAW_DLC 15

IMO the raw dlc use-case is a really special case for testing purposes. 
Touching the current can_frame.can_dlc handling could lead to more 
complexity and the fear to run into overflows as already stated by Joakim.

What do you think about the above approach?

Regards,
Oliver