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

[netdev-bot+sashiko@kernel.org]

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