Re: [PATCH v1] parisc: Use for_each_console() helper

[Helge Deller <deller@gmx.de>]

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