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: <20251023142655.FvZkeSa-@linutronix.de>
Date: Thu, 23 Oct 2025 16:26:55 +0200
From: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To: Oleg Nesterov <oleg@...hat.com>, Petr Mladek <pmladek@...e.com>
Cc: 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-23 12:32:34 [+0200], Oleg Nesterov wrote:
> printk_legacy_map is used on !PREEMPT_RT to avoid false positives from
> CONFIG_PROVE_RAW_LOCK_NESTING about raw_spinlock/spinlock nesting.

Could we please get rid of CONFIG_PROVE_RAW_LOCK_NESTING here? This is
lockdep internal implementation and has nothing to do with printk or
anything. The nesting is documented in
	Documentation/locking/locktypes.rst

and this counts regardless of PREEMPT_RT. The only reason why this is
still around is that non-RT architectures tend to do it wrong (sparc)
and I didn't get around to fix it all. And people started to give me
lockdep splats and after some time I gave up and it is enforced only on
RT architectures.


  printk_legacy_map is using the wrong override type. LD_WAIT_SLEEP is for
  always sleeping lock types like mutex_t. LD_WAIT_CONFIG is for sleeping
  on PREEMPT_RT lock types like spinlock_t.

> However, LD_WAIT_SLEEP is not exactly right; it fools lockdep as if it
> 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 make it more clear as suggested by Petr.
> 
> Signed-off-by: Oleg Nesterov <oleg@...hat.com>
> Reviewed-by: Petr Mladek <pmladek@...e.com>
> Reviewed-by: John Ogness <john.ogness@...utronix.de>
> ---
>  kernel/printk/printk.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 5aee9ffb16b9..80f0bedf5cb7 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -3003,11 +3003,12 @@ bool printk_get_next_message(struct printk_message *pmsg, u64 seq,
>  
>  /*
>   * Legacy console printing from printk() caller context does not respect
> - * raw_spinlock/spinlock nesting. For !PREEMPT_RT the lockdep warning is a
> - * false positive. For PREEMPT_RT the false positive condition does not
> - * occur.
> + * raw_spinlock/spinlock nesting. However, on PREEMPT_RT the printing
> + * path from atomic context is always avoided and the console driver
> + * is always invoked from a dedicated thread. Thus the lockdep splat
> + * on !PREEMPT_RT is a false positive.

  The legacy console always acquires a spinlock_t from its printing
  callback. This violates lock nesting if the caller acquired a
  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
  spinlock_t is okay here.

> - * This map is used to temporarily establish LD_WAIT_SLEEP context for the
> + * This map is used to temporarily establish LD_WAIT_CONFIG context for the
>   * console write() callback when legacy printing to avoid false positive
>   * lockdep complaints, thus allowing lockdep to continue to function for
>   * real issues.
> @@ -3016,7 +3017,7 @@ bool printk_get_next_message(struct printk_message *pmsg, u64 seq,
>  static inline void printk_legacy_allow_spinlock_enter(void) { }
>  static inline void printk_legacy_allow_spinlock_exit(void) { }
>  #else
> -static DEFINE_WAIT_OVERRIDE_MAP(printk_legacy_map, LD_WAIT_SLEEP);
> +static DEFINE_WAIT_OVERRIDE_MAP(printk_legacy_map, LD_WAIT_CONFIG);

We could use this lock_map_acquire_try() directly but okay having it in
one spot with a comment might have its benefit. But _why_ is here a
CONFIG_PREEMPT_RT? This is supposed to work in both cases. Should a
legacy driver be invoked on RT then the comment is not accurate, lockdep
won't yell but we still have CONFIG_DEBUG_ATOMIC_SLEEP making its own
judgement.

>  static inline void printk_legacy_allow_spinlock_enter(void)
>  {

Sebastian