Message-ID: <b2qps3uywhmjaym4mht2wpxul4yqtuuayeoq4iv4k3zf5wdgh3@tocu6c7mj4lt>
Date: Tue, 19 Aug 2025 10:27:47 -0700
From: Breno Leitao <leitao@...ian.org>
To: Pavel Begunkov <asml.silence@...il.com>
Cc: Jakub Kicinski <kuba@...nel.org>, 
	Johannes Berg <johannes@...solutions.net>, Mike Galbraith <efault@....de>, paulmck@...nel.org, 
	LKML <linux-kernel@...r.kernel.org>, netdev@...r.kernel.org, boqun.feng@...il.com
Subject: Re: netconsole: HARDIRQ-safe -> HARDIRQ-unsafe lock order warning

On Mon, Aug 18, 2025 at 05:10:24AM -0700, Breno Leitao wrote:
> On Fri, Aug 15, 2025 at 09:02:27PM +0100, Pavel Begunkov wrote:
> > On 8/15/25 18:29, Breno Leitao wrote:
> > > On Fri, Aug 15, 2025 at 09:42:17AM -0700, Jakub Kicinski wrote:
> > > > On Fri, 15 Aug 2025 11:44:45 +0100 Pavel Begunkov wrote:
> > > > > On 8/15/25 01:23, Jakub Kicinski wrote:
> > > > 
> > > > I suspect disabling netconsole over WiFi may be the most sensible way out.
> > > 
> > > I believe we might be facing a similar issue with virtio-net.
> > > Specifically, any network adapter where TX is not safe to use in IRQ
> > > context encounters this problem.
> > > 
> > > If we want to keep netconsole enabled on all TX paths, a possible
> > > solution is to defer the transmission work when netconsole is called
> > > inside an IRQ.
> > > 
> > > The idea is that netconsole first checks if it is running in an IRQ
> > > context using in_irq(). If so, it queues the skb without transmitting it
> > > immediately and schedules deferred work to handle the transmission
> > > later.
> > > 
> > > A rough implementation could be:
> > > 
> > > static void send_udp(struct netconsole_target *nt, const char *msg, int len) {
> > > 
> > > 	/* get the SKB that is already populated, with all the headers
> > > 	 * and ready to be sent
> > > 	 */
> > > 	struct sk_buff = netpoll_get_skb(&nt->np, msg, len);
> > > 
> > > 	if (in_irq()) {
> > 
> > It's not just irq handlers but any context that has irqs disabled, and
> > since it's nested under irq-safe console_owner it'd need to always be
> > deferred or somehow moved out of the console_owner critical section.
> 
> Agree. An IRQ-unsafe lock (fq lock) should not be reachable from an IRQ
> disabled code path. So, one solution might be to always send TX packets
> from a workqueue (unless it is on panic, as suggested by Calvin).

I’ve continued investigating possible solutions, and it looks like
moving netconsole over to the non‑blocking console (nbcon) framework
might be the right approach. Unlike the classic console path, nbcon
doesn’t rely on the global console lock, which was one of the main
concerns regarding the possible deadlock.

Migrating netconsole to nbcon was already on my TODO list, since nbcon
is the modern infrastructure, but this issue accelerated that
transition. I’ve put together a PoC, and so far I haven’t seen any
lockdep warnings — even when explicitly triggering printk() from
different contexts.

The new path is protected by NETCONSOLE_NBCON, which is disabled by
default. This allows us to experiment and test both approaches.


commit 9180c12086954d30b23ec2b4bbb7859aa1192aca
Author: Breno Leitao <leitao@...ian.org>
Date:   Tue Aug 19 04:14:58 2025 -0700

    netconsole: Add support for nbcon
    
    Add support for running netconsole using the new non‑blocking console
    (nbcon) infrastructure.
    
    The nbcon framework improves console handling by avoiding the global
    console lock and enabling asynchronous, non‑blocking writes from
    multiple contexts.
    
    Key changes:
       * Introduce CONFIG_NETCONSOLE_NBCON (EXPERIMENTAL, depends on
         NETCONSOLE_EXTENDED_LOG && EXPERT) to enable nbcon mode.
       * Split out do_write_msg() for chunked send logic, re‑used in both
         sync and nbcon paths.
       * Add .write_thread callbacks for normal and extended netconsole when
         nbcon is enabled.
       * Provide device_lock/unlock helpers wrapping target_list_lock for
         safe nbcon integration.
       * Keep existing synchronous .write paths as the default when nbcon
         support is disabled, preserving backward compatibility.
    
    With this option enabled, netconsole can operate as a fully
    asynchronous, lockless nbcon backend, not depending on console lock
    anymore.
    
    This support is marked experimental for now until it receives wider
    testing.
    
    This patch doesn't implement optional .write_atomic callbacks,
    I initially implemented it to deferred the skbs, but, later I found that
    even printks() from inside IRQs are sent using the .write_thread()
    callback. Reading the code, I got the impression that .write_atomic
    callback is only called from an atomic context and there is a write in
    opertion, which might get interrupted (?). More on this soon.
    
    Signed-off-by: Breno Leitao <leitao@...ian.org>

diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index b29628d46be9b..ec9a430aa160e 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -382,6 +382,16 @@ config NETCONSOLE_PREPEND_RELEASE
 	  message.  See <file:Documentation/networking/netconsole.rst> for
 	  details.
 
+config NETCONSOLE_NBCON
+	bool "Enable non blocking netconsole (EXPERIMENTAL)"
+	depends on NETCONSOLE
+	default n
+	help
+	  Move netconsole to use non-blocking console (nbcons).  Non-blocking
+	  console (nbcon) is a new console infrastructure introduced to improve
+	  console handling by avoiding the global console lock (Big Kernel
+	  Lock) and enabling non-blocking, asynchronous writes to the console.
+
 config NETPOLL
 	def_bool NETCONSOLE
 
diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index e3722de08ea9f..5cd279e09fc8b 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -1708,12 +1708,30 @@ static void write_ext_msg(struct console *con, const char *msg,
 	spin_unlock_irqrestore(&target_list_lock, flags);
 }
 
-static void write_msg(struct console *con, const char *msg, unsigned int len)
+static void do_write_msg(struct netconsole_target *nt, const char *msg, unsigned int len)
 {
+	const char *tmp;
 	int frag, left;
+
+	/*
+	 * We nest this inside the for-each-target loop above
+	 * so that we're able to get as much logging out to
+	 * at least one target if we die inside here, instead
+	 * of unnecessarily keeping all targets in lock-step.
+	 */
+	tmp = msg;
+	for (left = len; left;) {
+		frag = min(left, MAX_PRINT_CHUNK);
+		send_udp(nt, tmp, frag);
+		tmp += frag;
+		left -= frag;
+	}
+}
+
+static void write_msg(struct console *con, const char *msg, unsigned int len)
+{
 	unsigned long flags;
 	struct netconsole_target *nt;
-	const char *tmp;
 
 	if (oops_only && !oops_in_progress)
 		return;
@@ -1723,21 +1741,8 @@ static void write_msg(struct console *con, const char *msg, unsigned int len)
 
 	spin_lock_irqsave(&target_list_lock, flags);
 	list_for_each_entry(nt, &target_list, list) {
-		if (!nt->extended && nt->enabled && netif_running(nt->np.dev)) {
-			/*
-			 * We nest this inside the for-each-target loop above
-			 * so that we're able to get as much logging out to
-			 * at least one target if we die inside here, instead
-			 * of unnecessarily keeping all targets in lock-step.
-			 */
-			tmp = msg;
-			for (left = len; left;) {
-				frag = min(left, MAX_PRINT_CHUNK);
-				send_udp(nt, tmp, frag);
-				tmp += frag;
-				left -= frag;
-			}
-		}
+		if (!nt->extended && nt->enabled && netif_running(nt->np.dev))
+			do_write_msg(nt, msg, len);
 	}
 	spin_unlock_irqrestore(&target_list_lock, flags);
 }
@@ -1917,16 +1922,69 @@ static void free_param_target(struct netconsole_target *nt)
 	kfree(nt);
 }
 
+#ifdef CONFIG_NETCONSOLE_NBCON
+static void netcon_write_ext_thread(struct console *con, struct nbcon_write_context *wctxt)
+{
+	struct netconsole_target *nt;
+
+	list_for_each_entry(nt, &target_list, list)
+		if (nt->extended && nt->enabled && netif_running(nt->np.dev)) {
+			if (!nbcon_enter_unsafe(wctxt))
+				continue;
+			send_ext_msg_udp(nt, wctxt->outbuf, wctxt->len);
+			nbcon_exit_unsafe(wctxt);
+		}
+}
+
+static void netcon_write_thread(struct console *con, struct nbcon_write_context *wctxt)
+{
+	struct netconsole_target *nt;
+
+	list_for_each_entry(nt, &target_list, list)
+		if (!nt->extended && nt->enabled && netif_running(nt->np.dev)) {
+			if (!nbcon_enter_unsafe(wctxt))
+				continue;
+			do_write_msg(nt, wctxt->outbuf, wctxt->len);
+			nbcon_exit_unsafe(wctxt);
+		}
+}
+
+static void netconsole_device_lock(struct console *con, unsigned long *flags)
+{
+	/* protects all the targets at the same time */
+	spin_lock_irqsave(&target_list_lock, *flags);
+}
+
+static void netconsole_device_unlock(struct console *con, unsigned long flags)
+{
+	spin_unlock_irqrestore(&target_list_lock, flags);
+}
+#endif
+
 static struct console netconsole_ext = {
 	.name	= "netcon_ext",
+#ifdef CONFIG_NETCONSOLE_NBCON
+	.flags	= CON_ENABLED | CON_EXTENDED | CON_NBCON,
+	.write_thread = netcon_write_ext_thread,
+	.device_lock = netconsole_device_lock,
+	.device_unlock = netconsole_device_unlock,
+#else
 	.flags	= CON_ENABLED | CON_EXTENDED,
 	.write	= write_ext_msg,
+#endif
 };
 
 static struct console netconsole = {
 	.name	= "netcon",
+#ifdef CONFIG_NETCONSOLE_NBCON
+	.flags	= CON_ENABLED | CON_NBCON,
+	.write_thread = netcon_write_thread,
+	.device_lock = netconsole_device_lock,
+	.device_unlock = netconsole_device_unlock,
+#else
 	.flags	= CON_ENABLED,
 	.write	= write_msg,
+#endif
 };
 
 static int __init init_netconsole(void)

Powered by blists - more mailing lists