Quoting Alessandro Carminati (2023-09-08 01:36:14)
> Hello Stephen,
> Thank you for your review and the time you dedicated to it.
>
> Il giorno ven 8 set 2023 alle ore 00:33 Stephen Boyd
> <[email protected]> ha scritto:
> >
> > Quoting Alessandro Carminati (2023-09-07 07:15:36)
> > > this is a gentle ping
> > >
> >
> > I couldn't read your email because it was sent to nobody
> > (unlisted-recipients). Can you resend with a proper To: line?
> >
> > >
> > > Il giorno mar 22 ago 2023 alle ore 15:49 Alessandro Carminati
> > > <[email protected]> ha scritto:
> > > >
> > > > In the possible_parent_show function, ensure proper handling of the return
> > > > value from of_clk_get_parent_name to prevent potential issues arising from
> > > > a NULL return.
> > > > The current implementation invokes seq_puts directly on the result of
> > > > of_clk_get_parent_name without verifying the return value, which can lead
> > > > to kernel panic if the function returns NULL.
> > > >
> > > > This patch addresses the concern by introducing a check on the return
> > > > value of of_clk_get_parent_name. If the return value is not NULL, the
> >
> > Use of_clk_get_parent_name() to signify that it is a function.
> >
> > > > function proceeds to call seq_puts, providing the returned value as
> > > > argument.
> > > > However, if of_clk_get_parent_name returns NULL, the function provides a
> > > > static string as argument, avoiding the panic.
> > > >
> > > > Reported-by: Philip Daly <[email protected]>
> > > > Signed-off-by: Alessandro Carminati (Red Hat) <[email protected]>
> > > > ---
> >
> > It needs a Fixes tag.
> Sure!
> I need to be more careful on this.
>
> >
> > > > drivers/clk/clk.c | 11 ++++++-----
> > > > 1 file changed, 6 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > > > index c249f9791ae8..ab999644e185 100644
> > > > --- a/drivers/clk/clk.c
> > > > +++ b/drivers/clk/clk.c
> > > > @@ -3416,6 +3416,7 @@ static void possible_parent_show(struct seq_file *s, struct clk_core *core,
> > > > unsigned int i, char terminator)
> > > > {
> > > > struct clk_core *parent;
> > > > + const char *tmp;
> > > >
> > > > /*
> > > > * Go through the following options to fetch a parent's name.
> > > > @@ -3436,12 +3437,12 @@ static void possible_parent_show(struct seq_file *s, struct clk_core *core,
> > > > seq_puts(s, core->parents[i].name);
> > > > else if (core->parents[i].fw_name)
> > > > seq_printf(s, "<%s>(fw)", core->parents[i].fw_name);
> > > > - else if (core->parents[i].index >= 0)
> > > > - seq_puts(s,
> > > > - of_clk_get_parent_name(core->of_node,
> > > > - core->parents[i].index));
> > > > - else
> > > > + else if (core->parents[i].index >= 0) {
> > > > + tmp = of_clk_get_parent_name(core->of_node, core->parents[i].index);
> > > > + seq_puts(s, tmp ? tmp : "(none)");
> >
> > How about using seq_printf("%s", ...) instead? That should print out
> > "(null)" in the case that it is NULL, instead of "(none)" and it is a
> > one line change.
>
> I did consider using seq_printf("%s", ...) as an alternative approach to
> address the issue initially.
> However, after a review of the file's history, I arrived at a different
> conclusion.
>
> The commit [1] that introduced this bug was primarily focused on replacing
> seq_printf() with seq_putc().
> I considered that to maintain code consistency and align with the intentions
> of that commit, it may be more appropriate to continue using seq_putc() in
> this particular instance.
> I agree however with the idea of rolling back that particular change, but
> in this case, we perhaps may want to consider also [2], a similar change made
> in the same period.
> I haven't proceeded with a patch submission for it[2], mainly due to the lack
> of evidence of a kernel splash related to it and my uncertainty around the
> fact that can exist use cases where the name field in the struct cgroup_subsys
> can hit that code set to NULL.
Is nothing actually wrong? And this is a speculative patch?
All other arms of this conditional statement check the validity of the
pointer before printing the string. And when the parent isn't known we
print "(missing)", so it looks like we should do that instead. How about
this patch?
----8<---
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index c249f9791ae8..473563bc7496 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3416,6 +3416,7 @@ static void possible_parent_show(struct seq_file *s, struct clk_core *core,
unsigned int i, char terminator)
{
struct clk_core *parent;
+ const char *name = NULL;
/*
* Go through the following options to fetch a parent's name.
@@ -3430,18 +3431,20 @@ static void possible_parent_show(struct seq_file *s, struct clk_core *core,
* registered (yet).
*/
parent = clk_core_get_parent_by_index(core, i);
- if (parent)
+ if (parent) {
seq_puts(s, parent->name);
- else if (core->parents[i].name)
+ } else if (core->parents[i].name) {
seq_puts(s, core->parents[i].name);
- else if (core->parents[i].fw_name)
+ } else if (core->parents[i].fw_name) {
seq_printf(s, "<%s>(fw)", core->parents[i].fw_name);
- else if (core->parents[i].index >= 0)
- seq_puts(s,
- of_clk_get_parent_name(core->of_node,
- core->parents[i].index));
- else
- seq_puts(s, "(missing)");
+ } else {
+ if (core->parents[i].index >= 0)
+ name = of_clk_get_parent_name(core->of_node, core->parents[i].index);
+ if (!name)
+ name = "(missing)";
+
+ seq_puts(s, name);
+ }
seq_putc(s, terminator);
}
Hello Stephen,
Thank you for your time and your work.
Il giorno ven 8 set 2023 alle ore 23:25 Stephen Boyd
<[email protected]> ha scritto:
>
> Quoting Alessandro Carminati (2023-09-08 01:36:14)
> > Hello Stephen,
> > Thank you for your review and the time you dedicated to it.
> >
> > Il giorno ven 8 set 2023 alle ore 00:33 Stephen Boyd
> > <[email protected]> ha scritto:
> > >
> > > Quoting Alessandro Carminati (2023-09-07 07:15:36)
> > > > this is a gentle ping
> > > >
> > >
> > > I couldn't read your email because it was sent to nobody
> > > (unlisted-recipients). Can you resend with a proper To: line?
> > >
> > > >
> > > > Il giorno mar 22 ago 2023 alle ore 15:49 Alessandro Carminati
> > > > <[email protected]> ha scritto:
> > > > >
> > > > > In the possible_parent_show function, ensure proper handling of the return
> > > > > value from of_clk_get_parent_name to prevent potential issues arising from
> > > > > a NULL return.
> > > > > The current implementation invokes seq_puts directly on the result of
> > > > > of_clk_get_parent_name without verifying the return value, which can lead
> > > > > to kernel panic if the function returns NULL.
> > > > >
> > > > > This patch addresses the concern by introducing a check on the return
> > > > > value of of_clk_get_parent_name. If the return value is not NULL, the
> > >
> > > Use of_clk_get_parent_name() to signify that it is a function.
> > >
> > > > > function proceeds to call seq_puts, providing the returned value as
> > > > > argument.
> > > > > However, if of_clk_get_parent_name returns NULL, the function provides a
> > > > > static string as argument, avoiding the panic.
> > > > >
> > > > > Reported-by: Philip Daly <[email protected]>
> > > > > Signed-off-by: Alessandro Carminati (Red Hat) <[email protected]>
> > > > > ---
> > >
> > > It needs a Fixes tag.
> > Sure!
> > I need to be more careful on this.
> >
> > >
> > > > > drivers/clk/clk.c | 11 ++++++-----
> > > > > 1 file changed, 6 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > > > > index c249f9791ae8..ab999644e185 100644
> > > > > --- a/drivers/clk/clk.c
> > > > > +++ b/drivers/clk/clk.c
> > > > > @@ -3416,6 +3416,7 @@ static void possible_parent_show(struct seq_file *s, struct clk_core *core,
> > > > > unsigned int i, char terminator)
> > > > > {
> > > > > struct clk_core *parent;
> > > > > + const char *tmp;
> > > > >
> > > > > /*
> > > > > * Go through the following options to fetch a parent's name.
> > > > > @@ -3436,12 +3437,12 @@ static void possible_parent_show(struct seq_file *s, struct clk_core *core,
> > > > > seq_puts(s, core->parents[i].name);
> > > > > else if (core->parents[i].fw_name)
> > > > > seq_printf(s, "<%s>(fw)", core->parents[i].fw_name);
> > > > > - else if (core->parents[i].index >= 0)
> > > > > - seq_puts(s,
> > > > > - of_clk_get_parent_name(core->of_node,
> > > > > - core->parents[i].index));
> > > > > - else
> > > > > + else if (core->parents[i].index >= 0) {
> > > > > + tmp = of_clk_get_parent_name(core->of_node, core->parents[i].index);
> > > > > + seq_puts(s, tmp ? tmp : "(none)");
> > >
> > > How about using seq_printf("%s", ...) instead? That should print out
> > > "(null)" in the case that it is NULL, instead of "(none)" and it is a
> > > one line change.
> >
> > I did consider using seq_printf("%s", ...) as an alternative approach to
> > address the issue initially.
> > However, after a review of the file's history, I arrived at a different
> > conclusion.
> >
> > The commit [1] that introduced this bug was primarily focused on replacing
> > seq_printf() with seq_putc().
> > I considered that to maintain code consistency and align with the intentions
> > of that commit, it may be more appropriate to continue using seq_putc() in
> > this particular instance.
> > I agree however with the idea of rolling back that particular change, but
> > in this case, we perhaps may want to consider also [2], a similar change made
> > in the same period.
> > I haven't proceeded with a patch submission for it[2], mainly due to the lack
> > of evidence of a kernel splash related to it and my uncertainty around the
> > fact that can exist use cases where the name field in the struct cgroup_subsys
> > can hit that code set to NULL.
>
> Is nothing actually wrong? And this is a speculative patch?
In the current state, Linux can crash, so I would say that there's something
wrong.
I submitted this patch in response to a specific bug report that caused a
kernel crash during testing.
>
> All other arms of this conditional statement check the validity of the
> pointer before printing the string. And when the parent isn't known we
> print "(missing)", so it looks like we should do that instead. How about
> this patch?
Indeed, your patch appears to provide a more verbose coding and accurate
message, which is valuable for troubleshooting.
So yeah, for what it is worth, I think it is good as well.
>
> ----8<---
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index c249f9791ae8..473563bc7496 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -3416,6 +3416,7 @@ static void possible_parent_show(struct seq_file *s, struct clk_core *core,
> unsigned int i, char terminator)
> {
> struct clk_core *parent;
> + const char *name = NULL;
>
> /*
> * Go through the following options to fetch a parent's name.
> @@ -3430,18 +3431,20 @@ static void possible_parent_show(struct seq_file *s, struct clk_core *core,
> * registered (yet).
> */
> parent = clk_core_get_parent_by_index(core, i);
> - if (parent)
> + if (parent) {
> seq_puts(s, parent->name);
> - else if (core->parents[i].name)
> + } else if (core->parents[i].name) {
> seq_puts(s, core->parents[i].name);
> - else if (core->parents[i].fw_name)
> + } else if (core->parents[i].fw_name) {
> seq_printf(s, "<%s>(fw)", core->parents[i].fw_name);
> - else if (core->parents[i].index >= 0)
> - seq_puts(s,
> - of_clk_get_parent_name(core->of_node,
> - core->parents[i].index));
> - else
> - seq_puts(s, "(missing)");
> + } else {
> + if (core->parents[i].index >= 0)
> + name = of_clk_get_parent_name(core->of_node, core->parents[i].index);
> + if (!name)
> + name = "(missing)";
> +
> + seq_puts(s, name);
> + }
>
> seq_putc(s, terminator);
> }
Regards.
Alessandro