| Message ID | 20210113211410.917108-10-mkl@pengutronix.de |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
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
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]; >>>> }
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 --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]; }