* [PATCH] ACPI: scan: Do not hold physical_node_lock across device_offline() and device_online()
@ 2026-10-28 16:16 Sasha Levin
2026-10-28 20:03 ` Rafael J. Wysocki (Intel)
0 siblings, 1 reply; 2+ messages in thread
From: Sasha Levin @ 2026-10-28 16:16 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Sasha Levin, Len Brown, linux-acpi, linux-kernel,
syzbot+cea7285bc17b76ffe39f, syzbot+a265816368571f78ab88,
syzkaller-bugs
acpi_bus_offline() and acpi_bus_online() hold the physical_node_lock of
the ACPI device being hot-removed while they call device_offline() and
device_online() for its physical nodes. Offlining a CPU this way takes
cpu_add_remove_lock and the CPU hotplug lock under physical_node_lock,
and lockdep connects that to the uevent attribute of platform devices,
whose store method takes physical_node_lock in
acpi_primary_dev_companion():
physical_node_lock (acpi_bus_offline())
-> cpu_add_remove_lock -> cpu_hotplug_lock
-> cpuhp_state-down (mce_cpu_pre_down() unregistering a device)
-> device_links_lock (device_links_purge())
-> kn->active of a uevent attribute (device link unregistration)
-> physical_node_lock (uevent_store() -> acpi_companion_match())
syzbot has reported this cycle twice. The uevent attribute waited for in
the link unregistration is the device link's, not a platform device's,
and the physical_node_lock taken at the end is that of the platform
device's ACPI companion, not of the processor being ejected, so lockdep
only connects them because they share lock classes and this is unlikely
to deadlock. However, lockdep turns itself off after reporting it.
It can be reproduced in a QEMU guest by writing to the uevent attribute
of an ACPI-enumerated platform device, reading the uevent attribute of a
device link while the link is removed, offlining a CPU and ejecting one
through sysfs.
physical_node_lock only needs to protect the list of physical nodes, so
drop it around device_offline() and device_online(). Mark the nodes to
offline with a new offline_pending flag, hold a reference to each device
while the lock is dropped, and look its node up again afterwards to
record whether it needs to be put back online, because it may have been
unbound in the meantime. Physical nodes added while the lock is not
held are not offlined by the current pass, which is no different from
adding them right after it.
Since acpi_container_offline() no longer runs under the container's
physical_node_lock, the nesting annotation added to
acpi_scan_is_offline() by commit 4c533c801d1c ("ACPI / scan: Annotate
physical_node_lock in acpi_scan_is_offline()") is not needed anymore, so
drop it.
Fixes: 683058e315f0 ("ACPI / hotplug: Use device offline/online for graceful hot-removal")
Reported-by: syzbot+cea7285bc17b76ffe39f@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/r/6ae18ed0.6200f071.51519.0f60.GAE@google.com
Reported-by: syzbot+a265816368571f78ab88@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/r/6adf293a.67317f00.38c4bc.00af.GAE@google.com
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/acpi/scan.c | 112 ++++++++++++++++++++++++++++++++--------
include/acpi/acpi_bus.h | 1 +
2 files changed, 92 insertions(+), 21 deletions(-)
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 0b17a7e519569..3a6420872651f 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -114,11 +114,7 @@ bool acpi_scan_is_offline(struct acpi_device *adev, bool uevent)
bool offline = true;
char *envp[] = { "EVENT=offline", NULL };
- /*
- * acpi_container_offline() calls this for all of the container's
- * children under the container's physical_node_lock lock.
- */
- mutex_lock_nested(&adev->physical_node_lock, SINGLE_DEPTH_NESTING);
+ mutex_lock(&adev->physical_node_lock);
list_for_each_entry(pn, &adev->physical_node_list, node)
if (device_supports_offline(pn->dev) && !dev_offline(pn->dev)) {
@@ -133,6 +129,59 @@ bool acpi_scan_is_offline(struct acpi_device *adev, bool uevent)
return offline;
}
+static struct acpi_device_physical_node *
+acpi_find_physical_node(struct acpi_device *adev, struct device *dev)
+{
+ struct acpi_device_physical_node *pn;
+
+ lockdep_assert_held(&adev->physical_node_lock);
+
+ list_for_each_entry(pn, &adev->physical_node_list, node)
+ if (pn->dev == dev)
+ return pn;
+
+ return NULL;
+}
+
+/*
+ * Find the first physical node of @adev that is still to be offlined, clear its
+ * offline_pending flag and return its device with a reference held.
+ */
+static struct device *acpi_next_offline_pending(struct acpi_device *adev)
+{
+ struct acpi_device_physical_node *pn;
+
+ lockdep_assert_held(&adev->physical_node_lock);
+
+ list_for_each_entry(pn, &adev->physical_node_list, node)
+ if (pn->offline_pending) {
+ pn->offline_pending = false;
+ return get_device(pn->dev);
+ }
+
+ return NULL;
+}
+
+/*
+ * Find the first physical node of @adev that has been offlined by
+ * acpi_bus_offline(), clear its put_online flag and return its device with a
+ * reference held.
+ */
+static struct device *acpi_next_put_online(struct acpi_device *adev)
+{
+ struct acpi_device_physical_node *pn;
+
+ lockdep_assert_held(&adev->physical_node_lock);
+
+ list_for_each_entry(pn, &adev->physical_node_list, node)
+ if (pn->put_online) {
+ pn->put_online = false;
+ return get_device(pn->dev);
+ }
+
+ return NULL;
+}
+
static acpi_status acpi_bus_offline(acpi_handle handle, u32 lvl, void *data,
void **ret_p)
{
@@ -140,6 +189,7 @@ static acpi_status acpi_bus_offline(acpi_handle handle, u32 lvl, void *data,
struct acpi_device_physical_node *pn;
bool second_pass = (bool)data;
acpi_status status = AE_OK;
+ struct device *dev;
if (!device)
return AE_OK;
@@ -152,25 +202,44 @@ static acpi_status acpi_bus_offline(acpi_handle handle, u32 lvl, void *data,
mutex_lock(&device->physical_node_lock);
list_for_each_entry(pn, &device->physical_node_list, node) {
- int ret;
-
if (second_pass) {
/* Skip devices offlined by the first pass. */
- if (pn->put_online)
- continue;
+ pn->offline_pending = !pn->put_online;
} else {
+ pn->offline_pending = true;
pn->put_online = false;
}
- ret = device_offline(pn->dev);
+ }
+
+ /*
+ * Drop physical_node_lock around device_offline(), because the latter
+ * acquires locks, like the CPU hotplug ones, that must not nest under
+ * it.
+ */
+ while ((dev = acpi_next_offline_pending(device))) {
+ int ret;
+
+ mutex_unlock(&device->physical_node_lock);
+ ret = device_offline(dev);
+ mutex_lock(&device->physical_node_lock);
+
+ /* The device may have been unbound in the meantime. */
+ pn = acpi_find_physical_node(device, dev);
if (ret >= 0) {
- pn->put_online = !ret;
+ if (pn)
+ pn->put_online = !ret;
} else {
- *ret_p = pn->dev;
- if (second_pass) {
+ *ret_p = pn ? pn->dev : &device->dev;
+ if (second_pass)
status = AE_ERROR;
- break;
- }
}
+
+ mutex_unlock(&device->physical_node_lock);
+ put_device(dev);
+ if (ACPI_FAILURE(status))
+ return status;
+
+ mutex_lock(&device->physical_node_lock);
}
mutex_unlock(&device->physical_node_lock);
@@ -182,18 +251,19 @@ static acpi_status acpi_bus_online(acpi_handle handle, u32 lvl, void *data,
void **ret_p)
{
struct acpi_device *device = acpi_fetch_acpi_dev(handle);
- struct acpi_device_physical_node *pn;
+ struct device *dev;
if (!device)
return AE_OK;
mutex_lock(&device->physical_node_lock);
- list_for_each_entry(pn, &device->physical_node_list, node)
- if (pn->put_online) {
- device_online(pn->dev);
- pn->put_online = false;
- }
+ while ((dev = acpi_next_put_online(device))) {
+ mutex_unlock(&device->physical_node_lock);
+ device_online(dev);
+ put_device(dev);
+ mutex_lock(&device->physical_node_lock);
+ }
mutex_unlock(&device->physical_node_lock);
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index a755b15af9db9..29c59ea1e3e48 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -320,6 +320,7 @@ struct acpi_device_physical_node {
struct device *dev;
unsigned int node_id;
bool put_online:1;
+ bool offline_pending:1;
};
struct acpi_device_properties {
--
2.53.0
^ permalink raw reply [flat|nested] 2+ messages in thread* Re: [PATCH] ACPI: scan: Do not hold physical_node_lock across device_offline() and device_online()
2026-10-28 16:16 [PATCH] ACPI: scan: Do not hold physical_node_lock across device_offline() and device_online() Sasha Levin
@ 2026-10-28 20:03 ` Rafael J. Wysocki (Intel)
0 siblings, 0 replies; 2+ messages in thread
From: Rafael J. Wysocki (Intel) @ 2026-10-28 20:03 UTC (permalink / raw)
To: Sasha Levin
Cc: Rafael J. Wysocki, Len Brown, linux-acpi, linux-kernel,
syzbot+cea7285bc17b76ffe39f, syzbot+a265816368571f78ab88,
syzkaller-bugs
On Wed, Oct 28, 2026 at 5:16 PM Sasha Levin <sashal@kernel.org> wrote:
>
> acpi_bus_offline() and acpi_bus_online() hold the physical_node_lock of
> the ACPI device being hot-removed while they call device_offline() and
> device_online() for its physical nodes. Offlining a CPU this way takes
> cpu_add_remove_lock and the CPU hotplug lock under physical_node_lock,
> and lockdep connects that to the uevent attribute of platform devices,
> whose store method takes physical_node_lock in
> acpi_primary_dev_companion():
>
> physical_node_lock (acpi_bus_offline())
> -> cpu_add_remove_lock -> cpu_hotplug_lock
> -> cpuhp_state-down (mce_cpu_pre_down() unregistering a device)
> -> device_links_lock (device_links_purge())
> -> kn->active of a uevent attribute (device link unregistration)
> -> physical_node_lock (uevent_store() -> acpi_companion_match())
>
> syzbot has reported this cycle twice. The uevent attribute waited for in
> the link unregistration is the device link's, not a platform device's,
> and the physical_node_lock taken at the end is that of the platform
> device's ACPI companion, not of the processor being ejected, so lockdep
> only connects them because they share lock classes and this is unlikely
> to deadlock. However, lockdep turns itself off after reporting it.
Yeah, fair enough, and thanks for the patch!
> It can be reproduced in a QEMU guest by writing to the uevent attribute
> of an ACPI-enumerated platform device, reading the uevent attribute of a
> device link while the link is removed, offlining a CPU and ejecting one
> through sysfs.
>
> physical_node_lock only needs to protect the list of physical nodes,
There is somewhat more to it because acpi_unbind_one() blocks on
physical_node_lock of the device's ACPI companion, so holding it
prevents device_offline() from running in parallel with whatever
happens after acpi_unbind_one(). I don't think this would be a
problem in practice though.
> so drop it around device_offline() and device_online(). Mark the nodes to
> offline with a new offline_pending flag, hold a reference to each device
> while the lock is dropped, and look its node up again afterwards to
> record whether it needs to be put back online, because it may have been
> unbound in the meantime. Physical nodes added while the lock is not
> held are not offlined by the current pass, which is no different from
> adding them right after it.
>
> Since acpi_container_offline() no longer runs under the container's
> physical_node_lock, the nesting annotation added to
> acpi_scan_is_offline() by commit 4c533c801d1c ("ACPI / scan: Annotate
> physical_node_lock in acpi_scan_is_offline()") is not needed anymore, so
> drop it.
>
> Fixes: 683058e315f0 ("ACPI / hotplug: Use device offline/online for graceful hot-removal")
> Reported-by: syzbot+cea7285bc17b76ffe39f@syzkaller.appspotmail.com
> Closes: https://lore.kernel.org/r/6ae18ed0.6200f071.51519.0f60.GAE@google.com
> Reported-by: syzbot+a265816368571f78ab88@syzkaller.appspotmail.com
> Closes: https://lore.kernel.org/r/6adf293a.67317f00.38c4bc.00af.GAE@google.com
> Signed-off-by: Sasha Levin <sashal@kernel.org>
> ---
> drivers/acpi/scan.c | 112 ++++++++++++++++++++++++++++++++--------
> include/acpi/acpi_bus.h | 1 +
> 2 files changed, 92 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 0b17a7e519569..3a6420872651f 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -114,11 +114,7 @@ bool acpi_scan_is_offline(struct acpi_device *adev, bool uevent)
> bool offline = true;
> char *envp[] = { "EVENT=offline", NULL };
>
> - /*
> - * acpi_container_offline() calls this for all of the container's
> - * children under the container's physical_node_lock lock.
> - */
> - mutex_lock_nested(&adev->physical_node_lock, SINGLE_DEPTH_NESTING);
> + mutex_lock(&adev->physical_node_lock);
>
> list_for_each_entry(pn, &adev->physical_node_list, node)
> if (device_supports_offline(pn->dev) && !dev_offline(pn->dev)) {
> @@ -133,6 +129,59 @@ bool acpi_scan_is_offline(struct acpi_device *adev, bool uevent)
> return offline;
> }
>
> +static struct acpi_device_physical_node *
> +acpi_find_physical_node(struct acpi_device *adev, struct device *dev)
> +{
> + struct acpi_device_physical_node *pn;
> +
> + lockdep_assert_held(&adev->physical_node_lock);
> +
> + list_for_each_entry(pn, &adev->physical_node_list, node)
> + if (pn->dev == dev)
> + return pn;
> +
> + return NULL;
> +}
> +
> +/*
> + * Find the first physical node of @adev that is still to be offlined, clear its
> + * offline_pending flag and return its device with a reference held.
> + */
I'd slightly prefer this comment to go into the function body, next to
the loop (and analogously for the next one).
> +static struct device *acpi_next_offline_pending(struct acpi_device *adev)
> +{
> + struct acpi_device_physical_node *pn;
> +
> + lockdep_assert_held(&adev->physical_node_lock);
> +
> + list_for_each_entry(pn, &adev->physical_node_list, node)
> + if (pn->offline_pending) {
> + pn->offline_pending = false;
> + return get_device(pn->dev);
> + }
> +
> + return NULL;
> +}
> +
> +/*
> + * Find the first physical node of @adev that has been offlined by
> + * acpi_bus_offline(), clear its put_online flag and return its device with a
> + * reference held.
> + */
> +static struct device *acpi_next_put_online(struct acpi_device *adev)
> +{
> + struct acpi_device_physical_node *pn;
> +
> + lockdep_assert_held(&adev->physical_node_lock);
> +
> + list_for_each_entry(pn, &adev->physical_node_list, node)
> + if (pn->put_online) {
> + pn->put_online = false;
> + return get_device(pn->dev);
> + }
> +
> + return NULL;
> +}
> +
> static acpi_status acpi_bus_offline(acpi_handle handle, u32 lvl, void *data,
> void **ret_p)
> {
> @@ -140,6 +189,7 @@ static acpi_status acpi_bus_offline(acpi_handle handle, u32 lvl, void *data,
> struct acpi_device_physical_node *pn;
> bool second_pass = (bool)data;
> acpi_status status = AE_OK;
> + struct device *dev;
>
> if (!device)
> return AE_OK;
> @@ -152,25 +202,44 @@ static acpi_status acpi_bus_offline(acpi_handle handle, u32 lvl, void *data,
> mutex_lock(&device->physical_node_lock);
>
> list_for_each_entry(pn, &device->physical_node_list, node) {
> - int ret;
> -
> if (second_pass) {
> /* Skip devices offlined by the first pass. */
> - if (pn->put_online)
> - continue;
> + pn->offline_pending = !pn->put_online;
> } else {
> + pn->offline_pending = true;
> pn->put_online = false;
> }
> - ret = device_offline(pn->dev);
> + }
> +
> + /*
> + * Drop physical_node_lock around device_offline(), because the latter
> + * acquires locks, like the CPU hotplug ones, that must not nest under
> + * it.
> + */
> + while ((dev = acpi_next_offline_pending(device))) {
> + int ret;
> +
> + mutex_unlock(&device->physical_node_lock);
> + ret = device_offline(dev);
> + mutex_lock(&device->physical_node_lock);
> +
> + /* The device may have been unbound in the meantime. */
> + pn = acpi_find_physical_node(device, dev);
> if (ret >= 0) {
> - pn->put_online = !ret;
> + if (pn)
> + pn->put_online = !ret;
> } else {
> - *ret_p = pn->dev;
> - if (second_pass) {
> + *ret_p = pn ? pn->dev : &device->dev;
Returning pn->dev via ret_p and then using it for printing messages is
bogus both here and in the original code (sorry about that) because in
theory the device may be gone by the time the message gets printed.
It would be good to fix that beforehand and then you won't need to
return &device->dev here if pn is NULL which is a bit confusing.
> + if (second_pass)
> status = AE_ERROR;
> - break;
> - }
> }
> +
> + mutex_unlock(&device->physical_node_lock);
Does the lock really need to be dropped here?
put_device() can be called from under it and substituting "break" for
"return status" below should take care of the loop termination.
> + put_device(dev);
> + if (ACPI_FAILURE(status))
> + return status;
> +
> + mutex_lock(&device->physical_node_lock);
> }
>
> mutex_unlock(&device->physical_node_lock);
> @@ -182,18 +251,19 @@ static acpi_status acpi_bus_online(acpi_handle handle, u32 lvl, void *data,
> void **ret_p)
> {
> struct acpi_device *device = acpi_fetch_acpi_dev(handle);
> - struct acpi_device_physical_node *pn;
> + struct device *dev;
>
> if (!device)
> return AE_OK;
>
> mutex_lock(&device->physical_node_lock);
>
> - list_for_each_entry(pn, &device->physical_node_list, node)
> - if (pn->put_online) {
> - device_online(pn->dev);
> - pn->put_online = false;
> - }
> + while ((dev = acpi_next_put_online(device))) {
> + mutex_unlock(&device->physical_node_lock);
> + device_online(dev);
> + put_device(dev);
> + mutex_lock(&device->physical_node_lock);
> + }
>
> mutex_unlock(&device->physical_node_lock);
>
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index a755b15af9db9..29c59ea1e3e48 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -320,6 +320,7 @@ struct acpi_device_physical_node {
> struct device *dev;
> unsigned int node_id;
> bool put_online:1;
> + bool offline_pending:1;
> };
>
> struct acpi_device_properties {
> --
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2026-10-28 20:04 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-10-28 16:16 [PATCH] ACPI: scan: Do not hold physical_node_lock across device_offline() and device_online() Sasha Levin
2026-10-28 20:03 ` Rafael J. Wysocki (Intel)
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®