[PATCH v1] parisc: Use for_each_console() helper

[PATCH v1] parisc: Use for_each_console() helper

[Andy Shevchenko]

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);
 
 	/* force registering the pdc console */
 	pdc_console_init_force();
-- 
2.24.1

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

[James Bottomley]

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

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

[Andy Shevchenko]

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!

-- 
With Best Regards,
Andy Shevchenko

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

[James Bottomley]

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

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

[Andy Shevchenko]

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.


-- 
With Best Regards,
Andy Shevchenko

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

[Helge Deller]

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

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

[Andy Shevchenko]

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?

-- 
With Best Regards,
Andy Shevchenko

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

[Petr Mladek]

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