linux-kernel - Re: [RFC][PATCH v3 4/4] printk: set may_schedule for some of console

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160211144105.GM3305@pathway.suse.cz>
Date:	Thu, 11 Feb 2016 15:41:05 +0100
From:	Petr Mladek <pmladek@...e.com>
To:	Sergey Senozhatsky <sergey.senozhatsky@...il.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Jan Kara <jack@...e.com>, Tejun Heo <tj@...nel.org>,
	Kyle McMartin <kyle@...nel.org>,
	Dave Jones <davej@...emonkey.org.uk>,
	Calvin Owens <calvinowens@...com>,
	linux-kernel@...r.kernel.org,
	Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
Subject: Re: [RFC][PATCH v3 4/4] printk: set may_schedule for some of
 console_trylock callers

On Sat 2016-01-23 17:15:13, Sergey Senozhatsky wrote:
> console_unlock() allows to cond_resched() if its caller has
> set `console_may_schedule' to 1, since
> 'commit 8d91f8b15361 ("printk: do cond_resched() between lines while
> outputting to consoles")'.
> 
> The rules are:
> -- console_lock() always sets `console_may_schedule' to 1
> -- console_trylock() always sets `console_may_schedule' to 0
> 
> However, console_trylock() callers (among them is printk()) do
> not always call printk() from atomic contexts, and some of them
> can cond_resched() in console_unlock(), so console_trylock()
> can set `console_may_schedule' to 1 for such processes.
> 
> For !CONFIG_PREEMPT_COUNT kernels, however, console_trylock()
> always sets `console_may_schedule' to 0.
> 
> It's possible to drop explicit preempt_disable()/preempt_enable()
> in vprintk_emit(), because console_unlock() and console_trylock()
> are now smart enough:
> a) console_unlock() does not cond_resched() when it's unsafe
>   (console_trylock() takes care of that)
> b) console_unlock() does can_use_console() check.
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@...il.com>
> ---
>  kernel/printk/printk.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 99925ce..097ca8b 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -1769,20 +1769,12 @@ asmlinkage int vprintk_emit(int facility, int level,
>  	if (!in_sched) {
>  		lockdep_off();
>  		/*
> -		 * Disable preemption to avoid being preempted while holding
> -		 * console_sem which would prevent anyone from printing to
> -		 * console
> -		 */
> -		preempt_disable();
> -
> -		/*
>  		 * Try to acquire and then immediately release the console
>  		 * semaphore.  The release will print out buffers and wake up
>  		 * /dev/kmsg and syslog() users.
>  		 */
>  		if (console_trylock())
>  			console_unlock();
> -		preempt_enable();
>  		lockdep_on();
>  	}
>  
> @@ -2115,7 +2107,16 @@ int console_trylock(void)
>  		return 0;
>  	}
>  	console_locked = 1;
> -	console_may_schedule = 0;
> +	/*
> +	 * On !PREEMPT_COUNT kernels we can't reliably detect if it's safe
> +	 * to schedule -- e.g. calling printk while holding a spin_lock,
> +	 * because preempt_disable()/preempt_enable() are just barriers and
> +	 * don't modify preempt_count() there. console_may_schedule is
> +	 * always 0 on !PREEMPT_COUNT kernels.
> +	 */
> +	console_may_schedule = !oops_in_progress &&
> +			preemptible() &&
> +			!rcu_preempt_depth();
>  	return 1;

We discussed this a lot but I am still a bit nervous ;-)

Avoid scheduling when oops_in_progress makes sense.

preemptible() takes care of preemption and IRQ contexts.
The comment above explains that it is safe to use here.

The check for rcu_preempt_depth() makes sense. But is it
safe, please?

rcu_preempt_depth() returns 0 if CONFIG_PREEMPT_RCU is not
enabled. It means that you are not able to detect RCU read
section and it might cause problems.

I rather add Paul into CC.

Best Regards,
Petr