* [PATCH v1] parisc: Use for_each_console() helper
@ 2020-01-24 16:07 Andy Shevchenko
2020-01-24 16:39 ` James Bottomley
0 siblings, 1 reply; 8+ messages in thread
From: Andy Shevchenko @ 2020-01-24 16:07 UTC (permalink / raw)
To: James E.J. Bottomley, Helge Deller, linux-parisc; +Cc: 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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v1] parisc: Use for_each_console() helper
2020-01-24 16:07 [PATCH v1] parisc: Use for_each_console() helper Andy Shevchenko
@ 2020-01-24 16:39 ` James Bottomley
2020-01-24 17:38 ` Andy Shevchenko
0 siblings, 1 reply; 8+ messages in thread
From: James Bottomley @ 2020-01-24 16:39 UTC (permalink / raw)
To: Andy Shevchenko, Helge Deller, linux-parisc
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1] parisc: Use for_each_console() helper
2020-01-24 16:39 ` James Bottomley
@ 2020-01-24 17:38 ` Andy Shevchenko
2020-01-24 17:59 ` James Bottomley
0 siblings, 1 reply; 8+ messages in thread
From: Andy Shevchenko @ 2020-01-24 17:38 UTC (permalink / raw)
To: James Bottomley; +Cc: Helge Deller, linux-parisc
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1] parisc: Use for_each_console() helper
2020-01-24 17:38 ` Andy Shevchenko
@ 2020-01-24 17:59 ` James Bottomley
2020-01-25 10:25 ` Andy Shevchenko
0 siblings, 1 reply; 8+ messages in thread
From: James Bottomley @ 2020-01-24 17:59 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Helge Deller, linux-parisc
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1] parisc: Use for_each_console() helper
2020-01-24 17:59 ` James Bottomley
@ 2020-01-25 10:25 ` Andy Shevchenko
2020-01-27 8:48 ` Helge Deller
0 siblings, 1 reply; 8+ messages in thread
From: Andy Shevchenko @ 2020-01-25 10:25 UTC (permalink / raw)
To: James Bottomley; +Cc: Helge Deller, linux-parisc
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1] parisc: Use for_each_console() helper
2020-01-25 10:25 ` Andy Shevchenko
@ 2020-01-27 8:48 ` Helge Deller
2020-01-27 9:47 ` Andy Shevchenko
0 siblings, 1 reply; 8+ messages in thread
From: Helge Deller @ 2020-01-27 8:48 UTC (permalink / raw)
To: Andy Shevchenko, James Bottomley; +Cc: linux-parisc
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1] parisc: Use for_each_console() helper
2020-01-27 8:48 ` Helge Deller
@ 2020-01-27 9:47 ` Andy Shevchenko
2020-01-30 14:07 ` Petr Mladek
0 siblings, 1 reply; 8+ messages in thread
From: Andy Shevchenko @ 2020-01-27 9:47 UTC (permalink / raw)
To: Helge Deller
Cc: James Bottomley, linux-parisc, Petr Mladek, Sergey Senozhatsky,
Steven Rostedt
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1] parisc: Use for_each_console() helper
2020-01-27 9:47 ` Andy Shevchenko
@ 2020-01-30 14:07 ` Petr Mladek
0 siblings, 0 replies; 8+ messages in thread
From: Petr Mladek @ 2020-01-30 14:07 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Helge Deller, James Bottomley, linux-parisc, Sergey Senozhatsky,
Steven Rostedt
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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-01-30 14:07 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-24 16:07 [PATCH v1] parisc: Use for_each_console() helper Andy Shevchenko
2020-01-24 16:39 ` James Bottomley
2020-01-24 17:38 ` Andy Shevchenko
2020-01-24 17:59 ` James Bottomley
2020-01-25 10:25 ` Andy Shevchenko
2020-01-27 8:48 ` Helge Deller
2020-01-27 9:47 ` Andy Shevchenko
2020-01-30 14:07 ` Petr Mladek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).