[PATCH] ethtool: tsinfo: release net_device reference in ethnl_tsinfo_start()

[PATCH] ethtool: tsinfo: release net_device reference in ethnl_tsinfo_start()

[Hui Peng]

Unlike ethnl_default_start(), ethnl_tsinfo_start() keeps the net_device
reference acquired by ethnl_parse_header_dev_get() in
ctx->req_info->base.dev until ethnl_tsinfo_done().

When a Netlink dump (NLM_F_DUMP) of ETHTOOL_MSG_TSINFO_GET specifying a
device header fails on its initial netlink_dump() invocation (for
example, with -ENOBUFS when sk_rmem_alloc >= sk_rcvbuf), Netlink keeps
cb->cb_running = true without calling cb->done(). As long as the Netlink
socket remains open, the netdev_hold() reference is held, causing
unregister_netdevice() to hang indefinitely when the device is removed.

Save dev->ifindex in ctx->pos_ifindex with ctx->single_dev = true and
immediately release req_info->base.dev via ethnl_parse_header_dev_put()
in ethnl_tsinfo_start(), looking up the device transiently by ifindex
inside ethnl_tsinfo_dumpit().

Fixes: b9e3f7dc9ed9 ("net: ethtool: tsinfo: Enhance tsinfo to support several hwtstamp by net topology")
Assisted-by: LLM
Signed-off-by: Hui Peng <benquike@gmail.com>
---
diff --git a/net/ethtool/tsinfo.c b/net/ethtool/tsinfo.c
--- a/net/ethtool/tsinfo.c
+++ b/net/ethtool/tsinfo.c
@@ -295,6 +295,7 @@ struct ethnl_tsinfo_dump_ctx {
 	struct tsinfo_reply_data	*reply_data;
 	unsigned long			pos_ifindex;
 	bool				netdev_dump_done;
+	bool				single_dev;
 	unsigned long			pos_phyindex;
 	enum hwtstamp_provider_qualifier pos_phcqualifier;
 };
@@ -476,12 +477,15 @@ int ethnl_tsinfo_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
 	struct net *net = sock_net(skb->sk);
 	int ret = 0;
 
-	if (ctx->req_info->base.dev) {
-		struct net_device *dev = ctx->req_info->base.dev;
+	if (ctx->single_dev) {
+		struct net_device *dev = dev_get_by_index(net, ctx->pos_ifindex);
 
+		if (!dev)
+			return -ENODEV;
 		netdev_lock_ops_compat(dev);
 		ret = ethnl_tsinfo_dump_one_net_topo(skb, dev, cb);
 		netdev_unlock_ops_compat(dev);
+		dev_put(dev);
 		return ret;
 	}
 
@@ -533,6 +537,13 @@ int ethnl_tsinfo_start(struct netlink_callback *cb)
 	ctx->req_info = req_info;
 	ctx->reply_data = reply_data;
 	ctx->pos_ifindex = 0;
+	ctx->single_dev = false;
+	if (req_info->base.dev) {
+		ctx->pos_ifindex = req_info->base.dev->ifindex;
+		ctx->single_dev = true;
+		ethnl_parse_header_dev_put(&req_info->base);
+		req_info->base.dev = NULL;
+	}
 	ctx->pos_phyindex = 0;
 	ctx->netdev_dump_done = false;
 	ctx->pos_phcqualifier = HWTSTAMP_PROVIDER_QUALIFIER_PRECISE;
-- 
2.43.0

Re: [PATCH] ethtool: tsinfo: release net_device reference in ethnl_tsinfo_start()

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · 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).

- [Medium] Lookup-then-lock without post-lock revalidation in the new
  single-device path of ethnl_tsinfo_dumpit()…
- [Low] New code uses the untracked, explicitly deprecated refcount pair
  dev_get_by_index()/dev_put() (net/ethtool/tsinfo.c:481 and :488)…

--

ethtool: tsinfo: release net_device reference in ethnl_tsinfo_start()

The patch stops ethnl_tsinfo_start() from holding the net_device reference
for the lifetime of the dump. It saves dev->ifindex in ctx->pos_ifindex,
sets a new ctx->single_dev flag, releases the reference immediately, and
re-looks-up the device by ifindex on each ethnl_tsinfo_dumpit() invocation.

> diff --git a/net/ethtool/tsinfo.c b/net/ethtool/tsinfo.c
> index c9b680a9cc3f..06f22a826dfa 100644
> --- a/net/ethtool/tsinfo.c
> +++ b/net/ethtool/tsinfo.c

[ ... ]

> @@ -476,12 +477,15 @@ int ethnl_tsinfo_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
>  	struct net *net = sock_net(skb->sk);
>  	int ret = 0;
>  
> -	if (ctx->req_info->base.dev) {
> -		struct net_device *dev = ctx->req_info->base.dev;
> +	if (ctx->single_dev) {
> +		struct net_device *dev = dev_get_by_index(net, ctx->pos_ifindex);
>  
> +		if (!dev)
> +			return -ENODEV;
>  		netdev_lock_ops_compat(dev);
>  		ret = ethnl_tsinfo_dump_one_net_topo(skb, dev, cb);
>  		netdev_unlock_ops_compat(dev);
> +		dev_put(dev);
>  		return ret;
>  	}

[Severity: Medium]
Is the device state re-checked anywhere after the lock is taken here?
dev_get_by_index() does a lockless RCU lookup and only takes a refcount,
and netdev_lock_ops_compat(dev) is acquired afterwards without revisiting
dev->reg_state, dev->moving_ns or dev_net(dev).

The core helper for this pattern does that re-check:

net/core/dev.c:__netdev_put_lock_ops_compat() {
	netdev_lock_ops_compat(dev);
	if (dev->reg_state > NETREG_REGISTERED ||
	    dev->moving_ns || !net_eq(dev_net(dev), net)) {
		netdev_unlock_ops_compat(dev);
		dev_put(dev);
		return NULL;
	}
	...
}

and netdev_get_by_index_lock_ops_compat() wires the lookup and that check
together:

net/core/dev.c:netdev_get_by_index_lock_ops_compat() {
	dev = dev_get_by_index(net, ifindex);
	if (!dev)
		return NULL;

	return __netdev_put_lock_ops_compat(dev, net);
}

Can this sequence run against a device that finished unregistering?

CPU0 (dump)                        CPU1 (ip link del <dev>)
dev_get_by_index()                 unregister_netdevice_many() under rtnl
                                     dev_close(), NETDEV_UNREGISTER
                                     reg_state = NETREG_UNREGISTERED
                                   netdev_run_todo() drops rtnl, then
                                     waits for the refcnt
netdev_lock_ops_compat(dev)
ethnl_tsinfo_dump_one_net_topo()

netdev_run_todo() documents that it drops rtnl before waiting:

net/core/dev.c:netdev_run_todo() {
	 * 2) Since we run with the RTNL semaphore not held, we can sleep
	 *    safely in order to wait for the netdev refcnt to drop to zero.
}

So after the lock is finally acquired, would ethnl_tsinfo_dump_one_netdev()
call dev->ethtool_ops->get_ts_info() and walk dev->link_topo->phys on a
device whose driver has already been stopped?  The same window also covers
"ip link set <dev> netns ...", where dev_net(dev) != net by the time the
lock is held - would that emit the ifindex, name and PHC capabilities of a
device that now lives in another namespace to the requesting socket?

The all-devices branch below is not exposed to this because
for_each_netdev_lock_ops_compat_scoped() goes through
__netdev_put_lock_ops_compat().  net/ethtool/rss.c ethnl_rss_dumpit()
handles the single-device case with the same revalidating iterator plus a
match_ifindex filter:

net/ethtool/rss.c:ethnl_rss_dumpit() {
	for_each_netdev_lock_ops_compat_scoped(net, dev, ctx->ifindex) {
		if (ctx->match_ifindex && ctx->match_ifindex != ctx->ifindex)
			break;
	...
}

Would switching these three statements to
netdev_get_by_index_lock_ops_compat() (dropping the trailing dev_put(),
since that helper consumes the reference), or following the rss.c filtered
iterator, be preferable here?

[Severity: Low]
This isn't a bug, but the get/put pair added here is the untracked one.
net/core/dev.c carries this comment directly above dev_get_by_index():

	/* Deprecated for new users, call netdev_get_by_index() instead */
	struct net_device *dev_get_by_index(struct net *net, int ifindex)

The rest of net/ethtool uses the tracked API, including the peer
implementation of this exact "save ifindex in ->start(), re-acquire in
->dumpit()" pattern in ethnl_perphy_dumpit():

net/ethtool/netlink.c:ethnl_perphy_dumpit() {
		netdevice_tracker dev_tracker;
		struct net_device *dev;

		dev = netdev_get_by_index(genl_info_net(&info->info),
					  ctx->ifindex, &dev_tracker,
					  GFP_KERNEL);
	...
}

and ethnl_parse_header_dev_get()/ethnl_parse_header_dev_put(), which this
patch calls.  With CONFIG_NET_DEV_REFCNT_TRACKER=y an untracked reference is
the one that cannot be attributed when "unregister_netdevice: waiting for
<dev> to become free" is reported, which is the failure this patch is
addressing.  Could netdev_get_by_index(net, ifindex, &tracker, GFP_KERNEL)
and netdev_put(dev, &tracker) be used instead?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260919204803.2811673-1-benquike%40gmail.com

Re: [PATCH] ethtool: tsinfo: release net_device reference in ethnl_tsinfo_start()

[Jakub Kicinski]

On Sat, 19 Sep 2026 20:48:03 +0000 Hui Peng wrote:
> -	if (ctx->req_info->base.dev) {
> -		struct net_device *dev = ctx->req_info->base.dev;
> +	if (ctx->single_dev) {
> +		struct net_device *dev = dev_get_by_index(net, ctx->pos_ifindex);
>  
> +		if (!dev)
> +			return -ENODEV;
>  		netdev_lock_ops_compat(dev);
>  		ret = ethnl_tsinfo_dump_one_net_topo(skb, dev, cb);
>  		netdev_unlock_ops_compat(dev);
> +		dev_put(dev);
>  		return ret;

This issue gotta be theoretical, can you build a repro that actually
triggers multi-skb dump for a single netdev? We don't have that much
info, it's gonna fit.

If you can trigger pls update the commit msg, if you can't let's instead
make this "official" by adding:

diff --git a/net/ethtool/tsinfo.c b/net/ethtool/tsinfo.c
index c9b680a9cc3f..cbb5d6b7f3e3 100644
--- a/net/ethtool/tsinfo.c
+++ b/net/ethtool/tsinfo.c
@@ -482,6 +482,7 @@ int ethnl_tsinfo_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
                netdev_lock_ops_compat(dev);
                ret = ethnl_tsinfo_dump_one_net_topo(skb, dev, cb);
                netdev_unlock_ops_compat(dev);
+               /* We don't want to hold the device ref across messages */
+               WARN_ON_ONCE(ret == -EMSGSIZE);
                return ret;
        }