linux-kernel - Re: [RFC][PATCH -next 1/2] printk: move can_use_console out of console_trylock_for

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: <20160119133136.GA2148@dhcp128.suse.cz>
Date:	Tue, 19 Jan 2016 14:31:36 +0100
From:	Petr Mladek <pmladek@...e.com>
To:	Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Tejun Heo <tj@...nel.org>, Jan Kara <jack@...e.com>,
	Kyle McMartin <kyle@...nel.org>,
	Dave Jones <davej@...emonkey.org.uk>,
	Calvin Owens <calvinowens@...com>,
	linux-kernel@...r.kernel.org,
	Sergey Senozhatsky <sergey.senozhatsky@...il.com>
Subject: Re: [RFC][PATCH -next 1/2] printk: move can_use_console out of
 console_trylock_for_printk

On Tue 2016-01-19 09:42:36, Sergey Senozhatsky wrote:
> Hello,
> thanks for review.
> 
> On (01/18/16 16:42), Petr Mladek wrote:
> > On Thu 2016-01-14 13:57:22, Sergey Senozhatsky wrote:
> > > vprintk_emit() disables preemption around console_trylock_for_printk()
> > > and console_unlock() calls for a strong reason -- can_use_console()
> > > check. The thing is that vprintl_emit() can be called on a CPU that
> > > is not fully brought up yet (!cpu_online()), which potentially can
> > > cause problems if console driver accesses per-cpu data. A console
> > > driver can explicitly state that it's safe to call it from !online
> > > cpu by setting CON_ANYTIME bit in console ->flags. That's why for
> > > !cpu_online() can_use_console() iterates all the console to find out
> > > if there is a CON_ANYTIME console, otherwise console_unlock() must be
> > > avoided.
> > > 
> > > call_console_drivers(), called from console_cont_flush() and
> > > console_unlock(), does the same test during for_each_console() loop.
> > > However, we can have the following corner case. Assume that we have 2
> > > cpus -- CPU0 is online, CPU1 is !online; and no CON_ANYTIME consoles
> > > available.
> > > 
> > > CPU0 online                        CPU1 !online
> > >                                  console_trylock()
> > >                                  ...
> > >                                  console_unlock()
> > 
> > Please, where this console_unlock() comes from?
> 
> from UP* or DOWN* (_PREPARE f.e.) notifiers on this CPU, for example, we don't
> know what's going on there. what prevents it from calling console_trylock(),
> grabbing the console_sem and eventually doing console_unlock()? there is
> a can_use_console() check, but it handles only one case -- printk().

So, is it a theoretical problem or do you know about any
particular path where this happens?

Well, it might make sense to get rid of console_trylock_for_printk()
and do the can_use_console() check at the beginning of
unlock_console(). I mean to do

-	if (console_trylock_for_printk())
+	if (console_trylock())
		unlock_console();

But do we really need to repeat the check in every cycle?
can_use_console() checks available consoles and if the CPU
is online. Consoles could not get added or removed when
we own console_sem. It seems that CPUs get disabled
in a process context. Therefore it seems that it might happen
only when unlock_console() gets rescheduled. I guess that
it could not get scheduled back on an offline CPU. So, it
seems that it is enough to check can_use_console() only once at
the beginning.

Or did I miss anything?

Best Regards,
Petr