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

[netdev-bot+sashiko@kernel.org]

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@gmail.com

> 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
> Signed-off-by: Gustavo Luiz Duarte <gustavold@gmail.com>

[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().

> +
> +	return ret;
>  }

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928-netcons-fixes-v1-0-bb5ffe5e698a%40gmail.com