linux-kernel - Re: [RFC PATCH] nmi,printk: fix ABBA deadlock between nmi_backtrace and dump_stack

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: <87plrcqyii.fsf@jogness.linutronix.de>
Date: Wed, 17 Jul 2024 09:22:21 +0206
From: John Ogness <john.ogness@...utronix.de>
To: Rik van Riel <riel@...riel.com>, Andrew Morton <akpm@...ux-foundation.org>
Cc: Omar Sandoval <osandov@...a.com>, linux-kernel@...r.kernel.org, Petr
 Mladek <pmladek@...e.com>, Steven Rostedt <rostedt@...dmis.org>, Sergey
 Senozhatsky <senozhatsky@...omium.org>
Subject: Re: [RFC PATCH] nmi,printk: fix ABBA deadlock between nmi_backtrace
 and dump_stack_lvl

On 2024-07-15, Rik van Riel <riel@...riel.com> wrote:
> Both nmi_backtrace and dump_stack_lvl call printk_cpu_sync_get_irqsave.
>
> However, dump_stack_lvl will call into the printk code, while holding
> the printk_cpu_sync_get lock, and then take the console lock.
>
> Another CPU may end up getting an NMI stack trace printed, after
> being stuck printing something to serial console for too long,
> with the console lock held.
>
> This results in the following lock order:
> CPU A: printk_cpu_sync_get lock -> console_lock
> CPU B: console_lock -> (nmi) printk_cpu_sync_get lock
>
> This will cause the system to hang with an ABBA deadlock

The console lock is acquired via trylock, so that will not yield
deadlock here. However, if CPU B was printing, then CPU A will spin on
@console_waiter (in console_trylock_spinning()). _That_ is a deadlock.

The purpose of printk_cpu_sync_get_irqsave() is to avoid having the
different backtraces being interleaved in the _ringbuffer_. It really
isn't necessary that they are printed in that context. And indeed, there
is no guarantee that they will be printed in that context anyway.

Perhaps a simple solution would be for printk_cpu_sync_get_irqsave() to
call printk_deferred_enter/_exit. Something like the below patch.

John Ogness

diff --git a/include/linux/printk.h b/include/linux/printk.h
index 65c5184470f1..1a6f5aac28bf 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -315,8 +315,10 @@ extern void __printk_cpu_sync_put(void);
 #define printk_cpu_sync_get_irqsave(flags)		\
 	for (;;) {					\
 		local_irq_save(flags);			\
+		printk_deferred_enter();		\
 		if (__printk_cpu_sync_try_get())	\
 			break;				\
+		printk_deferred_exit();			\
 		local_irq_restore(flags);		\
 		__printk_cpu_sync_wait();		\
 	}
@@ -329,6 +331,7 @@ extern void __printk_cpu_sync_put(void);
 #define printk_cpu_sync_put_irqrestore(flags)	\
 	do {					\
 		__printk_cpu_sync_put();	\
+		printk_deferred_exit();		\
 		local_irq_restore(flags);	\
 	} while (0)