[PATCH net-next v2 0/3] netdev: address a handful of nit picks

[PATCH net-next v2 0/3] netdev: address a handful of nit picks

[Jakub Kicinski]

v1 of this has been sent back in June, it seems to have fallen thru
the cracks.

I got slightly concerned about the state of the netdev-genl implementation
after Gemini found the bug fixed by commit c849de7d8757 ("netdev: fix
double-free in netdev_nl_bind_rx_doit()"). I fed the file into a couple
of models to make sure we're not sitting on more bugs. The models haven't
found anything worthy of a Fixes tag but some of the nit picks are
borderline worth addressing.

v2: patch 1 (the only one with feedback) was sent separately
v1: https://lore.kernel.org/20260609190804.1137085-1-kuba@kernel.org

Jakub Kicinski (3):
  netdev: correct error code in netdev_nl_queue_fill_lease()
  netdev: avoid skipping objects on race with device disappearance
  netdev: don't use dev->flags for IFF_UP

 net/core/netdev-genl.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

-- 
2.55.0

[PATCH net-next v2 1/3] netdev: correct error code in netdev_nl_queue_fill_lease()

[Jakub Kicinski]

netdev_nl_queue_fill_lease() returns ENOMEM on nla_put failures.
This is wrong, the error code should be EMSGSIZE. But it doesn't
matter, caller of netdev_nl_queue_fill_lease() just checks if
the retcode is zero or not, and uses EMSGSIZE.

Reviewed-by: Joe Damato <joe@dama.to>
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
Reviewed-by: Bobby Eshleman <bobbyeshleman@meta.com>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/core/netdev-genl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
index cb18db681640..04dcd7fc614e 100644
--- a/net/core/netdev-genl.c
+++ b/net/core/netdev-genl.c
@@ -434,7 +434,7 @@ netdev_nl_queue_fill_lease(struct sk_buff *rsp, struct net_device *netdev,
 nla_put_failure_unlock:
 	rcu_read_unlock();
 nla_put_failure:
-	return -ENOMEM;
+	return -EMSGSIZE;
 }
 
 static int
-- 
2.55.0

Re: [PATCH net-next v2 1/3] netdev: correct error code in netdev_nl_queue_fill_lease()

[Nikolay Aleksandrov]

On 31/08/2026 19:41, Jakub Kicinski wrote:
> netdev_nl_queue_fill_lease() returns ENOMEM on nla_put failures.
> This is wrong, the error code should be EMSGSIZE. But it doesn't
> matter, caller of netdev_nl_queue_fill_lease() just checks if
> the retcode is zero or not, and uses EMSGSIZE.
> 
> Reviewed-by: Joe Damato <joe@dama.to>
> Acked-by: Daniel Borkmann <daniel@iogearbox.net>
> Reviewed-by: Bobby Eshleman <bobbyeshleman@meta.com>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>   net/core/netdev-genl.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
> index cb18db681640..04dcd7fc614e 100644
> --- a/net/core/netdev-genl.c
> +++ b/net/core/netdev-genl.c
> @@ -434,7 +434,7 @@ netdev_nl_queue_fill_lease(struct sk_buff *rsp, struct net_device *netdev,
>   nla_put_failure_unlock:
>   	rcu_read_unlock();
>   nla_put_failure:
> -	return -ENOMEM;
> +	return -EMSGSIZE;
>   }
>   
>   static int

Reviewed-by: Nikolay Aleksandrov <razor@blackwall.org>

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

[Jakub Kicinski]

If the currently dumped device disappears while we were mid-dump
we will get the next device without resetting the sub-object ID.
This is quite unlikely, it was reported by an AI tool not a real
user. Let's fix it for better dump consistency.

Reviewed-by: Bobby Eshleman <bobbyeshleman@meta.com>
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/core/netdev-genl.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
index 04dcd7fc614e..e427bab4c4d2 100644
--- a/net/core/netdev-genl.c
+++ b/net/core/netdev-genl.c
@@ -312,11 +312,14 @@ int netdev_nl_napi_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
 			err = -ENODEV;
 		}
 	} else {
+		unsigned long start_ifindex = ctx->ifindex;
+
 		for_each_netdev_lock_scoped(net, netdev, ctx->ifindex) {
+			if (ctx->ifindex != start_ifindex)
+				ctx->napi_id = 0;
 			err = netdev_nl_napi_dump_one(netdev, skb, info, ctx);
 			if (err < 0)
 				break;
-			ctx->napi_id = 0;
 		}
 	}
 
@@ -636,13 +639,17 @@ int netdev_nl_queue_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
 			err = -ENODEV;
 		}
 	} else {
+		unsigned long start_ifindex = ctx->ifindex;
+
 		for_each_netdev_lock_ops_compat_scoped(net, netdev,
 						       ctx->ifindex) {
+			if (ctx->ifindex != start_ifindex) {
+				ctx->rxq_idx = 0;
+				ctx->txq_idx = 0;
+			}
 			err = netdev_nl_queue_dump_one(netdev, skb, info, ctx);
 			if (err < 0)
 				break;
-			ctx->rxq_idx = 0;
-			ctx->txq_idx = 0;
 		}
 	}
 
@@ -788,8 +795,6 @@ netdev_nl_stats_by_queue(struct net_device *netdev, struct sk_buff *rsp,
 		ctx->txq_idx = ++i;
 	}
 
-	ctx->rxq_idx = 0;
-	ctx->txq_idx = 0;
 	return 0;
 }
 
@@ -904,6 +909,7 @@ int netdev_nl_qstats_get_dumpit(struct sk_buff *skb,
 	struct netdev_nl_dump_ctx *ctx = netdev_dump_ctx(cb);
 	const struct genl_info *info = genl_info_dump(cb);
 	struct net *net = sock_net(skb->sk);
+	unsigned long start_ifindex;
 	struct net_device *netdev;
 	unsigned int ifindex;
 	unsigned int scope;
@@ -936,7 +942,13 @@ int netdev_nl_qstats_get_dumpit(struct sk_buff *skb,
 		return err;
 	}
 
+	start_ifindex = ctx->ifindex;
+
 	for_each_netdev_lock_ops_compat_scoped(net, netdev, ctx->ifindex) {
+		if (ctx->ifindex != start_ifindex) {
+			ctx->rxq_idx = 0;
+			ctx->txq_idx = 0;
+		}
 		err = netdev_nl_qstats_get_dump_one(netdev, scope, skb,
 						    info, ctx);
 		if (err < 0)
-- 
2.55.0

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

[Nikolay Aleksandrov]

On 31/08/2026 19:41, Jakub Kicinski wrote:
> If the currently dumped device disappears while we were mid-dump
> we will get the next device without resetting the sub-object ID.
> This is quite unlikely, it was reported by an AI tool not a real
> user. Let's fix it for better dump consistency.
> 
> Reviewed-by: Bobby Eshleman <bobbyeshleman@meta.com>
> Acked-by: Daniel Borkmann <daniel@iogearbox.net>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>   net/core/netdev-genl.c | 22 +++++++++++++++++-----
>   1 file changed, 17 insertions(+), 5 deletions(-)
> 

Reviewed-by: Nikolay Aleksandrov <razor@blackwall.org>

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

[Hangbin Liu]

On Mon, Aug 31, 2026 at 09:41:57AM -0700, Jakub Kicinski wrote:
> If the currently dumped device disappears while we were mid-dump
> we will get the next device without resetting the sub-object ID.
> This is quite unlikely, it was reported by an AI tool not a real
> user. Let's fix it for better dump consistency.
> 
> Reviewed-by: Bobby Eshleman <bobbyeshleman@meta.com>
> Acked-by: Daniel Borkmann <daniel@iogearbox.net>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  net/core/netdev-genl.c | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
> index 04dcd7fc614e..e427bab4c4d2 100644
> --- a/net/core/netdev-genl.c
> +++ b/net/core/netdev-genl.c
> @@ -312,11 +312,14 @@ int netdev_nl_napi_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
>  			err = -ENODEV;
>  		}
>  	} else {
> +		unsigned long start_ifindex = ctx->ifindex;

The NETDEV_A_QUEUE_IFINDEX policy is u32. And all ifindex getting/setting
in 6b6171db7fc8 ("netdev-genl: Add netlink framework functions for queue")
is u32. Why the ifindex is defined to unsigned long?

Thanks
Hangbin

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

[Jakub Kicinski]

On Tue, 1 Sep 2026 17:15:14 +0800 Hangbin Liu wrote:
> > @@ -312,11 +312,14 @@ int netdev_nl_napi_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
> >  			err = -ENODEV;
> >  		}
> >  	} else {
> > +		unsigned long start_ifindex = ctx->ifindex;  
> 
> The NETDEV_A_QUEUE_IFINDEX policy is u32. And all ifindex getting/setting
> in 6b6171db7fc8 ("netdev-genl: Add netlink framework functions for queue")
> is u32. Why the ifindex is defined to unsigned long?

Off the top of my head - because it's am xarray iterator

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

[Hangbin Liu]

On Tue, Sep 01, 2026 at 08:02:34AM -0700, Jakub Kicinski wrote:
> On Tue, 1 Sep 2026 17:15:14 +0800 Hangbin Liu wrote:
> > > @@ -312,11 +312,14 @@ int netdev_nl_napi_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
> > >  			err = -ENODEV;
> > >  		}
> > >  	} else {
> > > +		unsigned long start_ifindex = ctx->ifindex;  
> > 
> > The NETDEV_A_QUEUE_IFINDEX policy is u32. And all ifindex getting/setting
> > in 6b6171db7fc8 ("netdev-genl: Add netlink framework functions for queue")
> > is u32. Why the ifindex is defined to unsigned long?
> 
> Off the top of my head - because it's am xarray iterator

Oh, thanks for the info. I find it.

#define for_each_netdev_dump(net, d, ifindex)                           \
	for (; (d = xa_find(&(net)->dev_by_index, &ifindex,             \
			    ULONG_MAX, XA_PRESENT)); ifindex++)

Thanks
Hangbin

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

[netdev-bot+sashiko]

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.

> diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
> index 04dcd7fc614e1..e427bab4c4d2e 100644
> --- a/net/core/netdev-genl.c
> +++ b/net/core/netdev-genl.c
> @@ -312,11 +312,14 @@ int netdev_nl_napi_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
>  			err = -ENODEV;
>  		}
>  	} else {
> +		unsigned long start_ifindex = ctx->ifindex;
> +
>  		for_each_netdev_lock_scoped(net, netdev, ctx->ifindex) {
> +			if (ctx->ifindex != start_ifindex)
> +				ctx->napi_id = 0;

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

>  			err = netdev_nl_napi_dump_one(netdev, skb, info, ctx);
>  			if (err < 0)
>  				break;
> -			ctx->napi_id = 0;
>  		}
>  	}
>  
> @@ -636,13 +639,17 @@ int netdev_nl_queue_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
>  			err = -ENODEV;
>  		}
>  	} else {
> +		unsigned long start_ifindex = ctx->ifindex;
> +
>  		for_each_netdev_lock_ops_compat_scoped(net, netdev,
>  						       ctx->ifindex) {
> +			if (ctx->ifindex != start_ifindex) {
> +				ctx->rxq_idx = 0;
> +				ctx->txq_idx = 0;
> +			}
>  			err = netdev_nl_queue_dump_one(netdev, skb, info, ctx);
>  			if (err < 0)
>  				break;
> -			ctx->rxq_idx = 0;
> -			ctx->txq_idx = 0;
>  		}
>  	}
>  

[ ... ]

> @@ -936,7 +942,13 @@ int netdev_nl_qstats_get_dumpit(struct sk_buff *skb,
>  		return err;
>  	}
>  
> +	start_ifindex = ctx->ifindex;
> +
>  	for_each_netdev_lock_ops_compat_scoped(net, netdev, ctx->ifindex) {
> +		if (ctx->ifindex != start_ifindex) {
> +			ctx->rxq_idx = 0;
> +			ctx->txq_idx = 0;
> +		}
>  		err = netdev_nl_qstats_get_dump_one(netdev, scope, skb,
>  						    info, ctx);
>  		if (err < 0)

[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

[PATCH net-next v2 3/3] netdev: don't use dev->flags for IFF_UP

[Jakub Kicinski]

dev->flags are not technically ops lock protected. The IFF_UP flag
will not change when dev->lock is held, but other flags may change
so KCSAN would probably not be impressed. Because of this we added
a dedicated dev->up which is safe to read under dev->lock.

qstats want to make sure device is up, use dev->up.

Reviewed-by: Bobby Eshleman <bobbyeshleman@meta.com>
Reviewed-by: Joe Damato <joe@dama.to>
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/core/netdev-genl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
index e427bab4c4d2..fa9edfdb32c2 100644
--- a/net/core/netdev-genl.c
+++ b/net/core/netdev-genl.c
@@ -775,7 +775,7 @@ netdev_nl_stats_by_queue(struct net_device *netdev, struct sk_buff *rsp,
 	const struct netdev_stat_ops *ops = netdev->stat_ops;
 	int i, err;
 
-	if (!(netdev->flags & IFF_UP))
+	if (!netdev->up)
 		return 0;
 
 	i = ctx->rxq_idx;
-- 
2.55.0

Re: [PATCH net-next v2 3/3] netdev: don't use dev->flags for IFF_UP

[Nikolay Aleksandrov]

On 31/08/2026 19:41, Jakub Kicinski wrote:
> dev->flags are not technically ops lock protected. The IFF_UP flag
> will not change when dev->lock is held, but other flags may change
> so KCSAN would probably not be impressed. Because of this we added
> a dedicated dev->up which is safe to read under dev->lock.
> 
> qstats want to make sure device is up, use dev->up.
> 
> Reviewed-by: Bobby Eshleman <bobbyeshleman@meta.com>
> Reviewed-by: Joe Damato <joe@dama.to>
> Acked-by: Daniel Borkmann <daniel@iogearbox.net>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>   net/core/netdev-genl.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 

Reviewed-by: Nikolay Aleksandrov <razor@blackwall.org>

Re: [PATCH net-next v2 0/3] netdev: address a handful of nit picks

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Mon, 31 Aug 2026 09:41:55 -0700 you wrote:
> v1 of this has been sent back in June, it seems to have fallen thru
> the cracks.
> 
> I got slightly concerned about the state of the netdev-genl implementation
> after Gemini found the bug fixed by commit c849de7d8757 ("netdev: fix
> double-free in netdev_nl_bind_rx_doit()"). I fed the file into a couple
> of models to make sure we're not sitting on more bugs. The models haven't
> found anything worthy of a Fixes tag but some of the nit picks are
> borderline worth addressing.
> 
> [...]

Here is the summary with links:
  - [net-next,v2,1/3] netdev: correct error code in netdev_nl_queue_fill_lease()
    https://git.kernel.org/netdev/net-next/c/fcfb5ff9dfc3
  - [net-next,v2,2/3] netdev: avoid skipping objects on race with device disappearance
    (no matching commit)
  - [net-next,v2,3/3] netdev: don't use dev->flags for IFF_UP
    https://git.kernel.org/netdev/net-next/c/17d7aa695408

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html