While originally it was fine to format strings using "%pOF" while
holding devtree_lock, this now causes a deadlock. Lockdep reports:
of_get_parent from of_fwnode_get_parent+0x18/0x24
^^^^^^^^^^^^^
of_fwnode_get_parent from fwnode_count_parents+0xc/0x28
fwnode_count_parents from fwnode_full_name_string+0x18/0xac
fwnode_full_name_string from device_node_string+0x1a0/0x404
device_node_string from pointer+0x3c0/0x534
pointer from vsnprintf+0x248/0x36c
vsnprintf from vprintk_store+0x130/0x3b4
Fix this by moving the printing in __of_changeset_entry_apply() outside
the lock. As the only difference in the the multiple prints is the
action name, use the existing "action_names" to refactor the prints into
a single print.
Fixes: a92eb7621b9fb2c2 ("lib/vsprintf: Make use of fwnode API to obtain node names and separators")
Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Rob Herring <robh@kernel.org>
---
v5 (v2 in this series):
- Move majority of refactoring to separate patch and minimize the fix
to just moving the print out of the locked section.
v4:
- Add missing 'static' reported by 0-day
v3:
- Re-implement Geert's fix to move the prints rather than move the
spinlock
v1 and v2 from Geert simply moved the devtree_lock into each case
statement:
https://lore.kernel.org/all/c593d8389352c574b5be69d4ca4810da13326a50.1690533838.git.geert+renesas@glider.be/
---
drivers/of/dynamic.c | 27 +++++----------------------
1 file changed, 5 insertions(+), 22 deletions(-)
diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index e311d406b170..2f0eb0053773 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -63,15 +63,13 @@ int of_reconfig_notifier_unregister(struct notifier_block *nb)
}
EXPORT_SYMBOL_GPL(of_reconfig_notifier_unregister);
-#ifdef DEBUG
-const char *action_names[] = {
+static const char *action_names[] = {
[OF_RECONFIG_ATTACH_NODE] = "ATTACH_NODE",
[OF_RECONFIG_DETACH_NODE] = "DETACH_NODE",
[OF_RECONFIG_ADD_PROPERTY] = "ADD_PROPERTY",
[OF_RECONFIG_REMOVE_PROPERTY] = "REMOVE_PROPERTY",
[OF_RECONFIG_UPDATE_PROPERTY] = "UPDATE_PROPERTY",
};
-#endif
int of_reconfig_notify(unsigned long action, struct of_reconfig_data *p)
{
@@ -620,21 +618,9 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
}
ret = __of_add_property(ce->np, ce->prop);
- if (ret) {
- pr_err("changeset: add_property failed @%pOF/%s\n",
- ce->np,
- ce->prop->name);
- break;
- }
break;
case OF_RECONFIG_REMOVE_PROPERTY:
ret = __of_remove_property(ce->np, ce->prop);
- if (ret) {
- pr_err("changeset: remove_property failed @%pOF/%s\n",
- ce->np,
- ce->prop->name);
- break;
- }
break;
case OF_RECONFIG_UPDATE_PROPERTY:
@@ -648,20 +634,17 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
}
ret = __of_update_property(ce->np, ce->prop, &old_prop);
- if (ret) {
- pr_err("changeset: update_property failed @%pOF/%s\n",
- ce->np,
- ce->prop->name);
- break;
- }
break;
default:
ret = -EINVAL;
}
raw_spin_unlock_irqrestore(&devtree_lock, flags);
- if (ret)
+ if (ret) {
+ pr_err("changeset: apply failed: cset<%p> %-15s %pOF:%s\n",
+ ce, action_names[ce->action], ce->np, ce->prop->name);
return ret;
+ }
switch (ce->action) {
case OF_RECONFIG_ATTACH_NODE:
--
2.40.1Hi Rob,
On Sat, Aug 5, 2023 at 12:42 AM Rob Herring <robh@kernel.org> wrote:
> While originally it was fine to format strings using "%pOF" while
> holding devtree_lock, this now causes a deadlock. Lockdep reports:
>
> of_get_parent from of_fwnode_get_parent+0x18/0x24
> ^^^^^^^^^^^^^
> of_fwnode_get_parent from fwnode_count_parents+0xc/0x28
> fwnode_count_parents from fwnode_full_name_string+0x18/0xac
> fwnode_full_name_string from device_node_string+0x1a0/0x404
> device_node_string from pointer+0x3c0/0x534
> pointer from vsnprintf+0x248/0x36c
> vsnprintf from vprintk_store+0x130/0x3b4
>
> Fix this by moving the printing in __of_changeset_entry_apply() outside
> the lock. As the only difference in the the multiple prints is the
> action name, use the existing "action_names" to refactor the prints into
> a single print.
>
> Fixes: a92eb7621b9fb2c2 ("lib/vsprintf: Make use of fwnode API to obtain node names and separators")
> Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
> v5 (v2 in this series):
> - Move majority of refactoring to separate patch and minimize the fix
> to just moving the print out of the locked section.
Thanks for your patch!
> --- a/drivers/of/dynamic.c
> +++ b/drivers/of/dynamic.c
> @@ -648,20 +634,17 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
> }
>
> ret = __of_update_property(ce->np, ce->prop, &old_prop);
> - if (ret) {
> - pr_err("changeset: update_property failed @%pOF/%s\n",
> - ce->np,
> - ce->prop->name);
> - break;
> - }
> break;
> default:
> ret = -EINVAL;
> }
> raw_spin_unlock_irqrestore(&devtree_lock, flags);
>
> - if (ret)
> + if (ret) {
> + pr_err("changeset: apply failed: cset<%p> %-15s %pOF:%s\n",
Printing the cset pointer will (needlessly?) complicate the EXPECT_*()
handling in the unit test.
> + ce, action_names[ce->action], ce->np, ce->prop->name);
This should check ce->action to avoid an out-of-bounds access beyond
the end of action_names[].
> return ret;
> + }
>
> switch (ce->action) {
> case OF_RECONFIG_ATTACH_NODE:
The rest LGTM to me.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Fri, Aug 18, 2023 at 10:36 AM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
> Hi Rob,
>
> On Sat, Aug 5, 2023 at 12:42 AM Rob Herring <robh@kernel.org> wrote:
> > While originally it was fine to format strings using "%pOF" while
> > holding devtree_lock, this now causes a deadlock. Lockdep reports:
> >
> > of_get_parent from of_fwnode_get_parent+0x18/0x24
> > ^^^^^^^^^^^^^
> > of_fwnode_get_parent from fwnode_count_parents+0xc/0x28
> > fwnode_count_parents from fwnode_full_name_string+0x18/0xac
> > fwnode_full_name_string from device_node_string+0x1a0/0x404
> > device_node_string from pointer+0x3c0/0x534
> > pointer from vsnprintf+0x248/0x36c
> > vsnprintf from vprintk_store+0x130/0x3b4
> >
> > Fix this by moving the printing in __of_changeset_entry_apply() outside
> > the lock. As the only difference in the the multiple prints is the
> > action name, use the existing "action_names" to refactor the prints into
> > a single print.
> >
> > Fixes: a92eb7621b9fb2c2 ("lib/vsprintf: Make use of fwnode API to obtain node names and separators")
> > Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > Signed-off-by: Rob Herring <robh@kernel.org>
> > ---
> > v5 (v2 in this series):
> > - Move majority of refactoring to separate patch and minimize the fix
> > to just moving the print out of the locked section.
>
> Thanks for your patch!
>
> > --- a/drivers/of/dynamic.c
> > +++ b/drivers/of/dynamic.c
>
> > @@ -648,20 +634,17 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
> > }
> >
> > ret = __of_update_property(ce->np, ce->prop, &old_prop);
> > - if (ret) {
> > - pr_err("changeset: update_property failed @%pOF/%s\n",
> > - ce->np,
> > - ce->prop->name);
> > - break;
> > - }
> > break;
> > default:
> > ret = -EINVAL;
> > }
> > raw_spin_unlock_irqrestore(&devtree_lock, flags);
> >
> > - if (ret)
> > + if (ret) {
> > + pr_err("changeset: apply failed: cset<%p> %-15s %pOF:%s\n",
>
> Printing the cset pointer will (needlessly?) complicate the EXPECT_*()
> handling in the unit test.
That's added largely because the other prints which I rework later in
this series had them. Either printing the changeset ptr is useful or
it isn't. I think people running the unittest and the post-processor
can easily enough filter this out when looking at the results.
Honestly, even I probably run it less than once a cycle.
>
> > + ce, action_names[ce->action], ce->np, ce->prop->name);
>
> This should check ce->action to avoid an out-of-bounds access beyond
> the end of action_names[].
Indeed.
I think I'll add "invalid" to action_names names and then do something
like: "(ce->action < FOO) ? ce->action : 0".
Rob
Hi Rob,
On Fri, Aug 18, 2023 at 6:17 PM Rob Herring <robh@kernel.org> wrote:
> On Fri, Aug 18, 2023 at 10:36 AM Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
> > On Sat, Aug 5, 2023 at 12:42 AM Rob Herring <robh@kernel.org> wrote:
> > > While originally it was fine to format strings using "%pOF" while
> > > holding devtree_lock, this now causes a deadlock. Lockdep reports:
> > >
> > > of_get_parent from of_fwnode_get_parent+0x18/0x24
> > > ^^^^^^^^^^^^^
> > > of_fwnode_get_parent from fwnode_count_parents+0xc/0x28
> > > fwnode_count_parents from fwnode_full_name_string+0x18/0xac
> > > fwnode_full_name_string from device_node_string+0x1a0/0x404
> > > device_node_string from pointer+0x3c0/0x534
> > > pointer from vsnprintf+0x248/0x36c
> > > vsnprintf from vprintk_store+0x130/0x3b4
> > >
> > > Fix this by moving the printing in __of_changeset_entry_apply() outside
> > > the lock. As the only difference in the the multiple prints is the
> > > action name, use the existing "action_names" to refactor the prints into
> > > a single print.
> > >
> > > Fixes: a92eb7621b9fb2c2 ("lib/vsprintf: Make use of fwnode API to obtain node names and separators")
> > > Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > Signed-off-by: Rob Herring <robh@kernel.org>
> > > ---
> > > v5 (v2 in this series):
> > > - Move majority of refactoring to separate patch and minimize the fix
> > > to just moving the print out of the locked section.
> >
> > Thanks for your patch!
> >
> > > --- a/drivers/of/dynamic.c
> > > +++ b/drivers/of/dynamic.c
> >
> > > @@ -648,20 +634,17 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
> > > }
> > >
> > > ret = __of_update_property(ce->np, ce->prop, &old_prop);
> > > - if (ret) {
> > > - pr_err("changeset: update_property failed @%pOF/%s\n",
> > > - ce->np,
> > > - ce->prop->name);
> > > - break;
> > > - }
> > > break;
> > > default:
> > > ret = -EINVAL;
> > > }
> > > raw_spin_unlock_irqrestore(&devtree_lock, flags);
> > >
> > > - if (ret)
> > > + if (ret) {
> > > + pr_err("changeset: apply failed: cset<%p> %-15s %pOF:%s\n",
> >
> > Printing the cset pointer will (needlessly?) complicate the EXPECT_*()
> > handling in the unit test.
>
> That's added largely because the other prints which I rework later in
> this series had them. Either printing the changeset ptr is useful or
> it isn't. I think people running the unittest and the post-processor
> can easily enough filter this out when looking at the results.
> Honestly, even I probably run it less than once a cycle.
Do you have a use for printing the pointer value?
And by default, it will be an obfuscated cookie anyway.
> > > + ce, action_names[ce->action], ce->np, ce->prop->name);
> >
> > This should check ce->action to avoid an out-of-bounds access beyond
> > the end of action_names[].
>
> Indeed.
>
> I think I'll add "invalid" to action_names names and then do something
> like: "(ce->action < FOO) ? ce->action : 0".
OK, zero is invalid.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Fri, Aug 18, 2023 at 12:37 PM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
> Hi Rob,
>
> On Fri, Aug 18, 2023 at 6:17 PM Rob Herring <robh@kernel.org> wrote:
> > On Fri, Aug 18, 2023 at 10:36 AM Geert Uytterhoeven
> > <geert@linux-m68k.org> wrote:
> > > On Sat, Aug 5, 2023 at 12:42 AM Rob Herring <robh@kernel.org> wrote:
> > > > While originally it was fine to format strings using "%pOF" while
> > > > holding devtree_lock, this now causes a deadlock. Lockdep reports:
> > > >
> > > > of_get_parent from of_fwnode_get_parent+0x18/0x24
> > > > ^^^^^^^^^^^^^
> > > > of_fwnode_get_parent from fwnode_count_parents+0xc/0x28
> > > > fwnode_count_parents from fwnode_full_name_string+0x18/0xac
> > > > fwnode_full_name_string from device_node_string+0x1a0/0x404
> > > > device_node_string from pointer+0x3c0/0x534
> > > > pointer from vsnprintf+0x248/0x36c
> > > > vsnprintf from vprintk_store+0x130/0x3b4
> > > >
> > > > Fix this by moving the printing in __of_changeset_entry_apply() outside
> > > > the lock. As the only difference in the the multiple prints is the
> > > > action name, use the existing "action_names" to refactor the prints into
> > > > a single print.
> > > >
> > > > Fixes: a92eb7621b9fb2c2 ("lib/vsprintf: Make use of fwnode API to obtain node names and separators")
> > > > Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > > Signed-off-by: Rob Herring <robh@kernel.org>
> > > > ---
> > > > v5 (v2 in this series):
> > > > - Move majority of refactoring to separate patch and minimize the fix
> > > > to just moving the print out of the locked section.
> > >
> > > Thanks for your patch!
> > >
> > > > --- a/drivers/of/dynamic.c
> > > > +++ b/drivers/of/dynamic.c
> > >
> > > > @@ -648,20 +634,17 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
> > > > }
> > > >
> > > > ret = __of_update_property(ce->np, ce->prop, &old_prop);
> > > > - if (ret) {
> > > > - pr_err("changeset: update_property failed @%pOF/%s\n",
> > > > - ce->np,
> > > > - ce->prop->name);
> > > > - break;
> > > > - }
> > > > break;
> > > > default:
> > > > ret = -EINVAL;
> > > > }
> > > > raw_spin_unlock_irqrestore(&devtree_lock, flags);
> > > >
> > > > - if (ret)
> > > > + if (ret) {
> > > > + pr_err("changeset: apply failed: cset<%p> %-15s %pOF:%s\n",
> > >
> > > Printing the cset pointer will (needlessly?) complicate the EXPECT_*()
> > > handling in the unit test.
> >
> > That's added largely because the other prints which I rework later in
> > this series had them. Either printing the changeset ptr is useful or
> > it isn't. I think people running the unittest and the post-processor
> > can easily enough filter this out when looking at the results.
> > Honestly, even I probably run it less than once a cycle.
>
> Do you have a use for printing the pointer value?
I have no use for overlays in general, so no. :)
I'd assumed it was there to provide a changeset ID to tell which
actions belong to the same changeset. But it's printing the changeset
entry rather than the changeset, so I agree it is not really useful.
Rob
On Fri, Aug 04, 2023 at 04:41:52PM -0600, Rob Herring wrote: ... > v5 (v2 in this series): > v4: > v3: > v1 and v2 from Geert simply moved the devtree_lock into each case > statement: > https://lore.kernel.org/all/c593d8389352c574b5be69d4ca4810da13326a50.1690533838.git.geert+renesas@glider.be/ Side note: More natural to use main versioning as of this series and put the other to the parentheses. -- With Best Regards, Andy Shevchenko
© 2016 - 2026 Red Hat, Inc.