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

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

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

>