Linux-Can Archive on lore.kernel.org
 help / color / Atom feed
From: Vincent MAILHOL <mailhol.vincent@wanadoo.fr>
To: Oliver Hartkopp <socketcan@hartkopp.net>
Cc: "Marc Kleine-Budde" <mkl@pengutronix.de>,
	linux-can@vger.kernel.org, kernel@pengutronix.de,
	"Stéphane Grosjean" <s.grosjean@peak-system.com>
Subject: Re: [net-rfc 04/16] can: dev: can_get_len(): add a helper function to get the correct length of Classical frames
Date: Sat, 24 Oct 2020 14:25:44 +0900
Message-ID: <CAMZ6Rq+tx93DuRYsLOpcxjRDOJxUTh-czntEYSv7tDMOwoaLQw@mail.gmail.com> (raw)
In-Reply-To: <e0678150-188a-7e6e-6e52-9b74e0c6068c@hartkopp.net>

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

  reply index

Thread overview: 45+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-19 19:05 [RFC]: can 2020-10-19 Marc Kleine-Budde
2020-10-19 19:05 ` [net-rfc 01/16] can: proc: can_remove_proc(): silence remove_proc_entry warning Marc Kleine-Budde
2020-10-19 19:05 ` [net-rfc 02/16] can: rx-offload: don't call kfree_skb() from IRQ context Marc Kleine-Budde
2020-10-19 19:05 ` [net-rfc 03/16] can: dev: can_get_echo_skb(): prevent call to kfree_skb() in hard " Marc Kleine-Budde
2020-10-19 19:05 ` [net-rfc 04/16] can: dev: can_get_len(): add a helper function to get the correct length of Classical frames Marc Kleine-Budde
2020-10-19 20:35   ` Oliver Hartkopp
2020-10-20  6:35     ` Marc Kleine-Budde
2020-10-20 11:30       ` Vincent Mailhol
2020-10-20 11:48         ` Marc Kleine-Budde
2020-10-20 12:38         ` Oliver Hartkopp
2020-10-20 15:02           ` Marc Kleine-Budde
2020-10-20 16:07           ` Vincent Mailhol
2020-10-20 17:04             ` Oliver Hartkopp
2020-10-20 18:50               ` Marc Kleine-Budde
2020-10-21  0:52               ` Vincent Mailhol
2020-10-21  6:23                 ` Vincent MAILHOL
2020-10-21  7:11                   ` Joakim Zhang
2020-10-21  7:21                     ` Marc Kleine-Budde
2020-10-21  7:48                       ` Joakim Zhang
2020-10-21  9:21                   ` Oliver Hartkopp
2020-10-21  9:48                     ` Oliver Hartkopp
2020-10-21 11:55                     ` Vincent MAILHOL
2020-10-21 17:52                       ` Oliver Hartkopp
2020-10-22  3:30                         ` Vincent MAILHOL
2020-10-22  7:15                           ` Oliver Hartkopp
2020-10-22 12:23                             ` Vincent MAILHOL
2020-10-22 13:28                               ` Oliver Hartkopp
2020-10-22 15:46                                 ` Vincent MAILHOL
2020-10-22 17:06                                   ` Oliver Hartkopp
2020-10-23 10:36                                     ` Vincent MAILHOL
2020-10-23 16:47                                       ` Oliver Hartkopp
2020-10-24  5:25                                         ` Vincent MAILHOL [this message]
2020-10-24 11:31                                           ` Oliver Hartkopp
2020-10-19 19:05 ` [net-rfc 05/16] can: dev: __can_get_echo_skb(): fix the returned length of CAN frame Marc Kleine-Budde
2020-10-19 19:05 ` [net-rfc 06/16] can: can_create_echo_skb(): fix echo skb generation: always use skb_clone() Marc Kleine-Budde
2020-10-19 19:05 ` [net-rfc 07/16] can: j1939: j1939_sk_bind(): return failure if netdev is down Marc Kleine-Budde
2020-10-19 19:05 ` [net-rfc 08/16] can: isotp: Explain PDU in CAN_ISOTP help text Marc Kleine-Budde
2020-10-19 19:05 ` [net-rfc 09/16] can: isotp: enable RX timeout handling in listen-only mode Marc Kleine-Budde
2020-10-19 19:05 ` [net-rfc 10/16] can: ti_hecc: add missed clk_disable_unprepare() in error path Marc Kleine-Budde
2020-10-19 19:05 ` [net-rfc 11/16] can: xilinx_can: handle failure cases of pm_runtime_get_sync Marc Kleine-Budde
2020-10-19 19:05 ` [net-rfc 12/16] can: peak_usb: fix timestamp wrapping Marc Kleine-Budde
2020-10-19 19:05 ` [net-rfc 13/16] can: peak_canfd: fix echo management when loopback is on Marc Kleine-Budde
2020-10-19 19:05 ` [net-rfc 14/16] can: mcp251xfd: mcp251xfd_regmap_crc_read(): increase severity of CRC read error messages Marc Kleine-Budde
2020-10-19 19:05 ` [net-rfc 15/16] can: mcp251xfd: mcp251xfd_regmap_nocrc_read(): fix semicolon.cocci warnings Marc Kleine-Budde
2020-10-19 19:05 ` [net-rfc 16/16] can: mcp251xfd: remove unneeded break Marc Kleine-Budde

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAMZ6Rq+tx93DuRYsLOpcxjRDOJxUTh-czntEYSv7tDMOwoaLQw@mail.gmail.com \
    --to=mailhol.vincent@wanadoo.fr \
    --cc=kernel@pengutronix.de \
    --cc=linux-can@vger.kernel.org \
    --cc=mkl@pengutronix.de \
    --cc=s.grosjean@peak-system.com \
    --cc=socketcan@hartkopp.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Can Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-can/0 linux-can/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-can linux-can/ https://lore.kernel.org/linux-can \
		linux-can@vger.kernel.org
	public-inbox-index linux-can

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-can


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git