From: Vladimir Oltean <olteanv@gmail.com>
To: Rob Herring <robh+dt@kernel.org>,
Frank Rowand <frowand.list@gmail.com>,
Sascha Hauer <s.hauer@pengutronix.de>,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
netdev@vger.kernel.org
Subject: of_node_put() usage is buggy all over drivers/of/base.c?!
Date: Sat, 14 Aug 2021 04:01:39 +0300 [thread overview]
Message-ID: <20210814010139.kzryimmp4rizlznt@skbuf> (raw)
Hi,
I was debugging an RCU stall which happened during the probing of a
driver. Activating lock debugging, I see:
[ 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
[ 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.
next reply other threads:[~2021-08-14 1:01 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-14 1:01 Vladimir Oltean [this message]
2021-08-16 14:33 ` of_node_put() usage is buggy all over drivers/of/base.c?! Frank Rowand
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=20210814010139.kzryimmp4rizlznt@skbuf \
--to=olteanv@gmail.com \
--cc=devicetree@vger.kernel.org \
--cc=frowand.list@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=robh+dt@kernel.org \
--cc=s.hauer@pengutronix.de \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).