Re: [PATCH v3 5/6] printk: nbcon: move printk_delay to console emiting code

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] printk_delay(true) busy-waits with IRQs disabled inside the console lock spinning critical section, causing cross-CPU hard lockups.
- [High] Per-message delays inside the flush loop hold console_sem and SRCU read lock hostage, severely blocking the console subsystem.
--

--- Patch [5]: [PATCH v3 5/6] printk: nbcon: move printk_delay to console emiting code ---
commit 34b9c50abab2d207953ba74a453cbbd56801857d
Author: Andrew Murray <[email protected]>

printk: nbcon: move printk_delay to console emiting code

This commit moves the printk_delay logic from the printk() calling context
into the console emitting code. The goal is to ensure that asynchronous
console emission respects the delay without stalling the original caller.

> diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
> index 4b03b019cd5ee..c8427906b1c54 100644
> --- a/kernel/printk/nbcon.c
> +++ b/kernel/printk/nbcon.c
[ ... ]
> @@ -1525,6 +1532,8 @@ bool nbcon_legacy_emit_next_record(struct console *con, bool *handover,
>  	}
>  
>  	progress = nbcon_emit_one(&wctxt, use_atomic);
> +	if (progress && ctxt->emitted)
> +		printk_delay(use_atomic);

[Severity: High]
Does this code hold console_sem and SRCU read lock hostage for the duration
of the delay?

If a user configures a non-zero printk_delay_msec, when legacy consoles are
flushed, the thread holds console_sem and console_srcu_read_lock(). Inside
the loop, the delay is now applied per-record while holding global locks.

A burst of messages will multiply the delay, holding console_sem hostage for
an extended period, which blocks TTY operations, CPU hotplug, module loading,
and suspend/resume which attempt to acquire console_lock().

Prior to this patch, the delay was isolated in vprintk_emit() before any
locks were taken.

>  
>  	if (use_atomic) {
>  		start_critical_timings();
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 4517266e9ca11..5df09e56cf8ab 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
[ ... ]
> @@ -3206,6 +3205,7 @@ static bool console_emit_next_record(struct console *con, bool *handover, int co
>  		printk_legacy_allow_spinlock_enter();
>  		con->write(con, outbuf, pmsg.outbuf_len);
>  		printk_legacy_allow_spinlock_exit();
> +		printk_delay(true);
>  
>  		start_critical_timings();
>  
> @@ -3214,6 +3214,7 @@ static bool console_emit_next_record(struct console *con, bool *handover, int co
>  		*handover = console_lock_spinning_disable_and_check(cookie);
>  		printk_safe_exit_irqrestore(flags);
>  	}

[Severity: High]
Can this busy-wait cause a cross-CPU hard lockup?

When printk_delay(true) executes, it uses an mdelay() loop while the CPU
holds the console owner state, console lock spinning is enabled, and local
interrupts are disabled (via printk_safe_enter_irqsave()).

If another CPU concurrently calls printk(), it enters
console_trylock_spinning() and spins indefinitely with its own interrupts
disabled, waiting for the first CPU to release the spinning lock.

Since the spinning CPU does not call touch_nmi_watchdog(), it will hard
lockup while waiting for the owner's mdelay() loop to finish for each
pending message.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5