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

[Gustavo Luiz Duarte]

On Fri, Oct 9, 2026 at 1:35 PM Breno Leitao <leitao@xxxxxxxxxx> wrote:
>
> On Thu, Oct 08, 2026 at 08:55:40PM +0100, Gustavo Luiz Duarte wrote:
> > Hi Eric, thanks for the review!
> >
> > On Tue, Oct 6, 2026 at 9:35 PM Eric Dumazet <edumazet@xxxxxxxxxx> wrote:
> > >
> > >
> > >
> > > On 10/6/26 20:58, Gustavo Luiz Duarte wrote:
> > > > 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 RTNL in addition to
> > > > dynamic_netconsole_mutex.
> > > >
> > > > 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>
> > > > ---
> > > >   drivers/net/netconsole.c | 62 +++++++++++++++++++++++++++++++++++++++---------
> > > >   1 file changed, 51 insertions(+), 11 deletions(-)
> > > >
> > > > diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> > > > index 267254f046de..188beacb308d 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;
> > > >   }
> > >
> > > Please do not add rtnl_lock() in a _show() sysfs handler unless there is
> > > no other way?
> > >
> > > Something like:
> > >
> > >      dynamic_netconsole_mutex_lock();
> > >      strscpy(name, nt->np.dev_name, sizeof(name));
> > >      if (nt->state == STATE_ENABLED) {
> > >          struct net_device *dev = nt->np.dev;
> > >
> > >          if (dev)
> > >              netdev_copy_name(dev, name);
> >
> > This could lead to a use-after-free if we race with NETDEV_UNREGISTER
> > and 'dev' gets freed.
>
> Any chance you can get the lock (either dynamic_netconsole_mutex or
> target_list_lock) in netdev notifiers, so, it doens't conflict with this
> one?

We can sidestep the device teardown entirely if we just stick to
np->dev_name and protect it with target_list_lock which is held in
netconsole_netdev_event() while handling NETDEV_CHANGENAME:

  dynamic_netconsole_mutex_lock();
  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();

The local_mac_show() case is not as simple because we don't keep a
copy of dev_addr, but we can couple target_list_lock with nt->state ==
STATE_ENABLED, which ensures the target is not queued for cleanup so
the device won't be freed from under us.

I will send a new revision for this and for local_mac_show() using
target_list_lock.