* Truncate DLC to 8 instead of dropping?
@ 2012-08-31 11:59 Wolfram Sang
2012-08-31 12:50 ` Kurt Van Dijck
0 siblings, 1 reply; 7+ messages in thread
From: Wolfram Sang @ 2012-08-31 11:59 UTC (permalink / raw)
To: linux-can
[-- Attachment #1: Type: text/plain, Size: 757 bytes --]
Hi,
I just noticed today that commit c7cd606f60e7679c7f9eee7010f02a6f000209c1
("can: Fix data length code handling in rx path") says regarding
overlong packets:
===
The ISO 11898-1 Chapter 8.4.2.3 (DLC field) says that register values > 8
should be reduced to 8 without any error reporting or frame drop.
===
and this is why get_can_dlc() came into existance.
However, functions like can_dropped_invalid_skb() from dev.h or can_rcv() from
af_can.c do check for a correct length, but drop the packet in the error case.
Don't those need to be fixed?
Regards,
Wolfram
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: Truncate DLC to 8 instead of dropping? 2012-08-31 11:59 Truncate DLC to 8 instead of dropping? Wolfram Sang @ 2012-08-31 12:50 ` Kurt Van Dijck 2012-08-31 16:35 ` Oliver Hartkopp 0 siblings, 1 reply; 7+ messages in thread From: Kurt Van Dijck @ 2012-08-31 12:50 UTC (permalink / raw) To: Wolfram Sang; +Cc: linux-can On Fri, Aug 31, 2012 at 01:59:04PM +0200, Wolfram Sang wrote: > Hi, > > I just noticed today that commit c7cd606f60e7679c7f9eee7010f02a6f000209c1 > ("can: Fix data length code handling in rx path") says regarding > overlong packets: > > === > > The ISO 11898-1 Chapter 8.4.2.3 (DLC field) says that register values > 8 > should be reduced to 8 without any error reporting or frame drop. > > === > > and this is why get_can_dlc() came into existance. > > However, functions like can_dropped_invalid_skb() from dev.h or can_rcv() from > af_can.c do check for a correct length, but drop the packet in the error case. > Don't those need to be fixed? > > Regards, > > Wolfram Interesting point. My first idea is to acknowledge your thinking, BUT: * ISO 11898-1 is about the wire format, whereas your issues address higher level interfaces. Higher level interfaces may IMHO use a stricter format that the wire format. * I find it very likely that DLC is normalized in user space. This could prevent a lot of confusion in userspace apps. We could now argue on the best place to do normalization, but device drivers seem the best place. * With CAN-FD in mind, the handling of DLC is stricter anyway. DLC > 8 is allowed, but is a real coding (probably), so we decided to communicate a 'len' field to userspace. Does this address your concerns? Kind regards, Kurt ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Truncate DLC to 8 instead of dropping? 2012-08-31 12:50 ` Kurt Van Dijck @ 2012-08-31 16:35 ` Oliver Hartkopp 2012-09-01 17:33 ` Wolfram Sang 0 siblings, 1 reply; 7+ messages in thread From: Oliver Hartkopp @ 2012-08-31 16:35 UTC (permalink / raw) To: Wolfram Sang, Kurt Van Dijck; +Cc: linux-can Hello Kurt and Wolfram, On 31.08.2012 14:50, Kurt Van Dijck wrote: > On Fri, Aug 31, 2012 at 01:59:04PM +0200, Wolfram Sang wrote: >> Hi, >> >> I just noticed today that commit c7cd606f60e7679c7f9eee7010f02a6f000209c1 >> ("can: Fix data length code handling in rx path") says regarding >> overlong packets: >> >> === >> >> The ISO 11898-1 Chapter 8.4.2.3 (DLC field) says that register values > 8 >> should be reduced to 8 without any error reporting or frame drop. >> >> === >> >> and this is why get_can_dlc() came into existance. >> >> However, functions like can_dropped_invalid_skb() from dev.h or can_rcv() from >> af_can.c do check for a correct length, but drop the packet in the error case. >> Don't those need to be fixed? >> > > Interesting point. My first idea is to acknowledge your thinking, BUT: > * ISO 11898-1 is about the wire format, whereas your issues address > higher level interfaces. Higher level interfaces may IMHO use a stricter > format that the wire format. Pointing to the wire format is a good idea. To me this "dlc error ignoring" requirement is implemented by the CAN controller. And when the dlc value in the controller register is not sanitized we should do it on driver level. > * I find it very likely that DLC is normalized in user space. > This could prevent a lot of confusion in userspace apps. > We could now argue on the best place to do normalization, but device drivers > seem the best place. ack. > * With CAN-FD in mind, the handling of DLC is stricter anyway. DLC > 8 is allowed, > but is a real coding (probably), so we decided to communicate a 'len' field > to userspace. Yes. This cleans up the detail that the current can_frame.can_dlc has values from 0..8 which is a 1:1 mapping to the data payload length. Usually people use this can_dlc element directly as a 'length' information, e.g. in for() statements. for (i=0; i < cframe.can_dlc; i++) printf("%02X ", cframe.data[i]); For CAN FD we defined a canfd_frame.len to point out the length information in a more abstract way. So that people can stay on the current 'length' usage schema e.g. in for() statements. for (i=0; i < cfdframe.len; i++) printf("%02X ", cfdframe.data[i]); The real DLC value is hidden from the userspace API - and all the length and DLC checking conversion stuff is done in one place: On driver level. > > Does this address your concerns? Does it? :-) Regards, Oliver ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Truncate DLC to 8 instead of dropping? 2012-08-31 16:35 ` Oliver Hartkopp @ 2012-09-01 17:33 ` Wolfram Sang 2012-09-01 18:11 ` Kurt Van Dijck 0 siblings, 1 reply; 7+ messages in thread From: Wolfram Sang @ 2012-09-01 17:33 UTC (permalink / raw) To: Oliver Hartkopp; +Cc: Kurt Van Dijck, linux-can [-- Attachment #1: Type: text/plain, Size: 1862 bytes --] On Fri, Aug 31, 2012 at 06:35:14PM +0200, Oliver Hartkopp wrote: > Hello Kurt and Wolfram, > > On 31.08.2012 14:50, Kurt Van Dijck wrote: > > > On Fri, Aug 31, 2012 at 01:59:04PM +0200, Wolfram Sang wrote: > >> Hi, > >> > >> I just noticed today that commit c7cd606f60e7679c7f9eee7010f02a6f000209c1 > >> ("can: Fix data length code handling in rx path") says regarding > >> overlong packets: > >> > >> === > >> > >> The ISO 11898-1 Chapter 8.4.2.3 (DLC field) says that register values > 8 > >> should be reduced to 8 without any error reporting or frame drop. > >> > >> === > >> > >> and this is why get_can_dlc() came into existance. > >> > >> However, functions like can_dropped_invalid_skb() from dev.h or can_rcv() from > >> af_can.c do check for a correct length, but drop the packet in the error case. > >> Don't those need to be fixed? > >> > > > > Interesting point. My first idea is to acknowledge your thinking, BUT: > > * ISO 11898-1 is about the wire format, whereas your issues address > > higher level interfaces. Higher level interfaces may IMHO use a stricter > > format that the wire format. > > > Pointing to the wire format is a good idea. > To me this "dlc error ignoring" requirement is implemented by the CAN > controller. And when the dlc value in the controller register is not sanitized > we should do it on driver level. If I understand both of you correctly, that means the checks in the functions I mentioned should be removed then? Fine with me if this is consistent, I just wondered if it wasn't nice to have a check in an upper layer in case the driver did not correctly implement the check. Thanks, Wolfram -- Pengutronix e.K. | Wolfram Sang | Industrial Linux Solutions | http://www.pengutronix.de/ | [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 198 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Truncate DLC to 8 instead of dropping? 2012-09-01 17:33 ` Wolfram Sang @ 2012-09-01 18:11 ` Kurt Van Dijck 2012-09-01 18:17 ` Wolfram Sang 0 siblings, 1 reply; 7+ messages in thread From: Kurt Van Dijck @ 2012-09-01 18:11 UTC (permalink / raw) To: Wolfram Sang; +Cc: Oliver Hartkopp, linux-can On Sat, Sep 01, 2012 at 07:33:13PM +0200, Wolfram Sang wrote: > On Fri, Aug 31, 2012 at 06:35:14PM +0200, Oliver Hartkopp wrote: > > Hello Kurt and Wolfram, > > > > On 31.08.2012 14:50, Kurt Van Dijck wrote: > > > > > On Fri, Aug 31, 2012 at 01:59:04PM +0200, Wolfram Sang wrote: > > >> Hi, > > >> > > >> I just noticed today that commit c7cd606f60e7679c7f9eee7010f02a6f000209c1 > > >> ("can: Fix data length code handling in rx path") says regarding > > >> overlong packets: > > >> > > >> === > > >> > > >> The ISO 11898-1 Chapter 8.4.2.3 (DLC field) says that register values > 8 > > >> should be reduced to 8 without any error reporting or frame drop. > > >> > > >> === > > >> > > >> and this is why get_can_dlc() came into existance. > > >> > > >> However, functions like can_dropped_invalid_skb() from dev.h or can_rcv() from > > >> af_can.c do check for a correct length, but drop the packet in the error case. > > >> Don't those need to be fixed? > > >> > > > > > > Interesting point. My first idea is to acknowledge your thinking, BUT: > > > * ISO 11898-1 is about the wire format, whereas your issues address > > > higher level interfaces. Higher level interfaces may IMHO use a stricter > > > format that the wire format. > > > > > > Pointing to the wire format is a good idea. > > To me this "dlc error ignoring" requirement is implemented by the CAN > > controller. And when the dlc value in the controller register is not sanitized > > we should do it on driver level. > > If I understand both of you correctly, that means the checks in the > functions I mentioned should be removed then? Not completely IMHO. The check in the recv path should not be necessary. The check in the send path is necessary to make sure that userspace does not put garbage into the driver. Kind regards, Kurt ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Truncate DLC to 8 instead of dropping? 2012-09-01 18:11 ` Kurt Van Dijck @ 2012-09-01 18:17 ` Wolfram Sang 2012-09-01 19:27 ` Oliver Hartkopp 0 siblings, 1 reply; 7+ messages in thread From: Wolfram Sang @ 2012-09-01 18:17 UTC (permalink / raw) To: Oliver Hartkopp, linux-can; +Cc: Kurt Van Dijck [-- Attachment #1: Type: text/plain, Size: 2069 bytes --] On Sat, Sep 01, 2012 at 08:11:06PM +0200, Kurt Van Dijck wrote: > On Sat, Sep 01, 2012 at 07:33:13PM +0200, Wolfram Sang wrote: > > On Fri, Aug 31, 2012 at 06:35:14PM +0200, Oliver Hartkopp wrote: > > > Hello Kurt and Wolfram, > > > > > > On 31.08.2012 14:50, Kurt Van Dijck wrote: > > > > > > > On Fri, Aug 31, 2012 at 01:59:04PM +0200, Wolfram Sang wrote: > > > >> Hi, > > > >> > > > >> I just noticed today that commit c7cd606f60e7679c7f9eee7010f02a6f000209c1 > > > >> ("can: Fix data length code handling in rx path") says regarding > > > >> overlong packets: > > > >> > > > >> === > > > >> > > > >> The ISO 11898-1 Chapter 8.4.2.3 (DLC field) says that register values > 8 > > > >> should be reduced to 8 without any error reporting or frame drop. > > > >> > > > >> === > > > >> > > > >> and this is why get_can_dlc() came into existance. > > > >> > > > >> However, functions like can_dropped_invalid_skb() from dev.h or can_rcv() from > > > >> af_can.c do check for a correct length, but drop the packet in the error case. > > > >> Don't those need to be fixed? > > > >> > > > > > > > > Interesting point. My first idea is to acknowledge your thinking, BUT: > > > > * ISO 11898-1 is about the wire format, whereas your issues address > > > > higher level interfaces. Higher level interfaces may IMHO use a stricter > > > > format that the wire format. > > > > > > > > > Pointing to the wire format is a good idea. > > > To me this "dlc error ignoring" requirement is implemented by the CAN > > > controller. And when the dlc value in the controller register is not sanitized > > > we should do it on driver level. > > > > If I understand both of you correctly, that means the checks in the > > functions I mentioned should be removed then? > > Not completely IMHO. The check in the recv path should not be necessary. OK. I only meant the rx path. -- Pengutronix e.K. | Wolfram Sang | Industrial Linux Solutions | http://www.pengutronix.de/ | [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 198 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Truncate DLC to 8 instead of dropping? 2012-09-01 18:17 ` Wolfram Sang @ 2012-09-01 19:27 ` Oliver Hartkopp 0 siblings, 0 replies; 7+ messages in thread From: Oliver Hartkopp @ 2012-09-01 19:27 UTC (permalink / raw) To: Wolfram Sang; +Cc: linux-can, Kurt Van Dijck On 01.09.2012 20:17, Wolfram Sang wrote: > On Sat, Sep 01, 2012 at 08:11:06PM +0200, Kurt Van Dijck wrote: >> On Sat, Sep 01, 2012 at 07:33:13PM +0200, Wolfram Sang wrote: >>> On Fri, Aug 31, 2012 at 06:35:14PM +0200, Oliver Hartkopp wrote: >>>> Hello Kurt and Wolfram, >>>> >>>> On 31.08.2012 14:50, Kurt Van Dijck wrote: >>>> >>>>> On Fri, Aug 31, 2012 at 01:59:04PM +0200, Wolfram Sang wrote: >>>>>> Hi, >>>>>> >>>>>> I just noticed today that commit c7cd606f60e7679c7f9eee7010f02a6f000209c1 >>>>>> ("can: Fix data length code handling in rx path") says regarding >>>>>> overlong packets: >>>>>> >>>>>> === >>>>>> >>>>>> The ISO 11898-1 Chapter 8.4.2.3 (DLC field) says that register values > 8 >>>>>> should be reduced to 8 without any error reporting or frame drop. >>>>>> >>>>>> === >>>>>> >>>>>> and this is why get_can_dlc() came into existance. >>>>>> >>>>>> However, functions like can_dropped_invalid_skb() from dev.h or can_rcv() from >>>>>> af_can.c do check for a correct length, but drop the packet in the error case. >>>>>> Don't those need to be fixed? >>>>>> >>>>> >>>>> Interesting point. My first idea is to acknowledge your thinking, BUT: >>>>> * ISO 11898-1 is about the wire format, whereas your issues address >>>>> higher level interfaces. Higher level interfaces may IMHO use a stricter >>>>> format that the wire format. >>>> >>>> >>>> Pointing to the wire format is a good idea. >>>> To me this "dlc error ignoring" requirement is implemented by the CAN >>>> controller. And when the dlc value in the controller register is not sanitized >>>> we should do it on driver level. >>> >>> If I understand both of you correctly, that means the checks in the >>> functions I mentioned should be removed then? >> >> Not completely IMHO. The check in the recv path should not be necessary. > > OK. I only meant the rx path. > The check on driver level should not be removed. Both the driver and the network layer should do it's own checks. Think about people using PF_PACKET sockets to access CAN netdevices. When you are able to put "userspace garbage" directly into CAN netdevices via PF_PACKET you need checks for rx/tx sanity in the driver. Regards, Oliver ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2012-09-01 19:27 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-08-31 11:59 Truncate DLC to 8 instead of dropping? Wolfram Sang 2012-08-31 12:50 ` Kurt Van Dijck 2012-08-31 16:35 ` Oliver Hartkopp 2012-09-01 17:33 ` Wolfram Sang 2012-09-01 18:11 ` Kurt Van Dijck 2012-09-01 18:17 ` Wolfram Sang 2012-09-01 19:27 ` Oliver Hartkopp
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.