linux-kernel - Re: [PATCH v10 1/2] printk: Make printk() completely async

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: <20160826015641.GA520@swordfish>
Date:   Fri, 26 Aug 2016 10:56:41 +0900
From:   Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
To:     Petr Mladek <pmladek@...e.com>
Cc:     Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>,
        Jan Kara <jack@...e.cz>,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Viresh Kumar <viresh.kumar@...aro.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Jan Kara <jack@...e.com>, Tejun Heo <tj@...nel.org>,
        Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Byungchul Park <byungchul.park@....com>, vlevenetz@...sol.com,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [PATCH v10 1/2] printk: Make printk() completely async

On (08/25/16 23:10), Petr Mladek wrote:
[..]
> I was so taken by the idea of temporary forcing a lockless and
> "trivial" printk implementation that I missed one thing.
> 
> Your patch use the alternative printk() variant around logbuf_lock.
> But this is not the problem with wake_up_process(). printk_deferred()
> takes logbuf_lock without problems.

you didn't miss anything, I think I wasn't too descriptive and that caused
some confusion. this patch is not a replacement of wake_up_process() patch
posted earlier in the loop, but an addition to it. not only every WARN/BUG
issued from wake_up_process() will do no good, but every lock we take is
potentially dangerous as well. In the simplest case because of $LOCK-debug.c
files in kernel/locking (spin_lock in our case); in the worst case --
because of WARNs issued by log_store() and friends (there may be custom
modifications) or by violations of spinlock atomicity requirements.

For example,

	vprintk_emit()
		local_irq_save()
		raw_spin_lock()
		text_len = vscnprintf(text, sizeof(textbuf), fmt, args)
		{
			vsnprintf()
			{
				if (WARN_ON_ONCE(size > INT_MAX))
					return 0;
			}
		}
		...

this is a rather unlikely event, sure, there must be some sort of
memory corruption or something else, but the thing is -- if it will
happen, printk() will not be willing to help.

wake_up_process() change, posted earlier, is using a deferred version of
WARN macro, but we definitely can (and we better do) switch to lockless
alternative printk() in both cases and don't bother with new macros.
replacing all of the existing ones with 'safe' deferred versions is
a difficult task, but keeping track of a newly introduced ones is even
harder (if possible at all).

> +DEFINE_PER_CPU(bool, printk_wakeup);
> +
>  asmlinkage int vprintk_emit(int facility, int level,
>                             const char *dict, size_t dictlen,
>                             const char *fmt, va_list args)
> @@ -1902,8 +1904,17 @@ asmlinkage int vprintk_emit(int facility, int level,
>         lockdep_off();
>  
>         if  (printk_kthread && !in_panic) {
> +               bool __percpu *printk_wakeup_ptr;
> +
>                 /* Offload printing to a schedulable context. */
> -               wake_up_process(printk_kthread);
> +               local_irq_save(flags);
> +               printk_wake_up_ptr = this_cpu_ptr(&printk_wake_up);
> +               if (!*printk_wakeup_ptr) {
> +                       *printk_wake_up_ptr = true;
> +                       wake_up_process(printk_kthread);
> +                       *printk_wake_up_ptr = false;
> +               }
> +               local_irq_restore(flags);
>                 goto out_lockdep;
>         } else {

this can do, thanks.
I would probably prefer, for the time being, to have a single mechanism
that we will use in both cases. something like this:

vprintk_emit()
{
	alt_printk_enter();
	...
	log_store();
	...
	alt_printk_exit();

	wakep_up_process() /* direct from async printk,
			      or indirect from console_unlock()->up() */
		alt_printk_enter();
		... enqueue task
		alt_printk_exit();
}

and we need to have some sort of rollback to default printk() if
BUG() goes to panic() (both on HAVE_ARCH_BUG and !HAVE_ARCH_BUG
platforms):

static void oops_end(...)
{
...
	if (in_interrupt())
		panic("Fatal exception in interrupt");
	if (panic_on_oops)
		panic("Fatal exception");
	if (signr)
		do_exit(signr);
}

not so sure about do_exit(). we are specifically talking here about
wake_up_process()->try_to_wake_up(), which does all of its job under
raw_spin_lock_irqsave(&p->pi_lock), so IF there is a BUG() that does
do_exit() /* hard to imagine that */, then nothing will help us out,
I think.

	-ss