Message-ID: <aMGVa5kGLQBvTRB9@pathway.suse.cz>
Date: Wed, 10 Sep 2025 17:12:43 +0200
From: Petr Mladek <pmladek@...e.com>
To: John Ogness <john.ogness@...utronix.de>
Cc: Breno Leitao <leitao@...ian.org>, Mike Galbraith <efault@....de>,
	Simon Horman <horms@...nel.org>, kuba@...nel.org, calvin@...nvd.org,
	Pavel Begunkov <asml.silence@...il.com>,
	Johannes Berg <johannes@...solutions.net>, paulmck@...nel.org,
	LKML <linux-kernel@...r.kernel.org>, netdev@...r.kernel.org,
	boqun.feng@...il.com, Sergey Senozhatsky <senozhatsky@...omium.org>,
	Steven Rostedt <rostedt@...dmis.org>
Subject: Re: netconsole: HARDIRQ-safe -> HARDIRQ-unsafe lock order warning

On Wed 2025-09-10 14:28:40, John Ogness wrote:
> (Added CC printk folks since we are now talking about the nbcon API.)
> 
> Hi Breno,
> 
> On 2025-09-09, Breno Leitao <leitao@...ian.org> wrote:
> > To summarize the problem:
> >
> > 1) netpoll calls .ndo_start_xmit() with IRQ disabled, which causes the
> > lockdep problem reported in this thread. (current code)
> >
> > 2) moving netconsole to use NBCON will help in the thread context, given
> > that .write_thread() doesn't need to have IRQ disabled. (This requires
> > rework of netconsole target_list_lock)
> 
> Aside from reworking the target_list_lock, be aware that ->device_lock()
> must at least disable migration. The kerneldoc for the ->device_lock()
> callback talks about this.
> 
> > 3) In the atomic context, there is no easy solution so far. The options
> > are not good, but, I will list them here for the sake of getting things
> > clear:
> >
> >   a) Defer the msg as proposed initially.
> >     Pro: If the machine is not crashing, it should simply work (?!)
> >     Cons: It cannot be expected that the workqueue is functional during panic, thus
> >           the messages might be lost
> 
> To be clear, "might be lost" means the message was not printed on the
> netconsole. The message still will have made it into the ringbuffer and
> attempted output to any other consoles as well as being available to
> crash tools.
> 
> My problem with implementing deferring is that is what ->write_thread()
> is already doing.

I agree. Deferring any job in write_atomic() callback does not make
much sense. It is primary called in panic() where the deferred
job would never be done.

And yes, if .write_atomic() callback is not implemented then the console
handling is already automatically deferred to the printk kthread.

> I wonder if we should extend the nbcon interface so that it is possible
> to specify that ->write_atomic() is not safe. Then it would only be used
> as a last resort in panic context.
> 
> @pmladek: We could introduce a new console flag (NBCON_ATOMIC_UNSAFE) so
> that the callback is only used by nbcon_atomic_flush_unsafe().

This might be an acceptable compromise. It would try to emit messages
only at the very end of panic() as the last desperate attempt.

Just to be sure, what do you mean with unsafe?

    + taking IRQ unsafe locks?
    + using trylock and ignoring result (keep using oops_in_progress check?)
    + ???

Note that write_atomic() might get serialized against other operations
using the nbcon_context locking. But it might require adding wrappers
which would take both netconsole-specific lock and nbcon_context at
the same time, similar to uart_port_*lock*() API, see
include/linux/serial_core.h

It might also require adding support for a nested nbcon_context locking.

And there is a risk that nbcon_context lock might become another big
kernel lock.


> >   b) Send the message anyway (and hope for the best)
> >     Cons: Netpoll will continue to call IRQ unsafe locks from IRQ safe
> >           context (lockdep will continue to be unhappy)
> >     Pro: This is how it works today already, so, it is not making the problem worse.
> >          In fact, it is narrowing the problem to only .write_atomic().
> 
> Two concerns here:
> 
> 1. ->write_atomic() is also used during normal operation
> 
> 2. It is expected that ->write_atomic() callbacks are implemented
>    safely. The other nbcon citizens are doing this. Having an nbcon
>    driver with an unsafe ->write_atomic() puts all nbcon drivers at risk
>    of not functioning during panic.
> 
> This could be combined with (a) so that ->write_atomic() implements its
> own deferred queue of messages to print and only when
> @legacy_allow_panic_sync is true, will it try to send immediately and
> hope for the best. @legacy_allow_panic_sync is set after all nbcon
> drivers have had a chance to flush their buffers safely and then the
> kernel starts to allow less safe drivers to flush.

I think that the important trick is adding the NBCON_ATOMIC_UNSAFE
flag. The practice will show where such callbacks might be
allowed (risk vs. gain).

> Although I would prefer the NBCON_ATOMIC_UNSAFE approach instead.

Yes, I would start with allowing unsafe write_atomic() only
in nbcon_atomic_flush_unsafe() and see if it is enough.


> >   c) Not implementing .write_atomic
> >     Cons: we lose the most important messages of the boot.
> >
> >   Any other option I am not seeing?
> 
> d) Not implementing ->write_atomic() and instead implement a kmsg_dumper
>    for netconsole. This registers a callback that is called during
>    panic.
> 
>    Con: The kmsg_dumper interface has nothing to do with consoles, so it
>         would require some effort coordinating with the console drivers.
> 
>    Pro: There is absolute freedom for the dumper to implement its own
>         panic-only solution to get messages out.

I guess that the dumper would use similar tricks as an "unsafe"
write_atomic() callback.

I would personally try the appraoch with the "unsafe" write_atomic()
callback first, IMHO, it would allow more flexibility than adding
a special kmsg_dumper.

> e) Involve support from the underlying network drivers to implement true
>    atomic sending. Thomas Gleixner talked [0] very briefly about how
>    this could be implemented for netconsole during the 2022
>    proof-of-concept presentation of the nbcon API.
> 
>    Cons: It most likely requires new API callbacks for the network
>          drivers to implement hardware-specific solutions. Many (most?)
>          drivers would not be able to support it.
> 
>    Pro: True reliable atomic printing via network.

Yeah, that is the dream solution :-)

Best Regards,
Petr

Powered by blists - more mailing lists