From: Vincent MAILHOL <mailhol.vincent@wanadoo.fr>
To: Oliver Hartkopp <socketcan@hartkopp.net>
Cc: Marc Kleine-Budde <mkl@pengutronix.de>,
netdev <netdev@vger.kernel.org>,
David Miller <davem@davemloft.net>,
Jakub Kicinski <kuba@kernel.org>,
linux-can <linux-can@vger.kernel.org>,
kernel@pengutronix.de
Subject: Re: [net-next 09/17] can: length: can_fd_len2dlc(): simplify length calculcation
Date: Fri, 15 Jan 2021 10:57:08 +0900 [thread overview]
Message-ID: <CAMZ6RqKZcuJH2DPeZjgqvL2MG+LoLScHTdd4s+K9OFYDUFT2ZQ@mail.gmail.com> (raw)
In-Reply-To: <75d3c8e9-acbd-09e9-e185-94833dbfb391@hartkopp.net>
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];
> }
next prev parent reply other threads:[~2021-01-15 1:58 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-13 21:13 pull-request: can-next 2021-01-13 Marc Kleine-Budde
2021-01-13 21:13 ` [net-next 01/17] MAINTAINERS: adjust entry to tcan4x5x file split Marc Kleine-Budde
2021-01-13 21:13 ` [net-next 02/17] MAINTAINERS: CAN network layer: add missing header file can-ml.h Marc Kleine-Budde
2021-01-13 21:13 ` [net-next 03/17] can: dev: move driver related infrastructure into separate subdir Marc Kleine-Budde
2021-01-13 21:13 ` [net-next 04/17] can: dev: move bittiming related code into seperate file Marc Kleine-Budde
2021-01-13 21:13 ` [net-next 05/17] can: dev: move length " Marc Kleine-Budde
2021-01-13 21:13 ` [net-next 06/17] can: dev: move skb related " Marc Kleine-Budde
2021-01-13 21:14 ` [net-next 07/17] can: dev: move netlink related code " Marc Kleine-Budde
2021-01-13 21:14 ` [net-next 08/17] can: length: convert to kernel coding style Marc Kleine-Budde
2021-01-13 21:14 ` [net-next 09/17] can: length: can_fd_len2dlc(): simplify length calculcation Marc Kleine-Budde
2021-01-14 1:59 ` Vincent MAILHOL
2021-01-14 7:34 ` Marc Kleine-Budde
2021-01-14 8:23 ` Oliver Hartkopp
2021-01-14 9:16 ` Vincent MAILHOL
2021-01-14 17:03 ` Oliver Hartkopp
2021-01-15 1:57 ` Vincent MAILHOL [this message]
2021-01-13 21:14 ` [net-next 10/17] can: length: canfd_sanitize_len(): add function to sanitize CAN-FD data length Marc Kleine-Budde
2021-01-13 21:14 ` [net-next 11/17] can: length: can_skb_get_frame_len(): introduce function to get data length of frame in data link layer Marc Kleine-Budde
2021-01-13 21:14 ` [net-next 12/17] can: dev: extend struct can_skb_priv to hold CAN frame length Marc Kleine-Budde
2021-01-13 21:14 ` [net-next 13/17] can: dev: can_put_echo_skb(): extend to handle frame_len Marc Kleine-Budde
2021-01-13 21:14 ` [net-next 14/17] can: dev: can_get_echo_skb(): extend to return can frame length Marc Kleine-Budde
2021-01-13 21:14 ` [net-next 15/17] can: dev: can_rx_offload_get_echo_skb(): " Marc Kleine-Budde
2021-01-13 21:14 ` [net-next 16/17] can: dev: can_put_echo_skb(): add software tx timestamps Marc Kleine-Budde
2021-01-13 21:14 ` [net-next 17/17] can: tcan4x5x: remove __packed attribute from struct tcan4x5x_map_buf Marc Kleine-Budde
2021-01-14 7:32 ` pull-request: can-next 2021-01-13 Marc Kleine-Budde
2021-01-14 7:56 pull-request: can-next 2021-01-14 Marc Kleine-Budde
2021-01-14 7:56 ` [net-next 09/17] can: length: can_fd_len2dlc(): simplify length calculcation Marc Kleine-Budde
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAMZ6RqKZcuJH2DPeZjgqvL2MG+LoLScHTdd4s+K9OFYDUFT2ZQ@mail.gmail.com \
--to=mailhol.vincent@wanadoo.fr \
--cc=davem@davemloft.net \
--cc=kernel@pengutronix.de \
--cc=kuba@kernel.org \
--cc=linux-can@vger.kernel.org \
--cc=mkl@pengutronix.de \
--cc=netdev@vger.kernel.org \
--cc=socketcan@hartkopp.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).