Re: [PATCH printk v5 26/30] printk: nbcon: Implement emergency sections

John Ogness
2024-05-06T10:40:03Z

On 2024-05-02, John Ogness <john.ogness@linutronix.de> wrote:
> diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
> index 8f6b3858ab27..991e2702915c 100644
> --- a/kernel/printk/nbcon.c
> +++ b/kernel/printk/nbcon.c
> +void nbcon_cpu_emergency_exit(void)
> +{
> + unsigned int *cpu_emergency_nesting;
> + bool do_trigger_flush = false;
> +
> + cpu_emergency_nesting = nbcon_get_cpu_emergency_nesting();
> +
> + /*
> + * Flush the messages before enabling preemtion to see them ASAP.
> + *
> + * Reduce the risk of potential softlockup by using the
> + * flush_pending() variant which ignores messages added later. It is
> + * called before decrementing the counter so that the printing context
> + * for the emergency messages is NBCON_PRIO_EMERGENCY.
> + */
> + if (*cpu_emergency_nesting == 1) {
> + nbcon_atomic_flush_pending();
> + do_trigger_flush = true;
> + }
> +
> + (*cpu_emergency_nesting)--;
> +
> + if (WARN_ON_ONCE(*cpu_emergency_nesting < 0))
> + *cpu_emergency_nesting = 0;

I see two issues here. First, this is unsigned. kernel test robot
reported:

> kernel/printk/nbcon.c:1540 nbcon_cpu_emergency_exit() warn: unsigned
> '*cpu_emergency_nesting' is never less than zero.

Also, in this situation, we are allowing a brief window of activated
emergency mode by allowing it to become !0 before correcting
it. Instead, we should avoid illegally decrementing. I suggest:

void nbcon_cpu_emergency_exit(void)
{
unsigned int *cpu_emergency_nesting;
bool do_trigger_flush = false;

cpu_emergency_nesting = nbcon_get_cpu_emergency_nesting();

/*
* Flush the messages before enabling preemtion to see them ASAP.
*
* Reduce the risk of potential softlockup by using the
* flush_pending() variant which ignores messages added later. It is
* called before decrementing the counter so that the printing context
* for the emergency messages is NBCON_PRIO_EMERGENCY.
*/
if (*cpu_emergency_nesting == 1) {
nbcon_atomic_flush_pending();
do_trigger_flush = true;
}

if (!WARN_ON_ONCE(*cpu_emergency_nesting == 0))
(*cpu_emergency_nesting)--;

preempt_enable();

if (do_trigger_flush)
printk_trigger_flush();
}

John