[PATCH net-next 0/4] netconsole: fix target enable race and configfs read locking

[PATCH net-next 0/4] 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] (fixed in patch 3/4), I
noticed a couple other places that need locking: patch 1/4 fixes a
null-ptr-deref when enabling a target races against a NETDEV_UNREGISTER
event, and patch 2/4 fixes a use-after-free when the local_mac_show()
callback races against a NETDEV_UNREGISTER event. Patch 4/4 is mostly
cosmetic but worth fixing.

[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>
---
Gustavo Luiz Duarte (4):
      netconsole: finish enabling the target before releasing RTNL
      netconsole: read np.dev under the RTNL in local_mac_show()
      netconsole: avoid printing partially updated target attributes
      netconsole: remove unnecessary target refcounting from the netdev notifier

 drivers/net/netconsole.c | 110 ++++++++++++++++++++++++++---------------------
 1 file changed, 60 insertions(+), 50 deletions(-)
---
base-commit: 014d795c73837ea2339a4ea8e8f82c6e959b845d
change-id: 20260925-netcons-fixes-93a3e14326b5

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

[PATCH net-next 1/4] 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 267254f046de..7dde00eb8e98 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);
 }
 
@@ -1079,7 +1081,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
@@ -2667,8 +2668,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.55.0

Re: [PATCH net-next 1/4] netconsole: finish enabling the target before releasing RTNL

[Breno Leitao]

On Mon, Sep 28, 2026 at 07:00:03PM +0100, Gustavo Luiz Duarte wrote:
> 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

Oh gosh. Thanks for hte fix.

> Store STATE_ENABLED before dropping the RTNL lock to avoid racing with
> netconsole_netdev_event().
> 
> Fixes: 2382b15bcc39 ("netconsole: take care of NETDEV_UNREGISTER event")

This should go to `net` instead of netdev.
>
> @@ -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();

Why do you need to synchornize-rcu with the RTNL held? Why not enabling
nt->state, releasing the lock and than synchronizing RCU?

--breno

Re: [PATCH net-next 1/4] netconsole: finish enabling the target before releasing RTNL

[Gustavo Luiz Duarte]

On Thu, Oct 1, 2026 at 4:03 PM Breno Leitao <leitao@debian.org> wrote:
>
> On Mon, Sep 28, 2026 at 07:00:03PM +0100, Gustavo Luiz Duarte wrote:
> > 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
>
> Oh gosh. Thanks for hte fix.
>
> > Store STATE_ENABLED before dropping the RTNL lock to avoid racing with
> > netconsole_netdev_event().
> >
> > Fixes: 2382b15bcc39 ("netconsole: take care of NETDEV_UNREGISTER event")
>
> This should go to `net` instead of netdev.

I was unsure whether to target net or net-next because these bugs
require root to be triggered.
I can resend it targeting net. I assume the same applies to patch 2/4,
so I will split 1,2 -> net and 3,4 -> net-next.

> >
> > @@ -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();
>
> Why do you need to synchornize-rcu with the RTNL held? Why not enabling
> nt->state, releasing the lock and than synchronizing RCU?

My understanding from the comment above is that we need to synchronize
RCU before allowing netconsole to start calling NAPI poll.
And (nt->state != STATE_ENABLED) is what is currently blocking
netconsole_write() from calling NAPI poll.
So if we enable nt->state before synchronizing RCU, it would regress
the hang fixed in [1].

[1] https://patch.msgid.link/20250726010846.1105875-1-kuba@kernel.org

[PATCH net-next 2/4] netconsole: read np.dev under the RTNL 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

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

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index 7dde00eb8e98..5f4311726d11 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -900,10 +900,17 @@ 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);
+	int ret;
 
-	return sysfs_emit(buf, "%pM\n", dev ? dev->dev_addr : bcast);
+	/* Hold RTNL here so netconsole_netdev_event() doesn't tear down np.dev
+	 * from under us.
+	 */
+	rtnl_lock();
+	ret = sysfs_emit(buf, "%pM\n", nt->np.dev ? nt->np.dev->dev_addr : bcast);
+	rtnl_unlock();
+	return ret;
 }
 
 static ssize_t remote_mac_show(struct config_item *item, char *buf)

-- 
2.55.0

[PATCH net-next 3/4] netconsole: avoid printing partially updated target attributes

[Gustavo Luiz Duarte]

The configfs store callbacks all serialize on dynamic_netconsole_mutex
but not on the read side, so reading an attribute while it is being
written returns a partially updated value.

Hold dynamic_netconsole_mutex on *_show() callbacks to avoid racing with
writers.

Reported-by: Sashiko <netdev-bot+sashiko@kernel.org>
Closes: 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>
---
 drivers/net/netconsole.c | 57 ++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 46 insertions(+), 11 deletions(-)

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index 5f4311726d11..627475f16383 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -861,7 +861,14 @@ static ssize_t release_show(struct config_item *item, char *buf)
 
 static ssize_t dev_name_show(struct config_item *item, char *buf)
 {
-	return sysfs_emit(buf, "%s\n", to_target(item)->np.dev_name);
+	struct netconsole_target *nt = to_target(item);
+	int ret;
+
+	dynamic_netconsole_mutex_lock();
+	ret = sysfs_emit(buf, "%s\n", nt->np.dev_name);
+	dynamic_netconsole_mutex_unlock();
+
+	return ret;
 }
 
 static ssize_t local_port_show(struct config_item *item, char *buf)
@@ -877,25 +884,39 @@ static ssize_t remote_port_show(struct config_item *item, char *buf)
 static ssize_t local_ip_show(struct config_item *item, char *buf)
 {
 	struct netconsole_target *nt = to_target(item);
+	int ret;
+
+	dynamic_netconsole_mutex_lock();
 
 	if (nt->local_ip.family == AF_UNSPEC)
-		return sysfs_emit(buf, "\n");
-	if (nt->local_ip.family == AF_INET6)
-		return sysfs_emit(buf, "%pI6c\n", &nt->local_ip.in6);
+		ret = sysfs_emit(buf, "\n");
+	else if (nt->local_ip.family == AF_INET6)
+		ret = sysfs_emit(buf, "%pI6c\n", &nt->local_ip.in6);
 	else
-		return sysfs_emit(buf, "%pI4\n", &nt->local_ip.ip);
+		ret = sysfs_emit(buf, "%pI4\n", &nt->local_ip.ip);
+
+	dynamic_netconsole_mutex_unlock();
+
+	return ret;
 }
 
 static ssize_t remote_ip_show(struct config_item *item, char *buf)
 {
 	struct netconsole_target *nt = to_target(item);
+	int ret;
+
+	dynamic_netconsole_mutex_lock();
 
 	if (nt->remote_ip.family == AF_UNSPEC)
-		return sysfs_emit(buf, "\n");
-	if (nt->remote_ip.family == AF_INET6)
-		return sysfs_emit(buf, "%pI6c\n", &nt->remote_ip.in6);
+		ret = sysfs_emit(buf, "\n");
+	else if (nt->remote_ip.family == AF_INET6)
+		ret = sysfs_emit(buf, "%pI6c\n", &nt->remote_ip.in6);
 	else
-		return sysfs_emit(buf, "%pI4\n", &nt->remote_ip.ip);
+		ret = sysfs_emit(buf, "%pI4\n", &nt->remote_ip.ip);
+
+	dynamic_netconsole_mutex_unlock();
+
+	return ret;
 }
 
 static ssize_t local_mac_show(struct config_item *item, char *buf)
@@ -915,7 +936,14 @@ static ssize_t local_mac_show(struct config_item *item, char *buf)
 
 static ssize_t remote_mac_show(struct config_item *item, char *buf)
 {
-	return sysfs_emit(buf, "%pM\n", to_target(item)->remote_mac);
+	struct netconsole_target *nt = to_target(item);
+	int ret;
+
+	dynamic_netconsole_mutex_lock();
+	ret = sysfs_emit(buf, "%pM\n", nt->remote_mac);
+	dynamic_netconsole_mutex_unlock();
+
+	return ret;
 }
 
 static ssize_t transmit_errors_show(struct config_item *item, char *buf)
@@ -1350,7 +1378,14 @@ static struct netconsole_target *userdata_to_target(struct userdata *ud)
 
 static ssize_t userdatum_value_show(struct config_item *item, char *buf)
 {
-	return sysfs_emit(buf, "%s\n", &(to_userdatum(item)->value[0]));
+	struct userdatum *udm = to_userdatum(item);
+	int ret;
+
+	dynamic_netconsole_mutex_lock();
+	ret = sysfs_emit(buf, "%s\n", udm->value);
+	dynamic_netconsole_mutex_unlock();
+
+	return ret;
 }
 
 /* Navigate configfs and calculate the lentgh of the formatted string

-- 
2.55.0

Re: [PATCH net-next 3/4] netconsole: avoid printing partially updated target attributes

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 0 · 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] The fix is incomplete for dev_name.
- [Low] The commit 'netconsole: avoid printing partially updated target
  attributes' fixes a user-visible race: configfs *_show() callbacks can…

--

netconsole: avoid printing partially updated target attributes

This patch takes dynamic_netconsole_mutex in the configfs show callbacks
for dev_name, local_ip, remote_ip, remote_mac and the userdatum value.
Readers are then serialized against the store callbacks and should no
longer see partially updated values.

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

> Reported-by: Sashiko <netdev-bot+sashiko@kernel.org>
> Closes: 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>

[Severity: Low]
Should this have a Fixes: tag?

These show callbacks have read the fields without dynamic_netconsole_mutex
for a long time. dev_name_show() and local_ip_show() date back to
0bcc1816188e ("[NET] netconsole: Support dynamic reconfiguration using
configfs").

The IPv6 rework (for example 56ddc8d48e75f, 46b3f9e7e761d, f6e96d72f64a8)
made the window larger by adding the separate family field.

The other two fixes in this series, "netconsole: finish enabling the
target before releasing RTNL" and "netconsole: read np.dev under the RTNL
in local_mac_show()", both carry Fixes: tags.

> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> index 5f4311726d11..627475f16383 100644
> --- a/drivers/net/netconsole.c
> +++ b/drivers/net/netconsole.c
> @@ -861,7 +861,14 @@ static ssize_t release_show(struct config_item *item, char *buf)
>  
>  static ssize_t dev_name_show(struct config_item *item, char *buf)
>  {
> -	return sysfs_emit(buf, "%s\n", to_target(item)->np.dev_name);
> +	struct netconsole_target *nt = to_target(item);
> +	int ret;
> +
> +	dynamic_netconsole_mutex_lock();
> +	ret = sysfs_emit(buf, "%s\n", nt->np.dev_name);
> +	dynamic_netconsole_mutex_unlock();

[Severity: High]
Does this fully close the torn read for dev_name?

dynamic_netconsole_mutex only excludes dev_name_store(). The netdev
notifier path has two other writers of np.dev_name, and neither of them
takes this mutex.

For an enabled target, the NETDEV_CHANGENAME case rewrites the name in
place. It holds RTNL, target_cleanup_list_lock and target_list_lock:

drivers/net/netconsole.c:netconsole_netdev_event() {
    ...
		if (nt->np.dev == dev) {
			switch (event) {
			case NETDEV_CHANGENAME:
				strscpy(nt->np.dev_name, dev->name, IFNAMSIZ);
				break;
    ...
}

For MAC-bound targets on unregister, release or join, the notifier calls
netconsole_process_cleanups_core()->netcons_release_dev(). That zeroes the
name with only RTNL and target_cleanup_list_lock held:

drivers/net/netconsole.c:netcons_release_dev() {
	do_netpoll_cleanup(&nt->np);
	if (bound_by_mac(nt))
		memset(&nt->np.dev_name, 0, IFNAMSIZ);
}

dev_name_store() refuses to write while the target is enabled, so it
never overlaps these writers. dev_name_show() has no such state check.

So a read of /sys/kernel/config/netconsole/<target>/dev_name can still
race with "ip link set eth0 name ..." or with deleting the device. The
read could return a mix of old and new bytes, or a partly zeroed name.
The buffer stays NUL terminated within IFNAMSIZ, so the result is wrong
output, not an out of bounds read.

This looks the same at the tip of the series. The other fields this patch
touches (local_ip, remote_ip, remote_mac and the userdatum value) have no
writers outside the mutex, so the gap only affects dev_name.

local_mac_show() in this same series already takes rtnl_lock() to
serialize with this notifier. Could dev_name_show() also take RTNL inside
the mutex? That would follow the existing dynamic_netconsole_mutex -> RTNL
order used by enabled_store().

> +
> +	return ret;
>  }

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928-netcons-fixes-v1-0-bb5ffe5e698a%40gmail.com

Re: [PATCH net-next 3/4] netconsole: avoid printing partially updated target attributes

[Gustavo Luiz Duarte]

On Thu, Oct 1, 2026 at 10:00 AM <netdev-bot+sashiko@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
>
> Critical: 0 · High: 1 · Medium: 0 · 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] The fix is incomplete for dev_name.
> - [Low] The commit 'netconsole: avoid printing partially updated target
>   attributes' fixes a user-visible race: configfs *_show() callbacks can…
>
> --
>
> netconsole: avoid printing partially updated target attributes
>
> This patch takes dynamic_netconsole_mutex in the configfs show callbacks
> for dev_name, local_ip, remote_ip, remote_mac and the userdatum value.
> Readers are then serialized against the store callbacks and should no
> longer see partially updated values.
>
> Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260814-netcons_ipv6-v3-7-bc0915e8c75f@gmail.com
>
> > Reported-by: Sashiko <netdev-bot+sashiko@kernel.org>
> > Closes: 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>
>
> [Severity: Low]
> Should this have a Fixes: tag?
>
> These show callbacks have read the fields without dynamic_netconsole_mutex
> for a long time. dev_name_show() and local_ip_show() date back to
> 0bcc1816188e ("[NET] netconsole: Support dynamic reconfiguration using
> configfs").
>
> The IPv6 rework (for example 56ddc8d48e75f, 46b3f9e7e761d, f6e96d72f64a8)
> made the window larger by adding the separate family field.
>
> The other two fixes in this series, "netconsole: finish enabling the
> target before releasing RTNL" and "netconsole: read np.dev under the RTNL
> in local_mac_show()", both carry Fixes: tags.
>
> > diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> > index 5f4311726d11..627475f16383 100644
> > --- a/drivers/net/netconsole.c
> > +++ b/drivers/net/netconsole.c
> > @@ -861,7 +861,14 @@ static ssize_t release_show(struct config_item *item, char *buf)
> >
> >  static ssize_t dev_name_show(struct config_item *item, char *buf)
> >  {
> > -     return sysfs_emit(buf, "%s\n", to_target(item)->np.dev_name);
> > +     struct netconsole_target *nt = to_target(item);
> > +     int ret;
> > +
> > +     dynamic_netconsole_mutex_lock();
> > +     ret = sysfs_emit(buf, "%s\n", nt->np.dev_name);
> > +     dynamic_netconsole_mutex_unlock();
>
> [Severity: High]
> Does this fully close the torn read for dev_name?
>
> dynamic_netconsole_mutex only excludes dev_name_store(). The netdev
> notifier path has two other writers of np.dev_name, and neither of them
> takes this mutex.
>
> For an enabled target, the NETDEV_CHANGENAME case rewrites the name in
> place. It holds RTNL, target_cleanup_list_lock and target_list_lock:
>
> drivers/net/netconsole.c:netconsole_netdev_event() {
>     ...
>                 if (nt->np.dev == dev) {
>                         switch (event) {
>                         case NETDEV_CHANGENAME:
>                                 strscpy(nt->np.dev_name, dev->name, IFNAMSIZ);
>                                 break;
>     ...
> }
>
> For MAC-bound targets on unregister, release or join, the notifier calls
> netconsole_process_cleanups_core()->netcons_release_dev(). That zeroes the
> name with only RTNL and target_cleanup_list_lock held:
>
> drivers/net/netconsole.c:netcons_release_dev() {
>         do_netpoll_cleanup(&nt->np);
>         if (bound_by_mac(nt))
>                 memset(&nt->np.dev_name, 0, IFNAMSIZ);
> }
>
> dev_name_store() refuses to write while the target is enabled, so it
> never overlaps these writers. dev_name_show() has no such state check.
>
> So a read of /sys/kernel/config/netconsole/<target>/dev_name can still
> race with "ip link set eth0 name ..." or with deleting the device. The
> read could return a mix of old and new bytes, or a partly zeroed name.
> The buffer stays NUL terminated within IFNAMSIZ, so the result is wrong
> output, not an out of bounds read.
>
> This looks the same at the tip of the series. The other fields this patch
> touches (local_ip, remote_ip, remote_mac and the userdatum value) have no
> writers outside the mutex, so the gap only affects dev_name.
>
> local_mac_show() in this same series already takes rtnl_lock() to
> serialize with this notifier. Could dev_name_show() also take RTNL inside
> the mutex? That would follow the existing dynamic_netconsole_mutex -> RTNL
> order used by enabled_store().

This sounds correct. In dev_name_show() we need an rtnl_lock() in
addition to the dynamic_netconsole_mutex_lock(), otherwise we would
race with netconsole_netdev_event(). I will fix this in v2.

pw-bot: cr

[PATCH net-next 4/4] netconsole: remove unnecessary target refcounting from the netdev notifier

[Gustavo Luiz Duarte]

Since netconsole_netdev_event() holds target_list_lock, there is no need
to also protect each target with netconsole_target_get/put()
refcounting. There is no way a target reachable from the target_list
would go away while we hold target_list_lock.

This is similar to commit a6d403ac9689 ("netconsole: remove unnecessary
netconsole_target_get/out() from write_msg()").

Since this is the only remaining user of netconsole_target_get/put(),
remove those helpers as well.

Signed-off-by: Gustavo Luiz Duarte <gustavold@gmail.com>
---
 drivers/net/netconsole.c | 31 -------------------------------
 1 file changed, 31 deletions(-)

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index 627475f16383..6c938e37b2f3 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -262,23 +262,6 @@ static void __exit dynamic_netconsole_exit(void)
 	configfs_unregister_subsystem(&netconsole_subsys);
 }
 
-/*
- * Targets that were created by parsing the boot/module option string
- * do not exist in the configfs hierarchy (and have NULL names) and will
- * never go away, so make these a no-op for them.
- */
-static void netconsole_target_get(struct netconsole_target *nt)
-{
-	if (config_item_name(&nt->group.cg_item))
-		config_group_get(&nt->group);
-}
-
-static void netconsole_target_put(struct netconsole_target *nt)
-{
-	if (config_item_name(&nt->group.cg_item))
-		config_group_put(&nt->group);
-}
-
 static void dynamic_netconsole_mutex_lock(void)
 {
 	mutex_lock(&dynamic_netconsole_mutex);
@@ -300,18 +283,6 @@ static void __exit dynamic_netconsole_exit(void)
 {
 }
 
-/*
- * No danger of targets going away from under us when dynamic
- * reconfigurability is off.
- */
-static void netconsole_target_get(struct netconsole_target *nt)
-{
-}
-
-static void netconsole_target_put(struct netconsole_target *nt)
-{
-}
-
 static void populate_configfs_item(struct netconsole_target *nt,
 				   int cmdline_count)
 {
@@ -1982,7 +1953,6 @@ static int netconsole_netdev_event(struct notifier_block *this,
 	mutex_lock(&target_cleanup_list_lock);
 	spin_lock_irqsave(&target_list_lock, flags);
 	list_for_each_entry_safe(nt, tmp, &target_list, list) {
-		netconsole_target_get(nt);
 		if (nt->np.dev == dev) {
 			switch (event) {
 			case NETDEV_CHANGENAME:
@@ -2012,7 +1982,6 @@ static int netconsole_netdev_event(struct notifier_block *this,
 			 * notifier.
 			 */
 			queue_work(netconsole_wq, &nt->resume_wq);
-		netconsole_target_put(nt);
 	}
 	spin_unlock_irqrestore(&target_list_lock, flags);
 	mutex_unlock(&target_cleanup_list_lock);

-- 
2.55.0