On Tue. 14 Jan 2021 at 17:23, Oliver Hartkopp <socket...@hartkopp.net> wrote: > On 14.01.21 02:59, Vincent MAILHOL wrote: > > On Tue. 14 Jan 2021 at 06:14, Marc Kleine-Budde <m...@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.vinc...@wanadoo.fr> > >> Link: https://lore.kernel.org/r/20210111141930.693847-9-...@pengutronix.de > >> Signed-off-by: Marc Kleine-Budde <m...@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]; > >> }
