diff mbox series

[01/13] of: dynamic: Do not use "%pOF" while holding devtree_lock

Message ID 416d1ea056bb2d7ec6f21d8919b96a3d48099344.1689776064.git.geert+renesas@glider.be (mailing list archive)
State Superseded
Delegated to: Geert Uytterhoeven
Headers show
Series of: overlay/unittest: Miscellaneous fixes and improvements | expand

Commit Message

Geert Uytterhoeven July 19, 2023, 3 p.m. UTC
Formatting strings using "%pOF" while holding devtree_lock 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 making the locking cover only the parts that really need it.

Fixes: 0d638a07d3a1e98a ("of: Convert to using %pOF instead of full_name")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/of/dynamic.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

Comments

Rob Herring (Arm) July 19, 2023, 11:02 p.m. UTC | #1
On Wed, Jul 19, 2023 at 05:00:01PM +0200, Geert Uytterhoeven wrote:
> Formatting strings using "%pOF" while holding devtree_lock causes a
> deadlock.  Lockdep reports:
> 
>     of_get_parent from of_fwnode_get_parent+0x18/0x24
>     ^^^^^^^^^^^^^

I'm wondering if we really need the lock in there. We never unset or 
change the parent. It gets detached, but we're not checking for that. 
The node could get freed, but the lock is not for that, refcounts are.

>     of_fwnode_get_parent from fwnode_count_parents+0xc/0x28

count parents? Huh? Isn't it always 1? 

>     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 making the locking cover only the parts that really need it.
> 
> Fixes: 0d638a07d3a1e98a ("of: Convert to using %pOF instead of full_name")
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  drivers/of/dynamic.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> index e311d406b1705306..eae45a1c673ee05f 100644
> --- a/drivers/of/dynamic.c
> +++ b/drivers/of/dynamic.c
> @@ -601,13 +601,16 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
>  
>  	__of_changeset_entry_dump(ce);
>  
> -	raw_spin_lock_irqsave(&devtree_lock, flags);
>  	switch (ce->action) {
>  	case OF_RECONFIG_ATTACH_NODE:
> +		raw_spin_lock_irqsave(&devtree_lock, flags);
>  		__of_attach_node(ce->np);
> +		raw_spin_unlock_irqrestore(&devtree_lock, flags);

I think you could just move the spinlock into __of_attach_node(). The 
only other caller looks like this.

>  		break;
>  	case OF_RECONFIG_DETACH_NODE:
> +		raw_spin_lock_irqsave(&devtree_lock, flags);
>  		__of_detach_node(ce->np);
> +		raw_spin_unlock_irqrestore(&devtree_lock, flags);
>  		break;
>  	case OF_RECONFIG_ADD_PROPERTY:
>  		/* If the property is in deadprops then it must be removed */
> @@ -619,7 +622,9 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
>  			}
>  		}
>  
> +		raw_spin_lock_irqsave(&devtree_lock, flags);
>  		ret = __of_add_property(ce->np, ce->prop);
> +		raw_spin_unlock_irqrestore(&devtree_lock, flags);
>  		if (ret) {
>  			pr_err("changeset: add_property failed @%pOF/%s\n",
>  				ce->np,
> @@ -628,7 +633,9 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
>  		}
>  		break;
>  	case OF_RECONFIG_REMOVE_PROPERTY:
> +		raw_spin_lock_irqsave(&devtree_lock, flags);
>  		ret = __of_remove_property(ce->np, ce->prop);
> +		raw_spin_unlock_irqrestore(&devtree_lock, flags);
>  		if (ret) {
>  			pr_err("changeset: remove_property failed @%pOF/%s\n",
>  				ce->np,
> @@ -647,7 +654,9 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
>  			}
>  		}
>  
> +		raw_spin_lock_irqsave(&devtree_lock, flags);
>  		ret = __of_update_property(ce->np, ce->prop, &old_prop);
> +		raw_spin_unlock_irqrestore(&devtree_lock, flags);
>  		if (ret) {
>  			pr_err("changeset: update_property failed @%pOF/%s\n",
>  				ce->np,
> @@ -658,7 +667,6 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
>  	default:
>  		ret = -EINVAL;
>  	}
> -	raw_spin_unlock_irqrestore(&devtree_lock, flags);
>  
>  	if (ret)
>  		return ret;
> -- 
> 2.34.1
>
Rob Herring (Arm) July 20, 2023, 9:30 p.m. UTC | #2
+Sakari

On Wed, Jul 19, 2023 at 05:02:56PM -0600, Rob Herring wrote:
> On Wed, Jul 19, 2023 at 05:00:01PM +0200, Geert Uytterhoeven wrote:
> > Formatting strings using "%pOF" while holding devtree_lock causes a
> > deadlock.  Lockdep reports:
> > 
> >     of_get_parent from of_fwnode_get_parent+0x18/0x24
> >     ^^^^^^^^^^^^^
> 
> I'm wondering if we really need the lock in there. We never unset or 
> change the parent. It gets detached, but we're not checking for that. 
> The node could get freed, but the lock is not for that, refcounts are.

The lock existed since 2.6.12 for powerpc. It's not clear to me whether 
it was really ever needed. There's lots of places we just access 
'parent' without a lock. Not to say that's right.

The lock doesn't even help in this case because we release the lock on 
each count and between counting and getting the names. If the tree 
changes, the lock isn't going to help.

> >     of_fwnode_get_parent from fwnode_count_parents+0xc/0x28
> 
> count parents? Huh? Isn't it always 1? 
> 
> >     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 making the locking cover only the parts that really need it.
> > 
> > Fixes: 0d638a07d3a1e98a ("of: Convert to using %pOF instead of full_name")

That's the wrong commit. My implementation in vsprintf.c worked with 
this. It's commit a92eb7621b9f ("lib/vsprintf: Make use of fwnode API to 
obtain node names and separators") which broke it. It came 2 years 
later.

The fwnode based implementation looks like the wrong level of 
abstraction to me. Why not just push 'give me the full name' down to the 
fwnode backends? The functions defined are *only* used by vsprintf.c.

I don't really understand the "let's change everything to use fwnode" 
even for things which will never be anything but DT. %pOF is DT 
only. </rant>

Rob
Geert Uytterhoeven July 27, 2023, 1:42 p.m. UTC | #3
Hi Rob,

On Thu, Jul 20, 2023 at 1:03 AM Rob Herring <robh@kernel.org> wrote:
> On Wed, Jul 19, 2023 at 05:00:01PM +0200, Geert Uytterhoeven wrote:
> > Formatting strings using "%pOF" while holding devtree_lock 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 making the locking cover only the parts that really need it.
> >
> > Fixes: 0d638a07d3a1e98a ("of: Convert to using %pOF instead of full_name")
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > ---
> >  drivers/of/dynamic.c | 12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> > index e311d406b1705306..eae45a1c673ee05f 100644
> > --- a/drivers/of/dynamic.c
> > +++ b/drivers/of/dynamic.c
> > @@ -601,13 +601,16 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
> >
> >       __of_changeset_entry_dump(ce);
> >
> > -     raw_spin_lock_irqsave(&devtree_lock, flags);
> >       switch (ce->action) {
> >       case OF_RECONFIG_ATTACH_NODE:
> > +             raw_spin_lock_irqsave(&devtree_lock, flags);
> >               __of_attach_node(ce->np);
> > +             raw_spin_unlock_irqrestore(&devtree_lock, flags);
>
> I think you could just move the spinlock into __of_attach_node(). The
> only other caller looks like this.

I'd rather not do that, as the double underscore is typically used to
indicate that this function does not take the lock.
Cfr. of_find_property() vs. __of_find_property().

Gr{oetje,eeting}s,

                        Geert
diff mbox series

Patch

diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index e311d406b1705306..eae45a1c673ee05f 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -601,13 +601,16 @@  static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
 
 	__of_changeset_entry_dump(ce);
 
-	raw_spin_lock_irqsave(&devtree_lock, flags);
 	switch (ce->action) {
 	case OF_RECONFIG_ATTACH_NODE:
+		raw_spin_lock_irqsave(&devtree_lock, flags);
 		__of_attach_node(ce->np);
+		raw_spin_unlock_irqrestore(&devtree_lock, flags);
 		break;
 	case OF_RECONFIG_DETACH_NODE:
+		raw_spin_lock_irqsave(&devtree_lock, flags);
 		__of_detach_node(ce->np);
+		raw_spin_unlock_irqrestore(&devtree_lock, flags);
 		break;
 	case OF_RECONFIG_ADD_PROPERTY:
 		/* If the property is in deadprops then it must be removed */
@@ -619,7 +622,9 @@  static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
 			}
 		}
 
+		raw_spin_lock_irqsave(&devtree_lock, flags);
 		ret = __of_add_property(ce->np, ce->prop);
+		raw_spin_unlock_irqrestore(&devtree_lock, flags);
 		if (ret) {
 			pr_err("changeset: add_property failed @%pOF/%s\n",
 				ce->np,
@@ -628,7 +633,9 @@  static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
 		}
 		break;
 	case OF_RECONFIG_REMOVE_PROPERTY:
+		raw_spin_lock_irqsave(&devtree_lock, flags);
 		ret = __of_remove_property(ce->np, ce->prop);
+		raw_spin_unlock_irqrestore(&devtree_lock, flags);
 		if (ret) {
 			pr_err("changeset: remove_property failed @%pOF/%s\n",
 				ce->np,
@@ -647,7 +654,9 @@  static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
 			}
 		}
 
+		raw_spin_lock_irqsave(&devtree_lock, flags);
 		ret = __of_update_property(ce->np, ce->prop, &old_prop);
+		raw_spin_unlock_irqrestore(&devtree_lock, flags);
 		if (ret) {
 			pr_err("changeset: update_property failed @%pOF/%s\n",
 				ce->np,
@@ -658,7 +667,6 @@  static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
 	default:
 		ret = -EINVAL;
 	}
-	raw_spin_unlock_irqrestore(&devtree_lock, flags);
 
 	if (ret)
 		return ret;