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
