linux-kernel - Re: [PATCH printk v1 1/1] printk: Avoid scheduling irq

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: <87ldkb9rnl.fsf@jogness.linutronix.de>
Date: Wed, 12 Nov 2025 16:56:22 +0106
From: John Ogness <john.ogness@...utronix.de>
To: Petr Mladek <pmladek@...e.com>
Cc: Sergey Senozhatsky <senozhatsky@...omium.org>, Steven Rostedt
 <rostedt@...dmis.org>, Sherry Sun <sherry.sun@....com>, Jacky Bai
 <ping.bai@....com>, Jon Hunter <jonathanh@...dia.com>, Thierry Reding
 <thierry.reding@...il.com>, Derek Barbosa <debarbos@...hat.com>,
 linux-kernel@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH printk v1 1/1] printk: Avoid scheduling irq_work on suspend

On 2025-11-11, Petr Mladek <pmladek@...e.com> wrote:
>> Introduce a new global variable @console_offload_blocked to flag
>
> s/console_offload_blocked/console_irqwork_blocked/

Ack.

>> when irq_work queueing is to be avoided. The flag is used by
>> printk_get_console_flush_type() to avoid allowing deferred printing
>> and switch NBCON consoles to atomic flushing. It is also used by
>> vprintk_emit() to avoid klogd waking.
>> 
>> --- a/kernel/printk/internal.h
>> +++ b/kernel/printk/internal.h
>> @@ -230,6 +230,8 @@ struct console_flush_type {
>>  	bool	legacy_offload;
>>  };
>>  
>> +extern bool console_irqwork_blocked;
>> +
>>  /*
>>   * Identify which console flushing methods should be used in the context of
>>   * the caller.
>> @@ -241,7 +243,7 @@ static inline void printk_get_console_flush_type(struct console_flush_type *ft)
>>  	switch (nbcon_get_default_prio()) {
>>  	case NBCON_PRIO_NORMAL:
>>  		if (have_nbcon_console && !have_boot_console) {
>> -			if (printk_kthreads_running)
>> +			if (printk_kthreads_running && !console_irqwork_blocked)
>>  				ft->nbcon_offload = true;
>>  			else
>>  				ft->nbcon_atomic = true;
>> @@ -251,7 +253,7 @@ static inline void printk_get_console_flush_type(struct console_flush_type *ft)
>>  		if (have_legacy_console || have_boot_console) {
>>  			if (!is_printk_legacy_deferred())
>>  				ft->legacy_direct = true;
>> -			else
>> +			else if (!console_irqwork_blocked)
>>  				ft->legacy_offload = true;
>>  		}
>>  		break;
>
> This is one possibility.
>
> Another possibility would be to block the irq work
> directly in defer_console_output() and wake_up_klogd().
> It would handle all situations, including printk_trigger_flush()
> or defer_console_output().
>
> Or is there any reason, why these two call paths are not handled?

My intention was to focus only on irq_work triggered directly by
printk() calls as well as transitioning NBCON from threaded to direct.

> I do not have strong opinion. This patch makes it more explicit
> when defer_console_output() or wake_up_klogd() is called.
>
> If we move the check into defer_console_output() or wake_up_klogd(),
> it would hide the behavior. But it will make the API more safe
> to use. And wake_up_klogd() is even exported via <linux/printk.h>.

Earlier test versions of this patch did exactly as you are suggesting
here. But I felt like "properly avoiding" deferred/offloaded printing
via printk_get_console_flush_type() (rather than just hacking
irq_work_queue() callers to abort) was a cleaner solution. Especially
since printk_get_console_flush_type() modifications were needed anyway
in order to transition NBCON from threaded to direct.

>> @@ -264,7 +266,7 @@ static inline void printk_get_console_flush_type(struct console_flush_type *ft)
>>  		if (have_legacy_console || have_boot_console) {
>>  			if (!is_printk_legacy_deferred())
>>  				ft->legacy_direct = true;
>> -			else
>> +			else if (!console_irqwork_blocked)
>>  				ft->legacy_offload = true;
>
> This change won't be needed if we move the check into
> defer_console_output() and wake_up_klogd().
>
>>  		}
>>  		break;
>> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
>> index 5aee9ffb16b9a..94fc4a8662d4b 100644
>> --- a/kernel/printk/printk.c
>> +++ b/kernel/printk/printk.c
>> @@ -2426,7 +2429,7 @@ asmlinkage int vprintk_emit(int facility, int level,
>>  
>>  	if (ft.legacy_offload)
>>  		defer_console_output();
>> -	else
>> +	else if (!console_irqwork_blocked)
>>  		wake_up_klogd();
>
> Same here.
>
>>  
>>  	return printed_len;

I would prefer to keep all the printk_get_console_flush_type() changes
since it returns proper available flush type information. If you would
like to _additionally_ short-circuit __wake_up_klogd() and
nbcon_kthreads_wake() in order to avoid all possible irq_work queueing,
I would be OK with that.

John