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

[Gustavo Luiz Duarte]

On Tue, Oct 13, 2026 at 6:20 PM <netdev-bot+sashiko@xxxxxxxxxx> wrote:
>
> 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@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]
> 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?

This might happen, though quite unlikely and it has limited impact:
printing a truncated device name.
The fix would be taking target_list_lock around the memset in
netcons_release_dev().
I'm not convinced it is worth it, but happy to spin another revision
if someone thinks otherwise.

>
> 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?

This is being addressed in
https://lore.kernel.org/all/20261012-netcons-fixes-v3-2-1aac85ed7665@xxxxxxxxx/