linux-kernel - Re: [PATCH v2] printk_legacy_map: use LD_WAIT_CONFIG instead of LD_WAIT

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20251024103808.umPAqCda@linutronix.de>
Date: Fri, 24 Oct 2025 12:38:08 +0200
From: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To: Petr Mladek <pmladek@...e.com>
Cc: Oleg Nesterov <oleg@...hat.com>,
	John Ogness <john.ogness@...utronix.de>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] printk_legacy_map: use LD_WAIT_CONFIG instead of
 LD_WAIT_SLEEP

On 2025-10-24 11:35:14 [+0200], Petr Mladek wrote:
> It is clear that the commit message and the comment above the mapping
> caused some confusion. I thought about better wording.
> 
> I wanted to be as clear as possible, But the problem is that everyone
> has different background and might understand the same term
> differently. Also I am not a native speaker.
> 
> 
> /*
>  * Some legacy console drivers might violate raw_spinlock/spinlock nesting
>  * rules when printk() was called under a raw_spinlock and the driver used
>  * a spinlock. It is not a real problem because the legacy drivers should
>  * never be called directly from printk() in PREEMPT_RT.
>  *
>  * This map is used to pretend that printk() was called under a normal spinlock
>  * to hide the above described locking violation. It still allows to catch
>  * other problems, for example, possible ABBA deadlocks or sleeping locks.

It is not "Some legacy console" but all of them. The only exception
would if they don't use any locking. Serial driver should use
uart_port::lock, VT has its printing_lock and so on.
Don't like the "might violate".
"should never be called" is misleading because we know how things work
and they must not be called. But this is minor…

But why bring ABBA deadlocks into this and sleeping locks? Especially
since different people assume different things when "sleeping locks" is
used. And clearly the last was not handled well :)

I would suggest simple and focus on the change and why:
The override addresses the nesting problem on !RT which does not occur
on RT because the code flow is different.

What about the suggested:

  The legacy console always acquires a spinlock_t from its printing
  callback. This violates lock nesting if the caller acquired an always
  spinning lock (raw_spinlock_t) while invoking printk(). This is not a
  problem on PREEMPT_RT because legacy consoles print always from a
  dedicated thread and never from within printk(). Therefore we tell
  lockdep that a sleeping spin lock (spinlock_t) is valid here.

>  *
>  * The mapping is not used in PREEMPT_RT which allows to catch bugs when
>  * the legacy console driver would get called from an atomic context by mistake.
>  */
> 
> 
> And the commit message might be:
> 
> <commit_message>
> printk_legacy_map: use LD_WAIT_CONFIG instead of LD_WAIT_SLEEP
> 
> printk_legacy_map is used to hide possible violations of
> raw_spinlock/spinlock nesting when printk() calls legacy console
> drivers directly. It is not a real problem in !PREEMPT_RT mode and
  s/real//

> the problematic code path should never be called in PREEMPT_RT mode.

  because this code path is never called on PREEMPT_RT.

> However, LD_WAIT_SLEEP is not exactly right. It fools lockdep as if it

Why is not exactly right? :) Usually you describe _why_ you do things
and because it wasn't right is okay if it is obvious to everyone.

> is fine to acquire a sleeping lock.
> 
> Change DEFINE_WAIT_OVERRIDE_MAP(printk_legacy_map) to use LD_WAIT_CONFIG.
> 
> Also, update the comment to better describe the purpose of the mapping.
> </commit_message>

For my taste it is too verbose and you bring too much context. It is
*just* the lock nest override. No need to bring other aspects of lockdep
into the game.

  printk_legacy_map is used to hide lock nesting violations caused by
  legacy drivers and is using the wrong override type. LD_WAIT_SLEEP is
  for always sleeping lock types such as mutex_t. LD_WAIT_CONFIG is for
  lock type which are sleeping while spinning on PREEMPT_RT such as
  spinlock_t.

> Is this better and acceptable, please?
> If not then please provide alternatives ;-)

I made some suggestions. However you got rid of the points I complained
about initially so I fine with it. Thank you.

> Best Regards,
> Petr

Sebastian