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
