Linux-Can Archive on lore.kernel.org
 help / color / Atom feed
From: Vincent MAILHOL <mailhol.vincent@wanadoo.fr>
To: Oliver Hartkopp <socketcan@hartkopp.net>,
	Marc Kleine-Budde <mkl@pengutronix.de>
Cc: linux-can@vger.kernel.org, kernel@pengutronix.de,
	"Stéphane Grosjean" <s.grosjean@peak-system.com>,
	"Vincent Mailhol" <mailhol.vincent@wanadoo.fr>
Subject: Re: [net-rfc 04/16] can: dev: can_get_len(): add a helper function to get the correct length of Classical frames
Date: Wed, 21 Oct 2020 20:55:20 +0900
Message-ID: <CAMZ6RqK2RCGLFmjiHTQAxZSGn-HQaAh_nHHGwKdODanZK5oJbg@mail.gmail.com> (raw)
In-Reply-To: <2711ea6f-e1ce-c3f9-dd98-83142bd33fc9@hartkopp.net>

> On 21.10.20 18:48, Oliver Hartkopp wrote:
>> 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
>
> To be more compatible to non raw dlc CAN frame sources the tx handling
> could also be like this:
>
> if ((can_dlc == CAN_MAX_DLEN) &&
>      (raw_dlc >= CAN_MAX_DLC && raw_dlc <= CAN_MAX_RAW_DLC))
> => use raw_dlc
>
>>
>> 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?

If I understand well, the idea would be not to use a setsockopt() but
instead rely on some logic on the can_dlc and raw_dlc to determine
which one to use.

Unfortunately, this approach has one issue in the TX path that could
break existing applications.

Consider below code (which I think is fairly realistic):

void send_frame (int soc, canid_t can_id, char *data, size_t len)
{
    struct can_frame cf;
    size_t dlc = len > sizeof(cf.data) ? sizeof(cf.data) : len;

    cf.can_id = can_id;
    cf.can_dlc = dlc;
    memcpy(cf.data, data, dlc);

    write(soc, &cf, sizeof(cf));
}

Here, the user did not initialize the can frame (cf) but assigned all
the relevant fields manually. Because cf is not initialized, the newly
introduced cf.dlc_raw field would have any of the values which was
present on the stack at the moment (rationale: the C standard does not
guarantee zero initialization). If 9 <= raw_dlc <= 15, the can frame
will be transmitted with a bad DLC value. If raw_dlc > 15, the can
frame will be discarded.

I think that it is mandatory to have the application declare that it
wants to use raw DLCs (this way, we know that the code is "DLC
aware"). I can not think of any transparent approach.

Next, we might think of using the netlink CAN_CTRLMODE_RAW_DLC and
the CAN_RAW_RAW_DLC socket option and the raw_dlc field. But I think
that this is overkill.

If not introducing new dlc_raw field, the drawback, as you pointed
out, will be that we would lose the backward compatibility of
canfd_frame with can_frame and that can_dlc field can not be used
directly as a length.

For userland, I think that this is acceptable because the very instant
the user calls setsockopt() with the CAN_RAW_RAW_DLC, he should be
aware of the consequences and should resign to use can_dlc field as a
plain length. That of course means that this should be clearly
highlighted when updating the documentation. And users not interested
by this feature can continue to use it as they did before.

Inside the kernel, all drivers advertising CAN_CTRLMODE_RAW_DLC will
also resign the right to use can_dlc as plain length. Drivers not
using it are safe with their existing code. Finally, the TX and RX
path would both need to be inspected in detail.

TLDR; socket options seem mandatory in all cases. We then have to
choose between breaking the can_dlc plain length property or
introducing a new raw_dlc field. My choice goes to the first option of
breaking the can_dlc plain length property.

That said, I am curious about what other people think.

  parent 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 [this message]
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
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=CAMZ6RqK2RCGLFmjiHTQAxZSGn-HQaAh_nHHGwKdODanZK5oJbg@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