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:	Tue, 19 Jan 2016 17:16:57 +0100
From:	Petr Mladek <pmladek@...e.com>
To:	Sergey Senozhatsky <sergey.senozhatsky@...il.com>
Cc:	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 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.


> console_unlock()    /* w/o can_use_console() in logbuf_lock section */
> ....
> again:
> 	for (;;) {
> 		raw_spin_lock_irqsave logbuf_lock
> 		msg_print_text
> 		raw_spin_unlock logbuf_lock
> 		call_console_drivers
> 		local_irq_restore
> 	}
> 
> 	up_console_sem
> 
> 	raw_spin_lock logbuf_lock
> 	retry = console_seq != log_next_seq
> 	raw_spin_unlock_irqrestore logbuf_lock
> 	
> 	if retry && console_trylock()
> 		goto again;
> 
> 
> 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;


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

It means that the code will be easier and protected against the
theoretical race.

How does that sound, please?

Best Regards,
Petr