[RFC][PATCH 0/2] Access console drivers list under console_sem

[RFC][PATCH 0/2] Access console drivers list under console_sem

[Sergey Senozhatsky]

Petr, Steven,

	RFC

	Normally, we grab console_sem lock before we iterate consoles
list, which is necessary if we want to be race free. The only exception
to this rule is console_flush_on_panic(). However, it seems that we are
not fully race free - register_console() iterates console drivers list
in unsafe manner in several places. E.g. the following scenarion:

	CPU0					CPU1
	register_console()			unregister_console()
						 console_lock()
	  for_each_console()			  // modify console_drivers
	    con->foo				    kfree(con)

So I have two quick-n-dirty patches, which remove unsafe console list
access.

What do you think?

Sergey Senozhatsky (2):
  printk: lock console_sem before we unregister boot consoles
  printk: take console_sem when accessing console drivers list

 kernel/printk/printk.c | 117 ++++++++++++++++++++++++-----------------
 1 file changed, 69 insertions(+), 48 deletions(-)

-- 
2.21.0

[RFC][PATCH 1/2] printk: lock console_sem before we unregister boot consoles

[Sergey Senozhatsky]

The following pattern is not completely safe:

     for_each_console(bcon)
         if (bcon->flags & CON_BOOT)
             unregister_console(bcon);

Because, in theory, console drivers list and console drivers
can be modified concurrently from another CPU. Take console_sem
lock, which protects console drivers list and console drivers,
before we start iterating console drivers list.

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

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 17102fd4c136..b0e361ca1bea 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2605,6 +2605,48 @@ static int __init keep_bootcon_setup(char *str)
 
 early_param("keep_bootcon", keep_bootcon_setup);
 
+static int __unregister_console(struct console *console)
+{
+	struct console *a, *b;
+	int res;
+
+	pr_info("%sconsole [%s%d] disabled\n",
+		(console->flags & CON_BOOT) ? "boot" : "",
+		console->name, console->index);
+
+	res = _braille_unregister_console(console);
+	if (res)
+		return res;
+
+	res = 1;
+	if (console_drivers == console) {
+		console_drivers = console->next;
+		res = 0;
+	} else if (console_drivers) {
+		for (a = console_drivers->next, b = console_drivers;
+		     a; b = a, a = b->next) {
+			if (a == console) {
+				b->next = a->next;
+				res = 0;
+				break;
+			}
+		}
+	}
+
+	if (!res && (console->flags & CON_EXTENDED))
+		nr_ext_console_drivers--;
+
+	/*
+	 * If this isn't the last console and it has CON_CONSDEV set, we
+	 * need to set it on the next preferred console.
+	 */
+	if (console_drivers != NULL && console->flags & CON_CONSDEV)
+		console_drivers->flags |= CON_CONSDEV;
+
+	console->flags &= ~CON_ENABLED;
+	return res;
+}
+
 /*
  * The console driver calls this routine during kernel initialization
  * to register the console printing procedure with printk() and to
@@ -2777,62 +2819,34 @@ void register_console(struct console *newcon)
 	pr_info("%sconsole [%s%d] enabled\n",
 		(newcon->flags & CON_BOOT) ? "boot" : "" ,
 		newcon->name, newcon->index);
-	if (bcon &&
-	    ((newcon->flags & (CON_CONSDEV | CON_BOOT)) == CON_CONSDEV) &&
-	    !keep_bootcon) {
-		/* We need to iterate through all boot consoles, to make
+
+	if (keep_bootcon)
+		return;
+
+	if (bcon && (newcon->flags & (CON_CONSDEV|CON_BOOT)) == CON_CONSDEV) {
+		console_lock();
+		/*
+		 * We need to iterate through all boot consoles, to make
 		 * sure we print everything out, before we unregister them.
 		 */
 		for_each_console(bcon)
 			if (bcon->flags & CON_BOOT)
-				unregister_console(bcon);
+				__unregister_console(bcon);
+		console_unlock();
+		console_sysfs_notify();
 	}
 }
 EXPORT_SYMBOL(register_console);
 
 int unregister_console(struct console *console)
 {
-        struct console *a, *b;
-	int res;
-
-	pr_info("%sconsole [%s%d] disabled\n",
-		(console->flags & CON_BOOT) ? "boot" : "" ,
-		console->name, console->index);
-
-	res = _braille_unregister_console(console);
-	if (res)
-		return res;
+	int ret;
 
-	res = 1;
 	console_lock();
-	if (console_drivers == console) {
-		console_drivers=console->next;
-		res = 0;
-	} else if (console_drivers) {
-		for (a=console_drivers->next, b=console_drivers ;
-		     a; b=a, a=b->next) {
-			if (a == console) {
-				b->next = a->next;
-				res = 0;
-				break;
-			}
-		}
-	}
-
-	if (!res && (console->flags & CON_EXTENDED))
-		nr_ext_console_drivers--;
-
-	/*
-	 * If this isn't the last console and it has CON_CONSDEV set, we
-	 * need to set it on the next preferred console.
-	 */
-	if (console_drivers != NULL && console->flags & CON_CONSDEV)
-		console_drivers->flags |= CON_CONSDEV;
-
-	console->flags &= ~CON_ENABLED;
+	ret = __unregister_console(console);
 	console_unlock();
 	console_sysfs_notify();
-	return res;
+	return ret;
 }
 EXPORT_SYMBOL(unregister_console);
 
-- 
2.21.0

[RFC][PATCH 2/2] printk: take console_sem when accessing console drivers list

[Sergey Senozhatsky]

We need to take console_sem lock when we iterate console
drivers list. Otherwise, another CPU can concurrently
modify console drivers list or console drivers.

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

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index b0e361ca1bea..c2bccf58d03e 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2674,12 +2674,17 @@ void register_console(struct console *newcon)
 	struct console_cmdline *c;
 	static bool has_preferred;
 
-	if (console_drivers)
-		for_each_console(bcon)
-			if (WARN(bcon == newcon,
-					"console '%s%d' already registered\n",
-					bcon->name, bcon->index))
-				return;
+	console_lock();
+	if (console_drivers) {
+		for_each_console(bcon) {
+			if (bcon != newcon)
+				continue;
+			WARN(1, "console '%s%d' already registered\n",
+			     bcon->name, bcon->index);
+			console_unlock();
+			return;
+		}
+	}
 
 	/*
 	 * before we register a new CON_BOOT console, make sure we don't
@@ -2691,6 +2696,7 @@ void register_console(struct console *newcon)
 			if (!(bcon->flags & CON_BOOT)) {
 				pr_info("Too late to register bootconsole %s%d\n",
 					newcon->name, newcon->index);
+				console_unlock();
 				return;
 			}
 		}
@@ -2701,6 +2707,7 @@ void register_console(struct console *newcon)
 
 	if (!has_preferred || bcon || !console_drivers)
 		has_preferred = preferred_console >= 0;
+	console_unlock();
 
 	/*
 	 *	See if we want to use this console driver. If we
-- 
2.21.0

Re: [RFC][PATCH 0/2] Access console drivers list under console_sem

[Steven Rostedt]

On Tue, 23 Apr 2019 15:25:09 +0900
Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> wrote:

> Petr, Steven,
> 
> 	RFC
> 
> 	Normally, we grab console_sem lock before we iterate consoles
> list, which is necessary if we want to be race free. The only exception
> to this rule is console_flush_on_panic(). However, it seems that we are
> not fully race free - register_console() iterates console drivers list
> in unsafe manner in several places. E.g. the following scenarion:
> 
> 	CPU0					CPU1
> 	register_console()			unregister_console()
> 						 console_lock()
> 	  for_each_console()			  // modify console_drivers
> 	    con->foo				    kfree(con)
> 
> So I have two quick-n-dirty patches, which remove unsafe console list
> access.
> 
> What do you think?

I just skimmed the patches and haven't done a thorough review, but the
concept seems sane to me.

-- Steve

> 
> Sergey Senozhatsky (2):
>   printk: lock console_sem before we unregister boot consoles
>   printk: take console_sem when accessing console drivers list
> 
>  kernel/printk/printk.c | 117 ++++++++++++++++++++++++-----------------
>  1 file changed, 69 insertions(+), 48 deletions(-)
> 

Re: [RFC][PATCH 0/2] Access console drivers list under console_sem

[Sergey Senozhatsky]

On (04/23/19 09:48), Steven Rostedt wrote:
> > 	RFC
> > 
> > 	Normally, we grab console_sem lock before we iterate consoles
> > list, which is necessary if we want to be race free. The only exception
> > to this rule is console_flush_on_panic(). However, it seems that we are
> > not fully race free - register_console() iterates console drivers list
> > in unsafe manner in several places. E.g. the following scenarion:
> > 
> > 	CPU0					CPU1
> > 	register_console()			unregister_console()
> > 						 console_lock()
> > 	  for_each_console()			  // modify console_drivers
> > 	    con->foo				    kfree(con)
> > 
> > So I have two quick-n-dirty patches, which remove unsafe console list
> > access.
> > 
> > What do you think?
> 
> I just skimmed the patches and haven't done a thorough review, but the
> concept seems sane to me.

Thank you Steven. Let me know if anything doesn't work for you.

I have vague memories of a kernel Oops at con->foo dereferencing
(saw a message on linux-rt-users list a while ago, if I'm not
mistaken). And it seems that we have some races in printk code.

	-ss

Re: [RFC][PATCH 1/2] printk: lock console_sem before we unregister boot consoles

[Petr Mladek]

On Tue 2019-04-23 15:25:10, Sergey Senozhatsky wrote:
> The following pattern is not completely safe:
> 
>      for_each_console(bcon)
>          if (bcon->flags & CON_BOOT)
>              unregister_console(bcon);
> 
> Because, in theory, console drivers list and console drivers
> can be modified concurrently from another CPU. Take console_sem
> lock, which protects console drivers list and console drivers,
> before we start iterating console drivers list.
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 17102fd4c136..b0e361ca1bea 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2777,62 +2819,34 @@ void register_console(struct console *newcon)
>  	pr_info("%sconsole [%s%d] enabled\n",
>  		(newcon->flags & CON_BOOT) ? "boot" : "" ,
>  		newcon->name, newcon->index);
> -	if (bcon &&
> -	    ((newcon->flags & (CON_CONSDEV | CON_BOOT)) == CON_CONSDEV) &&
> -	    !keep_bootcon) {
> -		/* We need to iterate through all boot consoles, to make
> +
> +	if (keep_bootcon)
> +		return;
> +
> +	if (bcon && (newcon->flags & (CON_CONSDEV|CON_BOOT)) == CON_CONSDEV) {
> +		console_lock();
> +		/*
> +		 * We need to iterate through all boot consoles, to make
>  		 * sure we print everything out, before we unregister them.
>  		 */

I wondered if moving the console locking could break the above
statement.

It seems that the comment has been invalid since the commit
8259cf4342029aad37660e ("printk: Ensure that "console
enabled" messages are printed on the console").

Could we remove it in this patch? It touches it indirectly anyway.

Otherwise, the patch looks fine to me:

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

>  		for_each_console(bcon)
>  			if (bcon->flags & CON_BOOT)
> -				unregister_console(bcon);
> +				__unregister_console(bcon);
> +		console_unlock();
> +		console_sysfs_notify();
>  	}
>  }
>  EXPORT_SYMBOL(register_console);

Best Regards,
Petr

Re: [RFC][PATCH 2/2] printk: take console_sem when accessing console drivers list

[Petr Mladek]

On Tue 2019-04-23 15:25:11, Sergey Senozhatsky wrote:
> We need to take console_sem lock when we iterate console
> drivers list. Otherwise, another CPU can concurrently
> modify console drivers list or console drivers.
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> ---
>  kernel/printk/printk.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index b0e361ca1bea..c2bccf58d03e 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2674,12 +2674,17 @@ void register_console(struct console *newcon)
>  	struct console_cmdline *c;
>  	static bool has_preferred;
>  
> -	if (console_drivers)
> -		for_each_console(bcon)
> -			if (WARN(bcon == newcon,
> -					"console '%s%d' already registered\n",
> -					bcon->name, bcon->index))
> -				return;
> +	console_lock();
> +	if (console_drivers) {
> +		for_each_console(bcon) {
> +			if (bcon != newcon)
> +				continue;
> +			WARN(1, "console '%s%d' already registered\n",
> +			     bcon->name, bcon->index);
> +			console_unlock();
> +			return;
> +		}
> +	}
>  
>  	/*
>  	 * before we register a new CON_BOOT console, make sure we don't
> @@ -2691,6 +2696,7 @@ void register_console(struct console *newcon)
>  			if (!(bcon->flags & CON_BOOT)) {
>  				pr_info("Too late to register bootconsole %s%d\n",
>  					newcon->name, newcon->index);
> +				console_unlock();
>  				return;
>  			}
>  		}
> @@ -2701,6 +2707,7 @@ void register_console(struct console *newcon)
>  
>  	if (!has_preferred || bcon || !console_drivers)
>  		has_preferred = preferred_console >= 0;
> +	console_unlock();

We should keep it until the console is added into the list. Otherwise
there are races with accessing the static has_preferred and
the global preferred_console variables.

Also the value of bcon should stay synchronized until we decide
about replaying the log.

IMHO, the only danger might be when con->match() or con->setup()
would want to take console_lock() as well. I checked few drivers
and they looked safe. But I did not check all of them.

What do you think, please?

Best Regards,
Petr

Re: [RFC][PATCH 1/2] printk: lock console_sem before we unregister boot consoles

[Sergey Senozhatsky]

On (04/24/19 16:49), Petr Mladek wrote:
> > +	if (bcon && (newcon->flags & (CON_CONSDEV|CON_BOOT)) == CON_CONSDEV) {
> > +		console_lock();
> > +		/*
> > +		 * We need to iterate through all boot consoles, to make
> >  		 * sure we print everything out, before we unregister them.
> >  		 */
> 
> I wondered if moving the console locking could break the above
> statement.
> 
> It seems that the comment has been invalid since the commit
> 8259cf4342029aad37660e ("printk: Ensure that "console
> enabled" messages are printed on the console").

That's very interesting. Yes, you are right, the comment is a
leftover. printk used to iterate consoles twice before
8259cf4342029aad37660e

               /* we need to iterate through twice, to make sure we print
                * everything out, before we unregister the console(s)
                */
               printk(KERN_INFO "console handover:");
               for_each_console(bcon)
                       printk("boot [%s%d] ", bcon->name, bcon->index);

               printk(" -> real [%s%d]\n", newcon->name, newcon->index);
               for_each_console(bcon)
                       unregister_console(bcon);

But 8259cf4342029aad37660e has changed that and has made comment
invalid.

> Could we remove it in this patch? It touches it indirectly anyway.

Sure we can.

We also can take extra care of pr_info("%sconsole [%s%d] enabled\n".
Right now we do

	...
	console_unlock();
	console_sysfs_notify();

	pr_info("%sconsole [%s%d] enabled\n",....


But we can simply move that pr_info() a bit up:

	pr_info("%sconsole [%s%d] enabled\n",
	console_unlock();
	console_sysfs_notify();


So the message will be printed on all consoles.

---

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index c2bccf58d03e..981eb6c27cdb 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2813,8 +2813,6 @@ void register_console(struct console *newcon)
 		exclusive_console_stop_seq = console_seq;
 		logbuf_unlock_irqrestore(flags);
 	}
-	console_unlock();
-	console_sysfs_notify();
 
 	/*
 	 * By unregistering the bootconsoles after we enable the real console
@@ -2827,6 +2825,9 @@ void register_console(struct console *newcon)
 		(newcon->flags & CON_BOOT) ? "boot" : "" ,
 		newcon->name, newcon->index);
 
+	console_unlock();
+	console_sysfs_notify();
+
 	if (keep_bootcon)
 		return;
 
---

> Otherwise, the patch looks fine to me:
> 
> Reviewed-by: Petr Mladek <pmladek@suse.com>

Thanks!

	-ss

Re: [RFC][PATCH 2/2] printk: take console_sem when accessing console drivers list

[Sergey Senozhatsky]

On (04/24/19 17:13), Petr Mladek wrote:
> >  	/*
> >  	 * before we register a new CON_BOOT console, make sure we don't
> > @@ -2691,6 +2696,7 @@ void register_console(struct console *newcon)
> >  			if (!(bcon->flags & CON_BOOT)) {
> >  				pr_info("Too late to register bootconsole %s%d\n",
> >  					newcon->name, newcon->index);
> > +				console_unlock();
> >  				return;
> >  			}
> >  		}
> > @@ -2701,6 +2707,7 @@ void register_console(struct console *newcon)
> >  
> >  	if (!has_preferred || bcon || !console_drivers)
> >  		has_preferred = preferred_console >= 0;
> > +	console_unlock();

Thanks for taking a look!

> We should keep it until the console is added into the list. Otherwise
> there are races with accessing the static has_preferred and
> the global preferred_console variables.

We don't modify `preferred_console' in register_console(), only
read-access it. Write-access, at the same time, is not completely
race free. That global `preferred_console' is modified from

 add_preferred_console() -> __add_preferred_console() -> WRITE preferred_console
 console_setup() -> __add_preferred_console() -> WRITE preferred_console

So `preferred_console' is not WRITE protected by console_sem, that's
why I didn't make sure to READ protected it in register_console().

As of static `has_preferred'... I kind of couldn't figure out if
we really need to protect it, but can do.

> Also the value of bcon should stay synchronized until we decide
> about replaying the log.

Good catch. So we, basically, can do the same thing as we did to
__unregister_console(): factor out the registration code and call
that new __register_console() under console_lock, and do
console_unlock()/console_lock() after we add console to the list,
but before we unregister boot consoles.

Except for one small detail:

> IMHO, the only danger might be when con->match() or con->setup()
> would want to take console_lock() as well. I checked few drivers
> and they looked safe. But I did not check all of them.
>
> What do you think, please?

That's a hard question. I would assume that ->match() has
no business in console_sem; but I'm not completely sure about
->setup().

E.g. 8250 does take console_sem during port configuration:
	config_port()
	 serial8250_config_port()
	  autoconfig_irq()
	   console_lock()

But it doesn't look like we hit this path from ->setup(); seems
to be early serial setup stage.

So may be we can move the whole thing under console_sem.

	-ss

Re: [RFC][PATCH 1/2] printk: lock console_sem before we unregister boot consoles

[Sergey Senozhatsky]

On (04/25/19 12:52), Sergey Senozhatsky wrote:
> > Could we remove it in this patch? It touches it indirectly anyway.
> 
> Sure we can.
> 
> We also can take extra care of pr_info("%sconsole [%s%d] enabled\n".
> Right now we do
> 
> 	...
> 	console_unlock();
> 	console_sysfs_notify();
> 
> 	pr_info("%sconsole [%s%d] enabled\n",....
> 
> 
> But we can simply move that pr_info() a bit up:
> 
> 	pr_info("%sconsole [%s%d] enabled\n",
> 	console_unlock();
> 	console_sysfs_notify();
> 
> 
> So the message will be printed on all consoles.

---

Petr, Steven, would you prefer to have it as two separate patches - one
removes the comment; the other one moves pr_info("console enabled") - or
as one patch? It's sort of trivial enough to be in just one patch, but
I also can submit it as separate changes.

	-ss

Re: [RFC][PATCH 2/2] printk: take console_sem when accessing console drivers list

[Sergey Senozhatsky]

On (04/25/19 14:19), Sergey Senozhatsky wrote:
> 
> So may be we can move the whole thing under console_sem.
> 

Something like below. I'll re-spin the series in a day or two,
we have time.

---
 kernel/printk/printk.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 52d1123d9fdc..0c2eb161e793 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2666,7 +2666,7 @@ static int __unregister_console(struct console *console)
  *  - Once a "real" console is registered, any attempt to register a
  *    bootconsoles will be rejected
  */
-void register_console(struct console *newcon)
+static void __register_console(struct console *newcon)
 {
 	int i;
 	unsigned long flags;
@@ -2771,7 +2771,6 @@ void register_console(struct console *newcon)
 	 *	Put this console in the list - keep the
 	 *	preferred driver at the head of the list.
 	 */
-	console_lock();
 	if ((newcon->flags & CON_CONSDEV) || console_drivers == NULL) {
 		newcon->next = console_drivers;
 		console_drivers = newcon;
@@ -2820,19 +2819,25 @@ void register_console(struct console *newcon)
 
 	console_unlock();
 	console_sysfs_notify();
+	console_lock();
 
 	if (keep_bootcon)
 		return;
 
 	if (bcon && (newcon->flags & (CON_CONSDEV|CON_BOOT)) == CON_CONSDEV) {
-		console_lock();
 		for_each_console(bcon)
 			if (bcon->flags & CON_BOOT)
 				__unregister_console(bcon);
-		console_unlock();
-		console_sysfs_notify();
 	}
 }
+
+void register_console(struct console *newcon)
+{
+	console_lock();
+	__register_console(newcon);
+	console_unlock();
+	console_sysfs_notify();
+}
 EXPORT_SYMBOL(register_console);
 
 int unregister_console(struct console *console)
-- 
2.21.0

Re: [RFC][PATCH 1/2] printk: lock console_sem before we unregister boot consoles

[Petr Mladek]

On Thu 2019-04-25 12:52:33, Sergey Senozhatsky wrote:
> On (04/24/19 16:49), Petr Mladek wrote:
> > > +	if (bcon && (newcon->flags & (CON_CONSDEV|CON_BOOT)) == CON_CONSDEV) {
> > > +		console_lock();
> > > +		/*
> > > +		 * We need to iterate through all boot consoles, to make
> > >  		 * sure we print everything out, before we unregister them.
> > >  		 */
> > 
> > I wondered if moving the console locking could break the above
> > statement.
> > 
> > It seems that the comment has been invalid since the commit
> > 8259cf4342029aad37660e ("printk: Ensure that "console
> > enabled" messages are printed on the console").
> 
> That's very interesting. Yes, you are right, the comment is a
> leftover. printk used to iterate consoles twice before
> 8259cf4342029aad37660e
> 
>                /* we need to iterate through twice, to make sure we print
>                 * everything out, before we unregister the console(s)
>                 */
>                printk(KERN_INFO "console handover:");
>                for_each_console(bcon)
>                        printk("boot [%s%d] ", bcon->name, bcon->index);
> 
>                printk(" -> real [%s%d]\n", newcon->name, newcon->index);
>                for_each_console(bcon)
>                        unregister_console(bcon);
> 
> But 8259cf4342029aad37660e has changed that and has made comment
> invalid.
> 
> > Could we remove it in this patch? It touches it indirectly anyway.
> 
> Sure we can.
> 
> We also can take extra care of pr_info("%sconsole [%s%d] enabled\n".
> Right now we do
> 
> 	...
> 	console_unlock();
> 	console_sysfs_notify();
> 
> 	pr_info("%sconsole [%s%d] enabled\n",....
> 
> 
> But we can simply move that pr_info() a bit up:
> 
> 	pr_info("%sconsole [%s%d] enabled\n",
> 	console_unlock();
> 	console_sysfs_notify();
> 
> 
> So the message will be printed on all consoles.

Great idea!

It would deserve a separate patch that moves the pr_info()
and removes the invalid comment.

Actually, the pr_info() would deserve a comment explaining
why it should be called before console_unlock().

Best Regards,
Petr

Re: [RFC][PATCH 1/2] printk: lock console_sem before we unregister boot consoles

[Sergey Senozhatsky]

On (04/25/19 09:50), Petr Mladek wrote:
> > Sure we can.
> > 
> > We also can take extra care of pr_info("%sconsole [%s%d] enabled\n".
> > Right now we do
> > 
> > 	...
> > 	console_unlock();
> > 	console_sysfs_notify();
> > 
> > 	pr_info("%sconsole [%s%d] enabled\n",....
> > 
> > 
> > But we can simply move that pr_info() a bit up:
> > 
> > 	pr_info("%sconsole [%s%d] enabled\n",
> > 	console_unlock();
> > 	console_sysfs_notify();
> > 
> > 
> > So the message will be printed on all consoles.
> 
> Great idea!
> 
> It would deserve a separate patch that moves the pr_info()
> and removes the invalid comment.
> 
> Actually, the pr_info() would deserve a comment explaining
> why it should be called before console_unlock().

Good. So I think I'll drop patch #1 from the series, add
two more patches - invalid comment + pr_info() - and rework
locking in patch #3 (this should take care of a race which
patch #1 was intended to fix).

	-ss

Re: [RFC][PATCH 2/2] printk: take console_sem when accessing console drivers list

[Petr Mladek]

On Thu 2019-04-25 14:19:44, Sergey Senozhatsky wrote:
> On (04/24/19 17:13), Petr Mladek wrote:
> > >  	/*
> > >  	 * before we register a new CON_BOOT console, make sure we don't
> > > @@ -2691,6 +2696,7 @@ void register_console(struct console *newcon)
> > >  			if (!(bcon->flags & CON_BOOT)) {
> > >  				pr_info("Too late to register bootconsole %s%d\n",
> > >  					newcon->name, newcon->index);
> > > +				console_unlock();
> > >  				return;
> > >  			}
> > >  		}
> > > @@ -2701,6 +2707,7 @@ void register_console(struct console *newcon)
> > >  
> > >  	if (!has_preferred || bcon || !console_drivers)
> > >  		has_preferred = preferred_console >= 0;
> > > +	console_unlock();
> 
> Thanks for taking a look!
> 
> > We should keep it until the console is added into the list. Otherwise
> > there are races with accessing the static has_preferred and
> > the global preferred_console variables.
> 
> We don't modify `preferred_console' in register_console(), only
> read-access it. Write-access, at the same time, is not completely
> race free. That global `preferred_console' is modified from
> 
>  add_preferred_console() -> __add_preferred_console() -> WRITE preferred_console
>  console_setup() -> __add_preferred_console() -> WRITE preferred_console
> 
> So `preferred_console' is not WRITE protected by console_sem, that's
> why I didn't make sure to READ protected it in register_console().

Sigh, it is complicated.

OK, preferred_console is used to iterate over console_cmdline array.
The good thing is that entries are never removed from there.
It is static and thus zeroed at boot. Therefore there is not
risk of buffer overflow by strcmp() or so.

Also the newly added entry should never match() console that
it being registered because the entry is added earlier when
parsing cmdline or it is added later by a probe call before
the probed console is registered[*].

IMHO, the only danger is that we wrongly detect preferred
console and unregister boot consoles too early. But I doubt
that it might even happen.

[*] Calling add_preferred_console() from a probe() call
    is not common. But I see, for example:

    + hv_probe()
      + sunserial_console_match()
	+ add_preferred_console()

Result: I think that we do not need to synchronize access to
	preferred_console.


> As of static `has_preferred'... I kind of couldn't figure out if
> we really need to protect it, but can do.

For example, I think that we might end up with two consoles
that have CON_CONSDEV set and are handled as preferred.

It would happen when two parallel register_console() calls enter
the following code:

	if (!has_preferred) {
		if (newcon->index < 0)
			newcon->index = 0;
		if (newcon->setup == NULL ||
		    newcon->setup(newcon, NULL) == 0) {
			newcon->flags |= CON_ENABLED;
			if (newcon->device) {
				newcon->flags |= CON_CONSDEV;
				has_preferred = true;
			}
		}
	}


> > Also the value of bcon should stay synchronized until we decide
> > about replaying the log.
> 
> Good catch. So we, basically, can do the same thing as we did to
> __unregister_console(): factor out the registration code and call
> that new __register_console() under console_lock, and do
> console_unlock()/console_lock() after we add console to the list,
> but before we unregister boot consoles.

Yup.

> Except for one small detail:
> 
> > IMHO, the only danger might be when con->match() or con->setup()
> > would want to take console_lock() as well. I checked few drivers
> > and they looked safe. But I did not check all of them.
> >
> > What do you think, please?
> 
> That's a hard question. I would assume that ->match() has
> no business in console_sem; but I'm not completely sure about
> ->setup().

Same here. It would be nice to check all possible paths but
I do not know how to do so.

IMHO, it could happen only when a console uses console_lock
for its own synchronization. I hope that it is only tty code
and it can be tested easily.


> E.g. 8250 does take console_sem during port configuration:
> 	config_port()
> 	 serial8250_config_port()
> 	  autoconfig_irq()
> 	   console_lock()
> 
> But it doesn't look like we hit this path from ->setup(); seems
> to be early serial setup stage.
> 
> So may be we can move the whole thing under console_sem.

I believe that it should be safe.

Best Regards,
Petr

Re: [RFC][PATCH 1/2] printk: lock console_sem before we unregister boot consoles

[Petr Mladek]

On Thu 2019-04-25 15:43:21, Sergey Senozhatsky wrote:
> On (04/25/19 12:52), Sergey Senozhatsky wrote:
> > > Could we remove it in this patch? It touches it indirectly anyway.
> > 
> > Sure we can.
> > 
> > We also can take extra care of pr_info("%sconsole [%s%d] enabled\n".
> > Right now we do
> > 
> > 	...
> > 	console_unlock();
> > 	console_sysfs_notify();
> > 
> > 	pr_info("%sconsole [%s%d] enabled\n",....
> > 
> > 
> > But we can simply move that pr_info() a bit up:
> > 
> > 	pr_info("%sconsole [%s%d] enabled\n",
> > 	console_unlock();
> > 	console_sysfs_notify();
> > 
> > 
> > So the message will be printed on all consoles.
> 
> ---
> 
> Petr, Steven, would you prefer to have it as two separate patches - one
> removes the comment; the other one moves pr_info("console enabled") - or
> as one patch? It's sort of trivial enough to be in just one patch, but
> I also can submit it as separate changes.

One patch is enough from my point of view. The moved pr_info()
solves the problem that is described by the comment.

Best Regards,
Petr

Re: [RFC][PATCH 1/2] printk: lock console_sem before we unregister boot consoles

[Sergey Senozhatsky]

On (04/25/19 16:56), Sergey Senozhatsky wrote:
> > Great idea!
> > 
> > It would deserve a separate patch that moves the pr_info()
> > and removes the invalid comment.
> > 
> > Actually, the pr_info() would deserve a comment explaining
> > why it should be called before console_unlock().
> 
> Good. So I think I'll drop patch #1 from the series, add
> two more patches - invalid comment + pr_info() - and rework
> locking in patch #3 (this should take care of a race which
> patch #1 was intended to fix).

D'oh, I can't drop patch #1, we still need to factor out
__unregister_console(). Calling unregister_console(bcon)
under console_sem will deadlock us. So I'll just end up
having __registed_console() and __unregister_console(),
both should be called under console_sem:

register_console()
{
	console_lock()
	__register_console()
	{
		...
		for_each_console(bcon)
			if (bcon->flags & CON_BOOT)
				__unregister_console(bcon);
	}
	console_unlock()
}

	-ss

Re: [RFC][PATCH 1/2] printk: lock console_sem before we unregister boot consoles

[Steven Rostedt]

On Thu, 25 Apr 2019 11:20:03 +0200
Petr Mladek <pmladek@suse.com> wrote:

> > 
> > Petr, Steven, would you prefer to have it as two separate patches - one
> > removes the comment; the other one moves pr_info("console enabled") - or
> > as one patch? It's sort of trivial enough to be in just one patch, but
> > I also can submit it as separate changes.  
> 
> One patch is enough from my point of view. The moved pr_info()
> solves the problem that is described by the comment.

+1

-- Steve