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 [off-list ref] wrote:

quoted

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.

quoted

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.

quoted

  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.

quoted

  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