[PATCH printk v1 0/2] Fix ABBA deadlock for legacy printing with cpu_sync

[PATCH printk v1 0/2] Fix ABBA deadlock for legacy printing with cpu_sync

[John Ogness]

Hi,

An RFC patch [0] was posted to fix an ABBA deadlock related to
nmi_backtrace() and dump_stack_lvl(). However, the underlying
problem was trying to perform legacy printing while holding the
printk_cpu_sync.

This series causes legacy printing to defer when holding the
printk_cpu_sync.

The first patch in the series is not necessary for the deadlock
fix, but was a redundancy I noticed while working on the series.

John Ogness

[0] https://lore.kernel.org/lkml/20240715232052.73eb7fb1@imladris.surriel.com

John Ogness (2):
  printk: Remove redundant deferred check in vprintk()
  printk: Defer legacy printing when holding printk_cpu_sync

 kernel/printk/internal.h    |  6 ++++++
 kernel/printk/printk.c      |  5 +++++
 kernel/printk/printk_safe.c | 16 ++++++----------
 3 files changed, 17 insertions(+), 10 deletions(-)


base-commit: 4022ef25504db2fb79a2acac0afe9bac934f8dd6
-- 
2.39.5

[PATCH printk v1 1/2] printk: Remove redundant deferred check in vprintk()

[John Ogness]

The helper printk_get_console_flush_type() is already calling
is_printk_legacy_deferred() to determine if legacy printing is
to be offloaded. Therefore there is no need for vprintk() to
perform this check as well. Remove the redundant check from
vprintk().

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 kernel/printk/printk_safe.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/kernel/printk/printk_safe.c b/kernel/printk/printk_safe.c
index 6f94418d53ff..6283bc0b55e6 100644
--- a/kernel/printk/printk_safe.c
+++ b/kernel/printk/printk_safe.c
@@ -74,15 +74,6 @@ asmlinkage int vprintk(const char *fmt, va_list args)
 	if (unlikely(kdb_trap_printk && kdb_printf_cpu < 0))
 		return vkdb_printf(KDB_MSGSRC_PRINTK, fmt, args);
 #endif
-
-	/*
-	 * Use the main logbuf even in NMI. But avoid calling console
-	 * drivers that might have their own locks.
-	 */
-	if (is_printk_legacy_deferred())
-		return vprintk_deferred(fmt, args);
-
-	/* No obstacles. */
 	return vprintk_default(fmt, args);
 }
 EXPORT_SYMBOL(vprintk);
-- 
2.39.5

[PATCH printk v1 2/2] printk: Defer legacy printing when holding printk_cpu_sync

[John Ogness]

The documentation of printk_cpu_sync_get() clearly states
that the owner must never perform any activities where it waits
for a CPU. For legacy printing there can be spinning on the
console_lock and on the port lock. Therefore legacy printing
must be deferred when holding the printk_cpu_sync.

Note that in the case of emergency states, atomic consoles
are not prevented from printing when printk is deferred. This
is appropriate because they do not spin-wait indefinitely for
other CPUs.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
Fixes: 55d6af1d6688 ("lib/nmi_backtrace: explicitly serialize banner and regs")
---
 kernel/printk/internal.h    | 6 ++++++
 kernel/printk/printk.c      | 5 +++++
 kernel/printk/printk_safe.c | 7 ++++++-
 3 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h
index c6bb47666aef..a91bdf802967 100644
--- a/kernel/printk/internal.h
+++ b/kernel/printk/internal.h
@@ -338,3 +338,9 @@ bool printk_get_next_message(struct printk_message *pmsg, u64 seq,
 void console_prepend_dropped(struct printk_message *pmsg, unsigned long dropped);
 void console_prepend_replay(struct printk_message *pmsg);
 #endif
+
+#ifdef CONFIG_SMP
+bool is_printk_cpu_sync_owner(void);
+#else
+static inline bool is_printk_cpu_sync_owner(void) { return false; }
+#endif
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index d27a64d58023..0856a77c4b7a 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -4916,6 +4916,11 @@ void console_try_replay_all(void)
 static atomic_t printk_cpu_sync_owner = ATOMIC_INIT(-1);
 static atomic_t printk_cpu_sync_nested = ATOMIC_INIT(0);
 
+bool is_printk_cpu_sync_owner(void)
+{
+	return (atomic_read(&printk_cpu_sync_owner) == raw_smp_processor_id());
+}
+
 /**
  * __printk_cpu_sync_wait() - Busy wait until the printk cpu-reentrant
  *                            spinning lock is not owned by any CPU.
diff --git a/kernel/printk/printk_safe.c b/kernel/printk/printk_safe.c
index 6283bc0b55e6..32a28f563b13 100644
--- a/kernel/printk/printk_safe.c
+++ b/kernel/printk/printk_safe.c
@@ -61,10 +61,15 @@ bool is_printk_legacy_deferred(void)
 	/*
 	 * The per-CPU variable @printk_context can be read safely in any
 	 * context. CPU migration is always disabled when set.
+	 *
+	 * A context holding the printk_cpu_sync must not spin waiting for
+	 * another CPU. For legacy printing, it could be the console_lock
+	 * or the port lock.
 	 */
 	return (force_legacy_kthread() ||
 		this_cpu_read(printk_context) ||
-		in_nmi());
+		in_nmi() ||
+		is_printk_cpu_sync_owner());
 }
 
 asmlinkage int vprintk(const char *fmt, va_list args)
-- 
2.39.5

Re: [PATCH printk v1 1/2] printk: Remove redundant deferred check in vprintk()

[Petr Mladek]

On Mon 2024-12-09 12:23:45, John Ogness wrote:
> The helper printk_get_console_flush_type() is already calling
> is_printk_legacy_deferred() to determine if legacy printing is
> to be offloaded. Therefore there is no need for vprintk() to
> perform this check as well. Remove the redundant check from
> vprintk().
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Great catch!

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH printk v1 2/2] printk: Defer legacy printing when holding printk_cpu_sync

[Petr Mladek]

On Mon 2024-12-09 12:23:46, John Ogness wrote:
> The documentation of printk_cpu_sync_get() clearly states
> that the owner must never perform any activities where it waits
> for a CPU. For legacy printing there can be spinning on the
> console_lock and on the port lock. Therefore legacy printing
> must be deferred when holding the printk_cpu_sync.
> 
> Note that in the case of emergency states, atomic consoles
> are not prevented from printing when printk is deferred. This
> is appropriate because they do not spin-wait indefinitely for
> other CPUs.
> 

It might be worth adding a reference to the original report
to show that the problem is real.

Reported-by: Rik van Riel <riel@surriel.com>
Closes: https://lore.kernel.org/r/20240715232052.73eb7fb1@imladris.surriel.com

> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> Fixes: 55d6af1d6688 ("lib/nmi_backtrace: explicitly serialize banner and regs")

Anyway, it looks good.

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

PS: I am going to wait few more days for eventual feedback. I'll push
    it when nobody complains.

    I could add the above mentioned references when pushing.

Re: [PATCH printk v1 2/2] printk: Defer legacy printing when holding printk_cpu_sync

[John Ogness]

On 2024-12-11, Petr Mladek <pmladek@suse.com> wrote:
> It might be worth adding a reference to the original report
> to show that the problem is real.
>
> Reported-by: Rik van Riel <riel@surriel.com>
> Closes: https://lore.kernel.org/r/20240715232052.73eb7fb1@imladris.surriel.com

Agreed.

>> Signed-off-by: John Ogness <john.ogness@linutronix.de>
>> Fixes: 55d6af1d6688 ("lib/nmi_backtrace: explicitly serialize banner and regs")
>
> Anyway, it looks good.
>
> Reviewed-by: Petr Mladek <pmladek@suse.com>
>
>     I could add the above mentioned references when pushing.

Great, thanks!

John Ogness

Re: [PATCH printk v1 2/2] printk: Defer legacy printing when holding printk_cpu_sync

[Petr Mladek]

On Wed 2024-12-11 17:54:31, John Ogness wrote:
> On 2024-12-11, Petr Mladek <pmladek@suse.com> wrote:
> > It might be worth adding a reference to the original report
> > to show that the problem is real.
> >
> > Reported-by: Rik van Riel <riel@surriel.com>
> > Closes: https://lore.kernel.org/r/20240715232052.73eb7fb1@imladris.surriel.com
> 
> Agreed.
> 
> >> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> >> Fixes: 55d6af1d6688 ("lib/nmi_backtrace: explicitly serialize banner and regs")
> >
> > Anyway, it looks good.
> >
> > Reviewed-by: Petr Mladek <pmladek@suse.com>
> >
> >     I could add the above mentioned references when pushing.
> 
> Great, thanks!

JFYI, the patchset has been committed into printk/linux.git,
branch for-6.14-cpu_sync-fixup.

Best Regards,
Petr

Re: [PATCH printk v1 2/2] printk: Defer legacy printing when holding printk_cpu_sync

[John Ogness]

Hi Petr,

On 2024-12-16, Petr Mladek <pmladek@suse.com> wrote:
> JFYI, the patchset has been committed into printk/linux.git,
> branch for-6.14-cpu_sync-fixup.

I noticed that series is not yet merged into the printk/linux.git
"for-6.14" branch. This email is just a ping so that you do not forget
it for the upcoming merge window. ;-)

John

Re: [PATCH printk v1 2/2] printk: Defer legacy printing when holding printk_cpu_sync

[Petr Mladek]

On Fri 2025-01-10 12:28:12, John Ogness wrote:
> Hi Petr,
> 
> On 2024-12-16, Petr Mladek <pmladek@suse.com> wrote:
> > JFYI, the patchset has been committed into printk/linux.git,
> > branch for-6.14-cpu_sync-fixup.
> 
> I noticed that series is not yet merged into the printk/linux.git
> "for-6.14" branch. This email is just a ping so that you do not forget
> it for the upcoming merge window. ;-)

All is fine. This patchset has been in linux-next since Dec 16.
And I am going to include it in the pull request for 6.14.

The branch "for-6.14" is intended only for simple fixes.
This patchset is committed into the separate "topic" branch
"for-6.14-cpu_sync-fixup".

Both branches are merged into "for-next" branch separately.

I am sorry for confusion but I have never merged the topic
branches into "for-<version>" ones.

Best Regards,
Petr