Linux-Can Archive on lore.kernel.org
 help / color / Atom feed
From: Oliver Hartkopp <socketcan@hartkopp.net>
To: Vincent MAILHOL <mailhol.vincent@wanadoo.fr>,
	Marc Kleine-Budde <mkl@pengutronix.de>,
	linux-can@vger.kernel.org
Cc: 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: Wed, 21 Oct 2020 11:21:25 +0200
Message-ID: <2711ea6f-e1ce-c3f9-dd98-83142bd33fc9@hartkopp.net> (raw)
In-Reply-To: <CAMZ6RqKFST4dcWZP_8NdDMB6GT09vhVWgN+nuMWkVovkh-EZdw@mail.gmail.com>

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

  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 [this message]
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
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=2711ea6f-e1ce-c3f9-dd98-83142bd33fc9@hartkopp.net \
    --to=socketcan@hartkopp.net \
    --cc=kernel@pengutronix.de \
    --cc=linux-can@vger.kernel.org \
    --cc=mailhol.vincent@wanadoo.fr \
    --cc=mkl@pengutronix.de \
    --cc=s.grosjean@peak-system.com \
    /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