linux-kernel - Re: [PATCHv2 4/4] printk: make sure we always print console disabled message

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] [day] [month] [year] [list]

Date:   Mon, 27 May 2019 14:45:01 +0200
From:   Petr Mladek <pmladek@...e.com>
To:     Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
Cc:     Steven Rostedt <rostedt@...dmis.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        linux-kernel@...r.kernel.org,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>
Subject: Re: [PATCHv2 4/4] printk: make sure we always print console disabled
 message

On Thu 2019-05-23 15:59:47, Sergey Senozhatsky wrote:
> On (05/15/19 16:47), Petr Mladek wrote:
> > On Fri 2019-04-26 14:44:45, Sergey Senozhatsky wrote:
> > > 
> > > Forgot to mention that the series is still in RFC phase.
> > > 
> > > 
> > > On (04/26/19 14:33), Sergey Senozhatsky wrote:
> > > [..]
> > > > +++ b/kernel/printk/printk.c
> > > > @@ -2613,6 +2613,12 @@ static int __unregister_console(struct console *console)
> > > >  	pr_info("%sconsole [%s%d] disabled\n",
> > > >  		(console->flags & CON_BOOT) ? "boot" : "",
> > > >  		console->name, console->index);
> > > > +	/*
> > > > +	 * Print 'console disabled' on all the consoles, including the
> > > > +	 * one we are about to unregister.
> > > > +	 */
> > > > +	console_unlock();
> > > > +	console_lock();
> > > >  
> > > >  	res = _braille_unregister_console(console);
> > > >  	if (res)
> > > 
> > > Need to think more if this is race free...
> > 
> > I am afraid that it is racy against for_each_console() when
> > removing the boot consoles.
> 
> Can you explain? Do you mean that we can execute two paths unregistering
> the same bcon?
> 
> 	CPU0					CPU1
> 
> 	console_lock();
> 	__unregister_console(bconA)		console_lock();
> 	console_unlock();
> 
> 						__unregister_console(bconA);
> 						for (a = console_drivers->next ...)
> 							if (a == console)
> 								unregister();
> 							// console bconA is
> 							// not in the list
> 							// anymore
> 						console_unlock();
> 	for (a = console_drivers->next ...)
> 		if (a == console)
> 	console_unlock();
> 
> 
> This CPU0 will never see bconA in the console drivers list.
> But... it will try to do
> 
> 	console->flags &= ~CON_ENABLED;
> 
> Which we need to fix.

I have to admit that I expected more races. But the most intuitive ones
are avoided by the 2nd for-cycle in __unregister_console(). As a
result, most operations are ignored when the console was
already unregistered in parallel.

Anyway, it is really tricky. My head is still spinning around it.
The console_lock handling is really hard to follow. And it is
error prone.

To make it clear. The code has already been tricky. Your patchset has
a potential. It fixes some races but it still keeps the code tricky
anoter way.

OK, all these nasty hacks are needed only because we need to flush
the messages to the console.

Much cleaner solution would be to refactor console_unlock()
and allow to handle existing messages using a separate function.
It is perfectly safe to flush all existing messages to all registered
consoles without releasing console_lock.

How does that sound, please?

Best Regards,
Petr