| Message ID | 20200124160739.65256-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
| Series | [v1] parisc: Use for_each_console() helper | expand |
On Fri, 2020-01-24 at 18:07 +0200, Andy Shevchenko wrote: > Replace open coded single-linked list iteration loop with > for_each_console() > helper in use. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > arch/parisc/kernel/pdc_cons.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/parisc/kernel/pdc_cons.c > b/arch/parisc/kernel/pdc_cons.c > index 7ed404c60a9e..aa01448f377c 100644 > --- a/arch/parisc/kernel/pdc_cons.c > +++ b/arch/parisc/kernel/pdc_cons.c > @@ -259,8 +259,8 @@ void pdc_console_restart(void) > if (console_drivers != NULL) > pdc_cons.flags &= ~CON_PRINTBUFFER; > > - while ((console = console_drivers) != NULL) > - unregister_console(console_drivers); > + for_each_console(console) > + unregister_console(console); This is wrong. The old formulation iterates correctly in the face of element removal. for_each_console is defined: #define for_each_console(con) \ for (con = console_drivers; con != NULL; con = con->next) So it's not safe for any iteration that alters the list elements. James
On Fri, Jan 24, 2020 at 08:39:02AM -0800, James Bottomley wrote: > On Fri, 2020-01-24 at 18:07 +0200, Andy Shevchenko wrote: > > Replace open coded single-linked list iteration loop with > > for_each_console() > > helper in use. > > - while ((console = console_drivers) != NULL) > > - unregister_console(console_drivers); > > + for_each_console(console) > > + unregister_console(console); > > This is wrong. The old formulation iterates correctly in the face of > element removal. for_each_console is defined: > > #define for_each_console(con) \ > for (con = console_drivers; con != NULL; con = con->next) > > So it's not safe for any iteration that alters the list elements. Ah, I see. In this case we need to keep a pointer to the next element. Though, the original code assumes that console_drivers after unregistration will be promoted to the next element. Do we have this assumption solid? Thanks!
On Fri, 2020-01-24 at 19:38 +0200, Andy Shevchenko wrote: > On Fri, Jan 24, 2020 at 08:39:02AM -0800, James Bottomley wrote: > > On Fri, 2020-01-24 at 18:07 +0200, Andy Shevchenko wrote: > > > Replace open coded single-linked list iteration loop with > > > for_each_console() > > > helper in use. > > > - while ((console = console_drivers) != NULL) > > > - unregister_console(console_drivers); > > > + for_each_console(console) > > > + unregister_console(console); > > > > This is wrong. The old formulation iterates correctly in the face > > of element removal. for_each_console is defined: > > > > #define for_each_console(con) \ > > for (con = console_drivers; con != NULL; con = con->next) > > > > So it's not safe for any iteration that alters the list elements. > > Ah, I see. In this case we need to keep a pointer to the next > element. Though, the original code assumes that console_drivers after > unregistration will be promoted to the next element. Do we have this > assumption solid? Yes, the original code simply removes the head until the list is empty. That's a recognized way of emptying any list while letting the remove code take care of the locking ... it works because parisc doesn't have a braille console. James
On Fri, Jan 24, 2020 at 09:59:48AM -0800, James Bottomley wrote: > On Fri, 2020-01-24 at 19:38 +0200, Andy Shevchenko wrote: > > On Fri, Jan 24, 2020 at 08:39:02AM -0800, James Bottomley wrote: > > > On Fri, 2020-01-24 at 18:07 +0200, Andy Shevchenko wrote: > > > > Replace open coded single-linked list iteration loop with > > > > for_each_console() > > > > helper in use. > > > > - while ((console = console_drivers) != NULL) > > > > - unregister_console(console_drivers); > > > > + for_each_console(console) > > > > + unregister_console(console); > > > > > > This is wrong. The old formulation iterates correctly in the face > > > of element removal. for_each_console is defined: > > > > > > #define for_each_console(con) \ > > > for (con = console_drivers; con != NULL; con = con->next) > > > > > > So it's not safe for any iteration that alters the list elements. > > > > Ah, I see. In this case we need to keep a pointer to the next > > element. Though, the original code assumes that console_drivers after > > unregistration will be promoted to the next element. Do we have this > > assumption solid? > > Yes, the original code simply removes the head until the list is empty. > That's a recognized way of emptying any list while letting the remove > code take care of the locking ... it works because parisc doesn't have > a braille console. By the way, consider this code from register_console() for_each_console(bcon) if (bcon->flags & CON_BOOT) unregister_console(bcon); It works based on assumption that next pointer of the just unregistered console is not damaged. So, My initial patch will work in the same way.
On 25.01.20 11:25, Andy Shevchenko wrote: > On Fri, Jan 24, 2020 at 09:59:48AM -0800, James Bottomley wrote: >> On Fri, 2020-01-24 at 19:38 +0200, Andy Shevchenko wrote: >>> On Fri, Jan 24, 2020 at 08:39:02AM -0800, James Bottomley wrote: >>>> On Fri, 2020-01-24 at 18:07 +0200, Andy Shevchenko wrote: >>>>> Replace open coded single-linked list iteration loop with >>>>> for_each_console() >>>>> helper in use. >>>>> - while ((console = console_drivers) != NULL) >>>>> - unregister_console(console_drivers); >>>>> + for_each_console(console) >>>>> + unregister_console(console); >>>> >>>> This is wrong. The old formulation iterates correctly in the face >>>> of element removal. for_each_console is defined: >>>> >>>> #define for_each_console(con) \ >>>> for (con = console_drivers; con != NULL; con = con->next) >>>> >>>> So it's not safe for any iteration that alters the list elements. >>> >>> Ah, I see. In this case we need to keep a pointer to the next >>> element. Though, the original code assumes that console_drivers after >>> unregistration will be promoted to the next element. Do we have this >>> assumption solid? >> >> Yes, the original code simply removes the head until the list is empty. >> That's a recognized way of emptying any list while letting the remove >> code take care of the locking ... it works because parisc doesn't have >> a braille console. > > By the way, consider this code from register_console() > > for_each_console(bcon) > if (bcon->flags & CON_BOOT) > unregister_console(bcon); > > It works based on assumption that next pointer of the just unregistered console > is not damaged. So, My initial patch will work in the same way. Yeah, but that's a typical use-after-free issue, which I wouldn't count on. Isn't there a way to make both safe? Helge
On Mon, Jan 27, 2020 at 09:48:58AM +0100, Helge Deller wrote: > On 25.01.20 11:25, Andy Shevchenko wrote: > > On Fri, Jan 24, 2020 at 09:59:48AM -0800, James Bottomley wrote: ... > > By the way, consider this code from register_console() > > > > for_each_console(bcon) > > if (bcon->flags & CON_BOOT) > > unregister_console(bcon); > > > > It works based on assumption that next pointer of the just unregistered console > > is not damaged. So, My initial patch will work in the same way. > > Yeah, but that's a typical use-after-free issue, which I wouldn't count on. I think here is misinterpretation, i.e. unregister != free. Entire console code is written in the assumption that console is not being freed when unregistered. +Cc: PRINTK people. > Isn't there a way to make both safe?
On Mon 2020-01-27 11:47:22, Andy Shevchenko wrote: > On Mon, Jan 27, 2020 at 09:48:58AM +0100, Helge Deller wrote: > > On 25.01.20 11:25, Andy Shevchenko wrote: > > > On Fri, Jan 24, 2020 at 09:59:48AM -0800, James Bottomley wrote: > > ... > > > > By the way, consider this code from register_console() > > > > > > for_each_console(bcon) > > > if (bcon->flags & CON_BOOT) > > > unregister_console(bcon); > > > > > > It works based on assumption that next pointer of the just unregistered console > > > is not damaged. So, My initial patch will work in the same way. > > > > Yeah, but that's a typical use-after-free issue, which I wouldn't count on. > > I think here is misinterpretation, i.e. unregister != free. > Entire console code is written in the assumption that console is not being > freed when unregistered. Honestly, I am not sure if this is true for all console drivers and if it is by design. I would prefer to stay on the safe side and keep the original code. Hotplug practices are more and more popular as everything gets virtualized. Best Regards, Petr
diff --git a/arch/parisc/kernel/pdc_cons.c b/arch/parisc/kernel/pdc_cons.c index 7ed404c60a9e..aa01448f377c 100644 --- a/arch/parisc/kernel/pdc_cons.c +++ b/arch/parisc/kernel/pdc_cons.c @@ -259,8 +259,8 @@ void pdc_console_restart(void) if (console_drivers != NULL) pdc_cons.flags &= ~CON_PRINTBUFFER; - while ((console = console_drivers) != NULL) - unregister_console(console_drivers); + for_each_console(console) + unregister_console(console); /* force registering the pdc console */ pdc_console_init_force();
Replace open coded single-linked list iteration loop with for_each_console() helper in use. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- arch/parisc/kernel/pdc_cons.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)