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

On Fri, Oct 9, 2026 at 1:35 PM Breno Leitao [off-list ref] wrote:

On Thu, Oct 08, 2026 at 08:55:40PM +0100, Gustavo Luiz Duarte wrote:

quoted

Hi Eric, thanks for the review!

On Tue, Oct 6, 2026 at 9:35 PM Eric Dumazet [off-list ref] wrote:

quoted

On 10/6/26 20:58, Gustavo Luiz Duarte wrote:

quoted

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?

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.