netdev - Re: [PATCH v3 5/5] can: do not increase tx

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 3 Dec 2021 09:44:51 +0100
From:   Jimmy Assarsson <extja@...ser.com>
To:     Vincent MAILHOL <mailhol.vincent@...adoo.fr>
Cc:     Marc Kleine-Budde <mkl@...gutronix.de>, linux-can@...r.kernel.org,
        Oliver Hartkopp <socketcan@...tkopp.net>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-sunxi@...ts.linux.dev,
        Nicolas Ferre <nicolas.ferre@...rochip.com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Ludovic Desroches <ludovic.desroches@...rochip.com>,
        Maxime Ripard <mripard@...nel.org>,
        Chen-Yu Tsai <wens@...e.org>,
        Jernej Skrabec <jernej.skrabec@...il.com>,
        Yasushi SHOJI <yashi@...cecubics.com>,
        Stephane Grosjean <s.grosjean@...k-system.com>
Subject: Re: [PATCH v3 5/5] can: do not increase tx_bytes statistics for RTR
 frames

On 2021-12-03 02:05, Vincent MAILHOL wrote:
> On Fri. 3 Dec. 2021 at 08:35, Jimmy Assarsson <extja@...ser.com> wrote:
>> On 2021-11-28 13:37, Vincent Mailhol wrote:
>>> The actual payload length of the CAN Remote Transmission Request (RTR)
>>> frames is always 0, i.e. nothing is transmitted on the wire. However,
>>> those RTR frames still use the DLC to indicate the length of the
>>> requested frame.
>>>
>>> As such, net_device_stats:tx_bytes should not be increased when
>>> sending RTR frames.
>>>
>>> The function can_get_echo_skb() already returns the correct length,
>>> even for RTR frames (c.f. [1]). However, for historical reasons, the
>>> drivers do not use can_get_echo_skb()'s return value and instead, most
>>> of them store a temporary length (or dlc) in some local structure or
>>> array. Using the return value of can_get_echo_skb() solves the
>>> issue. After doing this, such length/dlc fields become unused and so
>>> this patch does the adequate cleaning when needed.
>>>
>>> This patch fixes all the CAN drivers.
>>>
>>> Finally, can_get_echo_skb() is decorated with the __must_check
>>> attribute in order to force future drivers to correctly use its return
>>> value (else the compiler would emit a warning).
>>>
>>> [1] commit ed3320cec279 ("can: dev: __can_get_echo_skb():
>>> fix real payload length return value for RTR frames")
>>
>> Hi Vincent!
>>
>> Thanks for the patch!
>> I've reviewed and tested the changes affecting kvaser_usb.
>> Looks good to me, only a minor nitpick inline :)

...

>>> diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
>>> index 17fabd3d0613..9f423a5fb63f 100644
>>> --- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
>>> +++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c

...

>>> @@ -1493,13 +1489,13 @@ kvaser_usb_hydra_frame_to_cmd_std(const struct kvaser_usb_net_priv *priv,
>>>        if (cf->can_id & CAN_RTR_FLAG)
>>>                flags |= KVASER_USB_HYDRA_CF_FLAG_REMOTE_FRAME;
>>>
>>> -     flags |= (cf->can_id & CAN_ERR_FLAG ?
>>> -               KVASER_USB_HYDRA_CF_FLAG_ERROR_FRAME : 0);
>>> +     if (cf->can_id & CAN_ERR_FLAG)
>>> +             flags |= KVASER_USB_HYDRA_CF_FLAG_ERROR_FRAME;
>>
>> This has nothing to do with RTR. Maybe put it in a separate patch?
> 
> Arg... You are right. This should not be here. I saw it in my
> final check, removed it in my tree and forgot to redo a "git
> format-patch".
> 
> This is some leftover of a previous version in which I did more
> heavy changes to kvaser_usb_hydra_frame_to_cmd_std(). This is
> purely cosmetic though. I am not willing to go into a clean up
> crusade of all CAN drivers so I will just leave the ternary
> operator untouched. Free to you to reuse it if you want to do a
> clean up later on.

Sure, I'll save it for later :) If you got more changes, feel free to share
them.

Best regards,
jimmy