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

[Gustavo Luiz Duarte]

On Thu, Oct 1, 2026 at 10:00 AM <netdev-bot+sashiko@xxxxxxxxxx> wrote:
>
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
>
> Critical: 0 · High: 1 · 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] The fix is incomplete for dev_name.
> - [Low] The commit 'netconsole: avoid printing partially updated target
>   attributes' fixes a user-visible race: configfs *_show() callbacks can…
>
> --
>
> netconsole: avoid printing partially updated target attributes
>
> This patch takes dynamic_netconsole_mutex in the configfs show callbacks
> for dev_name, local_ip, remote_ip, remote_mac and the userdatum value.
> Readers are then serialized against the store callbacks and should no
> longer see partially updated values.
>
> Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260814-netcons_ipv6-v3-7-bc0915e8c75f@xxxxxxxxx
>
> > Reported-by: Sashiko <netdev-bot+sashiko@xxxxxxxxxx>
> > Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260814-netcons_ipv6-v3-7-bc0915e8c75f@xxxxxxxxx
> > Signed-off-by: Gustavo Luiz Duarte <gustavold@xxxxxxxxx>
>
> [Severity: Low]
> Should this have a Fixes: tag?
>
> These show callbacks have read the fields without dynamic_netconsole_mutex
> for a long time. dev_name_show() and local_ip_show() date back to
> 0bcc1816188e ("[NET] netconsole: Support dynamic reconfiguration using
> configfs").
>
> The IPv6 rework (for example 56ddc8d48e75f, 46b3f9e7e761d, f6e96d72f64a8)
> made the window larger by adding the separate family field.
>
> The other two fixes in this series, "netconsole: finish enabling the
> target before releasing RTNL" and "netconsole: read np.dev under the RTNL
> in local_mac_show()", both carry Fixes: tags.
>
> > diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> > index 5f4311726d11..627475f16383 100644
> > --- a/drivers/net/netconsole.c
> > +++ b/drivers/net/netconsole.c
> > @@ -861,7 +861,14 @@ 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();
> > +     ret = sysfs_emit(buf, "%s\n", nt->np.dev_name);
> > +     dynamic_netconsole_mutex_unlock();
>
> [Severity: High]
> Does this fully close the torn read for dev_name?
>
> dynamic_netconsole_mutex only excludes dev_name_store(). The netdev
> notifier path has two other writers of np.dev_name, and neither of them
> takes this mutex.
>
> For an enabled target, the NETDEV_CHANGENAME case rewrites the name in
> place. It holds RTNL, target_cleanup_list_lock and target_list_lock:
>
> drivers/net/netconsole.c:netconsole_netdev_event() {
>     ...
>                 if (nt->np.dev == dev) {
>                         switch (event) {
>                         case NETDEV_CHANGENAME:
>                                 strscpy(nt->np.dev_name, dev->name, IFNAMSIZ);
>                                 break;
>     ...
> }
>
> For MAC-bound targets on unregister, release or join, the notifier calls
> netconsole_process_cleanups_core()->netcons_release_dev(). That zeroes the
> name with only RTNL and target_cleanup_list_lock held:
>
> drivers/net/netconsole.c:netcons_release_dev() {
>         do_netpoll_cleanup(&nt->np);
>         if (bound_by_mac(nt))
>                 memset(&nt->np.dev_name, 0, IFNAMSIZ);
> }
>
> dev_name_store() refuses to write while the target is enabled, so it
> never overlaps these writers. dev_name_show() has no such state check.
>
> So a read of /sys/kernel/config/netconsole/<target>/dev_name can still
> race with "ip link set eth0 name ..." or with deleting the device. The
> read could return a mix of old and new bytes, or a partly zeroed name.
> The buffer stays NUL terminated within IFNAMSIZ, so the result is wrong
> output, not an out of bounds read.
>
> This looks the same at the tip of the series. The other fields this patch
> touches (local_ip, remote_ip, remote_mac and the userdatum value) have no
> writers outside the mutex, so the gap only affects dev_name.
>
> local_mac_show() in this same series already takes rtnl_lock() to
> serialize with this notifier. Could dev_name_show() also take RTNL inside
> the mutex? That would follow the existing dynamic_netconsole_mutex -> RTNL
> order used by enabled_store().

This sounds correct. In dev_name_show() we need an rtnl_lock() in
addition to the dynamic_netconsole_mutex_lock(), otherwise we would
race with netconsole_netdev_event(). I will fix this in v2.

pw-bot: cr