On Fri. 15 Jan 2021 at 02:03, Oliver Hartkopp <socketcan@xxxxxxxxxxxx> wrote: > On 14.01.21 10:16, Vincent MAILHOL wrote: > > On Tue. 14 Jan 2021 at 17:23, Oliver Hartkopp <socketcan@xxxxxxxxxxxx> wrote: > >> On 14.01.21 02:59, Vincent MAILHOL wrote: > >>> On Tue. 14 Jan 2021 at 06:14, Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx> 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@xxxxxxxxxx> > >>>> Link: https://lore.kernel.org/r/20210111141930.693847-9-mkl@xxxxxxxxxxxxxx > >>>> Signed-off-by: Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx> > >>>> --- > >>>> 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]; > }
