All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Théo Lebrun" <theo.lebrun@bootlin.com>
To: "Jakub Kicinski" <kuba@kernel.org>, <netdev-bot+sashiko@kernel.org>
Cc: <conor.dooley@microchip.com>, <andrew+netdev@lunn.ch>,
	<davem@davemloft.net>, <edumazet@google.com>, <pabeni@redhat.com>,
	<richardcochran@gmail.com>, <linux@armlinux.org.uk>,
	<netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<pvalerio@redhat.com>, <nb@tipi-net.de>,
	<vladimir.kondratiev@mobileye.com>, <gregory.clement@bootlin.com>,
	<benoit.monin@bootlin.com>, <tawfik.bayouk@mobileye.com>,
	<thomas.petazzoni@bootlin.com>
Subject: Re: [PATCH net-next v10 1/8] net: macb: introduce macb_context struct for buffer management
Date: Tue, 13 Oct 2026 17:02:54 +0200	[thread overview]
Message-ID: <DM3SVHWDO65N.SWXCPUUEFQNE@bootlin.com> (raw)
In-Reply-To: <20261007162223.595dee2e@kernel.org>

Hello Jakub,

On Thu Oct 8, 2026 at 1:22 AM CEST, Jakub Kicinski wrote:
> On Sun, 04 Oct 2026 23:33:24 +0000 netdev-bot+sashiko@kernel.org wrote:
>> - [Medium] The NAPI poll handlers read the context after
>>   napi_complete_done(), and nothing stops that context from being freed…
>
> Looks legit at the surface. I was going to try to double check and
> then realized the series no longer applies :S

Yes that report was correct!

napi_disable() only waits for napi_complete_done() to be called,
but our poll functions still dereference the rings after the
napi_complete_done() call.

That's an issue for all drivers (quite a few are broken in that sense).
I see two solutions:
 - Avoid all dangerous reads in NAPI poll after napi_complete_done().
 - Wait for full NAPI poll to be done using synchronize_net().

We cannot use the first solution in MACB as we must re-read rings after
unmasking IRQs to check if we lost a wakeup signal, see commit
0bf476fc3624 ("net: macb: Fix lost RX packet wakeup race in NAPI
receive"). So we'll take the second one.

This bug is unrelated to context swapping though; the upstream driver
already has the same race condition. Sashiko rather funnily attributes
it to our series; somehow the use-after-free got more serious.

I checked for precedent as I would have been surprised we were the first
ones to land in that napi_disable() subtlety. See commit eb41a264a3a5
("net/mlx5e: Fix race between DIM disable and net_dim()") which uses the
synchronize_net() mechanism to prevent against the exact same race.

V11 has been sent already, see:

[PATCH net-next v11 1/9] net: macb: close race between NAPI polls & ndo_stop
https://lore.kernel.org/netdev/20261013-macb-context-v11-1-b27785167566@bootlin.com/

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com


  reply	other threads:[~2026-10-13 15:03 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-09-30 22:53 [PATCH net-next v10 0/8] net: macb: implement context swapping Théo Lebrun
2026-09-30 22:53 ` [PATCH net-next v10 1/8] net: macb: introduce macb_context struct for buffer management Théo Lebrun
2026-10-04 23:33   ` netdev-bot+sashiko
2026-10-07 23:22     ` Jakub Kicinski
2026-10-13 15:02       ` Théo Lebrun [this message]
2026-09-30 22:53 ` [PATCH net-next v10 2/8] net: macb: avoid macb_init_rx_buffer_size() modifying state Théo Lebrun
2026-09-30 22:53 ` [PATCH net-next v10 3/8] net: macb: make `struct macb` subset reachable from macb_context struct Théo Lebrun
2026-09-30 22:53 ` [PATCH net-next v10 4/8] net: macb: change caps helpers signatures Théo Lebrun
2026-09-30 22:53 ` [PATCH net-next v10 5/8] net: macb: change function signatures to take contexts Théo Lebrun
2026-09-30 22:53 ` [PATCH net-next v10 6/8] net: macb: introduce macb_context_alloc() helper Théo Lebrun
2026-09-30 22:53 ` [PATCH net-next v10 7/8] net: macb: use context swapping in .set_ringparam() Théo Lebrun
2026-10-04 23:33   ` netdev-bot+sashiko
2026-10-13 14:13     ` Théo Lebrun
2026-09-30 22:53 ` [PATCH net-next v10 8/8] net: macb: use context swapping in .ndo_change_mtu() Théo Lebrun
2026-10-04 23:33   ` netdev-bot+sashiko

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=DM3SVHWDO65N.SWXCPUUEFQNE@bootlin.com \
    --to=theo.lebrun@bootlin.com \
    --cc=andrew+netdev@lunn.ch \
    --cc=benoit.monin@bootlin.com \
    --cc=conor.dooley@microchip.com \
    --cc=davem@davemloft.net \
    --cc=edumazet@google.com \
    --cc=gregory.clement@bootlin.com \
    --cc=kuba@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@armlinux.org.uk \
    --cc=nb@tipi-net.de \
    --cc=netdev-bot+sashiko@kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=pabeni@redhat.com \
    --cc=pvalerio@redhat.com \
    --cc=richardcochran@gmail.com \
    --cc=tawfik.bayouk@mobileye.com \
    --cc=thomas.petazzoni@bootlin.com \
    --cc=vladimir.kondratiev@mobileye.com \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.