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

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

[Maxime Chevallier]

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.

This won't happen until the next ____napi_schedule() call, which won't
happen as it's disabled.

This happens in 2 instances :

 - Drivers that create but don't use napi instances (stmmac's rxtx napi
   for AF_XDP for example), here it's a driver bug

 - Drivers that create their napi instances in .probe(). Here, setting
   threaded off while the interface is down will hang :

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

Checking on netif_running() isn't enough, as napi instances may be
transiently disabled when changing the MTU for example, so let's check
the NAPIF_STATE_NPSVC flag instead, that's clear when the napi instance
is enabled.

This fix only addresses the second case. For the first case where
drivers have unused napi instances, the "threaded" mode can't be
switched off once enabled, these drivers will need fixing. As this
hasn't been seen before on said drivers, I guess it's OK as it used to
just freeze the system.

This was tested on mvpp2 (probe-time napi_add) and stmmac.

Fixes: 689883de94dd ("net: stop napi kthreads when THREADED napi is disabled")
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
V2:
 - Different approach as per Jakub's review, just don't allow setting
   the napi threaded mode while interface is down.

   I'm unsure about the correctness of this, to me it seems that the
   NAPI_STATE_NPSVC flag indicates the condition we're interested in (is
   napi instance enabled or not ?), but I may be missing something
   obvious :(

V1:
https://lore.kernel.org/r/20260926194714.648819-1-maxime.chevallier@bootlin.com

 net/core/dev.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/core/dev.c b/net/core/dev.c
index f660fccfc0db..fcf18c9eedb8 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -7315,6 +7315,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))
+				return -EBUSY;
+		}
 	}
 
 	WRITE_ONCE(dev->threaded, threaded);
-- 
2.55.0

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

[netdev-bot+sashiko]

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

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

[Maxime Chevallier]

On 10/8/26 04:11, netdev-bot+sashiko@kernel.org wrote:
> 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.
> 

Ah true :/ hmmm ok is it even OK to have the check at disable time when there's
this sentence in the doc then ?

> 
>> +		}
>>  	}
>>  
>>  	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?

Is it a realistic thing to encounter though ? I'm ok with iterating on that,
but this seems to be something very unlikely ?

> 
> 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?

Hmmm maybe yes

Maxime

>