Re: [PATCH net-next 1/2] 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 patch is meant to stop *_show() callbacks from racing with
  writers, but local_mac_show() in drivers/net/netconsole.c is still…
- [Low] The commit fixes a user-visible race: reading local_ip,
  remote_ip, dev_name, remote_mac or userdatum value while it is being…

--

netconsole: avoid printing partially updated target attributes

This commit takes dynamic_netconsole_mutex in several configfs *_show()
callbacks so that reads are serialized against the store callbacks. It
also takes RTNL in dev_name_show() so the read cannot race with
netconsole_netdev_event() updating np.dev_name on NETDEV_CHANGENAME.

> Reported-by: Sashiko <netdev-bot+sashiko@xxxxxxxxxx>
> Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260814-netcons_ipv6-v3-7-bc0915e8c75f@xxxxxxxxx
> Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928-netcons-fixes-v1-0-bb5ffe5e698a%40gmail.com
> Signed-off-by: Gustavo Luiz Duarte <gustavold@xxxxxxxxx>

[Severity: Low]
This fixes a race that users can see, but the trailers have no Fixes: tag.
Should one be added so stable and backport tooling can tell which trees
need the fix?

The family-dependent formatting in local_ip_show() and remote_ip_show()
(the AF_UNSPEC and AF_INET6 branches) came from these commits:

46b3f9e7e761 ("netconsole: use the address family instead of the ipv6 flag")
56ddc8d48e75 ("netconsole: show empty string for an unset IP address")

The first Closes: link points at that netcons_ipv6 series.

The lockless reads in dev_name_show() and remote_mac_show() go back to:

0bcc1816188e ("[NET] netconsole: Support dynamic reconfiguration using configfs")

> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> index 267254f046de9..188beacb308d7 100644
> --- a/drivers/net/netconsole.c
> +++ b/drivers/net/netconsole.c
> @@ -859,7 +859,19 @@ 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();
> +	/* Hold RTNL to prevent racing against netconsole_netdev_event()
> +	 * changing np.dev_name.
> +	 */
> +	rtnl_lock();
> +	ret = sysfs_emit(buf, "%s\n", nt->np.dev_name);
> +	rtnl_unlock();
> +	dynamic_netconsole_mutex_unlock();
> +
> +	return ret;
>  }

[ ... ]

> @@ -906,7 +932,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]
local_mac_show(), just above this hunk, still has no locking. Is it still
open to the same race the commit message describes?

drivers/net/netconsole.c:local_mac_show() {
	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 and dereferences dev->dev_addr without holding
dynamic_netconsole_mutex, without RTNL, and without a netdev reference.

The netdev notifier can clear np.dev and drop its reference while holding
only RTNL:

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

enabled_store() also reaches this cleanup, under the mutex.

Suppose a reader of local_mac (world-readable through CONFIGFS_ATTR_RO)
loads a non-NULL dev and is then preempted while the device is
unregistered and freed. Could sysfs_emit() then read dev->dev_addr from a
freed net_device?

Even without unregistration, dev->dev_addr is rewritten under RTNL when
the MAC address changes. A lockless read could then print a mix of old
and new bytes.

dev_name_show() already takes RTNL because of the notifier-side writer.
Would taking dynamic_netconsole_mutex and then rtnl_lock() around both the
np.dev load and the sysfs_emit() call in local_mac_show() cover both
cases?

The later commit in this series, "netconsole: remove unnecessary target
refcounting from the netdev notifier", does not touch local_mac_show(). So
the problem still seems to be there at the end of the series.

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