diff mbox series

[net-next,09/17] can: length: can_fd_len2dlc(): simplify length calculcation

Message ID 20210113211410.917108-10-mkl@pengutronix.de (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [net-next,01/17] MAINTAINERS: adjust entry to tcan4x5x file split | expand

Checks

Context Check Description
netdev/cover_letter success Pull request
netdev/fixes_present success Link
netdev/patch_count warning Series longer than 15 patches
netdev/tree_selection success Clearly marked for net-next
netdev/subject_prefix success Link
netdev/cc_maintainers warning 1 maintainers not CCed: wg@grandegger.com
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch warning WARNING: 'paramter' may be misspelled - perhaps 'parameter'?
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/header_inline success Link
netdev/stable success Stable not CCed

Commit Message

Marc Kleine-Budde Jan. 13, 2021, 9:14 p.m. UTC 
If the length paramter in len2dlc() exceeds the size of the len2dlc array, we
return 0xF. This is equal to the last 16 members of the array.

This patch removes these members from the array, uses ARRAY_SIZE() for the
length check, and returns CANFD_MAX_DLC (which is 0xf).

Reviewed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
Link: https://lore.kernel.org/r/20210111141930.693847-9-mkl@pengutronix.de
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/dev/length.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Comments

Marc Kleine-Budde Jan. 14, 2021, 7:34 a.m. UTC | #1 
On 1/14/21 2:59 AM, Vincent MAILHOL wrote:
> On Tue. 14 Jan 2021 at 06:14, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>>
>> If the length paramter in len2dlc() exceeds the size of the len2dlc array, we
>> return 0xF. This is equal to the last 16 members of the array.
>>
>> This patch removes these members from the array, uses ARRAY_SIZE() for the
>> length check, and returns CANFD_MAX_DLC (which is 0xf).
>>
>> Reviewed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
>> Link: https://lore.kernel.org/r/20210111141930.693847-9-mkl@pengutronix.de
>> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
>> ---
>>  drivers/net/can/dev/length.c | 6 ++----
>>  1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/can/dev/length.c b/drivers/net/can/dev/length.c
>> index 5e7d481717ea..d695a3bee1ed 100644
>> --- a/drivers/net/can/dev/length.c
>> +++ b/drivers/net/can/dev/length.c
>> @@ -27,15 +27,13 @@ static const u8 len2dlc[] = {
>>         13, 13, 13, 13, 13, 13, 13, 13, /* 25 - 32 */
>>         14, 14, 14, 14, 14, 14, 14, 14, /* 33 - 40 */
>>         14, 14, 14, 14, 14, 14, 14, 14, /* 41 - 48 */
>> -       15, 15, 15, 15, 15, 15, 15, 15, /* 49 - 56 */
>> -       15, 15, 15, 15, 15, 15, 15, 15  /* 57 - 64 */
>>  };
>>
>>  /* map the sanitized data length to an appropriate data length code */
>>  u8 can_fd_len2dlc(u8 len)
>>  {
>> -       if (unlikely(len > 64))
>> -               return 0xF;
>> +       if (len > ARRAY_SIZE(len2dlc))
> 
> Sorry but I missed an of-by-one issue when I did my first
> review. Don't know why but it popped to my eyes this morning when
> casually reading the emails.
> 
> ARRAY_SIZE(len2dlc) is 49. If len is between 0 and 48, use the
> array, if len is greater *or equal* return CANFD_MAX_DLC.

Doh!

Looking for his brown paper bag,
Marc
Oliver Hartkopp Jan. 14, 2021, 8:23 a.m. UTC | #2 
On 14.01.21 02:59, Vincent MAILHOL wrote:
> On Tue. 14 Jan 2021 at 06:14, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>>
>> If the length paramter in len2dlc() exceeds the size of the len2dlc array, we
>> return 0xF. This is equal to the last 16 members of the array.
>>
>> This patch removes these members from the array, uses ARRAY_SIZE() for the
>> length check, and returns CANFD_MAX_DLC (which is 0xf).
>>
>> Reviewed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
>> Link: https://lore.kernel.org/r/20210111141930.693847-9-mkl@pengutronix.de
>> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
>> ---
>>   drivers/net/can/dev/length.c | 6 ++----
>>   1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/can/dev/length.c b/drivers/net/can/dev/length.c
>> index 5e7d481717ea..d695a3bee1ed 100644
>> --- a/drivers/net/can/dev/length.c
>> +++ b/drivers/net/can/dev/length.c
>> @@ -27,15 +27,13 @@ static const u8 len2dlc[] = {
>>          13, 13, 13, 13, 13, 13, 13, 13, /* 25 - 32 */
>>          14, 14, 14, 14, 14, 14, 14, 14, /* 33 - 40 */
>>          14, 14, 14, 14, 14, 14, 14, 14, /* 41 - 48 */
>> -       15, 15, 15, 15, 15, 15, 15, 15, /* 49 - 56 */
>> -       15, 15, 15, 15, 15, 15, 15, 15  /* 57 - 64 */
>>   };
>>
>>   /* map the sanitized data length to an appropriate data length code */
>>   u8 can_fd_len2dlc(u8 len)
>>   {
>> -       if (unlikely(len > 64))
>> -               return 0xF;
>> +       if (len > ARRAY_SIZE(len2dlc))
> 
> Sorry but I missed an of-by-one issue when I did my first
> review. Don't know why but it popped to my eyes this morning when
> casually reading the emails.

Oh, yes.

The fist line is 0 .. 8 which has 9 bytes.

I also looked on it (from the back), and wondered if it was correct. But 
didn't see it either at first sight.

> 
> ARRAY_SIZE(len2dlc) is 49. If len is between 0 and 48, use the
> array, if len is greater *or equal* return CANFD_MAX_DLC.

All these changes and discussions make it very obviously more tricky to 
understand that code.

I don't really like this kind of improvement ...

Before that it was pretty clear that we only catch an out of bounds 
value and usually grab the value from the table.

Regards,
Oliver

> 
> In short, replace > by >=:
> +       if (len >= ARRAY_SIZE(len2dlc))
> 
>> +               return CANFD_MAX_DLC;
>>
>>          return len2dlc[len];
>>   }
> 
> Yours sincerely,
> Vincent
>
Vincent Mailhol Jan. 14, 2021, 9:16 a.m. UTC | #3 
On Tue. 14 Jan 2021 at 17:23, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> On 14.01.21 02:59, Vincent MAILHOL wrote:
> > On Tue. 14 Jan 2021 at 06:14, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> >>
> >> If the length paramter in len2dlc() exceeds the size of the len2dlc array, we
> >> return 0xF. This is equal to the last 16 members of the array.
> >>
> >> This patch removes these members from the array, uses ARRAY_SIZE() for the
> >> length check, and returns CANFD_MAX_DLC (which is 0xf).
> >>
> >> Reviewed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> >> Link: https://lore.kernel.org/r/20210111141930.693847-9-mkl@pengutronix.de
> >> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> >> ---
> >>   drivers/net/can/dev/length.c | 6 ++----
> >>   1 file changed, 2 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/net/can/dev/length.c b/drivers/net/can/dev/length.c
> >> index 5e7d481717ea..d695a3bee1ed 100644
> >> --- a/drivers/net/can/dev/length.c
> >> +++ b/drivers/net/can/dev/length.c
> >> @@ -27,15 +27,13 @@ static const u8 len2dlc[] = {
> >>          13, 13, 13, 13, 13, 13, 13, 13, /* 25 - 32 */
> >>          14, 14, 14, 14, 14, 14, 14, 14, /* 33 - 40 */
> >>          14, 14, 14, 14, 14, 14, 14, 14, /* 41 - 48 */
> >> -       15, 15, 15, 15, 15, 15, 15, 15, /* 49 - 56 */
> >> -       15, 15, 15, 15, 15, 15, 15, 15  /* 57 - 64 */
> >>   };
> >>
> >>   /* map the sanitized data length to an appropriate data length code */
> >>   u8 can_fd_len2dlc(u8 len)
> >>   {
> >> -       if (unlikely(len > 64))
> >> -               return 0xF;
> >> +       if (len > ARRAY_SIZE(len2dlc))
> >
> > Sorry but I missed an of-by-one issue when I did my first
> > review. Don't know why but it popped to my eyes this morning when
> > casually reading the emails.
>
> Oh, yes.
>
> The fist line is 0 .. 8 which has 9 bytes.
>
> I also looked on it (from the back), and wondered if it was correct. But
> didn't see it either at first sight.
>
> >
> > ARRAY_SIZE(len2dlc) is 49. If len is between 0 and 48, use the
> > array, if len is greater *or equal* return CANFD_MAX_DLC.
>
> All these changes and discussions make it very obviously more tricky to
> understand that code.
>
> I don't really like this kind of improvement ...
>
> Before that it was pretty clear that we only catch an out of bounds
> value and usually grab the value from the table.

I understand your point: all three of us initially missed that
bug. But now that it is fixed, I would still prefer to keep
Marc's patch.


Yours sincerely,
Vincent

> >
> > In short, replace > by >=:
> > +       if (len >= ARRAY_SIZE(len2dlc))
> >
> >> +               return CANFD_MAX_DLC;
> >>
> >>          return len2dlc[len];
> >>   }
Oliver Hartkopp Jan. 14, 2021, 5:03 p.m. UTC | #4 
On 14.01.21 10:16, Vincent MAILHOL wrote:
> On Tue. 14 Jan 2021 at 17:23, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
>> On 14.01.21 02:59, Vincent MAILHOL wrote:
>>> On Tue. 14 Jan 2021 at 06:14, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>>>>
>>>> If the length paramter in len2dlc() exceeds the size of the len2dlc array, we
>>>> return 0xF. This is equal to the last 16 members of the array.
>>>>
>>>> This patch removes these members from the array, uses ARRAY_SIZE() for the
>>>> length check, and returns CANFD_MAX_DLC (which is 0xf).
>>>>
>>>> Reviewed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
>>>> Link: https://lore.kernel.org/r/20210111141930.693847-9-mkl@pengutronix.de
>>>> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
>>>> ---
>>>>    drivers/net/can/dev/length.c | 6 ++----
>>>>    1 file changed, 2 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/net/can/dev/length.c b/drivers/net/can/dev/length.c
>>>> index 5e7d481717ea..d695a3bee1ed 100644
>>>> --- a/drivers/net/can/dev/length.c
>>>> +++ b/drivers/net/can/dev/length.c
>>>> @@ -27,15 +27,13 @@ static const u8 len2dlc[] = {
>>>>           13, 13, 13, 13, 13, 13, 13, 13, /* 25 - 32 */
>>>>           14, 14, 14, 14, 14, 14, 14, 14, /* 33 - 40 */
>>>>           14, 14, 14, 14, 14, 14, 14, 14, /* 41 - 48 */
>>>> -       15, 15, 15, 15, 15, 15, 15, 15, /* 49 - 56 */
>>>> -       15, 15, 15, 15, 15, 15, 15, 15  /* 57 - 64 */
>>>>    };
>>>>
>>>>    /* map the sanitized data length to an appropriate data length code */
>>>>    u8 can_fd_len2dlc(u8 len)
>>>>    {
>>>> -       if (unlikely(len > 64))
>>>> -               return 0xF;
>>>> +       if (len > ARRAY_SIZE(len2dlc))
>>>
>>> Sorry but I missed an of-by-one issue when I did my first
>>> review. Don't know why but it popped to my eyes this morning when
>>> casually reading the emails.
>>
>> Oh, yes.
>>
>> The fist line is 0 .. 8 which has 9 bytes.
>>
>> I also looked on it (from the back), and wondered if it was correct. But
>> didn't see it either at first sight.
>>
>>>
>>> ARRAY_SIZE(len2dlc) is 49. If len is between 0 and 48, use the
>>> array, if len is greater *or equal* return CANFD_MAX_DLC.
>>
>> All these changes and discussions make it very obviously more tricky to
>> understand that code.
>>
>> I don't really like this kind of improvement ...
>>
>> Before that it was pretty clear that we only catch an out of bounds
>> value and usually grab the value from the table.
> 
> I understand your point: all three of us initially missed that
> bug. But now that it is fixed, I would still prefer to keep
> Marc's patch.

No, I'm still against it as it is now.

Even

	if (len >= ARRAY_SIZE(len2dlc))

would need some comment that values > 48 lead to a DLC = 15.

This is not intuitively understandable from that value 
"ARRAY_SIZE(len2dlc)" !

Using ARRAY_SIZE() is a bad choice IMO.

If it's really worth to save 16 bytes I would suggest this:

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 3486704c8a95..0b0a5a16943a 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -42,18 +42,17 @@ static const u8 len2dlc[] = {0, 1, 2, 3, 4, 5, 6, 7, 
8,             /* 0 - 8 */
                              10, 10, 10, 10,                    /* 13 - 
16 */
                              11, 11, 11, 11,                    /* 17 - 
20 */
                              12, 12, 12, 12,                    /* 21 - 
24 */
                              13, 13, 13, 13, 13, 13, 13, 13,    /* 25 - 
32 */
                              14, 14, 14, 14, 14, 14, 14, 14,    /* 33 - 
40 */
-                            14, 14, 14, 14, 14, 14, 14, 14,    /* 41 - 
48 */
-                            15, 15, 15, 15, 15, 15, 15, 15,    /* 49 - 
56 */
-                            15, 15, 15, 15, 15, 15, 15, 15};   /* 57 - 
64 */
+                            14, 14, 14, 14, 14, 14, 14, 14};   /* 41 - 
48 */
+                            /* 49 - 64 is checked in  can_fd_len2dlc() */

  /* map the sanitized data length to an appropriate data length code */
  u8 can_fd_len2dlc(u8 len)
  {
-       if (unlikely(len > 64))
+       if (len > 48)
                 return 0xF;

         return len2dlc[len];
  }
  EXPORT_SYMBOL_GPL(can_fd_len2dlc);

Regards,
Oliver

> 
> 
> Yours sincerely,
> Vincent
> 
>>>
>>> In short, replace > by >=:
>>> +       if (len >= ARRAY_SIZE(len2dlc))
>>>
>>>> +               return CANFD_MAX_DLC;
>>>>
>>>>           return len2dlc[len];
>>>>    }
Vincent Mailhol Jan. 15, 2021, 1:57 a.m. UTC | #5 
On Fri. 15 Jan 2021 at 02:03, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> On 14.01.21 10:16, Vincent MAILHOL wrote:
> > On Tue. 14 Jan 2021 at 17:23, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> >> On 14.01.21 02:59, Vincent MAILHOL wrote:
> >>> On Tue. 14 Jan 2021 at 06:14, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> >>>>
> >>>> If the length paramter in len2dlc() exceeds the size of the len2dlc array, we
> >>>> return 0xF. This is equal to the last 16 members of the array.
> >>>>
> >>>> This patch removes these members from the array, uses ARRAY_SIZE() for the
> >>>> length check, and returns CANFD_MAX_DLC (which is 0xf).
> >>>>
> >>>> Reviewed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> >>>> Link: https://lore.kernel.org/r/20210111141930.693847-9-mkl@pengutronix.de
> >>>> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> >>>> ---
> >>>>    drivers/net/can/dev/length.c | 6 ++----
> >>>>    1 file changed, 2 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/drivers/net/can/dev/length.c b/drivers/net/can/dev/length.c
> >>>> index 5e7d481717ea..d695a3bee1ed 100644
> >>>> --- a/drivers/net/can/dev/length.c
> >>>> +++ b/drivers/net/can/dev/length.c
> >>>> @@ -27,15 +27,13 @@ static const u8 len2dlc[] = {
> >>>>           13, 13, 13, 13, 13, 13, 13, 13, /* 25 - 32 */
> >>>>           14, 14, 14, 14, 14, 14, 14, 14, /* 33 - 40 */
> >>>>           14, 14, 14, 14, 14, 14, 14, 14, /* 41 - 48 */
> >>>> -       15, 15, 15, 15, 15, 15, 15, 15, /* 49 - 56 */
> >>>> -       15, 15, 15, 15, 15, 15, 15, 15  /* 57 - 64 */
> >>>>    };
> >>>>
> >>>>    /* map the sanitized data length to an appropriate data length code */
> >>>>    u8 can_fd_len2dlc(u8 len)
> >>>>    {
> >>>> -       if (unlikely(len > 64))
> >>>> -               return 0xF;
> >>>> +       if (len > ARRAY_SIZE(len2dlc))
> >>>
> >>> Sorry but I missed an of-by-one issue when I did my first
> >>> review. Don't know why but it popped to my eyes this morning when
> >>> casually reading the emails.
> >>
> >> Oh, yes.
> >>
> >> The fist line is 0 .. 8 which has 9 bytes.
> >>
> >> I also looked on it (from the back), and wondered if it was correct. But
> >> didn't see it either at first sight.
> >>
> >>>
> >>> ARRAY_SIZE(len2dlc) is 49. If len is between 0 and 48, use the
> >>> array, if len is greater *or equal* return CANFD_MAX_DLC.
> >>
> >> All these changes and discussions make it very obviously more tricky to
> >> understand that code.
> >>
> >> I don't really like this kind of improvement ...
> >>
> >> Before that it was pretty clear that we only catch an out of bounds
> >> value and usually grab the value from the table.
> >
> > I understand your point: all three of us initially missed that
> > bug. But now that it is fixed, I would still prefer to keep
> > Marc's patch.
>
> No, I'm still against it as it is now.
>
> Even
>
>         if (len >= ARRAY_SIZE(len2dlc))
>
> would need some comment that values > 48 lead to a DLC = 15.
>
> This is not intuitively understandable from that value
> "ARRAY_SIZE(len2dlc)" !
>
> Using ARRAY_SIZE() is a bad choice IMO.
>
> If it's really worth to save 16 bytes I would suggest this:
>
> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
> index 3486704c8a95..0b0a5a16943a 100644
> --- a/drivers/net/can/dev.c
> +++ b/drivers/net/can/dev.c
> @@ -42,18 +42,17 @@ static const u8 len2dlc[] = {0, 1, 2, 3, 4, 5, 6, 7,
> 8,             /* 0 - 8 */
>                               10, 10, 10, 10,                    /* 13 -
> 16 */
>                               11, 11, 11, 11,                    /* 17 -
> 20 */
>                               12, 12, 12, 12,                    /* 21 -
> 24 */
>                               13, 13, 13, 13, 13, 13, 13, 13,    /* 25 -
> 32 */
>                               14, 14, 14, 14, 14, 14, 14, 14,    /* 33 -
> 40 */
> -                            14, 14, 14, 14, 14, 14, 14, 14,    /* 41 -
> 48 */
> -                            15, 15, 15, 15, 15, 15, 15, 15,    /* 49 -
> 56 */
> -                            15, 15, 15, 15, 15, 15, 15, 15};   /* 57 -
> 64 */
> +                            14, 14, 14, 14, 14, 14, 14, 14};   /* 41 -
> 48 */
> +                            /* 49 - 64 is checked in  can_fd_len2dlc() */

Ack

>
>   /* map the sanitized data length to an appropriate data length code */
>   u8 can_fd_len2dlc(u8 len)
>   {
> -       if (unlikely(len > 64))
> +       if (len > 48)

I personally prefer the use of macros instead of hardcoded values. 48 is the
last index of the table, i.e. it is ARRAY_SIZE(len2dlc) - 1.

For me, it is like doing this:
for (i = 0; i <= harcoded_value_representing_last_index_of_array; i++)
instead of this:
for (i = 0; i < ARRAY_SIZE(array); i++)

Definitely prefer the later and (len >= ARRAY_SIZE(len2dlc)) is nothing less
than the negation of the i < ARRAY_SIZE(array) that we usually see in a for
loop.

I recognize below patterns to be correct:
   i < ARRAY_SIZE(array): check that variable is inbound.
   i >= ARRAY_SIZE(array): check that variable is outbound.

Anything which deviates from those patterns is fishy and it is actually how
I spotted the bug.

If we don’t use ARRAY_SIZE() we lose that recognizable pattern and we need
to be aware of the actual content of len2dlc[] to understand the code.
(And I know that the table is just above the function and that this makes my
argument weaker).

So IMO, checks done against the array size should use the ARRAY_SIZE() macro
in order 1/ to make it a recognizable pattern and 2/ to make it work regardless
of the actual size of the table (i.e. no hardcoded value).

>                  return 0xF;

I would also prefer to use the CANFD_MAX_DLC macro here.


Yours sincerely,
Vincent


>          return len2dlc[len];
>   }
diff mbox series

Patch

diff --git a/drivers/net/can/dev/length.c b/drivers/net/can/dev/length.c
index 5e7d481717ea..d695a3bee1ed 100644
--- a/drivers/net/can/dev/length.c
+++ b/drivers/net/can/dev/length.c
@@ -27,15 +27,13 @@  static const u8 len2dlc[] = {
 	13, 13, 13, 13, 13, 13, 13, 13,	/* 25 - 32 */
 	14, 14, 14, 14, 14, 14, 14, 14,	/* 33 - 40 */
 	14, 14, 14, 14, 14, 14, 14, 14,	/* 41 - 48 */
-	15, 15, 15, 15, 15, 15, 15, 15,	/* 49 - 56 */
-	15, 15, 15, 15, 15, 15, 15, 15	/* 57 - 64 */
 };
 
 /* map the sanitized data length to an appropriate data length code */
 u8 can_fd_len2dlc(u8 len)
 {
-	if (unlikely(len > 64))
-		return 0xF;
+	if (len > ARRAY_SIZE(len2dlc))
+		return CANFD_MAX_DLC;
 
 	return len2dlc[len];
 }