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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220421141921.GD11747@pathway.suse.cz>
Date:   Thu, 21 Apr 2022 16:25:03 +0200
From:   Petr Mladek <pmladek@...e.com>
To:     John Ogness <john.ogness@...utronix.de>
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 Wed 2022-04-20 22:08:39, John Ogness wrote:
> 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.

Make sense.

Let me think more about it. To be sure that we see all aspects:

There are also other system wide variables checked by
allow_direct_printing() and can be modified asynchronously:

	+ printk_kthreads_available
	+ system_state
	+ oops_in_progress

"oops_in_progress" and "system_state" are similar to
"printk_prefer_direct". The context that modifies this
variable will see the right value. The other contexts
do not care that much and do not need to be strictly
synchronized. Also the value means that the direct
mode is preferred but it is never guaranteed and
never was.

"printk_kthreads_available" is more tricky.
__printk_fallback_preferred_direct() causes that printk kthreads
will not be used any longer. It should make sure that the pending
messages will be printed directly. And it works because
it is called under console_lock and the pending messages
will be printed by the console_unlock().

Everything looks fine after all. I wonder if we could somehow
document it somewhere. I think about adding a comment
above allow_direct_printing() definition:

/*
 * printk() always wakes printk kthreads so that they could
 * flush the new message to the consoles. Also it tries
 * the flush the messages directly when it is allowed.
 *
 * The direct priting is allowed in situations when the kthreads
 * are not available or the system is in a problematic state.
 *
 * See the implementation about possible races.
 */
static inline bool allow_direct_printing(void)
{
	/*
	 * The kthreads are disabled under console_lock.
	 * Any pending messages will be handled by
	 * console_unlock().
	 */
	if (!printk_kthreads_available)
		return false;

	/*
	 * Prefer direct printing when the system is in a problematic
	 * state. The context that sets this state will always see
	 * the updated value. The other contexts do not care that
	 * much. Anyway, it is just a best effort. The direct output
	 * is possible only when console_lock is not already taken.
	 */
	return (system_state > SYSTEM_RUNNING ||
		oops_in_progress ||
		atomic_read(&printk_prefer_direct));
}

Best Regards,
Petr