[PATCH] tty/vt: Add console_lock check to vt_console_print()

[PATCH] tty/vt: Add console_lock check to vt_console_print()

[Daniel Vetter]

I'm scratching my head why we have this printing_lock. Digging through
historical git trees shows that:
- Added in 1.1.73, and I found absolutely no reason why.
- Converted to atomic bitops in 2.1.125pre2, I guess as part of SMP
  enabling/bugfixes.
- Converted to a proper spinlock in b0940003f25d ("vt: bitlock fix")
  because the hand-rolled atomic version lacked necessary memory
  barriers.

Digging around in lore for that time period did also not shed further
light.

The only reason I think this might still be relevant today is that (to
my understanding at least, ymmv) during an oops we might be printing
without console_lock held. See console_flush_on_panic() and the
comments in there - we flush out the console buffers irrespective of
whether we managed to acquire the right locks.

The strange thing is that this reason is fairly recent, because the
console flushing was historically done without oops_in_progress set.
This only changed in c7c3f05e341a ("panic: avoid deadlocks in
re-entrant console drivers"), which removed the call to
bust_spinlocks(0) (which decrements oops_in_progress again) before
flushing out the console (which back then was open coded as a
console_trylock/unlock pair).

Note that this entire mess should be properly fixed in the
printk/console layer, and not inflicted on each implementation.

For now just document what's going on and check that in all other
cases callers obey the locking rules.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jirislaby@kernel.org>
Cc: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Xuezhi Zhang <zhangxuezhi1@coolpad.com>
Cc: Yangxi Xiang <xyangxi5@gmail.com>
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: nick black <dankamongmen@gmail.com>
Cc: Petr Mladek <pmladek@suse.com>
Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: John Ogness <john.ogness@linutronix.de>
Cc: Sam Ravnborg <sam@ravnborg.org>
--
Note that this applies on top of my earlier vt patch:

https://lore.kernel.org/lkml/20220826202419.198535-1-daniel.vetter@ffwll.ch/

Expect more, I'm digging around in here a bit ...
-Daniel
---
 drivers/tty/vt/vt.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index 4d29e4a17db7..54399dcc334e 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -3083,7 +3083,10 @@ static void vt_console_print(struct console *co, const char *b, unsigned count)
 	ushort start_x, cnt;
 	int kmsg_console;
 
-	/* console busy or not yet initialized */
+	if (!oops_in_progress)
+		WARN_CONSOLE_UNLOCKED();
+
+	/* this protects against concurrent oops only */
 	if (!spin_trylock(&printing_lock))
 		return;
 
-- 
2.37.2

[PATCH] tty/vt: Remove printable variable

[Daniel Vetter]

Every since the 0.99.7A release when console_register() was introduced
it's become impossible to call vt_console_print (called
console_print() back then still) directly. Which means the
initialization issue this variable protected against is no more.

Give it a send off with style and let it rest in peace.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jirislaby@kernel.org>
Cc: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
Cc: nick black <dankamongmen@gmail.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: Yangxi Xiang <xyangxi5@gmail.com>
Cc: Xuezhi Zhang <zhangxuezhi1@coolpad.com>
---
 drivers/tty/vt/vt.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index ae9c926acd6f..4d29e4a17db7 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -157,7 +157,6 @@ static void set_palette(struct vc_data *vc);
 
 #define vt_get_kmsg_redirect() vt_kmsg_redirect(-1)
 
-static int printable;		/* Is console ready for printing? */
 int default_utf8 = true;
 module_param(default_utf8, int, S_IRUGO | S_IWUSR);
 int global_cursor_default = -1;
@@ -3085,8 +3084,6 @@ static void vt_console_print(struct console *co, const char *b, unsigned count)
 	int kmsg_console;
 
 	/* console busy or not yet initialized */
-	if (!printable)
-		return;
 	if (!spin_trylock(&printing_lock))
 		return;
 
@@ -3537,7 +3534,6 @@ static int __init con_init(void)
 	pr_info("Console: %s %s %dx%d\n",
 		vc->vc_can_do_color ? "colour" : "mono",
 		display_desc, vc->vc_cols, vc->vc_rows);
-	printable = 1;
 
 	console_unlock();
 
-- 
2.37.2

Re: [PATCH] tty/vt: Remove printable variable

[Daniel Vetter]

On Tue, Aug 30, 2022 at 03:41:17PM +0200, Daniel Vetter wrote:
> Every since the 0.99.7A release when console_register() was introduced
> it's become impossible to call vt_console_print (called
> console_print() back then still) directly. Which means the
> initialization issue this variable protected against is no more.
> 
> Give it a send off with style and let it rest in peace.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Jiri Slaby <jirislaby@kernel.org>
> Cc: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
> Cc: nick black <dankamongmen@gmail.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Cc: Yangxi Xiang <xyangxi5@gmail.com>
> Cc: Xuezhi Zhang <zhangxuezhi1@coolpad.com>

Please disregard this, sent it accidentally instead of v2 of "tty/vt: Add
console_lock check to vt_console_print()".
-Daniel

> ---
>  drivers/tty/vt/vt.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> index ae9c926acd6f..4d29e4a17db7 100644
> --- a/drivers/tty/vt/vt.c
> +++ b/drivers/tty/vt/vt.c
> @@ -157,7 +157,6 @@ static void set_palette(struct vc_data *vc);
>  
>  #define vt_get_kmsg_redirect() vt_kmsg_redirect(-1)
>  
> -static int printable;		/* Is console ready for printing? */
>  int default_utf8 = true;
>  module_param(default_utf8, int, S_IRUGO | S_IWUSR);
>  int global_cursor_default = -1;
> @@ -3085,8 +3084,6 @@ static void vt_console_print(struct console *co, const char *b, unsigned count)
>  	int kmsg_console;
>  
>  	/* console busy or not yet initialized */
> -	if (!printable)
> -		return;
>  	if (!spin_trylock(&printing_lock))
>  		return;
>  
> @@ -3537,7 +3534,6 @@ static int __init con_init(void)
>  	pr_info("Console: %s %s %dx%d\n",
>  		vc->vc_can_do_color ? "colour" : "mono",
>  		display_desc, vc->vc_cols, vc->vc_rows);
> -	printable = 1;
>  
>  	console_unlock();
>  
> -- 
> 2.37.2
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] tty/vt: Remove printable variable

[Sam Ravnborg]

Hi Daniel,

On Tue, Aug 30, 2022 at 03:41:17PM +0200, Daniel Vetter wrote:
> Every since the 0.99.7A release when console_register() was introduced
> it's become impossible to call vt_console_print (called
> console_print() back then still) directly. Which means the
> initialization issue this variable protected against is no more.
> 
> Give it a send off with style and let it rest in peace.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Jiri Slaby <jirislaby@kernel.org>
> Cc: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
> Cc: nick black <dankamongmen@gmail.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Cc: Yangxi Xiang <xyangxi5@gmail.com>
> Cc: Xuezhi Zhang <zhangxuezhi1@coolpad.com>

I saw this was sent again on accident, but that sdoes not prevent me
from adding a:
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>

I was a bit reluctant to r-b it, but well this looks obviously correct,
so ...

	Sam

[PATCH] tty/vt: Add console_lock check to vt_console_print()

[Daniel Vetter]

I'm scratching my head why we have this printing_lock. Digging through
historical git trees shows that:
- Added in 1.1.73, and I found absolutely no reason why.
- Converted to atomic bitops in 2.1.125pre2, I guess as part of SMP
  enabling/bugfixes.
- Converted to a proper spinlock in b0940003f25d ("vt: bitlock fix")
  because the hand-rolled atomic version lacked necessary memory
  barriers.

Digging around in lore for that time period did also not shed further
light.

The only reason I think this might still be relevant today is that (to
my understanding at least, ymmv) during an oops we might be printing
without console_lock held. See console_flush_on_panic() and the
comments in there - we flush out the console buffers irrespective of
whether we managed to acquire the right locks.

The strange thing is that this reason is fairly recent, because the
console flushing was historically done without oops_in_progress set.
This only changed in c7c3f05e341a ("panic: avoid deadlocks in
re-entrant console drivers"), which removed the call to
bust_spinlocks(0) (which decrements oops_in_progress again) before
flushing out the console (which back then was open coded as a
console_trylock/unlock pair).

Note that this entire mess should be properly fixed in the
printk/console layer, and not inflicted on each implementation.

For now just document what's going on and check that in all other
cases callers obey the locking rules.

v2: WARN_CONSOLE_UNLOCKED already checks for oops_in_progress
(something else that should be fixed I guess), hence remove the
open-coded check I've had.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jirislaby@kernel.org>
Cc: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Xuezhi Zhang <zhangxuezhi1@coolpad.com>
Cc: Yangxi Xiang <xyangxi5@gmail.com>
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: nick black <dankamongmen@gmail.com>
Cc: Petr Mladek <pmladek@suse.com>
Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: John Ogness <john.ogness@linutronix.de>
Cc: Sam Ravnborg <sam@ravnborg.org>
--
Note that this applies on top of my earlier vt patch:

https://lore.kernel.org/lkml/20220826202419.198535-1-daniel.vetter@ffwll.ch/

Expect more, I'm digging around in here a bit ...
-Daniel
---
 drivers/tty/vt/vt.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index 4d29e4a17db7..a6be32798fad 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -3083,7 +3083,9 @@ static void vt_console_print(struct console *co, const char *b, unsigned count)
 	ushort start_x, cnt;
 	int kmsg_console;
 
-	/* console busy or not yet initialized */
+	WARN_CONSOLE_UNLOCKED();
+
+	/* this protects against concurrent oops only */
 	if (!spin_trylock(&printing_lock))
 		return;
 
-- 
2.37.2

Re: [PATCH] tty/vt: Add console_lock check to vt_console_print()

[Sam Ravnborg]

Hi Daniel,

On Tue, Aug 30, 2022 at 04:49:45PM +0200, Daniel Vetter wrote:
> I'm scratching my head why we have this printing_lock. Digging through
> historical git trees shows that:
> - Added in 1.1.73, and I found absolutely no reason why.
> - Converted to atomic bitops in 2.1.125pre2, I guess as part of SMP
>   enabling/bugfixes.
> - Converted to a proper spinlock in b0940003f25d ("vt: bitlock fix")
>   because the hand-rolled atomic version lacked necessary memory
>   barriers.
> 
> Digging around in lore for that time period did also not shed further
> light.
> 
> The only reason I think this might still be relevant today is that (to
> my understanding at least, ymmv) during an oops we might be printing
> without console_lock held. See console_flush_on_panic() and the
> comments in there - we flush out the console buffers irrespective of
> whether we managed to acquire the right locks.
> 
> The strange thing is that this reason is fairly recent, because the
> console flushing was historically done without oops_in_progress set.
> This only changed in c7c3f05e341a ("panic: avoid deadlocks in
> re-entrant console drivers"), which removed the call to
> bust_spinlocks(0) (which decrements oops_in_progress again) before
> flushing out the console (which back then was open coded as a
> console_trylock/unlock pair).
> 
> Note that this entire mess should be properly fixed in the
> printk/console layer, and not inflicted on each implementation.
> 
> For now just document what's going on and check that in all other
> cases callers obey the locking rules.
> 
> v2: WARN_CONSOLE_UNLOCKED already checks for oops_in_progress
> (something else that should be fixed I guess), hence remove the
> open-coded check I've had.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Jiri Slaby <jirislaby@kernel.org>
> Cc: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Xuezhi Zhang <zhangxuezhi1@coolpad.com>
> Cc: Yangxi Xiang <xyangxi5@gmail.com>
> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Cc: nick black <dankamongmen@gmail.com>
> Cc: Petr Mladek <pmladek@suse.com>
> Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: John Ogness <john.ogness@linutronix.de>
> Cc: Sam Ravnborg <sam@ravnborg.org>

It is always good to warn in case assumptions do not hold.
And thanks for the comment.

Reviewed-by: Sam Ravnborg <sam@ravnborg.org>

Hmm, I prefer to start comments with upper-case, but in vt.c there is no
specific style.

	Sam

> --
> Note that this applies on top of my earlier vt patch:
> 
> https://lore.kernel.org/lkml/20220826202419.198535-1-daniel.vetter@ffwll.ch/
> 
> Expect more, I'm digging around in here a bit ...
> -Daniel
> ---
>  drivers/tty/vt/vt.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> index 4d29e4a17db7..a6be32798fad 100644
> --- a/drivers/tty/vt/vt.c
> +++ b/drivers/tty/vt/vt.c
> @@ -3083,7 +3083,9 @@ static void vt_console_print(struct console *co, const char *b, unsigned count)
>  	ushort start_x, cnt;
>  	int kmsg_console;
>  
> -	/* console busy or not yet initialized */
> +	WARN_CONSOLE_UNLOCKED();
> +
> +	/* this protects against concurrent oops only */
>  	if (!spin_trylock(&printing_lock))
>  		return;
>  
> -- 
> 2.37.2

Re: [PATCH] tty/vt: Add console_lock check to vt_console_print()

[Petr Mladek]

On Tue 2022-08-30 15:28:03, Daniel Vetter wrote:
> I'm scratching my head why we have this printing_lock. Digging through
> historical git trees shows that:
> - Added in 1.1.73, and I found absolutely no reason why.
> - Converted to atomic bitops in 2.1.125pre2, I guess as part of SMP
>   enabling/bugfixes.
> - Converted to a proper spinlock in b0940003f25d ("vt: bitlock fix")
>   because the hand-rolled atomic version lacked necessary memory
>   barriers.
> 
> Digging around in lore for that time period did also not shed further
> light.
> 
> The only reason I think this might still be relevant today is that (to
> my understanding at least, ymmv) during an oops we might be printing
> without console_lock held. See console_flush_on_panic() and the
> comments in there - we flush out the console buffers irrespective of
> whether we managed to acquire the right locks.
> 
> The strange thing is that this reason is fairly recent, because the
> console flushing was historically done without oops_in_progress set.
> This only changed in c7c3f05e341a ("panic: avoid deadlocks in
> re-entrant console drivers"), which removed the call to
> bust_spinlocks(0) (which decrements oops_in_progress again) before
> flushing out the console (which back then was open coded as a
> console_trylock/unlock pair).
> 
> Note that this entire mess should be properly fixed in the
> printk/console layer, and not inflicted on each implementation.
> 
> For now just document what's going on and check that in all other
> cases callers obey the locking rules.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Jiri Slaby <jirislaby@kernel.org>
> Cc: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Xuezhi Zhang <zhangxuezhi1@coolpad.com>
> Cc: Yangxi Xiang <xyangxi5@gmail.com>
> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Cc: nick black <dankamongmen@gmail.com>
> Cc: Petr Mladek <pmladek@suse.com>
> Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: John Ogness <john.ogness@linutronix.de>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> --
> Note that this applies on top of my earlier vt patch:
> 
> https://lore.kernel.org/lkml/20220826202419.198535-1-daniel.vetter@ffwll.ch/
> 
> Expect more, I'm digging around in here a bit ...
> -Daniel
> ---
>  drivers/tty/vt/vt.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> index 4d29e4a17db7..54399dcc334e 100644
> --- a/drivers/tty/vt/vt.c
> +++ b/drivers/tty/vt/vt.c
> @@ -3083,7 +3083,10 @@ static void vt_console_print(struct console *co, const char *b, unsigned count)
>  	ushort start_x, cnt;
>  	int kmsg_console;
>  
> -	/* console busy or not yet initialized */
> +	if (!oops_in_progress)
> +		WARN_CONSOLE_UNLOCKED();
> +
> +	/* this protects against concurrent oops only */
>  	if (!spin_trylock(&printing_lock))
>  		return;

I am not sure how this was supposed to work. But it reminds me
similar games in other console drivers, see how oops_in_progress
is used.

Typical code looks like:

void serial8250_console_write(struct uart_8250_port *up, const char *s,
			      unsigned int count)
{
	int locked = 1;

	if (oops_in_progress)
		locked = spin_trylock_irqsave(&port->lock, flags);
	else
		spin_lock_irqsave(&port->lock, flags);

	/* Write the given string to the serial port */

	if (locked)
		spin_unlock_irqrestore(&port->lock, flags);
}

The logic is actually opposite in compare with vt_console().
Most console drivers allow to re-enter console->write() callback
during Oops or panic().

The "locked" variable is used to prevent double unlock in Oops
message when the system might try to continue working after
the Oops messages are printed.

IMHO, it works this way because there is a high-chance that the serial
console will print the message even when con->write() is called twice
in parallel. The message might be messed but it might be better than
nothing.

I am not sure how vt-code could deal with re-entrance. I guess that
there will be a big risk of deadlocks. It might explain why
printing_lock prevents the re-entrance completely.

Anyway, this explains why it is not solved on the higher level.
Serial consoles actually allow re-entrance. And they need
to handle the port->lock the special way.

The atomic consoles might eventually allow to remove these hacks.

Best Regards,
Petr