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

[Vincent MAILHOL <mailhol.vincent@wanadoo.fr>]

On 24.10.20 01:47, Oliver Hartkopp wrote:
> On 23.10.20 12:36, Vincent MAILHOL wrote:
>> On 23.10.20 02:06, Oliver Hartkopp wrote:
>>> On 22.10.20 17:46, Vincent MAILHOL wrote:
>
>>> And what do you mean with "if the device has a DLC filter"?
>>
>> A DLC filter is a sub-component of a CAN frame filter which is a type
>> of Intrusion Prevention System (IPS). The CAN frame filter consists of
>> an allow list which entries are usually CAN IDs and DLCs. If the CAN
>> ID and DLC of a received frame do not match any of the entries in the
>> allow list, the frame is directly discarded. This is also sometimes
>> referred to as CAN firewall. Modern CAN gateways are starting to implement
>> such protection mechanism (in addition to the IDS).
>>
>> If an ECU has a filter rule to only allow DLC equal to 8, it would
>> discard a frame with a DLC greater than 8 even if the length is
>> actually 8.
>
> Yes, which is fine. If you want your CAN network work as expected, do
> not enable CAN_CTRLMODE_RAW_DLC.
>
> If you want to test the behaviour of other nodes in your network enable
> CAN_CTRLMODE_RAW_DLC.
>
>>> You told me the DLCs from 9..15 are correct from the ISO standpoint.
>>> That a good programmer checks the DLC and makes sure he only processes
>>> max. 8 bytes is a common thing and no 'filtering'.
>>>
>>> You might introduce CAN_RAW_RAW_DLC sockopt for CAN_RAW sockets but when
>>> you use packet sockets e.g. with Wireshark and forge some CAN frames
>>> there your only chance to have proper 0..8 DLCs is to disable
>>> CAN_CTRLMODE_RAW_DLC.
>>
>> Did not think of that use case but yes, I agree that
>> CAN_CTRLMODE_RAW_DLC is needed. I see CAN_RAW_RAW_DLC as an addition,
>> not a replacement.
>
> Yes. And CAN_RAW_RAW_DLC is also pretty easy to implement on the tx
> side. But as I already wrote: It still does not help with AF_PACKET sockets.
>
>>> Btw. do you really see any legacy SocketCAN applications (*together*
>>> with your testing application on the same Linux host) where you don't
>>> have the source code to check whether they properly initialize the
>>> reserved/padding bytes?
>
> Do you?
>
>>> You can also use the can-gw to let 'malicious' CAN apps run on a private
>>> virtual CAN. Forwarded modified CAN frames definitely initialize the
>>> reserved/padding bytes.
>>
>> Here, I am replying to you about the case of the 'legacy' applications
>> with uninitialized raw_dlc send CAN frames.
>>
>> Even if this is *my* intention, is it the intention of every other
>> user activating CAN_CTRLMODE_RAW_DLC?
>>
>> Consider either of:
>>
>>    * the newbie user who just wants a normal netlink configuration but
>>      copy/pasted the command from the internet without realising the
>>      raw_dlc option is here.
>
> You can enable the option only as root user. You can not protect every
> noob. If you fiddle with things you have no clue about, you can fail.
> That is a learning curve :-)
>
> We provide reasonable defaults.
>
>>    * the 'more helps more' guys that switch everything to 'on' which
>>      you mentioned before. This guys does not understand the RAW_DLC
>>      thing but yet activated it.
>>
>>    * the expert user who turned on the feature for tests but also runs
>>      legacy applications in parallel or after doing the tests.
>
> Again, as I asked before: What legacy applications for the (Open Source)
> SocketCAN could there be on an *experts* system where he could not look
> into to search uninitialized CAN frame structs?
>
> IMO this is an academical question without value.
>
>>
>> All of these users are subjected to the issue on the legacy
>> application I explained. It is not their intention to send DLC greater
>> than 8.
>
> Then they should not enable CAN_CTRLMODE_RAW_DLC.
>
>> Furthermore, the first two users do not necessarily know how to
>> program. They are using downloaded application and do not have the
>> knowledge to check for the issue nor to even understand it. (The third
>> user should understand. Maybe he or she is not the best example, wish
>> I had started my argumentation with the first two user cases).
>>
>> I see two options:
>>
>>    1. The user used an expert command, it is his responsibility: we do
>>       not care, it is his fault.
>>
>>    2. We (as kernel hackers) bare responsibility for all usage scenario
>>       of the "ip addr set canX..." options and do not allow a scenario
>>       which can break an existing application.
>>
>> My vote is 2. I draw the line at the code level: user (as a
>> programmer) is responsible for the code he or she writes but we (as
>> kernel hackers) try to prevent any system configuration from breaking
>> existing applications which are working fine.
>
> By default CAN_CTRLMODE_RAW_DLC is disabled.
>
> Even with CAN_CTRLMODE_RAW_DLC enabled all existing applications would
> still work fine.
>
> They will still be able to send and receive CAN frames having proper
> length information in can_dlc - so nothing breaks.
>
> The only thing that could happen is, that their sent CAN frames with 8
> bytes of payload may have a DLC from 8..15 which is still covered by the
> ISO standard. This is no fault.
>
> You can not take responsibility for broken implementations on other ECUs.

We clearly share a difference stance on this subject, I think it is
time to close the debate. I agree to go with your solution.

One more time, thank you for your time and patience :-)

>>>> You won :-)
>>>> Sorry for the long exchange and thank you for your patience.
>>>
>>> I really don't want to 'win'. But by the time the features and
>>> functionalities have been grown and many people rely on its
>>> functionality and performance.
>>>
>>> The discussion helps to find the hopefully best solution and brings all
>>> of us to new insights.
>>>
>>> The difference is to make a new door into a house or to replace its
>>> entire water system. You need a VERY good reason to replace the water
>>> system ... when you want a new door.
>>
>> OK then let me try to re-explain another point.
>>
>> I understand that you do not want to drop malformed frames so I stop
>> replying on that because I feel that I would only annoy you more. But
>> in reality I do not yet understand why you do not want to.
>>
>> Below are all the valid pairs of Lengths and DLCs. Every pair outside
>> of the table is incorrect.
>>
>>      +-----------+-----------+
>>      | Length    | DLC       |
>>      | (can_dlc) | (raw_dlc) |
>>      +-----------+-----------+
>>      | 0         | 0         |
>>      | 1         | 1         |
>>      | 2         | 2         |
>>      | 3         | 3         |
>>      | 4         | 4         |
>>      | 5         | 5         |
>>      | 6         | 6         |
>>      | 7         | 7         |
>>      | 8         | 8..15     |
>>      +-----------+-----------+
>>
>> If the user sets, let say, can_dlc to 8 and raw_dlc to 2, he expects
>> to send a frame of length 8 and *DLC 2*.
>
> This is BS! How can you create such an impossible case after all of our
> discussions?
>
> I write it AGAIN, ONLY FOR YOU:
>
> if (can_dlc == 8) && (raw_dlc > 8 && raw_dlc <= 15)
>      dlc = raw_dlc;
> else
>      dlc = can_dlc;
>
> can_dlc == 8 is the entry door to write dlc values from 9..15 into the
> controller register.
> Which always leads to a valid CAN frame with 8 bytes.
>
> So what is so hard to write this into the documentation then?
>
> There is no expectation that anything else then can_dlc is used when
> raw_dlc is set to 2.
>
> And therefore there will never be an invalid frame.

I saw your patch. Changing the name from raw_can to len8_dlc clears
the concerns I had for invalid frames. Thank you.

Thank you also for depreciating the can_dlc field and introducing the
len field in struct can_frame. It is a smart change.

I will now test the patch.


Yours sincerely,
Vincent Mailhol