[PATCH v4 0/2] let printk()/console_trylock() callers to cond_resched()

[PATCH v4 0/2] let printk()/console_trylock() callers to cond_resched()

[Sergey Senozhatsky]

Hello,

console_unlock() allows to cond_resched() if its caller has
set `console_may_schedule' to 1 (this functionality present
since commit '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

printk() calls console_unlock() with preemption desabled, which
basically can lead to RCU stalls, watchdog soft lockups, etc. if
something is simultaneously calling printk() frequent enough (IOW,
console_sem owner always has new data to send to console divers
and can't leave console_unlock() for a long time).

printk()->console_trylock() callers do not necessarily execute in
atomic contexts, and some of them can cond_resched() in
console_unlock(). console_trylock() can set `console_may_schedule'
to 1 (allow cond_resched() later in consoe_unlock()) when it's safe.

v4:
-- added "printk: check CON_ENABLED in have_callable_console()" patch
-- drop a "optional optimization" patch from the series (Petr Mladek)
-- merge 0001 and "drop console_trylock_for_printk" patches (Petr Mladek)
-- tweak console_trylock() comment (Petr Mladek)

v2-v3 (thanks to Petr Mladek for reviews):
-- do not call can_use_console() on every iteration in console_unlock() (Petr Mladek)
-- move "This stops the holder of console_sem.." comment (noted by Petr Mladek)
-- take extra care of !PREEMPT_COUNT kernels (Petr Mladek)
-- call_console_drivers() still must check cpu_online && CON_ANYTIME (Petr Mladek)
-- removed console_trylock_for_printk() (noted by Petr Mladek)

v1-v2:
-- make have_callable_console() available for !PRINTK configs (lkp@intel.com)
-- take care of RCU preempt kernels in console_trylock()


Sergey Senozhatsky (3):
  printk: move can_use_console out of console_trylock_for_printk
  printk: set may_schedule for some of console_trylock callers
  printk: check CON_ENABLED in have_callable_console()

 kernel/printk/printk.c | 121 +++++++++++++++++++++++--------------------------
 1 file changed, 57 insertions(+), 64 deletions(-)

-- 
2.7.1

[PATCH v4 1/3] printk: move can_use_console out of console_trylock_for_printk

[Sergey Senozhatsky]

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 wants to access 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.

can_use_console() ensures that console_unlock() call is safe in
vprintk_emit() only; console_lock() and console_trylock() are not
covered by this check. Even though call_console_drivers(), invoked
from console_cont_flush() and console_unlock(), tests
`!cpu_online() && CON_ANYTIME' for_each_console(), it may be
too late, which can result in messages loss.

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()
                                   console_cont_flush
                                     spin_lock logbuf_lock
                                     if (!cont.len) {
                                        spin_unlock logbuf_lock
                                        return
                                     }
                                   for (;;) {
vprintk_emit
  spin_lock logbuf_lock
  log_store
  spin_unlock logbuf_lock
                                     spin_lock logbuf_lock
  !console_trylock_for_printk        msg_print_text
 return                              console_idx = log_next()
                                     console_seq++
                                     console_prev = msg->flags
                                     spin_unlock logbuf_lock

                                     call_console_drivers()
                                       for_each_console(con) {
                                         if (!cpu_online() &&
                                             !(con->flags & CON_ANYTIME))
                                                 continue;
                                         }
                                   /*
                                    * no message printed, we lost it
                                    */
vprintk_emit
  spin_lock logbuf_lock
  log_store
  spin_unlock logbuf_lock
  !console_trylock_for_printk
 return
                                   /*
                                    * go to the beginning of the loop,
                                    * find out there are new messages,
                                    * lose it
                                    */
                                   }

console_trylock()/console_lock() call on CPU1 may come from cpu
notifiers registered on that CPU. Since notifiers are not getting
unregistered when CPU is going DOWN, all of the notifiers receive
notifications during CPU UP. For example, on my x86_64, I see around
50 notification sent from offline CPU to itself

 [swapper/2] from cpu:2 to:2 action:CPU_STARTING hotplug_hrtick
 [swapper/2] from cpu:2 to:2 action:CPU_STARTING blk_mq_main_cpu_notify
 [swapper/2] from cpu:2 to:2 action:CPU_STARTING blk_mq_queue_reinit_notify
 [swapper/2] from cpu:2 to:2 action:CPU_STARTING console_cpu_notify

while doing
  echo 0 > /sys/devices/system/cpu/cpu2/online
  echo 1 > /sys/devices/system/cpu/cpu2/online

So grabbing the console_sem lock while CPU is !online is possible,
in theory.

This patch moves can_use_console() check out of
console_trylock_for_printk(). Instead it calls it in
console_unlock(), so now console_lock()/console_unlock() are
also 'protected' by can_use_console(). This also means that
console_trylock_for_printk() is not really needed anymore
and can be removed.

Reviewed-by: Petr Mladek <pmladek@suse.com>
Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
 kernel/printk/printk.c | 97 ++++++++++++++++++++++----------------------------
 1 file changed, 42 insertions(+), 55 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 7ebcfea..0d65a81 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -1484,58 +1484,6 @@ static void zap_locks(void)
 	sema_init(&console_sem, 1);
 }
 
-/*
- * Check if we have any console that is capable of printing while cpu is
- * booting or shutting down. Requires console_sem.
- */
-static int have_callable_console(void)
-{
-	struct console *con;
-
-	for_each_console(con)
-		if (con->flags & CON_ANYTIME)
-			return 1;
-
-	return 0;
-}
-
-/*
- * Can we actually use the console at this time on this cpu?
- *
- * Console drivers may assume that per-cpu resources have been allocated. So
- * unless they're explicitly marked as being able to cope (CON_ANYTIME) don't
- * call them until this CPU is officially up.
- */
-static inline int can_use_console(unsigned int cpu)
-{
-	return cpu_online(cpu) || have_callable_console();
-}
-
-/*
- * Try to get console ownership to actually show the kernel
- * messages from a 'printk'. Return true (and with the
- * console_lock held, and 'console_locked' set) if it
- * is successful, false otherwise.
- */
-static int console_trylock_for_printk(void)
-{
-	unsigned int cpu = smp_processor_id();
-
-	if (!console_trylock())
-		return 0;
-	/*
-	 * If we can't use the console, we need to release the console
-	 * semaphore by hand to avoid flushing the buffer. We need to hold the
-	 * console semaphore in order to do this test safely.
-	 */
-	if (!can_use_console(cpu)) {
-		console_locked = 0;
-		up_console_sem();
-		return 0;
-	}
-	return 1;
-}
-
 int printk_delay_msec __read_mostly;
 
 static inline void printk_delay(void)
@@ -1683,7 +1631,6 @@ asmlinkage int vprintk_emit(int facility, int level,
 	boot_delay_msec(level);
 	printk_delay();
 
-	/* This stops the holder of console_sem just where we want him */
 	local_irq_save(flags);
 	this_cpu = smp_processor_id();
 
@@ -1707,6 +1654,7 @@ asmlinkage int vprintk_emit(int facility, int level,
 	}
 
 	lockdep_off();
+	/* This stops the holder of console_sem just where we want him */
 	raw_spin_lock(&logbuf_lock);
 	logbuf_cpu = this_cpu;
 
@@ -1832,7 +1780,7 @@ asmlinkage int vprintk_emit(int facility, int level,
 		 * semaphore.  The release will print out buffers and wake up
 		 * /dev/kmsg and syslog() users.
 		 */
-		if (console_trylock_for_printk())
+		if (console_trylock())
 			console_unlock();
 		preempt_enable();
 		lockdep_on();
@@ -2177,6 +2125,33 @@ int is_console_locked(void)
 	return console_locked;
 }
 
+/*
+ * Check if we have any console that is capable of printing while cpu is
+ * booting or shutting down. Requires console_sem.
+ */
+static int have_callable_console(void)
+{
+	struct console *con;
+
+	for_each_console(con)
+		if (con->flags & CON_ANYTIME)
+			return 1;
+
+	return 0;
+}
+
+/*
+ * Can we actually use the console at this time on this cpu?
+ *
+ * Console drivers may assume that per-cpu resources have been allocated. So
+ * unless they're explicitly marked as being able to cope (CON_ANYTIME) don't
+ * call them until this CPU is officially up.
+ */
+static inline int can_use_console(void)
+{
+	return cpu_online(raw_smp_processor_id()) || have_callable_console();
+}
+
 static void console_cont_flush(char *text, size_t size)
 {
 	unsigned long flags;
@@ -2247,9 +2222,21 @@ void console_unlock(void)
 	do_cond_resched = console_may_schedule;
 	console_may_schedule = 0;
 
+again:
+	/*
+	 * We released the console_sem lock, so we need to recheck if
+	 * cpu is online and (if not) is there at least one CON_ANYTIME
+	 * console.
+	 */
+	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;
 		size_t ext_len = 0;
-- 
2.7.1

[PATCH v4 2/3] printk: set may_schedule for some of console_trylock callers

[Sergey Senozhatsky]

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@gmail.com>
---
 kernel/printk/printk.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 0d65a81..9f0c3e1 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,20 @@ int console_trylock(void)
 		return 0;
 	}
 	console_locked = 1;
-	console_may_schedule = 0;
+	/*
+	 * When PREEMPT_COUNT disabled 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 there
+	 * and preempt_count() is always 0.
+	 *
+	 * RCU read sections have a separate preemption counter when
+	 * PREEMPT_RCU enabled thus we must take extra care and check
+	 * rcu_preempt_depth(), otherwise RCU read sections modify
+	 * preempt_count().
+	 */
+	console_may_schedule = !oops_in_progress &&
+			preemptible() &&
+			!rcu_preempt_depth();
 	return 1;
 }
 EXPORT_SYMBOL(console_trylock);
-- 
2.7.1

[PATCH v4 3/3] printk: check CON_ENABLED in have_callable_console()

[Sergey Senozhatsky]

have_callable_console() must also test CON_ENABLED bit, not just
CON_ANYTIME. We may have disabled CON_ANYTIME console so printk
can wrongly assume that it's safe to call_console_drivers().

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
 kernel/printk/printk.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 9f0c3e1..9917f69 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2139,7 +2139,8 @@ static int have_callable_console(void)
 	struct console *con;
 
 	for_each_console(con)
-		if (con->flags & CON_ANYTIME)
+		if ((con->flags & CON_ENABLED) &&
+				(con->flags & CON_ANYTIME))
 			return 1;
 
 	return 0;
-- 
2.7.1

Re: [PATCH v4 2/3] printk: set may_schedule for some of console_trylock callers

[Petr Mladek]

On Sat 2016-02-13 03:37:11, 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@gmail.com>

It looks safe after all.

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH v4 3/3] printk: check CON_ENABLED in have_callable_console()

[Petr Mladek]

On Sat 2016-02-13 03:37:12, Sergey Senozhatsky wrote:
> have_callable_console() must also test CON_ENABLED bit, not just
> CON_ANYTIME. We may have disabled CON_ANYTIME console so printk
> can wrongly assume that it's safe to call_console_drivers().
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

Makes perfect sense.

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH v4 3/3] printk: check CON_ENABLED in have_callable_console()

[Sergey Senozhatsky]

On (02/16/16 15:44), Petr Mladek wrote:
> On Sat 2016-02-13 03:37:12, Sergey Senozhatsky wrote:
> > have_callable_console() must also test CON_ENABLED bit, not just
> > CON_ANYTIME. We may have disabled CON_ANYTIME console so printk
> > can wrongly assume that it's safe to call_console_drivers().
> > 
> > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> 
> Makes perfect sense.
> 
> Reviewed-by: Petr Mladek <pmladek@suse.com>

thanks.

	-ss

Re: [PATCH v4 2/3] printk: set may_schedule for some of console_trylock callers

[Sergey Senozhatsky]

On (02/16/16 15:43), Petr Mladek wrote:
> On Sat 2016-02-13 03:37:11, 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@gmail.com>
> 
> It looks safe after all.
> 
> Reviewed-by: Petr Mladek <pmladek@suse.com>
> 

thanks.

	-ss