Re: [PATCH net-next] netconsole: register one console per target

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.

Critical: 0 · High: 0 (1) · Medium: 2 · Low: 2

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

- [Medium] netconsole_register_console() calls register_console(), which
  returns void and can fail silently.
- [Medium] netconsole: console left registered on error path in
  enabled_store()
- [Low] In make_netconsole_target() (drivers/net/netconsole.c) the patch
  sets `nt->console.index = -1` with the comment "No index, the console…
- [Low] The patch names a target's console in two places, and they handle
  the 'cmdline<N>' name differently.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch
  keeps it and wires a console straight onto it.

--

netconsole: register one console per target

This commit replaces the two shared netconsole consoles (netcon and
netcon_ext) with a struct console embedded in each netconsole_target,
named netcons_<target name>. A target's console is registered when the
target is enabled or resumes, and unregistered when it is disabled or
dropped. The write callback now sends only to the target that owns the
console.

Link: https://lore.kernel.org/all/20261024-netcons-replay-upstream-v1-0-9613f38496e5@gmail.com/ [1]
Link: https://lore.kernel.org/all/auDjNKi336lstU7j@pathway.suse.cz/ [2]

> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> index 267254f046de9..8353dd56789c1 100644
> --- a/drivers/net/netconsole.c
> +++ b/drivers/net/netconsole.c

[ ... ]

> @@ -1080,6 +1048,7 @@ static ssize_t enabled_store(struct config_item *item,
>  		}
>  
>  		nt->state = STATE_ENABLED;
> +		netconsole_register_console(nt, 0);
>  		pr_info("network logging started\n");

[Severity: High]
This isn't a bug introduced by this patch, but this patch now registers
a console right after this lockless store of STATE_ENABLED. Can the store
overwrite a deactivation done at the same time by a NETDEV_UNREGISTER?

By the time we get here, netcons_netpoll_setup() has published np.dev,
dropped the RTNL and waited for a grace period:

drivers/net/netconsole.c:netcons_netpoll_setup() {
    ...
	err = __netpoll_setup(np, ndev);
	if (err)
		goto put;
	rtnl_unlock();
    ...
	synchronize_rcu();
    ...
}

The target is on target_list during that window, so this sequence looks
possible:

enabled_store()                     NETDEV_UNREGISTER
  netcons_netpoll_setup()
    rtnl_unlock()
    synchronize_rcu()               netconsole_netdev_event()
                                      nt->state = STATE_DEACTIVATED;
                                      list_move(..., &target_cleanup_list);
                                    netconsole_process_cleanups_core()
                                      netcons_release_dev()
                                        do_netpoll_cleanup()
                                          np->dev = NULL
                                      target back on target_list
  nt->state = STATE_ENABLED;
  netconsole_register_console()

The next netconsole_write() would pass the state check and call
netif_running() with a NULL dev:

	if (nt->state != STATE_ENABLED || !netif_running(nt->np.dev))
		return;

process_resume_target() handles this race with the reg_state re-check
under the RTNL, but enabled_store() has no such check. Does it need one?

The commit message also says:

  target_list_lock remains the device_lock of all the consoles, as the
  state changes that race with the write callback are made under it.

This store is not done under target_list_lock. Is that statement accurate
for enabled_store()?

[ ... ]

> @@ -1770,6 +1736,10 @@ static struct config_group *make_netconsole_target(struct config_group *group,
>  
>  	/* Initialize the config_group member */
>  	init_target_config_group(nt, name);
> +	snprintf(nt->console.name, sizeof(nt->console.name), "netcons_%s",
> +		 name);

[Severity: Low]
What happens to a configfs target named cmdline<N> when no boot target
has that name? This can happen after mkdir cmdline5, or when the matching
netconsole= entry failed to parse in dynamic mode.

find_cmdline_target() only matches a boot target with exactly that name,
so the target is created here. "netcons_cmdline5" needs 17 bytes, so
snprintf() truncates it to "netcons_cmdline" and the 5 is lost.

alloc_param_target() already handles this exact name pattern by moving
N into the index:

	strscpy(nt->console.name, "netcons_" NETCONSOLE_PARAM_TARGET_PREFIX);
	nt->console.index = cmdline_count;

This path sets the index to -1 instead. The console then shows up as
netcons_cmdline-1, or as netcons_cmdline0 once printk rewrites the
negative index (see below). netcons_cmdline0 is also the console name of
boot target cmdline0.

Should make_netconsole_target() apply the same rule to cmdline<N> names?

> +	/* No index, the console is named after the target */
> +	nt->console.index = -1;

[Severity: Low]
Does printk treat an index of -1 as "no index"? It looks like the core
treats a negative index as "not assigned yet" and rewrites it:

kernel/printk/printk.c:try_enable_default_console() {
	if (newcon->index < 0)
		newcon->index = 0;
    ...
}

kernel/printk/printk.c:try_enable_preferred_console() {
    ...
			if (newcon->index < 0)
				newcon->index = c->index;
    ...
}

try_enable_default_console() runs when preferred_console < 0 and the
first console in the list has no ->device or is a boot console.
netconsole consoles never set ->device. So if a netcons_cmdline<N>
console is first in the list, for example on a headless box booted with
netconsole= and no console=, every configfs console registered after it
gets index 0.

The index is always printed after the name: in con_printk(), in
/proc/consoles via show_console_dev(), and in the "pr/%s%d" printer
thread name. The same target can therefore show up as netcons_foo-1 on
one system and as netcons_foo0 on another.

netconsole_register_console() resets flags and callbacks on every
registration, but not ->index. Once the core rewrites the index, the new
value stays across later disable/enable and resume cycles.

This also doesn't match the example in the commit message:

        netcons_foo-1        -W- (E  N   )

[ ... ]

> @@ -2692,29 +2655,38 @@ static void free_param_target(struct netconsole_target *nt)

[ ... ]

> +/* Register the console of an enabled @nt, @flags can request CON_PRINTBUFFER */
> +static void netconsole_register_console(struct netconsole_target *nt,
> +					short flags)
> +{
> +	struct console *con = &nt->console;

[ ... ]

> +	if (nt->state != STATE_ENABLED)
> +		return;
> +
> +	con->flags = CON_ENABLED | CON_NBCON | CON_NBCON_ATOMIC_UNSAFE | flags;
> +	if (nt->extended)
> +		con->flags |= CON_EXTENDED;
> +	con->write_thread = netconsole_write;
> +	con->write_atomic = netconsole_write;
> +	con->device_lock = netconsole_device_lock;
> +	con->device_unlock = netconsole_device_unlock;
> +
> +	register_console(con);
> +}

[Severity: Medium]
What happens to the target if this register_console() call fails?
register_console() returns void and can fail silently.

For nbcon consoles, nbcon_alloc() fails if the pbufs allocation fails or
if nbcon_kthread_create() fails. The kthread can fail with -EINTR when
the task writing to the configfs enabled file has a fatal signal
pending:

kernel/printk/nbcon.c:nbcon_alloc() {
    ...
			if (!nbcon_kthread_create(con)) {
				kfree(con->pbufs);
				con->pbufs = NULL;
				return false;
			}
    ...
}

With this patch, enabled_store() and process_resume_target() unregister
the console and register it again each time. If registration fails, the
target stays in STATE_ENABLED and keeps its netpoll, its net_device
reference and its skb pool. It has no console, though, so it never sends
anything.

enabled_store() still returns count and prints "network logging
started", and resume_target() prints "network logging resumed".

Before this patch, the shared netcon/netcon_ext console stayed registered
across a resume, so the resume path could not lose its console this way.
On the configfs path, the next enable of any target of that type retried
the registration. Now the failure is per target and lasts until enabled
is toggled again.

Should netconsole_register_console() check console_is_registered()
afterwards and report the failure, for example by returning an error
from enabled_store() or by leaving the target disabled?

[ ... ]

> @@ -1046,18 +1024,8 @@ static ssize_t enabled_store(struct config_item *item,
>  			goto out_unlock;
>  		}
>  
> -		if (nt->extended && !console_is_registered(&netconsole_ext)) {
> -			netconsole_ext.flags |= CON_ENABLED;
> -			register_console(&netconsole_ext);
> -		}
> -
> -		/* User might be enabling the basic format target for the very
> -		 * first time, make sure the console is registered.
> -		 */
> -		if (!nt->extended && !console_is_registered(&netconsole)) {
> -			netconsole.flags |= CON_ENABLED;
> -			register_console(&netconsole);
> -		}
> +		/* Interface events disable a target but keep its console */
> +		netconsole_unregister_console(nt);
>  
>  		/*
>  		 * Skip netconsole_parser_cmdline() -- all the attributes are

[Severity: Medium]
Can this path leave the console of a deactivated target registered?

With this patch, a target stopped by NETDEV_UNREGISTER keeps its
console, and netconsole_netdev_event() only moves it to
STATE_DEACTIVATED. At the top of enabled_store() that state is turned
into STATE_DISABLED before anything else is checked:

	if (nt->state == STATE_DEACTIVATED)
		nt->state = STATE_DISABLED;

While the target is deactivated, the user can set release to 1 and
extended to 0. extended_store() and release_store() only refuse changes
while the target is STATE_ENABLED.

Writing 1 to enabled then hits the release && !extended check. That
check jumps to out_unlock before the netconsole_unregister_console()
call that follows it.

The write fails with -EINVAL, but the switch to STATE_DISABLED is kept.
The console of the target stays in console_list and in /proc/consoles,
together with its pbufs and its nbcon printer thread.

deactivated_target_match() no longer matches a STATE_DISABLED target,
so process_resume_target() never runs for it either. Nothing replaces
or drops the console until one of these happens:

  - the user writes 0 to enabled
  - the user fixes the configuration and enables the target again
  - the user removes the target directory

netconsole_write() skips the target because its state is not
STATE_ENABLED, so nothing is sent. This still contradicts the commit
message, which says /proc/consoles lists only enabled targets and
targets stopped by an interface event.

Would it be simpler to drop the console at the point where
STATE_DEACTIVATED is turned into STATE_DISABLED? Every exit from
enabled_store() would then see a target without a stale console. That
would also cover the "already stopped" branch, which currently
unregisters for the same reason.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261106-netcons_one_console_per_target-v1-1-c29dacdb9c1f%40debian.org