Truncate DLC to 8 instead of dropping?

Truncate DLC to 8 instead of dropping?

[Wolfram Sang]

[-- 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 --]

Re: Truncate DLC to 8 instead of dropping?

[Kurt Van Dijck]

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

Re: Truncate DLC to 8 instead of dropping?

[Oliver Hartkopp]

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

Re: Truncate DLC to 8 instead of dropping?

[Wolfram Sang]

[-- 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 --]

Re: Truncate DLC to 8 instead of dropping?

[Kurt Van Dijck]

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

Re: Truncate DLC to 8 instead of dropping?

[Wolfram Sang]

[-- 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 --]

Re: Truncate DLC to 8 instead of dropping?

[Oliver Hartkopp]

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