[PATCH net v4 0/3] netconsole: fix target enable race and configfs read locking

[PATCH net v4 0/3] netconsole: fix target enable race and configfs read locking

[Gustavo Luiz Duarte]

While looking into the sashiko report about local_ip_show() and
remote_ip_show() missing proper locking [1], I noticed a couple other
places that need locking:

Patch 1 fixes a null-ptr-deref when enabling a target races against a
NETDEV_UNREGISTER event.

Patch 2 fixes a use-after-free when the local_mac_show() callback
races against a NETDEV_UNREGISTER event.

Patch 3 fixes yet another race, in netcons_netpoll_setup(), where
netconsole could miss the NETDEV_UNREGISTER notification and keep
holding a reference to an unregistered device.

[1] https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260814-netcons_ipv6-v3-7-bc0915e8c75f@gmail.com

Signed-off-by: Gustavo Luiz Duarte <gustavold@gmail.com>
---
Changes in v4:
- Add patch 3: netconsole: fix target enable race and configfs read locking.
- Link to v3: https://patch.msgid.link/20261012-netcons-fixes-v3-0-1aac85ed7665@gmail.com

Changes in v3:
- Use target_list_lock instead of RTNL in local_mac_show.
- Link to v2: https://patch.msgid.link/20261006-netcons-fixes-v2-0-ca652d55fd4a@gmail.com

Changes in v2:
- Resending for the 'net' tree.
- Link to v1: https://patch.msgid.link/20260928-netcons-fixes-v1-0-bb5ffe5e698a@gmail.com

To: Breno Leitao <leitao@debian.org>
To: Andrew Lunn <andrew+netdev@lunn.ch>
To: "David S. Miller" <davem@davemloft.net>
To: Eric Dumazet <edumazet@kernel.org>
To: Jakub Kicinski <kuba@kernel.org>
To: Paolo Abeni <pabeni@redhat.com>
To: Stephen Hemminger <stephen@networkplumber.org>
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

---
Gustavo Luiz Duarte (3):
      netconsole: finish enabling the target before releasing RTNL
      netconsole: read np.dev under target_list_lock in local_mac_show()
      netconsole: recheck device registration after waiting for carrier

 drivers/net/netconsole.c | 40 +++++++++++++++++++++++++++++++---------
 1 file changed, 31 insertions(+), 9 deletions(-)
---
base-commit: abeacca61eea4f6803df4a595b656ea45c2d3249
change-id: 20260925-netcons-fixes-93a3e14326b5

Best regards,
--  
Gustavo Luiz Duarte <gustavold@gmail.com>

[PATCH net v4 1/3] netconsole: finish enabling the target before releasing RTNL

[Gustavo Luiz Duarte]

netcons_netpoll_setup() publishes nt->np.dev inside __netpoll_setup(),
then releases the RTNL and waits for an RCU grace period before
returning. Only after that does its caller store STATE_ENABLED.

A NETDEV_UNREGISTER event landing in that window tears the target down
and we end up storing STATE_ENABLED with a NULL np->dev, leading to a
null-ptr-deref in netconsole_write():

  BUG: KASAN: null-ptr-deref in netconsole_write+0xb9/0x7f0
  Read of size 8 at addr 00000000000000a8 by task pr/netcon_ext0/135
   netconsole_write+0xb9/0x7f0
   nbcon_emit_next_record+0x501/0x550
   nbcon_emit_one+0x10e/0x170
   nbcon_kthread_func+0x2ff/0x3b0
   kthread+0x199/0x1e0

Store STATE_ENABLED before dropping the RTNL lock to avoid racing with
netconsole_netdev_event().

Fixes: 2382b15bcc39 ("netconsole: take care of NETDEV_UNREGISTER event")
Signed-off-by: Gustavo Luiz Duarte <gustavold@gmail.com>
---
 drivers/net/netconsole.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index b358e5c36735..1fb846ad07d1 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -552,13 +552,16 @@ static int netcons_netpoll_setup(struct netconsole_target *nt)
 	err = __netpoll_setup(np, ndev);
 	if (err)
 		goto put;
-	rtnl_unlock();
 
 	/* Make sure all NAPI polls which started before dev->npinfo
 	 * was visible have exited before we start calling NAPI poll.
 	 * NAPI skips locking if dev->npinfo is NULL.
+	 * Hold RTNL until enable is finished so we don't race with
+	 * netconsole_netdev_event()
 	 */
-	synchronize_rcu();
+	synchronize_net();
+	nt->state = STATE_ENABLED;
+	rtnl_unlock();
 
 	return 0;
 
@@ -589,7 +592,6 @@ static void resume_target(struct netconsole_target *nt)
 		return;
 	}
 
-	nt->state = STATE_ENABLED;
 	pr_info("network logging resumed on interface %s\n", nt->np.dev_name);
 }
 
@@ -1067,7 +1069,6 @@ static ssize_t enabled_store(struct config_item *item,
 			goto out_unlock;
 		}
 
-		nt->state = STATE_ENABLED;
 		pr_info("network logging started\n");
 	} else {	/* false */
 		/* We need to disable the netconsole before cleaning it up
@@ -2669,8 +2670,6 @@ static struct netconsole_target *alloc_param_target(char *target_config,
 			 * otherwise, keep the target in the list, but disabled.
 			 */
 			goto fail;
-	} else {
-		nt->state = STATE_ENABLED;
 	}
 	populate_configfs_item(nt, cmdline_count);
 

-- 
2.56.0

[PATCH net v4 2/3] netconsole: read np.dev under target_list_lock in local_mac_show()

[Gustavo Luiz Duarte]

local_mac_show() loads nt->np.dev and dereferences it with no lock. It
can race with the interface going away and netconsole_netdev_event()
eventually calling netdev_put() and freeing the device before we
dereference it, leading to a UAF.

The window is only a few instructions wide, so I could only reproduce it
with an artificially widened window between the load and the
dereference:

  BUG: KASAN: slab-use-after-free in local_mac_show+0x4e/0x80
  Read of size 8 at addr ffff88800e990458 by task uaf/130
   local_mac_show+0x4e/0x80
   configfs_read_iter+0x187/0x260
   vfs_read+0x453/0x5a0

  Allocated by task 116:
   alloc_netdev_mqs+0x78/0x830
   rtnl_create_link+0x53b/0x5c0

  Freed by task 121:
   kfree+0x159/0x420
   device_release+0x77/0x120
   kobject_put+0xb5/0x160
   netdev_run_todo+0x420/0x850
   rtnl_dellink+0x259/0x5b0

Two places can free np.dev: (1) netconsole_netdev_event() moves a target
out of STATE_ENABLED under target_list_lock before netdev_put(), and (2)
process_resume_target() releases np.dev under dynamic_netconsole_mutex.

Fixes: 0953864160bd ("[NETPOLL]: no need to store local_mac")
Signed-off-by: Gustavo Luiz Duarte <gustavold@gmail.com>
---
 drivers/net/netconsole.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index 1fb846ad07d1..be7eb632ee42 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -888,10 +888,23 @@ static ssize_t remote_ip_show(struct config_item *item, char *buf)
 
 static ssize_t local_mac_show(struct config_item *item, char *buf)
 {
-	struct net_device *dev = to_target(item)->np.dev;
-	static const u8 bcast[ETH_ALEN] = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff };
+	struct netconsole_target *nt = to_target(item);
+	unsigned long flags;
+	u8 mac[ETH_ALEN];
+
+	eth_broadcast_addr(mac);
+
+	dynamic_netconsole_mutex_lock();
+	/* Hold target_list_lock here so netconsole_netdev_event() doesn't tear
+	 * down np.dev from under us.
+	 */
+	spin_lock_irqsave(&target_list_lock, flags);
+	if (nt->state == STATE_ENABLED && nt->np.dev)
+		memcpy(mac, nt->np.dev->dev_addr, ETH_ALEN);
+	spin_unlock_irqrestore(&target_list_lock, flags);
+	dynamic_netconsole_mutex_unlock();
 
-	return sysfs_emit(buf, "%pM\n", dev ? dev->dev_addr : bcast);
+	return sysfs_emit(buf, "%pM\n", mac);
 }
 
 static ssize_t remote_mac_show(struct config_item *item, char *buf)

-- 
2.56.0

[PATCH net v4 3/3] netconsole: recheck device registration after waiting for carrier

[Gustavo Luiz Duarte]

netcons_netpoll_setup() drops RTNL while it waits for carrier when the
device is not yet up. If the device is unregistered in that window, the
NETDEV_UNREGISTER notifier runs before np->dev is set and has nothing
to match.

We might then end up setting up netpoll on a device that is being
unregistered while holding a reference to it, stalling
unregister_netdevice() in netdev_wait_allrefs_any().

Add a check to confirm the device is still registered once we retake
RTNL. Release the device reference and return -ENODEV otherwise.

Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261012-netcons-fixes-v3-1-1aac85ed7665@gmail.com
Signed-off-by: Gustavo Luiz Duarte <gustavold@gmail.com>
---
 drivers/net/netconsole.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index be7eb632ee42..bd36ebd334aa 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -534,6 +534,16 @@ static int netcons_netpoll_setup(struct netconsole_target *nt)
 		rtnl_unlock();
 		netcons_wait_carrier(np, ndev);
 		rtnl_lock();
+
+		/* Check that ndev hasn't been unregistered when we released
+		 * the RTNL above.
+		 */
+		if (ndev->reg_state != NETREG_REGISTERED) {
+			np_err(np, "%s was unregistered while waiting for carrier, aborting\n",
+			       netcons_egress_dev(np, buf, sizeof(buf)));
+			err = -ENODEV;
+			goto put;
+		}
 	}
 
 	if (netcons_local_ip_unset(nt)) {

-- 
2.56.0

Re: [PATCH net v4 3/3] netconsole: recheck device registration after waiting for carrier

[netdev-bot+sashiko]

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

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

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] The stall the commit message names is still reachable during
  the carrier wait.
- [Low] The patch fixes a user-visible bug but has no Fixes: tag.
- [Low] The new check in netcons_netpoll_setup()
  (drivers/net/netconsole.c:541) reads ndev->reg_state with a plain load.

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but the same
  RTNL-drop window in netcons_netpoll_setup() loses other notifier…

--

netconsole: recheck device registration after waiting for carrier

This patch adds a check in netcons_netpoll_setup(). After RTNL is retaken
following the carrier wait, it confirms that ndev is still registered. If
the device was unregistered in that window, the device reference is
released and -ENODEV is returned.

Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261012-netcons-fixes-v3-1-1aac85ed7665@gmail.com

> Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261012-netcons-fixes-v3-1-1aac85ed7665@gmail.com
> Signed-off-by: Gustavo Luiz Duarte <gustavold@gmail.com>

[Severity: Low]
Should this have a Fixes: tag? The commit message only has Closes: and
Signed-off-by:.

In netcons_netpoll_setup(), the rtnl_unlock() / netcons_wait_carrier() /
rtnl_lock() sequence blames to 672ecd3bb145 ("netconsole: take over
netpoll_setup() from netpoll"). That commit describes itself as pure code
motion.

Before that commit, the same window existed in
net/core/netpoll.c:netpoll_setup():

	rtnl_unlock();
	netpoll_wait_carrier(np, ndev);
	rtnl_lock();

It was followed by __netpoll_setup() with no recheck.

So the bug appears to be older than the netconsole.c history shows. Older
stable trees would need a backport against net/core/netpoll.c rather than
drivers/net/netconsole.c.

> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> index be7eb632ee424..bd36ebd334aa4 100644
> --- a/drivers/net/netconsole.c
> +++ b/drivers/net/netconsole.c
> @@ -534,6 +534,16 @@ static int netcons_netpoll_setup(struct netconsole_target *nt)
>  		rtnl_unlock();
>  		netcons_wait_carrier(np, ndev);

[Severity: Medium]
Can the unregister_netdevice() stall from the commit message still happen
during this wait?

netcons_netpoll_setup() takes netdev_hold(ndev, &np->dev_tracker) before it
drops RTNL. netcons_wait_carrier() only watches carrier:

drivers/net/netconsole.c:netcons_wait_carrier() {
	...
	while (!netif_carrier_ok(ndev)) {
		if (time_after(jiffies, atmost)) {
			np_notice(np, "timeout waiting for carrier\n");
			break;
		}
		msleep(1);
	}
}

Suppose the device is unregistered while this loop runs. netdev_run_todo()
then waits in netdev_wait_allrefs_any() and resends NETDEV_UNREGISTER.
netconsole can't drop its reference in response, because np->dev is still
NULL and netconsole_netdev_event() only matches on nt->np.dev == dev.

A device that was just opened and then torn down usually won't get
carrier. The loop then tends to run until carrier_timeout expires (default
4 seconds, a writable module parameter). Only after that does the new
reg_state check reach netdev_put().

Doesn't that leave unregister_netdevice() blocked for up to
carrier_timeout seconds? If that is longer than
netdev_unregister_timeout_secs, the "waiting for ... to become free"
warning is also printed.

The resend already limited the stall before this patch, so the change
seems to shorten it by about one resend interval at most.

Would it make sense to also break out of netcons_wait_carrier() when
READ_ONCE(ndev->reg_state) != NETREG_REGISTERED? The new check after
rtnl_lock() would then do the cleanup.

>  		rtnl_lock();
> +
> +		/* Check that ndev hasn't been unregistered when we released
> +		 * the RTNL above.
> +		 */
> +		if (ndev->reg_state != NETREG_REGISTERED) {

[Severity: Low]
Should this be READ_ONCE(ndev->reg_state)?

RTNL doesn't serialize this read against every writer. netdev_run_todo()
drops RTNL before it writes the final state:

net/core/dev.c:netdev_run_todo() {
	...
	__rtnl_unlock();
	...
		netdev_lock(dev);
		WRITE_ONCE(dev->reg_state, NETREG_UNREGISTERED);
		netdev_unlock(dev);
	...
}

This means netconsole can hold RTNL and read reg_state while that
WRITE_ONCE() runs. A plain read racing a marked write is a data race that
KCSAN can report.

The behavior is the same either way, since both NETREG_UNREGISTERING and
NETREG_UNREGISTERED fail the check. Other lockless readers in dev.c use
READ_ONCE() here. process_resume_target() has the same plain read.

> +			np_err(np, "%s was unregistered while waiting for carrier, aborting\n",
> +			       netcons_egress_dev(np, buf, sizeof(buf)));
> +			err = -ENODEV;
> +			goto put;
> +		}
>  	}

[Severity: Medium]
This isn't a bug introduced by this patch, but the same RTNL-drop window
also loses other notifier events, and the reg_state check doesn't catch
them.

np->dev stays NULL until __netpoll_setup(). Any NETDEV_UNREGISTER,
NETDEV_JOIN or NETDEV_CHANGENAME sent during netcons_wait_carrier() is
therefore ignored by netconsole_netdev_event():

	if (nt->np.dev == dev) {

Two cases seem to get past the new check.

The first is a netns move. __dev_change_net_namespace() calls netif_close(),
unlist_netdevice() and call_netdevice_notifiers(NETDEV_UNREGISTER, dev).
Its comment says "dev->reg_state stays at NETREG_REGISTERED". If local_ip
was set explicitly, __netpoll_setup() then binds an enabled target to a
device that now lives in another netns.

The second is enslavement. bond_enslave() and br_add_if() send NETDEV_JOIN,
and that event is lost too. The netdev_master_upper_dev_get() check is not
run again after rtnl_lock(), and __netpoll_setup() only checks
IFF_DISABLE_NETPOLL. netconsole can then end up on a slave device, which the
-EBUSY check is meant to prevent.

process_resume_target() has the same reg_state-only recheck.

Would it be worth also checking net_eq(dev_net(ndev), net) and
!netdev_master_upper_dev_get(ndev) after RTNL is retaken?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261021-netcons-fixes-v4-0-3857120c6340%40gmail.com