linux-kernel - Re: [PATCH printk v3 13/15] printk: add kthread console printers

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: <87h76nwmjk.fsf@jogness.linutronix.de>
Date:   Wed, 20 Apr 2022 22:08:39 +0206
From:   John Ogness <john.ogness@...utronix.de>
To:     Petr Mladek <pmladek@...e.com>
Cc:     Sergey Senozhatsky <senozhatsky@...omium.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        linux-kernel@...r.kernel.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Helge Deller <deller@....de>
Subject: Re: [PATCH printk v3 13/15] printk: add kthread console printers

On 2022-04-20, Petr Mladek <pmladek@...e.com> wrote:
> On Wed 2022-04-20 01:52:35, John Ogness wrote:
>> @@ -2280,10 +2295,10 @@ asmlinkage int vprintk_emit(int facility, int level,
>>  	printed_len = vprintk_store(facility, level, dev_info, fmt, args);
>>  
>>  	/* If called from the scheduler, we can not call up(). */
>> -	if (!in_sched) {
>> +	if (!in_sched && allow_direct_printing()) {
>
> allow_direct_printing() is racy here. But I think that we could live
> with it, see below.

Well, it is not racy for its intended purpose, which is a context that
does:

printk_prefer_direct_enter();
printk();
printk_prefer_direct_exit();

It is only racy for _other_ contexts that might end up direct
printing. But since those other contexts don't have a preference, I see
no problem with it.

>> @@ -3524,7 +3774,16 @@ void defer_console_output(void)
>>  	 * New messages may have been added directly to the ringbuffer
>>  	 * using vprintk_store(), so wake any waiters as well.
>>  	 */
>> -	__wake_up_klogd(PRINTK_PENDING_WAKEUP | PRINTK_PENDING_OUTPUT);
>> +	int val = PRINTK_PENDING_WAKEUP;
>> +
>> +	/*
>> +	 * If console deferring was called with preferred direct printing,
>> +	 * make the irqwork perform the direct printing.
>> +	 */
>> +	if (atomic_read(&printk_prefer_direct))
>> +		val |= PRINTK_PENDING_DIRECT_OUTPUT;
>
> We actually need:
>
> 	/*
> 	 * Make sure that someone will handle the messages when direct
> 	 * printing is allowed. It happens when the kthreads are less
> 	 * reliable or unusable at all.
> 	 */
> 	if (allow_direct_printing())
> 		val |= PRINTK_PENDING_DIRECT_OUTPUT;

Agreed. I will update the comments appropriately as well.

> It is racy. But the same race is also in vprintk_emit().

It is not racy for the intended purpose, so I think it is fine.

> False positive is fine. console_flush_all() will bail out when
> the direct printing gets disabled in the meantime.
>
> False negative is worse. But we will still queue PRINTK_PENDING_WAKEUP
> that will try to wake up the kthreads that should still be around.
>
> And it was always problem even with console_trylock() approach.
> Failure means an expectation that someone else is doing the printing.
> It might be either a kthread or the current console_lock owner.
> But it is never guaranteed because both might be sleeping.

By "sleeping" I guess you mean "scheduled out". The console_lock owner
or mutex/atomic_t holder will be within printing code. And if a kthread
sees new records available, it will continue rather than wait.

> We do our best by calling pr_flush() or console_flush_on_panic()
> on various places. Also PRINTK_PENDING_WAKEUP will always try to wake
> up the kthreads.

Yes.

> Anyway, we should document this somewhere. At least in the commit
> message.
>
> My dream is Documentation/core-api/printk-design.rst but I do not
> want to force you to do it ;-)

I would be happy to contribute to such a document.

John