drivers/clk/clk.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
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
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 <pdaly@redhat.com>
Signed-off-by: Alessandro Carminati (Red Hat) <alessandro.carminati@gmail.com>
---
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)");
+ } else {
seq_puts(s, "(missing)");
+ }
seq_putc(s, terminator);
}
--
2.34.1this is a gentle ping
thank you
Alessandro Carminati
Il giorno mar 22 ago 2023 alle ore 15:49 Alessandro Carminati
<alessandro.carminati@gmail.com> 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
> 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 <pdaly@redhat.com>
> Signed-off-by: Alessandro Carminati (Red Hat) <alessandro.carminati@gmail.com>
> ---
> 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)");
> + } else {
> seq_puts(s, "(missing)");
> + }
>
> seq_putc(s, terminator);
> }
> --
> 2.34.1
>
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
> <alessandro.carminati@gmail.com> 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 <pdaly@redhat.com>
> > Signed-off-by: Alessandro Carminati (Red Hat) <alessandro.carminati@gmail.com>
> > ---
It needs a Fixes tag.
> > 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.
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
<sboyd@kernel.org> 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
> > <alessandro.carminati@gmail.com> 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 <pdaly@redhat.com>
> > > Signed-off-by: Alessandro Carminati (Red Hat) <alessandro.carminati@gmail.com>
> > > ---
>
> 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.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/patch/?id=1ccc0ddf046a0197f2f9acca02a64da10aa6112d
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/patch/?id=85db0023376f529c477c6110043e069ccee16d9c
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
> <sboyd@kernel.org> 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
> > > <alessandro.carminati@gmail.com> 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 <pdaly@redhat.com>
> > > > Signed-off-by: Alessandro Carminati (Red Hat) <alessandro.carminati@gmail.com>
> > > > ---
> >
> > 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
<sboyd@kernel.org> 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
> > <sboyd@kernel.org> 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
> > > > <alessandro.carminati@gmail.com> 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 <pdaly@redhat.com>
> > > > > Signed-off-by: Alessandro Carminati (Red Hat) <alessandro.carminati@gmail.com>
> > > > > ---
> > >
> > > 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
Quoting Alessandro Carminati (2023-09-12 10:05:19) > Il giorno ven 8 set 2023 alle ore 23:25 Stephen Boyd > <sboyd@kernel.org> ha scritto: > > > > 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. Where is the bug report? On some public tracker? Can you link to it? > > > > > 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. Cool, can you send it after testing it out?
Alessandro Carminati Mobile: +39.339.8870183 Il giorno mar 12 set 2023 alle ore 19:59 Stephen Boyd <sboyd@kernel.org> ha scritto: > > Quoting Alessandro Carminati (2023-09-12 10:05:19) > > Il giorno ven 8 set 2023 alle ore 23:25 Stephen Boyd > > <sboyd@kernel.org> ha scritto: > > > > > > 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. > > Where is the bug report? On some public tracker? Can you link to it? Sorry for the wait, I used this time to verify internally if the test can be shared. Unfortunately, we can't share it on a public tracker or the test outside our company. But I can definitely provide a simple reproducer. It's a breeze once you've got the right hardware, like the Qualcomm SA8540P-based board where I found it. Here it is: # cat /sys/kernel/debug/clk/gcc_usb4_phy_sys_clk_src/clk_possible_parents [ 139.322764] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000 [ 139.331802] Mem abort info: [ 139.334679] ESR = 0x0000000096000004 [ 139.338536] EC = 0x25: DABT (current EL), IL = 32 bits [ 139.343991] SET = 0, FnV = 0 [ 139.347131] EA = 0, S1PTW = 0 [ 139.350356] FSC = 0x04: level 0 translation fault [ 139.355360] Data abort info: [ 139.358326] ISV = 0, ISS = 0x00000004 [ 139.362270] CM = 0, WnR = 0 [ 139.365322] user pgtable: 4k pages, 48-bit VAs, pgdp=0000000109abb000 [ 139.371931] [0000000000000000] pgd=0000000000000000, p4d=0000000000000000 [ 139.378909] Internal error: Oops: 0000000096000004 [#1] PREEMPT_RT SMP [ 139.378915] Modules linked in: rfkill dwc3 pinctrl_spmi_gpio rtc_pm8xxx qcom_spmi_pmic marvell regmap_spmi udc_core dwmac_qcom_ethqos socinfo stmmac_platform stmmac qrtr crct10dif_ce ghash_ce sha2_ce qcom_q6v5_pas sha256_arm64 sha1_ce qcom_pil_info spmi_pmic_arb mdt_loader qcom_common spmi qcom_glink_smem phy_qcom_qmp_usb dwc3_qcom qcom_glink llcc_qcom pcs_xpcs qcom_smd phy_qcom_qmp_combo phylink qcom_q6v5 phy_qcom_snps_femto_v2 qcom_sysmon rpmsg_core qcom_hwspinlock qmi_helpers qcom_cpufreq_hw qcom_aoss qcom_wdt qcom_rng smp2p smem sg fuse ext4 mbcache jbd2 ufs_qcom ufshcd_pltfrm ufshcd_core qcom_geni_serial rpmhpd qcom_geni_se qcom_rpmh_regulator pinctrl_sc8280xp pinctrl_sa8775p pinctrl_msm phy_qcom_qmp_ufs qnoc_sc8280xp qnoc_sa8775p icc_rpmh icc_bcm_voter governor_simpleondemand gcc_sc8280xp gcc_sa8775p clk_rpmh qcom_rpmh cmd_db clk_qcom [ 139.379012] CPU: 7 PID: 623 Comm: cat Not tainted 5.14.0+ #149 [ 139.379017] Hardware name: Qualcomm SA8540P Ride (DT) [ 139.379019] pstate: 80400005 (Nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) [ 139.379024] pc : __pi_strlen+0x14/0x150 [ 139.379037] lr : seq_puts+0x28/0x70 [ 139.379047] sp : ffff800009bbba90 [ 139.379048] x29: ffff800009bbba90 x28: ffff10604726fdd0 x27: 0000000000400cc0 [ 139.379054] x26: 000000007ffff000 x25: ffff10604726fdc0 x24: 0000000000000000 [ 139.379058] x23: ffff800009bbbbd0 x22: 0000000000000020 x21: ffff10604076a500 [ 139.379063] x20: 0000000000000000 x19: ffff10604726fd98 x18: 0000000000000000 [ 139.379067] x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000000 [ 139.379071] x14: 0000000000000000 x13: 0000000000000000 x12: 0000000000000000 [ 139.379076] x11: 0000000000000000 x10: ffffa59b267f6f1b x9 : ffffa19b2397332c [ 139.379080] x8 : 0101010101010101 x7 : 00000000736c6c65 x6 : 0080e3ecef636b2d [ 139.379085] x5 : 2d6b636f6c630000 x4 : 0000000000000000 x3 : 0000000000000000 [ 139.379090] x2 : 0000000000000000 x1 : 0000000000000000 x0 : 0000000000000000 [ 139.379094] Call trace: [ 139.379095] __pi_strlen+0x14/0x150 [ 139.379101] possible_parent_show+0x7c/0x100 [ 139.379108] possible_parents_show+0x4c/0x90 [ 139.379114] seq_read_iter+0xdc/0x480 [ 139.379120] seq_read+0xc0/0x100 [ 139.379127] full_proxy_read+0x64/0xb0 [ 139.379135] vfs_read+0xb4/0x1d0 [ 139.379142] ksys_read+0x70/0x100 [ 139.379148] __arm64_sys_read+0x20/0x30 [ 139.379154] invoke_syscall.constprop.0+0x7c/0xd0 [ 139.379164] el0_svc_common.constprop.0+0x124/0x134 [ 139.379170] do_el0_svc+0x2c/0xac [ 139.379176] el0_svc+0x38/0x1c0 [ 139.379184] el0t_64_sync_handler+0x10c/0x11c [ 139.379187] el0t_64_sync+0x17c/0x180 [ 139.379192] Code: 92402c04 b200c3e8 f13fc09f 5400088c (a9400c02) [ 139.631968] ---[ end trace f38c177abbfb4315 ]--- [ 139.631971] Kernel panic - not syncing: Oops: Fatal exception [ 139.642601] SMP: stopping secondary CPUs [ 139.643039] Kernel Offset: 0x219b1b280000 from 0xffff800008000000 [ 139.643040] PHYS_OFFSET: 0xffffefa0c0000000 [ 139.643040] CPU features: 0x0000000,05c01018,c801720b [ 139.643042] Memory Limit: none > > > > > > > > > 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. > > Cool, can you send it after testing it out? I will thank you for your assistance.
© 2016 - 2025 Red Hat, Inc.