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

[Gustavo Luiz Duarte]

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. It is the same issue I'm trying to fix in [1].
Claude suggested adding an rcu_read_lock(). Since
unregister_netdevice_many_notify() calls synchronize_rcu(), it is
guaranteed that the net_device won't be freed until the grace period
finishes.
Something like:

  dynamic_netconsole_mutex_lock();
  strscpy(name, nt->np.dev_name, sizeof(name));
  rcu_read_lock();
  dev = READ_ONCE(nt->np.dev);
  if (nt->state == STATE_ENABLED && dev)
        netdev_copy_name(dev, name);
  rcu_read_unlock();
  dynamic_netconsole_mutex_unlock();
  return sysfs_emit(buf, "%s\n", name);

Also, netdev_copy_name is not an exported symbol, I would have to
EXPORT_SYMBOL(netdev_copy_name).

Would something like this be acceptable?

I can try and apply the same RCU approach to [1]. We don't have a
similar seqlock for dev_addr though, does it make sense to create one?

[1] https://lore.kernel.org/all/20261006-netcons-fixes-v2-2-ca652d55fd4a@xxxxxxxxx