[PATCHv2 0/4] Access console drivers list under console_sem

[PATCHv2 0/4] Access console drivers list under console_sem

[Sergey Senozhatsky]

Hello,

        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)

I factored out register_console() and unregister_console() and now
the bulk of the work is done under console_sem. Both in register
and unregister paths we now have that oddly looking thing

	pr_info("console enabled/disabled");
	console_unlock();
	console_lock();

Which is not really odd, in fact. This is to make sure that we always
print messages on all the consoles.

v2:
- removed outdated comment (Petr)
- factor out register_console() and always run it under console_sem (Petr)
- added a patch which enusures that we always print "console disabled'
  on every console, before we unregister one of them

Sergey Senozhatsky (4):
  printk: factor out __unregister_console() code
  printk: remove invalid register_console() comment
  printk: factor out register_console() code
  printk: make sure we always print console disabled message

 kernel/printk/printk.c | 125 +++++++++++++++++++++++++----------------
 1 file changed, 76 insertions(+), 49 deletions(-)

-- 
2.21.0

[PATCHv2 1/4] printk: factor out __unregister_console() code

[Sergey Senozhatsky]

The following pattern in register_console() 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. We need to grab
console_sem lock, which protects console drivers list and console
drivers, before we start iterating console drivers list.

Factor out __unregister_console(), which will be called from
unregister_console() and register_console(), in both cases
under console_sem lock.

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

[PATCHv2 2/4] printk: remove invalid register_console() comment

[Sergey Senozhatsky]

We don't iterate consoles twice, since commit 8259cf434202
("printk: Ensure that "console enabled" messages are printed
 on the console"), so the comment is not valid anymore, and
can be removed, as was suggested by Petr.

The patch also invokes pr_info("%sconsole [%s%d] enabled\n")
before we unlock_consoles(), just to make sure that we really
print that message on every registered and enabled console.

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

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index b0e361ca1bea..3ac71701afa3 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2806,9 +2806,22 @@ void register_console(struct console *newcon)
 		exclusive_console_stop_seq = console_seq;
 		logbuf_unlock_irqrestore(flags);
 	}
+
+	/*
+	 * We are still under console_sem, pr_info() will only add the message
+	 * to the kernel's log buffer. console_unlock() will print it on all
+	 * registered and enabled consoles.
+	 */
+	pr_info("%sconsole [%s%d] enabled\n",
+		(newcon->flags & CON_BOOT) ? "boot" : "",
+		newcon->name, newcon->index);
+
 	console_unlock();
 	console_sysfs_notify();
 
+	if (keep_bootcon)
+		return;
+
 	/*
 	 * By unregistering the bootconsoles after we enable the real console
 	 * we get the "console xxx enabled" message on all the consoles -
@@ -2816,19 +2829,8 @@ void register_console(struct console *newcon)
 	 * users know there might be something in the kernel's log buffer that
 	 * went to the bootconsole (that they do not see on the real console)
 	 */
-	pr_info("%sconsole [%s%d] enabled\n",
-		(newcon->flags & CON_BOOT) ? "boot" : "" ,
-		newcon->name, newcon->index);
-
-	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);
-- 
2.21.0

[PATCHv2 3/4] printk: factor out register_console() code

[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. Current register_console() has several
race conditions - for_each_console() must be done under console_sem.

Factor out console registration code and hold console_sem throughout
entire registration process. Note that we need to unlock console_sem
and lock it again after we added new console to the list and before
we unregister boot consoles. This might look a bit weird, but this
is how we print pending logbuf messages to all registered and
available consoles.

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
 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 3ac71701afa3..3b36e26d4a51 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;
@@ -2818,6 +2817,7 @@ void register_console(struct console *newcon)
 
 	console_unlock();
 	console_sysfs_notify();
+	console_lock();
 
 	if (keep_bootcon)
 		return;
@@ -2830,14 +2830,19 @@ void register_console(struct console *newcon)
 	 * went to the bootconsole (that they do not see on the real console)
 	 */
 	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

[PATCHv2 4/4] printk: make sure we always print console disabled message

[Sergey Senozhatsky]

Make sure that we print 'console disabled' messages on all
the consoles, including the one we are about to unregister.
Otherwise, unregistered console will not have that message,
because pr_info() under console_sem doesn't print anything.

We do the same thing in __register_console() with the
'console enabled' message.

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

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 3b36e26d4a51..20c702b963a9 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2613,6 +2613,12 @@ static int __unregister_console(struct console *console)
 	pr_info("%sconsole [%s%d] disabled\n",
 		(console->flags & CON_BOOT) ? "boot" : "",
 		console->name, console->index);
+	/*
+	 * Print 'console disabled' on all the consoles, including the
+	 * one we are about to unregister.
+	 */
+	console_unlock();
+	console_lock();
 
 	res = _braille_unregister_console(console);
 	if (res)
-- 
2.21.0

Re: [PATCHv2 4/4] printk: make sure we always print console disabled message

[Sergey Senozhatsky]

Forgot to mention that the series is still in RFC phase.


On (04/26/19 14:33), Sergey Senozhatsky wrote:
[..]
> +++ b/kernel/printk/printk.c
> @@ -2613,6 +2613,12 @@ static int __unregister_console(struct console *console)
>  	pr_info("%sconsole [%s%d] disabled\n",
>  		(console->flags & CON_BOOT) ? "boot" : "",
>  		console->name, console->index);
> +	/*
> +	 * Print 'console disabled' on all the consoles, including the
> +	 * one we are about to unregister.
> +	 */
> +	console_unlock();
> +	console_lock();
>  
>  	res = _braille_unregister_console(console);
>  	if (res)

Need to think more if this is race free...

	-ss

Re: [PATCHv2 1/4] printk: factor out __unregister_console() code

[Petr Mladek]

On Fri 2019-04-26 14:32:59, Sergey Senozhatsky wrote:
> The following pattern in register_console() 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. We need to grab
> console_sem lock, which protects console drivers list and console
> drivers, before we start iterating console drivers list.
> 
> Factor out __unregister_console(), which will be called from
> unregister_console() and register_console(), in both cases
> under console_sem lock.
> 
> 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);

It looks safe to call _braille_unregister_console() under
console_lock().

Therefore this patch looks fine and make sense, so:

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

Best Regards,
Petr

Re: [PATCHv2 2/4] printk: remove invalid register_console() comment

[Petr Mladek]

On Fri 2019-04-26 14:33:00, Sergey Senozhatsky wrote:
> We don't iterate consoles twice, since commit 8259cf434202
> ("printk: Ensure that "console enabled" messages are printed
>  on the console"), so the comment is not valid anymore, and
> can be removed, as was suggested by Petr.
> 
> The patch also invokes pr_info("%sconsole [%s%d] enabled\n")
> before we unlock_consoles(), just to make sure that we really
> print that message on every registered and enabled console.
> 
> Suggested-by: Petr Mladek <pmladek@suse.com>
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

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

Best Regards,
Petr

Re: [PATCHv2 3/4] printk: factor out register_console() code

[Petr Mladek]

On Fri 2019-04-26 14:33:01, 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. Current register_console() has several
> race conditions - for_each_console() must be done under console_sem.
> 
> Factor out console registration code and hold console_sem throughout
> entire registration process. Note that we need to unlock console_sem
> and lock it again after we added new console to the list and before
> we unregister boot consoles. This might look a bit weird, but this
> is how we print pending logbuf messages to all registered and
> available consoles.

My main concern was whether we could call newcon->setup() under
console_lock. I checked many console drivers and all looked safe.
There should not be much reasons to do it.


> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> ---
>  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 3ac71701afa3..3b36e26d4a51 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -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;
> @@ -2818,6 +2817,7 @@ void register_console(struct console *newcon)
>  
>  	console_unlock();
>  	console_sysfs_notify();
> +	console_lock();

I have got an idea how to get rid of this weirdness:

1. The check for bcon seems to be just an optimization. There is not need
   to remove boot consoles when there are none.

2. The condition (newcon->flags & (CON_CONSDEV|CON_BOOT)) == CON_CONSDEV)
   is valid only when the preferred console was really added.

Therefore we could move the code to a separate function, e.g.

void unregister_boot_consoles(void)
{
	struct console *bcon;

	console_lock();
	for_each_console(bcon)
		if (bcon->flags & CON_BOOT)
			__unregister_console(bcon);
	}
	console_unlock();
	console_sysfs_notify();
}

Then we could do something like:

void register_console(struct console *newcon)
{
	bool newcon_is_preferred = false;

	console_lock();
	__register_console(newcon);
	if ((newcon->flags & (CON_CONSDEV|CON_BOOT)) == CON_CONSDEV)
		newcon_is_preferred = true;
	console_unlock();
	console_sysfs_notify();

	/*
	 * By unregistering the bootconsoles after we enable the real console
	 * we get the "console xxx enabled" message on all the consoles -
	 * boot consoles, real consoles, etc - this is to ensure that end
	 * users know there might be something in the kernel's log buffer that
	 * went to the bootconsole (that they do not see on the real console)
	 */
	if (newcon_is_preferred && !keep_bootcon)
		unregister_boot_consoles();
}

How does that sound?

Otherwise, the patch looks fine to me.

Best Regards,
Petr

Re: [PATCHv2 4/4] printk: make sure we always print console disabled message

[Petr Mladek]

On Fri 2019-04-26 14:44:45, Sergey Senozhatsky wrote:
> 
> Forgot to mention that the series is still in RFC phase.
> 
> 
> On (04/26/19 14:33), Sergey Senozhatsky wrote:
> [..]
> > +++ b/kernel/printk/printk.c
> > @@ -2613,6 +2613,12 @@ static int __unregister_console(struct console *console)
> >  	pr_info("%sconsole [%s%d] disabled\n",
> >  		(console->flags & CON_BOOT) ? "boot" : "",
> >  		console->name, console->index);
> > +	/*
> > +	 * Print 'console disabled' on all the consoles, including the
> > +	 * one we are about to unregister.
> > +	 */
> > +	console_unlock();
> > +	console_lock();
> >  
> >  	res = _braille_unregister_console(console);
> >  	if (res)
> 
> Need to think more if this is race free...

I am afraid that it is racy against for_each_console() when
removing the boot consoles.

I think that it is _not_ worth a code complication. It worked
this way for ages and people do not seem to complain.

Best Regards,
Petr

Re: [PATCHv2 3/4] printk: factor out register_console() code

[Sergey Senozhatsky]

Hello,

On (05/15/19 16:36), Petr Mladek wrote:
[..]
> >  
> >  	console_unlock();
> >  	console_sysfs_notify();
> > +	console_lock();
> 
> I have got an idea how to get rid of this weirdness:
>
> 1. The check for bcon seems to be just an optimization. There is not need
>    to remove boot consoles when there are none.
> 
> 2. The condition (newcon->flags & (CON_CONSDEV|CON_BOOT)) == CON_CONSDEV)
>    is valid only when the preferred console was really added.
> 
> Therefore we could move the code to a separate function, e.g.
> 
> void unregister_boot_consoles(void)
> {
> 	struct console *bcon;
> 
> 	console_lock();
> 	for_each_console(bcon)
> 		if (bcon->flags & CON_BOOT)
> 			__unregister_console(bcon);
> 	}
> 	console_unlock();
> 	console_sysfs_notify();
> }
> 
> Then we could do something like:
> 
> void register_console(struct console *newcon)
> {
> 	bool newcon_is_preferred = false;
> 
> 	console_lock();
> 	__register_console(newcon);
> 	if ((newcon->flags & (CON_CONSDEV|CON_BOOT)) == CON_CONSDEV)
> 		newcon_is_preferred = true;
> 	console_unlock();
> 	console_sysfs_notify();
> 
> 	/*
> 	 * By unregistering the bootconsoles after we enable the real console
> 	 * we get the "console xxx enabled" message on all the consoles -
> 	 * boot consoles, real consoles, etc - this is to ensure that end
> 	 * users know there might be something in the kernel's log buffer that
> 	 * went to the bootconsole (that they do not see on the real console)
> 	 */
> 	if (newcon_is_preferred && !keep_bootcon)
> 		unregister_boot_consoles();
> }
> 
> How does that sound?

Hmm, may be I'm missing something. I think that the 'weirdness'
is still needed. This

	console_lock();
	__unregister_console(bcon);  // pr_info("%sconsole disabled\n")
	console_unlock();

is going to change the visible behaviour - we need to show
pr_info("%sconsole [%s%d] disabled\n") on all consoles, especially
on the console which we are disabling. Who knows, maybe that's the
last remaining properly working console. Doing __unregister_console()
under console_sem will end up in a lost/missing message on bcon (or
on any other console we are unregistering).

	-ss

Re: [PATCHv2 4/4] printk: make sure we always print console disabled message

[Sergey Senozhatsky]

On (05/15/19 16:47), Petr Mladek wrote:
> On Fri 2019-04-26 14:44:45, Sergey Senozhatsky wrote:
> > 
> > Forgot to mention that the series is still in RFC phase.
> > 
> > 
> > On (04/26/19 14:33), Sergey Senozhatsky wrote:
> > [..]
> > > +++ b/kernel/printk/printk.c
> > > @@ -2613,6 +2613,12 @@ static int __unregister_console(struct console *console)
> > >  	pr_info("%sconsole [%s%d] disabled\n",
> > >  		(console->flags & CON_BOOT) ? "boot" : "",
> > >  		console->name, console->index);
> > > +	/*
> > > +	 * Print 'console disabled' on all the consoles, including the
> > > +	 * one we are about to unregister.
> > > +	 */
> > > +	console_unlock();
> > > +	console_lock();
> > >  
> > >  	res = _braille_unregister_console(console);
> > >  	if (res)
> > 
> > Need to think more if this is race free...
> 
> I am afraid that it is racy against for_each_console() when
> removing the boot consoles.

Can you explain? Do you mean that we can execute two paths unregistering
the same bcon?

	CPU0					CPU1

	console_lock();
	__unregister_console(bconA)		console_lock();
	console_unlock();

						__unregister_console(bconA);
						for (a = console_drivers->next ...)
							if (a == console)
								unregister();
							// console bconA is
							// not in the list
							// anymore
						console_unlock();

	for (a = console_drivers->next ...)
		if (a == console)
	console_unlock();


This CPU0 will never see bconA in the console drivers list.
But... it will try to do

	console->flags &= ~CON_ENABLED;

Which we need to fix.

Do not disable console which is not on the console_drivers list.

---
index 1177ea4b3fe1..e729992cb4e4 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2660,7 +2660,8 @@ static int __unregister_console(struct console *console)
 	if (console_drivers != NULL && console->flags & CON_CONSDEV)
 		console_drivers->flags |= CON_CONSDEV;
 
-	console->flags &= ~CON_ENABLED;
+	if (!res)
+		console->flags &= ~CON_ENABLED;
 	return res;
 }
 
---
	-ss

Re: [PATCHv2 3/4] printk: factor out register_console() code

[Petr Mladek]

On Thu 2019-05-23 15:51:44, Sergey Senozhatsky wrote:
> Hello,
> 
> On (05/15/19 16:36), Petr Mladek wrote:
> [..]
> > >  
> > >  	console_unlock();
> > >  	console_sysfs_notify();
> > > +	console_lock();
> > 
> > I have got an idea how to get rid of this weirdness:
> >
> > 1. The check for bcon seems to be just an optimization. There is not need
> >    to remove boot consoles when there are none.
> > 
> > 2. The condition (newcon->flags & (CON_CONSDEV|CON_BOOT)) == CON_CONSDEV)
> >    is valid only when the preferred console was really added.
> > 
> > Therefore we could move the code to a separate function, e.g.
> > 
> > void unregister_boot_consoles(void)
> > {
> > 	struct console *bcon;
> > 
> > 	console_lock();
> > 	for_each_console(bcon)
> > 		if (bcon->flags & CON_BOOT)
> > 			__unregister_console(bcon);
> > 	}
> > 	console_unlock();
> > 	console_sysfs_notify();
> > }
> > 
> > Then we could do something like:
> > 
> > void register_console(struct console *newcon)
> > {
> > 	bool newcon_is_preferred = false;
> > 
> > 	console_lock();
> > 	__register_console(newcon);
> > 	if ((newcon->flags & (CON_CONSDEV|CON_BOOT)) == CON_CONSDEV)
> > 		newcon_is_preferred = true;
> > 	console_unlock();
> > 	console_sysfs_notify();
> > 
> > 	/*
> > 	 * By unregistering the bootconsoles after we enable the real console
> > 	 * we get the "console xxx enabled" message on all the consoles -
> > 	 * boot consoles, real consoles, etc - this is to ensure that end
> > 	 * users know there might be something in the kernel's log buffer that
> > 	 * went to the bootconsole (that they do not see on the real console)
> > 	 */
> > 	if (newcon_is_preferred && !keep_bootcon)
> > 		unregister_boot_consoles();
> > }
> > 
> > How does that sound?
> 
> Hmm, may be I'm missing something. I think that the 'weirdness'
> is still needed.

I probably used wrong words. For me the most weird thing was
that the original code temporary released a lock that
was originally taken in another function.

My proposal is just a refactoring. It allows to do all
the locking/unlocking operations in a single function.
It makes is easier to track.

> 
> 	console_lock();
> 	__unregister_console(bcon);  // pr_info("%sconsole disabled\n")
> 	console_unlock();
> 
> is going to change the visible behaviour - we need to show
> pr_info("%sconsole [%s%d] disabled\n") on all consoles, especially
> on the console which we are disabling.

It was the 1st patch that changed the behavior. It moved
the pr_info() under console_lock. Therefore it never
appears on the console.

The 4th patch tries to fix this but it looks racy. I am going
to comment the race in the thread about the 4th patch.


> Who knows, maybe that's the last remaining properly working
> console. Doing __unregister_console() under console_sem will
> end up in a lost/missing message on bcon (or
> on any other console we are unregistering).

I agree that we should make sure that the message is printed on
the console that is being disabled.

Sigh, I am afraid that a proper solution would result in a messy
code. It would be pity. The original problem is rather theoretical.
The fix is not worth making the code even more hairy.

Best Regards,
Petr

Re: [PATCHv2 4/4] printk: make sure we always print console disabled message

[Petr Mladek]

On Thu 2019-05-23 15:59:47, Sergey Senozhatsky wrote:
> On (05/15/19 16:47), Petr Mladek wrote:
> > On Fri 2019-04-26 14:44:45, Sergey Senozhatsky wrote:
> > > 
> > > Forgot to mention that the series is still in RFC phase.
> > > 
> > > 
> > > On (04/26/19 14:33), Sergey Senozhatsky wrote:
> > > [..]
> > > > +++ b/kernel/printk/printk.c
> > > > @@ -2613,6 +2613,12 @@ static int __unregister_console(struct console *console)
> > > >  	pr_info("%sconsole [%s%d] disabled\n",
> > > >  		(console->flags & CON_BOOT) ? "boot" : "",
> > > >  		console->name, console->index);
> > > > +	/*
> > > > +	 * Print 'console disabled' on all the consoles, including the
> > > > +	 * one we are about to unregister.
> > > > +	 */
> > > > +	console_unlock();
> > > > +	console_lock();
> > > >  
> > > >  	res = _braille_unregister_console(console);
> > > >  	if (res)
> > > 
> > > Need to think more if this is race free...
> > 
> > I am afraid that it is racy against for_each_console() when
> > removing the boot consoles.
> 
> Can you explain? Do you mean that we can execute two paths unregistering
> the same bcon?
> 
> 	CPU0					CPU1
> 
> 	console_lock();
> 	__unregister_console(bconA)		console_lock();
> 	console_unlock();
> 
> 						__unregister_console(bconA);
> 						for (a = console_drivers->next ...)
> 							if (a == console)
> 								unregister();
> 							// console bconA is
> 							// not in the list
> 							// anymore
> 						console_unlock();
> 	for (a = console_drivers->next ...)
> 		if (a == console)
> 	console_unlock();
> 
> 
> This CPU0 will never see bconA in the console drivers list.
> But... it will try to do
> 
> 	console->flags &= ~CON_ENABLED;
> 
> Which we need to fix.

I have to admit that I expected more races. But the most intuitive ones
are avoided by the 2nd for-cycle in __unregister_console(). As a
result, most operations are ignored when the console was
already unregistered in parallel.

Anyway, it is really tricky. My head is still spinning around it.
The console_lock handling is really hard to follow. And it is
error prone.

To make it clear. The code has already been tricky. Your patchset has
a potential. It fixes some races but it still keeps the code tricky
anoter way.

OK, all these nasty hacks are needed only because we need to flush
the messages to the console.

Much cleaner solution would be to refactor console_unlock()
and allow to handle existing messages using a separate function.
It is perfectly safe to flush all existing messages to all registered
consoles without releasing console_lock.

How does that sound, please?

Best Regards,
Petr