parisc: Use for_each_console() helper

Replace open coded single-linked list iteration loop with for_each_console()
helper in use.

Signed-off-by Andy Shevchenko <[email protected]>

---
 arch/parisc/kernel/pdc_cons.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

arch/parisc/kernel/pdc_cons.c +0 -0

index 7ed404c60a9e..aa01448f377c 100644

arch/parisc/kernel/pdc_cons.c void pdc_console_restart(void) · line 263

Quoted 8 lines · Andy Shevchenko

Replace open coded single-linked list iteration loop with for_each_console()
helper in use.

arch/parisc/kernel/pdc_cons.c +2 -2

-	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.

Quoted 15 lines · James Bottomley

arch/parisc/kernel/pdc_cons.c void pdc_console_restart(void) · line 263

Quoted 7 lines · Andy Shevchenko

Replace open coded single-linked list iteration loop with for_each_console()
helper in use.

arch/parisc/kernel/pdc_cons.c +2 -2

-	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)

Quoted 1 line

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?

Quoted 16 lines · Andy Shevchenko

Quoted 15 lines · James Bottomley

arch/parisc/kernel/pdc_cons.c void pdc_console_restart(void) · line 263

Quoted 7 lines · Andy Shevchenko

Replace open coded single-linked list iteration loop with for_each_console()
helper in use.

arch/parisc/kernel/pdc_cons.c +2 -2

-	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)

Quoted 6 lines

Quoted 1 line

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.

Quoted 24 lines · James Bottomley

Quoted 16 lines · Andy Shevchenko

Quoted 15 lines · James Bottomley

arch/parisc/kernel/pdc_cons.c void pdc_console_restart(void) · line 263

Quoted 7 lines · Andy Shevchenko

Replace open coded single-linked list iteration loop with for_each_console()
helper in use.

arch/parisc/kernel/pdc_cons.c +2 -2

-	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)

Quoted 6 lines

Quoted 1 line

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?

Quoted 4 lines

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.

Quoted 1 line · Andy Shevchenko

On Fri, Jan 24, 2020 at 09:59:48AM -0800, James Bottomley wrote:

Quoted 8 lines

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?

Quoted 2 lines · Helge Deller

Quoted 1 line · Andy Shevchenko

On Fri, Jan 24, 2020 at 09:59:48AM -0800, James Bottomley wrote:

...

Quoted 6 lines

Quoted 6 lines

By the way, consider this code from register_console()

  for_each_console(bcon)
    if (bcon->flags & CON_BOOT)
      unregister_console(bcon);

Quoted 4 lines

Quoted 2 lines

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.