Re: [net-next,v2,2/3] netdev: avoid skipping objects on race with device disappearance

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

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

- [Medium] Incomplete fix across the netdev generic-netlink family:
  netdev_nl_page_pool_get_dump() in net/core/page_pool_user.c (backing…
- [Medium] The new continuation logic identifies the resumed device only
  by its numeric ifindex (ctx->ifindex != start_ifindex).
- [Low] Commit message / provenance: the patch fixes a real user-visible
  dump-correctness bug (a netdev-genl dump can silently omit…

--

netdev: avoid skipping objects on race with device disappearance

The dumpit handlers for napi-get, queue-get and qstats-get now snapshot
ctx->ifindex before iterating and only clear the sub-object cursor
(ctx->napi_id, ctx->rxq_idx, ctx->txq_idx) when the iterator has moved
to a different ifindex, instead of clearing it after each device.  The
unconditional reset at the end of netdev_nl_stats_by_queue() is dropped.

No Link: tags in the commit header.

[Severity: Low]

The commit message says:

    If the currently dumped device disappears while we were mid-dump
    we will get the next device without resetting the sub-object ID.

Since the effect of that is a dump that silently omits the NAPIs, queues
or per-queue stats rows of the device following the one that went away,
should this carry a Fixes: tag for the commits that introduced the
per-device cursor reset in netdev_nl_napi_get_dumpit(),
netdev_nl_queue_get_dumpit() and netdev_nl_qstats_get_dumpit()?

The message does state "This is quite unlikely, it was reported by an AI
tool not a real user. Let's fix it for better dump consistency.", so if
skipping stable is intentional then this is only a question about making
the provenance explicit.

[Severity: Medium]

Is the numeric ifindex enough to establish that the resumed device is
the same object we stopped inside?

Between two dumpit invocations the per-device lock taken by
for_each_netdev_lock_scoped() is dropped by the __free() cleanup in
net/core/dev.h, and nothing pins the device or its index.  A different
device can take over the same index in that window, because a requested
non-zero index is accepted directly in dev_index_reserve():

net/core/dev.c:dev_index_reserve() {
	...
	if (!ifindex)
		err = xa_alloc_cyclic(&net->dev_by_index, &ifindex, NULL,
				      xa_limit_31b, &net->ifindex, GFP_KERNEL);
	else
		err = xa_insert(&net->dev_by_index, ifindex, NULL, GFP_KERNEL);
	...
}

and rtnl_newlink_create() then assigns it:

net/core/rtnetlink.c:rtnl_newlink_create() {
	...
	dev->ifindex = ifm->ifi_index;
	...
}

A device moved in from another netns can also keep its index.  In that
case ctx->ifindex == start_ifindex, the reset is skipped, and the old
device's cursor is applied to the replacement device:

net/core/netdev-genl.c:netdev_nl_napi_dump_one() {
	...
		if (ctx->napi_id && napi->napi_id >= ctx->napi_id)
			continue;
	...
}

A freshly created device gets higher napi ids, so would every NAPI of
the replacement device be filtered out here?  The same applies to the
queue-get and qstats-get hunks below, where the stale ctx->rxq_idx /
ctx->txq_idx would cause the replacement device's low-numbered queues to
never be reported.

Would keying the reset on something other than the raw ifindex, or
invalidating the sub-cursor whenever object identity cannot be proven
(for example a dump generation counter plus NLM_F_DUMP_INTR), close this
remaining case?

[ ... ]

[Severity: Medium]

Should netdev_nl_page_pool_get_dump() in net/core/page_pool_user.c get
the same treatment?  It backs page-pool-get and page-pool-stats-get in
the same netdev family and still uses the idiom this patch replaces:

net/core/page_pool_user.c:netdev_nl_page_pool_get_dump() {
	...
	for_each_netdev_dump(net, netdev, state->ifindex) {
		...
		hlist_for_each_entry(pool, &netdev->page_pools, user.list) {
			if (state->pp_id && state->pp_id < pool->user.id)
				continue;

			state->pp_id = pool->user.id;
			err = fill(skb, pool, info);
			if (err)
				goto out;
		}

		state->pp_id = 0;
	}
out:
	...
}

The "state->pp_id = 0" is bypassed by the goto out taken when fill()
returns -EMSGSIZE, which is the normal dump continuation path.  If the
device is unregistered before the next recvmsg(), for_each_netdev_dump()
lands on the following device while state->pp_id still holds the
previous device's page-pool id.

Since ids come from xa_alloc_cyclic() in page_pool_list():

net/core/page_pool_user.c:page_pool_list() {
	...
	err = xa_alloc_cyclic(&page_pools, &pool->user.id, pool, xa_limit_32b,
			      &id_alloc_next, GFP_KERNEL);
	...
}

they grow monotonically, so would the "state->pp_id < pool->user.id"
test then silently skip every page pool of the new device?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831164159.1124679-1-kuba%40kernel.org