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

[Breno Leitao <leitao@debian.org>]

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@kernel.org> 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@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>
> > > ---
> > >   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?