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

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

[Breno Leitao]

netconsole registers a single console (or two in case someone wants
basic and extended at the same time), independent of the number of
targets.

This was not ideal, but, there was no reason to change.

Now that we want to replay some messages [1], we cannot do it per
target, but, per console [2].

Embed a struct console in netconsole_target, so, each target has its own
console, named `netcons_<target name>`. struct console holds 15
characters, so longer names are truncated, and two targets can end up
with the same console name.

No ->match() callback is added: netconsole consoles set CON_ENABLED and
register without a console= match. console=netcon0 no longer refers to
netconsole.

Unregistering a console sleeps until the console has flushed and its
printer thread has stopped, which the netdev notifier should not do
under RTNL. A target stopped by an interface event keeps its console
instead, and the write callback skips the target as it does today.

The console is replaced when the target is enabled again or resumes.
The user may have switched the format in between, and printk has to
format the records the way the target sends them.

Today, a target switched to the other format while deactivated resumes
silently if no console of that format is registered.

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. The
printer threads of the targets serialize on it, like the two shared
consoles do today.

/proc/consoles now lists one entry per enabled target, and per target
stopped by an interface event, for example:

	netcons_foo-1        -W- (E  N   )

This makes the mental model easier to understand, and also simplifies
the code.

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]
Signed-off-by: Breno Leitao <leitao@debian.org>
---
 drivers/net/netconsole.c | 194 +++++++++++++++++++----------------------------
 1 file changed, 76 insertions(+), 118 deletions(-)

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index 267254f046de9a..8353dd56789c19 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -111,23 +111,12 @@ static DEFINE_MUTEX(target_cleanup_list_lock);
 
 static struct workqueue_struct *netconsole_wq;
 
-/*
- * Console driver for netconsoles.  Register only consoles that have
- * an associated target of the same type.
- */
-static struct console netconsole_ext, netconsole;
-
 struct netconsole_target_stats  {
 	u64_stats_t xmit_drop_count;
 	u64_stats_t enomem_count;
 	struct u64_stats_sync syncp;
 };
 
-enum console_type {
-	CONS_BASIC = BIT(0),
-	CONS_EXTENDED = BIT(1),
-};
-
 /* Features enabled in sysdata. Contrary to userdata, this data is populated by
  * the kernel. The fields are designed as bitwise flags, allowing multiple
  * features to be set in sysdata_fields.
@@ -214,6 +203,7 @@ struct netcons_userdata {
  *		successful netpoll_setup() / netpoll_cleanup() pair on @np.
  * @refill_wq:	Work item that asynchronously tops @skb_pool back up to
  *		MAX_SKBS after find_skb() drains an entry.
+ * @console:	Console printing to this target, registered once it is enabled.
  */
 struct netconsole_target {
 	struct list_head	list;
@@ -243,8 +233,13 @@ struct netconsole_target {
 	struct work_struct	resume_wq;
 	struct sk_buff_head	skb_pool;
 	struct work_struct	refill_wq;
+	struct console		console;
 };
 
+static void netconsole_register_console(struct netconsole_target *nt,
+					short flags);
+static void netconsole_unregister_console(struct netconsole_target *nt);
+
 #ifdef	CONFIG_NETCONSOLE_DYNAMIC
 
 static struct configfs_subsystem netconsole_subsys;
@@ -630,6 +625,11 @@ static void process_resume_target(struct work_struct *work)
 	list_del_init(&nt->list);
 	spin_unlock_irqrestore(&target_list_lock, flags);
 
+	/* The console registered before the deactivation may still be around,
+	 * and extended may have changed since. Drop it here, a new one is
+	 * registered below once the target is enabled again.
+	 */
+	netconsole_unregister_console(nt);
 	resume_target(nt);
 
 	/* netpoll_setup() took a net_device reference and dropped the RTNL
@@ -660,6 +660,8 @@ static void process_resume_target(struct work_struct *work)
 	spin_unlock_irqrestore(&target_list_lock, flags);
 	rtnl_unlock();
 
+	netconsole_register_console(nt, 0);
+
 out_unlock:
 	dynamic_netconsole_mutex_unlock();
 }
@@ -964,33 +966,6 @@ static ssize_t sysdata_release_enabled_show(struct config_item *item,
 	return sysfs_emit(buf, "%d\n", release_enabled);
 }
 
-/* Iterate in the list of target, and make sure we don't have any console
- * register without targets of the same type
- */
-static void unregister_netcons_consoles(void)
-{
-	struct netconsole_target *nt;
-	u32 console_type_needed = 0;
-	unsigned long flags;
-
-	spin_lock_irqsave(&target_list_lock, flags);
-	list_for_each_entry(nt, &target_list, list) {
-		if (nt->extended)
-			console_type_needed |= CONS_EXTENDED;
-		else
-			console_type_needed |= CONS_BASIC;
-	}
-	spin_unlock_irqrestore(&target_list_lock, flags);
-
-	if (!(console_type_needed & CONS_EXTENDED) &&
-	    console_is_registered(&netconsole_ext))
-		unregister_console(&netconsole_ext);
-
-	if (!(console_type_needed & CONS_BASIC) &&
-	    console_is_registered(&netconsole))
-		unregister_console(&netconsole);
-}
-
 static ssize_t sysdata_msgid_enabled_show(struct config_item *item,
 					  char *buf)
 {
@@ -1037,6 +1012,9 @@ static ssize_t enabled_store(struct config_item *item,
 	if (enabled == current_enabled) {
 		pr_info("network logging has already %s\n",
 			current_enabled ? "started" : "stopped");
+		/* Interface events stop a target but keep its console */
+		if (!enabled)
+			netconsole_unregister_console(nt);
 		goto out_unlock;
 	}
 
@@ -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
@@ -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");
 	} else {	/* false */
 		/* We need to disable the netconsole before cleaning it up
@@ -1095,10 +1064,7 @@ static ssize_t enabled_store(struct config_item *item,
 		list_move(&nt->list, &target_cleanup_list);
 		spin_unlock_irqrestore(&target_list_lock, flags);
 		mutex_unlock(&target_cleanup_list_lock);
-		/* Unregister consoles, whose the last target of that type got
-		 * disabled.
-		 */
-		unregister_netcons_consoles();
+		netconsole_unregister_console(nt);
 	}
 
 	ret = count;
@@ -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);
+	/* No index, the console is named after the target */
+	nt->console.index = -1;
 
 	/* Adding, but it is disabled */
 	spin_lock_irqsave(&target_list_lock, flags);
@@ -1788,6 +1758,8 @@ static void drop_netconsole_target(struct config_group *group,
 
 	dynamic_netconsole_mutex_lock();
 
+	netconsole_unregister_console(nt);
+
 	mutex_lock(&target_cleanup_list_lock);
 	spin_lock_irqsave(&target_list_lock, flags);
 	/* A target moved to target_cleanup_list by netconsole_netdev_event()
@@ -2470,50 +2442,37 @@ static void send_msg_udp(struct netconsole_target *nt, const char *msg,
 }
 
 /**
- * netconsole_write - Generic function to send a msg to all targets
+ * netconsole_write - Send a msg to the target of a console
+ * @con: console of the target
  * @wctxt: nbcon write context
- * @extended: "true" for extended console mode
  *
- * Given an nbcon write context, send the message to the netconsole targets
+ * Given an nbcon write context, send the message to the netconsole target
  */
-static void netconsole_write(struct nbcon_write_context *wctxt, bool extended)
+static void netconsole_write(struct console *con,
+			     struct nbcon_write_context *wctxt)
 {
 	struct netconsole_target *nt;
 
+	nt = container_of(con, struct netconsole_target, console);
+
 	if (oops_only && !oops_in_progress)
 		return;
 
-	list_for_each_entry(nt, &target_list, list) {
-		if (nt->extended != extended || nt->state != STATE_ENABLED ||
-		    !netif_running(nt->np.dev))
-			continue;
-
-		/* If nbcon_enter_unsafe() fails, just return given netconsole
-		 * lost the ownership, and iterating over the targets will not
-		 * be able to re-acquire.
-		 */
-		if (!nbcon_enter_unsafe(wctxt))
-			return;
-
-		if (extended)
-			send_ext_msg_udp(nt, wctxt);
-		else
-			send_msg_udp(nt, wctxt->outbuf, wctxt->len);
+	if (nt->state != STATE_ENABLED || !netif_running(nt->np.dev))
+		return;
 
-		nbcon_exit_unsafe(wctxt);
-	}
-}
+	/* If nbcon_enter_unsafe() fails, just return given netconsole
+	 * lost the ownership.
+	 */
+	if (!nbcon_enter_unsafe(wctxt))
+		return;
 
-static void netconsole_write_ext(struct console *con __always_unused,
-				 struct nbcon_write_context *wctxt)
-{
-	netconsole_write(wctxt, true);
-}
+	if (nt->extended)
+		send_ext_msg_udp(nt, wctxt);
+	else
+		send_msg_udp(nt, wctxt->outbuf, wctxt->len);
 
-static void netconsole_write_basic(struct console *con __always_unused,
-				   struct nbcon_write_context *wctxt)
-{
-	netconsole_write(wctxt, false);
+	nbcon_exit_unsafe(wctxt);
 }
 
 static void netconsole_device_lock(struct console *con __always_unused,
@@ -2670,6 +2629,9 @@ static struct netconsole_target *alloc_param_target(char *target_config,
 	} else {
 		nt->state = STATE_ENABLED;
 	}
+	/* netcons_cmdline<N> does not fit, printk shows the index after it */
+	strscpy(nt->console.name, "netcons_" NETCONSOLE_PARAM_TARGET_PREFIX);
+	nt->console.index = cmdline_count;
 	populate_configfs_item(nt, cmdline_count);
 
 	return nt;
@@ -2682,6 +2644,7 @@ static struct netconsole_target *alloc_param_target(char *target_config,
 /* Cleanup netpoll for given target (from boot/module param) and free it */
 static void free_param_target(struct netconsole_target *nt)
 {
+	netconsole_unregister_console(nt);
 	cancel_work_sync(&nt->resume_wq);
 	if (nt->state == STATE_ENABLED)
 		netconsole_skb_pool_flush(nt);
@@ -2692,29 +2655,38 @@ static void free_param_target(struct netconsole_target *nt)
 	kfree(nt);
 }
 
-static struct console netconsole_ext = {
-	.name = "netcon_ext",
-	.flags = CON_ENABLED | CON_EXTENDED | CON_NBCON | CON_NBCON_ATOMIC_UNSAFE,
-	.write_thread = netconsole_write_ext,
-	.write_atomic = netconsole_write_ext,
-	.device_lock = netconsole_device_lock,
-	.device_unlock = netconsole_device_unlock,
-};
+/* 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;
 
-static struct console netconsole = {
-	.name = "netcon",
-	.flags = CON_ENABLED | CON_NBCON | CON_NBCON_ATOMIC_UNSAFE,
-	.write_thread = netconsole_write_basic,
-	.write_atomic = netconsole_write_basic,
-	.device_lock = netconsole_device_lock,
-	.device_unlock = netconsole_device_unlock,
-};
+	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);
+}
+
+static void netconsole_unregister_console(struct netconsole_target *nt)
+{
+	if (!console_is_registered(&nt->console))
+		return;
+
+	unregister_console(&nt->console);
+}
 
 static int __init init_netconsole(void)
 {
 	int err;
 	struct netconsole_target *nt, *tmp;
-	u32 console_type_needed = 0;
 	unsigned int count = 0;
 	unsigned long flags;
 	char *target_config;
@@ -2730,13 +2702,7 @@ static int __init init_netconsole(void)
 				goto fail;
 			}
 			/* Dump existing printks when we register */
-			if (nt->extended) {
-				console_type_needed |= CONS_EXTENDED;
-				netconsole_ext.flags |= CON_PRINTBUFFER;
-			} else {
-				console_type_needed |= CONS_BASIC;
-				netconsole.flags |= CON_PRINTBUFFER;
-			}
+			netconsole_register_console(nt, CON_PRINTBUFFER);
 
 			spin_lock_irqsave(&target_list_lock, flags);
 			list_add(&nt->list, &target_list);
@@ -2759,10 +2725,6 @@ static int __init init_netconsole(void)
 	if (err)
 		goto undonotifier;
 
-	if (console_type_needed & CONS_EXTENDED)
-		register_console(&netconsole_ext);
-	if (console_type_needed & CONS_BASIC)
-		register_console(&netconsole);
 	pr_info("network logging started\n");
 
 	return err;
@@ -2795,10 +2757,6 @@ static void __exit cleanup_netconsole(void)
 {
 	struct netconsole_target *nt, *tmp;
 
-	if (console_is_registered(&netconsole_ext))
-		unregister_console(&netconsole_ext);
-	if (console_is_registered(&netconsole))
-		unregister_console(&netconsole);
 	dynamic_netconsole_exit();
 	unregister_netdevice_notifier(&netconsole_netdev_notifier);
 	flush_workqueue(netconsole_wq);

---
base-commit: ddfe51b0a0f5dc9e415cc2c8ad8db17532dd426b
base-tree: git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master
change-id: 20261105-netcons_one_console_per_target-78872c5ec841

Best regards,
--  
Breno Leitao <leitao@debian.org>

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

[netdev-bot+sashiko]

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

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

[Breno Leitao]

On Tue, Nov 10, 2026 at 01:42:17AM +0000, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential
> [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?

I understand that this one got fixed by Gustavo's commit f94b9640bcf22b
("netconsole: finish enabling the target before releasing RTNL")