LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Frank Rowand <frowand.list@gmail.com>
To: Vladimir Oltean <olteanv@gmail.com>,
	Rob Herring <robh+dt@kernel.org>,
	Sascha Hauer <s.hauer@pengutronix.de>,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	netdev@vger.kernel.org
Subject: Re: of_node_put() usage is buggy all over drivers/of/base.c?!
Date: Mon, 16 Aug 2021 09:33:03 -0500	[thread overview]
Message-ID: <9accd63a-961c-4dab-e167-9e2654917a94@gmail.com> (raw)
In-Reply-To: <20210814010139.kzryimmp4rizlznt@skbuf>

Hi Vladimir,

On 8/13/21 8:01 PM, Vladimir Oltean wrote:
> Hi,
> 
> I was debugging an RCU stall which happened during the probing of a
> driver. Activating lock debugging, I see:

I took a quick look at sja1105_mdiobus_register() in v5.14-rc1 and v5.14-rc6.

Looking at the following stack trace, I did not see any calls to
of_find_compatible_node() in sja1105_mdiobus_register().  I am
guessing that maybe there is an inlined function that calls
of_find_compatible_node().  This would likely be either
sja1105_mdiobus_base_tx_register() or sja1105_mdioux_base_t1_register().

> 
> [  101.710694] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:938
> [  101.719119] in_atomic(): 1, irqs_disabled(): 128, non_block: 0, pid: 1534, name: sh
> [  101.726763] INFO: lockdep is turned off.
> [  101.730674] irq event stamp: 0
> [  101.733716] hardirqs last  enabled at (0): [<0000000000000000>] 0x0
> [  101.739973] hardirqs last disabled at (0): [<ffffd3ebecb10120>] copy_process+0xa78/0x1a98
> [  101.748146] softirqs last  enabled at (0): [<ffffd3ebecb10120>] copy_process+0xa78/0x1a98
> [  101.756313] softirqs last disabled at (0): [<0000000000000000>] 0x0
> [  101.762569] CPU: 4 PID: 1534 Comm: sh Not tainted 5.14.0-rc5+ #272
> [  101.774558] Call trace:
> [  101.794734]  __might_sleep+0x50/0x88
> [  101.798297]  __mutex_lock+0x60/0x938
> [  101.801863]  mutex_lock_nested+0x38/0x50
> [  101.805775]  kernfs_remove+0x2c/0x50             <---- this takes mutex_lock(&kernfs_mutex);
> [  101.809341]  sysfs_remove_dir+0x54/0x70

The __kobject_del() occurs only if the refcount on the node
becomes zero.  This should never be true when of_find_compatible_node()
calls of_node_put() unless a "from" node is passed to of_find_compatible_node().

In both sja1105_mdiobus_base_tx_register() and sja1105_mdioux_base_t1_register()
a from node ("mdio") is passed to of_find_compatible_node() without first doing an
of_node_get(mdio).  If you add the of_node_get() calls the problem should be fixed.

-Frank


> [  101.813166]  __kobject_del+0x3c/0x80
> [  101.816733]  kobject_put+0xf8/0x108
> [  101.820211]  of_node_put+0x18/0x28
> [  101.823602]  of_find_compatible_node+0xa8/0xf8    <--- this takes raw_spin_lock_irqsave(&devtree_lock)
> [  101.828036]  sja1105_mdiobus_register+0x264/0x7a8
> 
> The pattern of calling of_node_put from under the atomic devtree_lock
> context is pretty widespread in drivers/of/base.c.
> 
> Just by inspecting the code, this seems to be an issue since commit:
> 
> commit 75b57ecf9d1d1e17d099ab13b8f48e6e038676be
> Author: Grant Likely <grant.likely@linaro.org>
> Date:   Thu Feb 20 18:02:11 2014 +0000
> 
>     of: Make device nodes kobjects so they show up in sysfs
> 
>     Device tree nodes are already treated as objects, and we already want to
>     expose them to userspace which is done using the /proc filesystem today.
>     Right now the kernel has to do a lot of work to keep the /proc view in
>     sync with the in-kernel representation. If device_nodes are switched to
>     be kobjects then the device tree code can be a whole lot simpler. It
>     also turns out that switching to using /sysfs from /proc results in
>     smaller code and data size, and the userspace ABI won't change if
>     /proc/device-tree symlinks to /sys/firmware/devicetree/base.
> 
>     v7: Add missing sysfs_bin_attr_init()
>     v6: Add __of_add_property() early init fixes from Pantelis
>     v5: Rename firmware/ofw to firmware/devicetree
>         Fix updating property values in sysfs
>     v4: Fixed build error on Powerpc
>         Fixed handling of dynamic nodes on powerpc
>     v3: Fixed handling of duplicate attribute and child node names
>     v2: switch to using sysfs bin_attributes which solve the problem of
>         reporting incorrect property size.
> 
>     Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
>     Tested-by: Sascha Hauer <s.hauer@pengutronix.de>
>     Cc: Rob Herring <rob.herring@calxeda.com>
>     Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>     Cc: David S. Miller <davem@davemloft.net>
>     Cc: Nathan Fontenot <nfont@linux.vnet.ibm.com>
>     Cc: Pantelis Antoniou <panto@antoniou-consulting.com>
> 
> because up until that point, of_node_put() was:
> 
> void of_node_put(struct device_node *node)
> {
> 	if (node)
> 		kref_put(&node->kref, of_node_release);
> }
> 
> and not:
> 
> void of_node_put(struct device_node *node)
> {
> 	if (node)
> 		kobject_put(&node->kobj);
> }
> 
> Either I'm holding it very, very wrong, or this is a very severe
> oversight that just happened somehow to go unnoticed for 7 years.
> 
> Please tell me it's me.
> 


  reply	other threads:[~2021-08-16 14:33 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-14  1:01 Vladimir Oltean
2021-08-16 14:33 ` Frank Rowand [this message]
2021-08-16 14:46   ` Vladimir Oltean
2021-08-16 15:14     ` Frank Rowand
2021-08-16 19:20       ` Rob Herring
2021-08-16 19:56         ` Frank Rowand
2021-08-16 20:00           ` Frank Rowand
2021-08-16 20:25           ` Rob Herring

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=9accd63a-961c-4dab-e167-9e2654917a94@gmail.com \
    --to=frowand.list@gmail.com \
    --cc=devicetree@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=olteanv@gmail.com \
    --cc=robh+dt@kernel.org \
    --cc=s.hauer@pengutronix.de \
    --subject='Re: of_node_put() usage is buggy all over drivers/of/base.c?'\!'' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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).