Re: [PATCH net v2] net: Don't allow disabling napi kthread mode while napi instances are disabled

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 2 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The fix is incomplete.
- [Medium] The new guard in netif_set_threaded()
  (net/core/dev.c:7352-7354) treats NAPI_STATE_NPSVC as meaning 'this…
- [Medium] After this patch, writing 0 to /sys/class/net/<iface>/threaded
  (or calling netif_set_threaded()/dev_set_threaded() with 0) returns…
- [Low] The commit message gets the hang mechanism wrong.

--

net: Don't allow disabling napi kthread mode while napi instances are disabled

This patch makes netif_set_threaded() return -EBUSY for a request to
disable threaded mode when any NAPI instance with a kthread has
NAPI_STATE_NPSVC set. The patch uses that bit to mean the instance is
disabled. The goal is to avoid a hang in napi_stop_kthread() when threaded
mode is turned off on a down interface whose driver added its NAPIs at
probe time.

> Trying to disable the napi threaded state while the napi instance is
> disabled will hang forever in napi_stop_kthread(), as it waits for
> NAPIF_STATE_SCHED_THREADED to clear.

[Severity: Low]
Is this the right loop? On a disabled NAPI, SCHED is set and
SCHED_THREADED is clear. That means the hang is in the first wait loop of
napi_stop_kthread():

net/core/dev.c:napi_stop_kthread() {
    ...
		if ((val & NAPIF_STATE_SCHED_THREADED) ||
		    !(val & NAPIF_STATE_SCHED)) {
			new = val & (~(NAPIF_STATE_THREADED |
				       NAPIF_STATE_THREADED_BUSY_POLL));
		} else {
			msleep(20);
			continue;
		}
    ...
}

The second loop is the one that waits for SCHED_THREADED to clear. It
would exit right away, but the code never gets there.

The real blocking condition is that napi_disable() or netif_napi_add()
leaves SCHED set, and only napi_enable_locked() can clear it. That
function needs the netdev lock, which the caller already holds.

Could the description be updated to match? With the right root cause it
is easier to see whether other paths reach the same state (see the
rollback comment below).

> diff --git a/net/core/dev.c b/net/core/dev.c
> index e76762e29360e..83237591cfe85 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -7347,6 +7347,12 @@ int netif_set_threaded(struct net_device *dev,
>  				}
>  			}
>  		}
> +	} else {
> +		list_for_each_entry(napi, &dev->napi_list, dev_list) {
> +			if (napi->thread &&
> +			    test_bit(NAPI_STATE_NPSVC, &napi->state))

[Severity: Medium]
Can NAPI_STATE_NPSVC be set here on a NAPI that is enabled? The bit is
documented as a netpoll servicing flag:

include/linux/netdevice.h:
	NAPI_STATE_NPSVC,		/* Netpoll - don't dequeue from poll_list */

poll_one_napi() sets it for a short time around the poll, without holding
the netdev instance lock:

net/core/netpoll.c:poll_one_napi() {
	if (test_and_set_bit(NAPI_STATE_NPSVC, &napi->state))
		return;
	...
	work = napi->poll(napi, 0);
	...
	clear_bit(NAPI_STATE_NPSVC, &napi->state);
}

netpoll_poll_dev() only takes ni->dev_lock with down_trylock(), and
nothing serializes it against netif_set_threaded().

Suppose netconsole or bonding is polling the device when 0 is written to
/sys/class/net/<iface>/threaded. Would this return -EBUSY even though
every NAPI is enabled and disabling would be safe?

A disabled NAPI always has NPSVC set. The reverse doesn't hold: NPSVC
being set does not mean the NAPI is disabled.

> +				return -EBUSY;

[Severity: Medium]
Does this change the documented behaviour of
/sys/class/net/<iface>/threaded? For drivers that add their NAPIs at probe
time, writing 0 now fails with -EBUSY on any down interface. Enabling has
no matching check:

  ip link set eth0 down
  echo 1 > /sys/class/net/eth0/threaded   (succeeds, creates kthreads)
  echo 0 > /sys/class/net/eth0/threaded   (-EBUSY until link up)

Drivers that call netif_threaded_enable() at probe, such as atl1c and
ravb, cannot leave threaded mode before the first link-up. Drivers whose
NAPIs are added but never enabled can never leave it.

Documentation/ABI/testing/sysfs-class-net says the attribute lets the
user "enable/disable threaded mode for all napi belonging to this device,
without the need to do device up/down", and it lists no error cases.

Should that document be updated? Alternatively, should enabling be
refused in the same state so the two directions behave the same way?

The V2 changelog note ("just don't allow setting the napi threaded mode
while interface is down") doesn't match the code either. The code only
refuses disabling.

> +		}
>  	}
>  
>  	WRITE_ONCE(dev->threaded, threaded);

[Severity: High]
Can the same hang still be reached through the rollback in the enable
path? The new check only runs in the else branch, which handles an
explicit disable. In the if (threaded) branch, napi_kthread_create() can
succeed for some NAPIs and then fail for a later one:

net/core/dev.c:netif_set_threaded() {
	if (threaded) {
		list_for_each_entry(napi, &dev->napi_list, dev_list) {
			if (!napi->thread) {
				err = napi_kthread_create(napi);
				if (err) {
					threaded = NETDEV_NAPI_THREADED_DISABLED;
					break;
				}
			}
		}
	}
    ...
	list_for_each_entry(napi, &dev->napi_list, dev_list)
		WARN_ON_ONCE(napi_set_threaded(napi, threaded));
    ...
}

kthread_run() in napi_kthread_create() can fail with -ENOMEM. It can
also fail with -EINTR if a fatal signal reaches the writer during the
killable wait in kthread creation.

When that happens, threaded becomes DISABLED and napi_set_threaded(napi,
0) runs on every listed NAPI without the NPSVC check. Some NAPIs may be
disabled but have a thread (SCHED and NPSVC set, SCHED_THREADED clear).
The thread may have been created moments earlier in the same loop, or
left over from before.

Each of those NAPIs would go into napi_stop_kthread() and spin in the
first msleep(20) loop with the netdev lock held. Only napi_enable_locked()
can clear SCHED, and it needs the same lock.

The first way to reach this is on a driver that adds its NAPIs at probe
time (the mvpp2 case from the commit message), when a kthread creation
fails part-way through:

  ip link set eth0 down
  echo 1 > /sys/class/net/eth0/threaded

The second is a probe-time netif_threaded_enable() after
netif_napi_add(). For example, ravb_probe() adds the RAVB_BE and RAVB_NC
NAPIs and then calls netif_threaded_enable(). If creating the second
kthread fails, would probe hang here?

Would it be better to handle disabled NAPIs in the stop path itself,
napi_stop_kthread() or napi_set_threaded(), rather than only guarding the
explicit disable branch?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261006141119.85562-1-maxime.chevallier%40bootlin.com