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]

Date:	Wed, 20 Jan 2016 13:18:04 +0900
From:	Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
To:	Petr Mladek <pmladek@...e.com>
Cc:	Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
	Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>,
	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
Subject: Re: [RFC][PATCH -next 1/2] printk: move can_use_console out of
 console_trylock_for_printk

On (01/19/16 17:16), Petr Mladek wrote:
> On Wed 2016-01-20 00:00:40, Sergey Senozhatsky wrote:
> > > But do we really need to repeat the check in every cycle?
> > 
> > well, on every iteration in the best case we check cpu_online()
> > only. which is what we would have done anyway in vprintk_emit(),
> > so no additional checks added. at the same time call_console_drivers
> > does not do '!cpu_online && !CON_ANYTIME' for each console now, so
> > in some sense there are less checks now (this is far even from a
> > micro-optimization, just noted).
> 
> Hmm, we need to keep the check in call_console_drivers(). It iterates
> over all registered consoles. Only some of them could habe CON_ANYTIME
> flag. We need to skip the others when the CPU is not online.

oh... good point, you are right! my bad. so we basically need both. the
first one (can_use_console() before call_console_drivers()) ensures that
it's _generally_ OK to call call_console_drivers() later and that it will
not drain messages, the second one _in_ call_console_drivers() filters out
only CON_ANYTIME consoles if !cpu_online(), but by this time we are sure
that there is at least one CON_ANYTIME console.

[..]
> > when we up_console_sem(), consoles may appear and disappear, since we
> > don't keep the semaphore. Suppose that we have OFFLINE cpu and we had a
> > CON_ANYTIME console, but in between up_console_sem--console_trylock
> > that single CON_ANYTIME console was removed. So now we have !cpu_online
> > and !CON_ANYTIME.
> 
> Ah, I have missed that the console_sem is released/taken before goto
> again. Hmm, your proposed solution adds even more twists into the code.
> I think how to make it easier. I think that we could move the again:
> target and call console_cont_flush() when there is some new content.
> It is a corner case, anyway. Then we could do:
> 
> 
> 	do_cond_resched = console_may_schedule;
> 	console_may_schedule = 0;
> 
> +again:
> +	if (!can_use_console()) {
> +		console_locked = 0;
> +		up_console_sem();
> +		return;
> 	}
> 
> 	/* flush buffered message fragment immediately to console */
> 	console_cont_flush(text, sizeof(text));
> -again:
> 	for (;;) {
> 		struct printk_log *msg;
> 

looks better. we do extra IRQ disable/enable (spin lock irq) when we jump
to `again' label, but I don't think this introduces any significant overhead.
however, if it does, we always can avoid extra console_cont_flush() by simply
checking @retry -- it's `false' only once, when we execute this part for
the first time in the current console_unlock() call; all goto-jumps imply
that @retry is `true'.

IOW:

	bool retry = false;

again:
	can_use_console

	if (!retry)	/* if we jumped to again, retry is true */
		console_cont_flush
	
	for (;;) {
		...
	}

	retry = ...

	if retry && console_trylock()
		goto retry;


> Then we remove this check from console_trylock_for_printk() and
> eventually remove this function altogether.

yes, that was the plan :)

> It means that the code will be easier and protected against the
> theoretical race.
> 
> How does that sound, please?

sounds good, thanks.

> Best Regards,
> Petr

	-ss