* Re: [PATCH net] net: gro_cells: prevent enabling threaded NAPI on gro_cells
2026-09-18 17:34 [PATCH net] net: gro_cells: prevent enabling threaded NAPI on gro_cells Naman Gulati
2026-09-20 0:28 ` Jakub Kicinski
@ 2026-09-22 18:03 ` netdev-bot+sashiko
1 sibling, 0 replies; 5+ messages in thread
From: netdev-bot+sashiko @ 2026-09-22 18:03 UTC (permalink / raw)
To: namangulati
Cc: netdev, davem, edumazet, kuba, pabeni, horms, bigeasy, kuniyu,
syzbot+f0661448aa9511ce744a
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] NAPI_STATE_NO_THREADED is only consulted at the two uAPI entry
points, not where threaded mode is actually turned on, so the new per-…
- [Medium] The Fixes: tag (25718fdcbdd2 "net: gro_cells: Use nested-BH
locking for gro_cell") only covers the lockdep DEBUG_LOCKS_WARN_ON()…
- [Medium] Both new guards reject the *presence* of a threaded request
rather than a request to *enable* threading, which silently narrows…
- [Low] The commit message states that threaded NAPI is enabled on a
gro_cells-backed device "via sysfs or Netlink", but the Netlink path…
--
net: gro_cells: prevent enabling threaded NAPI on gro_cells
This adds a new NAPI_STATE_NO_THREADED state bit, sets it on every
per-CPU gro_cell NAPI in gro_cells_init(), and returns -EOPNOTSUPP from
the sysfs threaded attribute and from the netdev netlink napi-set
handler when the bit is present.
Closes: https://lore.kernel.org/netdev/6aad6d7b.0c43d342.320d00.0001.GAE@google.com
A couple of questions on the commit message first, then some questions
on the code.
> When threaded NAPI is enabled on a device backed by gro_cells (e.g.
> gre0, vxlan, geneve) via sysfs or Netlink, unbound kernel threads are
> created for each per-CPU NAPI.
[Severity: Low]
Is the Netlink half of this sentence reachable today? gro_cells_init()
sets NAPI_STATE_NO_BUSY_POLL before netif_napi_add() and napi_enable(),
and napi_hash_add() bails out for such NAPIs:
net/core/dev.c:napi_hash_add() {
if (test_bit(NAPI_STATE_NO_BUSY_POLL, &napi->state))
return;
...
}
So gro_cell NAPIs never enter napi_hash and keep napi_id == 0.
NETDEV_CMD_NAPI_SET requires NETDEV_A_NAPI_ID and resolves it through
netdev_napi_by_id_lock() -> napi_by_id(), which only walks napi_hash, so
netdev_nl_napi_set_doit() returns -ENOENT before
netdev_nl_napi_set_config() ever runs.
Would it be clearer to say that only the sysfs path reproduces the
reported splat and that the netlink check is a forward-looking
restriction?
> On !CONFIG_PREEMPT_RT kernels without lockdep, concurrent execution of
> __skb_queue_tail() on CPU A and __skb_dequeue() on CPU B without
> cross-CPU locks leads to silent queue corruption.
[ ... ]
> Fixes: 25718fdcbdd2 ("net: gro_cells: Use nested-BH locking for gro_cell")
[Severity: Medium]
Does the Fixes: tag match the second failure mode described above?
At the parent of 25718fdcbdd2, gro_cells_receive() called a bare
__skb_queue_tail(&cell->napi_skbs, skb) and gro_cell_poll() a bare
__skb_dequeue(&cell->napi_skbs) with no lock at all, and that commit
states it adds lockdep coverage only and does not change functional
behaviour for !PREEMPT_RT.
So the lockdep DEBUG_LOCKS_WARN_ON(l->owner) splat is new with
25718fdcbdd2, but the cross-CPU sk_buff_head access that the message
calls silent queue corruption predates it, and trees older than that
commit will not pick this up via the tag.
Would it make sense to separate the two claims in the message, or to
point the tag at the commit that first made threaded NAPI settable on
these devices?
> diff --git a/net/core/gro_cells.c b/net/core/gro_cells.c
> index d8c0a2867120..9f8884d56f59 100644
> --- a/net/core/gro_cells.c
> +++ b/net/core/gro_cells.c
> @@ -92,6 +92,7 @@ int gro_cells_init(struct gro_cells *gcells, struct net_device *dev)
> local_lock_init(&cell->bh_lock);
>
> set_bit(NAPI_STATE_NO_BUSY_POLL, &cell->napi.state);
> + set_bit(NAPI_STATE_NO_THREADED, &cell->napi.state);
>
> netif_napi_add(dev, &cell->napi, gro_cell_poll);
> napi_enable(&cell->napi);
[Severity: High]
Since the new bit is only tested in the two userspace front ends and not
in the code that actually turns threading on, can threaded mode still be
armed on these NAPIs?
The two calls right below the set_bit() do not test it.
netif_napi_add() -> netif_napi_add_weight_locked() consults
napi_get_threaded_config(), which falls back to dev->threaded when
napi->config is NULL - always the case for gro_cell NAPIs added with
netif_napi_add():
net/core/dev.c:netif_napi_add_weight_locked() {
...
if (napi_get_threaded_config(dev, napi))
if (napi_kthread_create(napi))
dev->threaded = NETDEV_NAPI_THREADED_DISABLED;
...
}
and napi_enable_locked() then sets the bit purely from dev->threaded:
net/core/dev.c:napi_enable_locked() {
...
if (n->dev->threaded && n->thread)
new |= NAPIF_STATE_THREADED;
...
}
The in-kernel setters do not test it either: napi_set_threaded(),
netif_set_threaded(), netif_threaded_enable() and the exported
dev_set_threaded() all reach napi_set_threaded() without looking at
NAPI_STATE_NO_THREADED.
Would moving the test into napi_set_threaded() (and the per-NAPI loop of
netif_set_threaded()) make the bit self-enforcing instead of advisory?
> diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
> index 352173df7578..296fdc007862 100644
> --- a/net/core/net-sysfs.c
> +++ b/net/core/net-sysfs.c
> @@ -743,9 +743,11 @@ static ssize_t threaded_show(struct device *dev,
>
> static int modify_napi_threaded(struct net_device *dev, unsigned long val)
> {
> + struct napi_struct *napi;
> int ret;
>
> - if (list_empty(&dev->napi_list))
> + napi = list_first_entry_or_null(&dev->napi_list, typeof(*napi), dev_list);
> + if (!napi || test_bit(NAPI_STATE_NO_THREADED, &napi->state))
> return -EOPNOTSUPP;
>
> if (val != 0 && val != 1)
[Severity: Medium]
Does this change the result of writing 0 to the attribute?
The new test runs before val is inspected, so on a gro_cells-backed
netdev:
echo 0 > /sys/class/net/gre0/threaded
now returns -EOPNOTSUPP. Before the patch the only rejection here was
list_empty(&dev->napi_list), which is never true for a gro_cells device,
so the write reached netif_set_threaded(dev, 0) and succeeded as a
no-op.
threaded_show() still reports 0 for the same device:
net/core/net-sysfs.c:threaded_show() {
...
ret = sysfs_emit(buf, fmt_dec, READ_ONCE(netdev->threaded));
...
}
so a value that can be read back can no longer be written to this RW
attribute.
The netlink side has the same ordering: the test_bit() sits before
nla_get_uint(), so NETDEV_A_NAPI_THREADED carrying "disabled" is also
refused, and the early return happens before
NETDEV_A_NAPI_DEFER_HARD_IRQS, NETDEV_A_NAPI_IRQ_SUSPEND_TIMEOUT and
NETDEV_A_NAPI_GRO_FLUSH_TIMEOUT are applied. Since napi-get always
emits threaded via nla_put_uint() in netdev_nl_napi_fill_one(), a
get-modify-set round trip would apply none of the other requested
settings.
Should both checks be gated on the requested value being enabled, so
that disabling threading keeps working and the other attributes in the
same request are still honoured?
> diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
> index cb18db681640..661f87e2f015 100644
> --- a/net/core/netdev-genl.c
> +++ b/net/core/netdev-genl.c
> @@ -334,6 +334,9 @@ netdev_nl_napi_set_config(struct napi_struct *napi, struct genl_info *info)
> if (info->attrs[NETDEV_A_NAPI_THREADED]) {
> int ret;
>
> + if (test_bit(NAPI_STATE_NO_THREADED, &napi->state))
> + return -EOPNOTSUPP;
> +
> threaded = nla_get_uint(info->attrs[NETDEV_A_NAPI_THREADED]);
> ret = napi_set_threaded(napi, threaded);
> if (ret)
One more question on the sysfs guard: modify_napi_threaded() only tests
the first entry of dev->napi_list, while the operation it gates,
netif_set_threaded(), walks the whole list and also stamps every
napi_config entry:
net/core/dev.c:netif_set_threaded() {
...
list_for_each_entry(napi, &dev->napi_list, dev_list)
WARN_ON_ONCE(napi_set_threaded(napi, threaded));
/* Override the config for all NAPIs even if currently not listed */
for (i = 0; i < dev->num_napi_configs; i++)
dev->napi_config[i].threaded = threaded;
}
netif_napi_dev_list_add() orders entries by napi_id, and a config-less
NAPI gets new_id = UINT_MAX, so it lands at the head:
net/core/dev.c:netif_napi_dev_list_add() {
new_id = UINT_MAX;
if (napi->config && napi->config->napi_id)
new_id = napi->config->napi_id;
...
if (pos_id <= new_id)
break;
...
}
If a device registers such a NAPI after gro_cells_init(), does the guard
then pass while every gro_cell NAPI still gets NAPIF_STATE_THREADED?
And in the other direction, does a gro_cell NAPI at the head refuse
threading for a device's real NAPIs? I did not find an in-tree
gro_cells user that registers an extra NAPI today, so this looks latent
rather than immediately triggerable.
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260918173413.3222413-1-namangulati%40google.com
^ permalink raw reply [flat|nested] 5+ messages in thread