netdev - Re: [net 05/27] can: dev: can_get_echo_skb(): prevent call to kfree

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:   Thu, 5 Nov 2020 13:47:53 +0900
From:   Vincent MAILHOL <mailhol.vincent@...adoo.fr>
To:     Oliver Hartkopp <socketcan@...tkopp.net>
Cc:     Jakub Kicinski <kuba@...nel.org>,
        Marc Kleine-Budde <mkl@...gutronix.de>,
        Eric Dumazet <edumazet@...gle.com>,
        netdev <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>,
        linux-can <linux-can@...r.kernel.org>, kernel@...gutronix.de
Subject: Re: [net 05/27] can: dev: can_get_echo_skb(): prevent call to
 kfree_skb() in hard IRQ context

On Wed, 5 Nov 2020 02:46, Oliver Hartkopp wrote:
> On 04.11.20 17:02, Jakub Kicinski wrote:
>> On Wed, 4 Nov 2020 15:59:25 +0100 Oliver Hartkopp wrote:
>>> On 04.11.20 09:16, Eric Dumazet wrote:
>
>>>> So skb_orphan(skb) in CAN before calling netif_rx() is better IMO.
>>>>
>>>
>>> Unfortunately you missed the answer from Vincent, why skb_orphan() does
>>> not work here:

Hope that my answer did not go to the spam box.

>>> https://lore.kernel.org/linux-can/CAMZ6RqJyZTcqZcq6jEzm5LLM_MMe=dYDbwvv=Y+dBR0drWuFmw@mail.gmail.com/
>>
>> Okay, we can take this as a quick fix but to me it seems a little
>> strange to be dropping what is effectively locally generated frames.

For those who are not familiar with SocketCAN and to make sure that we
are all aligned here, let me give a bit of context of how the echo on CAN
skbs is usually implement in the drivers:
 * In the xmit() branch, the driver would queue the skb using
   can_put_echo_skb().
 * Whenever the driver gets notified of a successful TX, it will loopback
   the skb using can_get_echo_skb().

This is why the loopback is usually done in hardware IRQ context (but
drivers are also free to skip the second step and directly loopback the
skb in the xmit() branch).

> Thanks! So this patch doesn't hinder Marc's PR :-)
>
>> Can we use a NAPI poll model here and back pressure TX if the echo
>> is not keeping up?
>
> Some of the CAN network drivers already support NAPI.

I had a quick look at NAPI in the past and my understanding is that it
requires the ability to turn off hardware interrupts meaning that it can
be only used on some NIC, not but not, for example, on USB devices.

It would be nice to extend the NAPI with skb loopback for drivers which
already supports it but I am not sure how to include the other drivers.

> @Marc: Can we also use NAPI for echo'ing the skbs?

Yours sincerely,
Vincent Mailhol