[PATCH net-next v2 0/2] netconsole: add locking to configfs _show callbacks

[PATCH net-next v2 0/2] netconsole: add locking to configfs _show callbacks

[Gustavo Luiz Duarte]

Patch 1 fixes a couple of reports from sashiko by adding proper locking
to configs read callbacks to avoid printing partially updated data.
Patch 2 is mostly cosmetic but worth fixing.

Signed-off-by: Gustavo Luiz Duarte <gustavold@gmail.com>
---
Changes in v2:
- Use target_list_lock instead of RTNL in dev_name_show().
- Link to v1: https://patch.msgid.link/20261006-netcons-fixes-next-v1-0-231cd26f8c51@gmail.com

---
Gustavo Luiz Duarte (2):
      netconsole: avoid printing partially updated target attributes
      netconsole: remove unnecessary target refcounting from the netdev notifier

 drivers/net/netconsole.c | 94 ++++++++++++++++++++++++++----------------------
 1 file changed, 52 insertions(+), 42 deletions(-)
---
base-commit: 29f144016f03608abfddcd6ebaeabafd7aeaadd2
change-id: 20261005-netcons-fixes-next-af0db028f570

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

[PATCH net-next v2 1/2] 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.

The dev_name_show() callback can also race with
netconsole_netdev_event() writing to np.dev_name due to
NETDEV_CHANGENAME. So it needs to hold target_list_lock in addition to
dynamic_netconsole_mutex.

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
Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928-netcons-fixes-v1-0-bb5ffe5e698a%40gmail.com
Signed-off-by: Gustavo Luiz Duarte <gustavold@gmail.com>
---
 drivers/net/netconsole.c | 63 +++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 52 insertions(+), 11 deletions(-)

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index 267254f046de..953a1da07186 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -859,7 +859,20 @@ 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);
+	unsigned long flags;
+	int ret;
+
+	dynamic_netconsole_mutex_lock();
+	/* Hold target_list_lock to prevent racing against
+	 * netconsole_netdev_event() changing np.dev_name.
+	 */
+	spin_lock_irqsave(&target_list_lock, flags);
+	ret = sysfs_emit(buf, "%s\n", nt->np.dev_name);
+	spin_unlock_irqrestore(&target_list_lock, flags);
+	dynamic_netconsole_mutex_unlock();
+
+	return ret;
 }
 
 static ssize_t local_port_show(struct config_item *item, char *buf)
@@ -875,25 +888,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)
@@ -906,7 +933,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)
@@ -1342,7 +1376,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.56.0

Re: [PATCH net-next v2 1/2] netconsole: avoid printing partially updated target attributes

[netdev-bot+sashiko]

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

Critical: 0 · High: 2 · 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] In drivers/net/netconsole.c, local_mac_show() still takes no
  lock.
- [High] dev_name_show() still races with netconsole_netdev_event()
  writing np.dev_name, even though the commit message says this race is…
- [Low] The commit fixes a user-visible race: configfs *_show() callbacks
  can return torn values while a store or the netdev notifier updates…

--

netconsole: avoid printing partially updated target attributes

This patch makes the netconsole configfs *_show() callbacks take
dynamic_netconsole_mutex, so they no longer return values that a
concurrent store has only partly updated. dev_name_show() also takes
target_list_lock to serialize against the NETDEV_CHANGENAME update in
netconsole_netdev_event().

> 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
> Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928-netcons-fixes-v1-0-bb5ffe5e698a%40gmail.com
> Signed-off-by: Gustavo Luiz Duarte <gustavold@gmail.com>

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

The unlocked readers in dev_name_show(), remote_mac_show() and
local_mac_show() date back to commit 0bcc1816188e ("[NET] netconsole:
Support dynamic reconfiguration using configfs").

The family/address tearing in local_ip_show() and remote_ip_show() comes
from the more recent inet_addr/family rework (commits 46b3f9e7e761,
56ddc8d48e75 and f6e96d72f64a).

> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> index 267254f046de9..953a1da071862 100644
> --- a/drivers/net/netconsole.c
> +++ b/drivers/net/netconsole.c
> @@ -859,7 +859,20 @@ 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);
> +	unsigned long flags;
> +	int ret;
> +
> +	dynamic_netconsole_mutex_lock();
> +	/* Hold target_list_lock to prevent racing against
> +	 * netconsole_netdev_event() changing np.dev_name.
> +	 */
> +	spin_lock_irqsave(&target_list_lock, flags);
> +	ret = sysfs_emit(buf, "%s\n", nt->np.dev_name);
> +	spin_unlock_irqrestore(&target_list_lock, flags);

[Severity: High]
Does target_list_lock cover every place where netconsole_netdev_event()
changes np.dev_name?

It covers the NETDEV_CHANGENAME strscpy(). For NETDEV_UNREGISTER,
NETDEV_RELEASE and NETDEV_JOIN, though, the notifier drops
target_list_lock before it cleans up the target:

netconsole_netdev_event()
    spin_unlock_irqrestore(&target_list_lock, flags);
    mutex_unlock(&target_cleanup_list_lock);
    ...
    netconsole_process_cleanups_core()
        mutex_lock(&target_cleanup_list_lock);
        netcons_release_dev(nt)
            do_netpoll_cleanup(&nt->np);
            if (bound_by_mac(nt))
                memset(&nt->np.dev_name, 0, IFNAMSIZ);
        spin_lock_irqsave(&target_list_lock, flags);
        list_move(&nt->list, &target_list);

The memset in netcons_release_dev() runs with only RTNL and
target_cleanup_list_lock held. Neither dynamic_netconsole_mutex nor
target_list_lock is held, so the new dev_name_show() has no lock in
common with this writer.

MAC-bound targets do show up in configfs. A netconsole= cmdline target
bound by MAC is exposed through populate_configfs_item(), and
netpoll_setup() fills np.dev_name from ndev->name.

For such a target, can the sysfs_emit() here still copy a partly zeroed
name?

Two options that look possible: take target_cleanup_list_lock in
dev_name_show(), following the dynamic_netconsole_mutex ->
target_cleanup_list_lock -> target_list_lock order that
drop_netconsole_target() already uses. Or do the memset in
netcons_release_dev() under target_list_lock.

> +	dynamic_netconsole_mutex_unlock();
> +
> +	return ret;
>  }
>  

[ ... ]

> @@ -906,7 +933,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;
>  }

[Severity: High]
This patch leaves local_mac_show(), just above this hunk, without any
locking:

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 };

	return sysfs_emit(buf, "%pM\n", dev ? dev->dev_addr : bcast);
}

It loads np.dev with no lock, no RCU read section and no netdev
reference, then dereferences dev->dev_addr.

enabled_store() sets and clears np.dev through netcons_netpoll_setup()
and netconsole_process_cleanups(). process_resume_target() sets and
clears it too. Both run under dynamic_netconsole_mutex.

The notifier also clears np.dev, without that mutex:

netconsole_netdev_event()
    netconsole_process_cleanups_core()
        netcons_release_dev()
            do_netpoll_cleanup()
                netdev_put(np->dev, &np->dev_tracker);
                np->dev = NULL;

If netconsole holds the last reference on a device that is being
unregistered, netdev_run_todo() can free the net_device after this put.

Can a reader that already loaded dev then read dev->dev_addr from freed
memory?

The unlocked read dates back to the original configfs support in commit
0bcc1816188e. This patch is meant to make the *_show() callbacks safe
against concurrent writers, though. Should local_mac_show() be handled
here as well?

Taking dynamic_netconsole_mutex alone would cover enabled_store() and
process_resume_target(), but not the notifier path. Would the device
need to be pinned some other way? RTNL, RCU, or a netdev reference
taken under a lock the notifier also holds are possible options.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261012-netcons-fixes-next-v2-0-ac8ec4c62edb%40gmail.com

[PATCH net-next v2 2/2] 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 953a1da07186..e40ef6c4f761 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)
 {
@@ -1980,7 +1951,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:
@@ -2010,7 +1980,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.56.0

Re: [PATCH net-next v2 0/2] netconsole: add locking to configfs _show callbacks

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.