Re: [PATCH net v2] net: gro_cells: prevent enabling threaded NAPI on gro_cells

[netdev-bot+sashiko@kernel.org]

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

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

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 rewritten modify_napi_threaded() (net/core/net-sysfs.c) uses
  NAPI_STATE_NO_BUSY_POLL to mean "this NAPI cannot be threaded".
- [Medium] The commit message says the patch makes the write fail with
  -EOPNOTSUPP "when trying to enable threaded NAPI via sysfs" on netdevs…

--

net: gro_cells: prevent enabling threaded NAPI on gro_cells

This patch changes modify_napi_threaded() in net/core/net-sysfs.c.
A write to /sys/class/net/<dev>/threaded now reaches netif_set_threaded()
only if at least one NAPI on the device lacks NAPI_STATE_NO_BUSY_POLL.
The goal is to stop threaded NAPI from being enabled on per-CPU gro_cells
NAPIs, where the local_lock does not provide cross-CPU exclusion.

Closes: https://lore.kernel.org/netdev/6aad6d7b.0c43d342.320d00.0001.GAE@google.com

> diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
> index 352173df75785..286c7b4139df1 100644
> --- a/net/core/net-sysfs.c
> +++ b/net/core/net-sysfs.c
> @@ -743,17 +743,17 @@ static ssize_t threaded_show(struct device *dev,
>  
>  static int modify_napi_threaded(struct net_device *dev, unsigned long val)
>  {
> -	int ret;
> -
> -	if (list_empty(&dev->napi_list))
> -		return -EOPNOTSUPP;
> +	struct napi_struct *napi;
>  
>  	if (val != 0 && val != 1)
>  		return -EOPNOTSUPP;
>  
> -	ret = netif_set_threaded(dev, val);
> +	list_for_each_entry(napi, &dev->napi_list, dev_list) {
> +		if (!test_bit(NAPI_STATE_NO_BUSY_POLL, &napi->state))
> +			return netif_set_threaded(dev, val);
> +	}
>  
> -	return ret;
> +	return -EOPNOTSUPP;
>  }

[Severity: High]
Does this break the threaded control on WireGuard devices?

WireGuard sets NAPI_STATE_NO_BUSY_POLL on every per-peer NAPI. It also
runs those NAPIs threaded by default:

drivers/net/wireguard/peer.c:wg_peer_create() {
    ...
	set_bit(NAPI_STATE_NO_BUSY_POLL, &peer->napi.state);
	netif_napi_add(wg->dev, &peer->napi, wg_packet_rx_poll);
	napi_enable(&peer->napi);
    ...
}

drivers/net/wireguard/device.c:wg_newlink() {
    ...
	netif_threaded_enable(dev);
	ret = register_netdevice(dev);
    ...
}

Once a peer has been added, the new loop never finds a NAPI without
NO_BUSY_POLL. Writing either 0 or 1 to /sys/class/net/wg0/threaded then
returns -EOPNOTSUPP:

threaded_store()->netdev_lock_store()->modify_napi_threaded()

Before this patch, a non-empty napi_list reached netif_set_threaded().
A call to netif_set_threaded(dev, 0) succeeded and stopped the kthreads.

Netlink does not offer another way to change this. napi_hash_add() skips
NO_BUSY_POLL NAPIs, so WireGuard NAPIs never get a NAPI ID. netdev-genl
exposes threaded only per NAPI, through napi-set.

As a result, threaded_show() still reports 1 on WireGuard, but users can
no longer turn threaded mode off. The ABI document for this file says:

    User could set this value to enable/disable threaded mode for all
    napi belonging to this device

The gro_cells problem comes from per-CPU cells protected by a local_lock,
not from NO_BUSY_POLL itself. Would a dedicated marker for NAPIs that
cannot be threaded work better here, similar to the NAPI_STATE_NO_THREADED
approach in v1? The patch carries Fixes: tags, so this change would also
reach stable kernels.

[Severity: Medium]
Should the NO_BUSY_POLL check apply only when val is 1?

The commit message says:

    Check NAPI_STATE_NO_BUSY_POLL in modify_napi_threaded(), and fail early
    with -EOPNOTSUPP when trying to enable threaded NAPI via sysfs on
    netdevs where all NAPIs have NAPI_STATE_NO_BUSY_POLL set.

The loop also runs when val == 0, though. gro_cells_init() sets the flag
on every cell NAPI:

net/core/gro_cells.c:gro_cells_init() {
    ...
		set_bit(NAPI_STATE_NO_BUSY_POLL, &cell->napi.state);
    ...
}

On gre, vxlan, geneve and other devices backed only by gro_cells, writing
0 to threaded used to reach netif_set_threaded(dev, 0) and succeed as a
no-op. It now returns -EOPNOTSUPP.

The commit message avoids napi_id_valid() because it "can potentially
break userspace". Could this change also break scripts that write 0 to
threaded on every interface? If the change is intended, could the commit
message mention it?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260929010058.4063305-1-namangulati%40google.com