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]

Message-ID: <02c5f3c7-7b6a-a487-39fc-50b5fad0eebc@pengutronix.de>
Date:   Thu, 5 Nov 2020 08:44:31 +0100
From:   Marc Kleine-Budde <mkl@...gutronix.de>
To:     Vincent MAILHOL <mailhol.vincent@...adoo.fr>,
        Oliver Hartkopp <socketcan@...tkopp.net>
Cc:     Jakub Kicinski <kuba@...nel.org>,
        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 11/5/20 5:47 AM, Vincent MAILHOL wrote:
>>> 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.

And some drivers (i.e. flexcan and mcp251xfd) already queue the ech_skb via 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?

The flexcan driver does:

https://elixir.bootlin.com/linux/v5.10-rc2/source/drivers/net/can/flexcan.c#L1080

In the interrupt handler, the driver pushes the echo_skb into the rx-offload
queue and triggers NAPI. The rx-offload pushed the skb into the networking stack
via NAPI.

Here the code in the mcp251xfd driver:

https://elixir.bootlin.com/linux/v5.10-rc2/source/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c#L1237

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |



Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)