ACPI: bus: Only call notify for a completely bound ACPI device

Commit Message

Uwe Kleine-König March 24, 2023, 9:09 a.m. UTC

Commit d6fb6ee1820c ("ACPI: bus: Drop driver member of struct
acpi_device") removed acpi_device::driver in favour of struct
acpi_device::dev.driver.

However there is a problem: While the two pointers are equivalent after
the device is completely probed, there is a small time frame where
acpi_device::dev->driver is already set but acpi_device::driver wasn't.

The function acpi_bus_notify() used to have a check for
acpi_device::driver that was changed to a check for adev->dev.driver in
Commit d6fb6ee1820c.

Pierre Asselin reports that starting with above change his laptop
crashed during boot when on AC power. That's because acpi_bus_notify()
is called in that small time frame (triggered by acpi_ac_add()) which
results in a call to acpi_ac_notify while this function isn't ready yet.

So in acpi_bus_notify() check for the device being bound (which becomes
true only after the acpi probe call succeeds) instead of for
acpi_device::dev.driver.

Note that usually you have to hold the device lock to call
device_is_bound(). I don't think this is the case here, so there likely
is a race condition. The problem might be that the driver unbinds after
the check but before adev->dev.driver is evaluated. However this race
already existed before commit d6fb6ee1820c, so the change here is a net
improvement even though it might not result in a completely correct
handling.

A similar check in the acpi sysfs code is also converted. This is less
critical as it happens in code that is run when a sysfs file is
accessed. That shouldn't happen for a device that isn't bound.

Fixes: d6fb6ee1820c ("ACPI: bus: Drop driver member of struct acpi_device")
Reported-by: Pierre Asselin <[e-mail]>
Link: https://lore.kernel.org/linux-acpi/[e-mail]
Signed-off-by: Uwe Kleine-König <[e-mail]>
---
 drivers/acpi/bus.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


base-commit: fe15c26ee26efa11741a7b632e9f23b01aca4cc6

Comments

Uwe Kleine-König March 24, 2023, 12:30 p.m. UTC | #1

Hello,

On Fri, Mar 24, 2023 at 10:09:55AM +0100, Uwe Kleine-König wrote:
> Commit d6fb6ee1820c ("ACPI: bus: Drop driver member of struct
> acpi_device") removed acpi_device::driver in favour of struct
> acpi_device::dev.driver.
> 
> However there is a problem: While the two pointers are equivalent after
> the device is completely probed, there is a small time frame where
> acpi_device::dev->driver is already set but acpi_device::driver wasn't.
> 
> The function acpi_bus_notify() used to have a check for
> acpi_device::driver that was changed to a check for adev->dev.driver in
> Commit d6fb6ee1820c.
> 
> Pierre Asselin reports that starting with above change his laptop
> crashed during boot when on AC power. That's because acpi_bus_notify()
> is called in that small time frame (triggered by acpi_ac_add()) which
> results in a call to acpi_ac_notify while this function isn't ready yet.
> 
> So in acpi_bus_notify() check for the device being bound (which becomes
> true only after the acpi probe call succeeds) instead of for
> acpi_device::dev.driver.
> 
> Note that usually you have to hold the device lock to call
> device_is_bound(). I don't think this is the case here, so there likely
> is a race condition. The problem might be that the driver unbinds after
> the check but before adev->dev.driver is evaluated. However this race
> already existed before commit d6fb6ee1820c, so the change here is a net
> improvement even though it might not result in a completely correct
> handling.
> 
> A similar check in the acpi sysfs code is also converted. This is less
> critical as it happens in code that is run when a sysfs file is
> accessed. That shouldn't happen for a device that isn't bound.
> 
> Fixes: d6fb6ee1820c ("ACPI: bus: Drop driver member of struct acpi_device")
> Reported-by: Pierre Asselin <[e-mail]>
> Link: https://lore.kernel.org/linux-acpi/[e-mail]
> Signed-off-by: Uwe Kleine-König <[e-mail]>
> ---
>  drivers/acpi/bus.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index 9531dd0fef50..a5a8f82981ce 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -518,7 +518,7 @@ static void acpi_bus_notify(acpi_handle handle, u32 type, void *data)
>  	if (!adev)
>  		goto err;
>  
> -	if (adev->dev.driver) {
> +	if (device_is_bound(&adev->dev)) {
>  		struct acpi_driver *driver = to_acpi_driver(adev->dev.driver);
>  
>  		if (driver && driver->ops.notify &&
> 

there is a hunk that I failed to add to the commit:

diff --git a/drivers/acpi/device_sysfs.c b/drivers/acpi/device_sysfs.c
index 0fbfbaa8d8e3..2ee756d68052 100644
--- a/drivers/acpi/device_sysfs.c
+++ b/drivers/acpi/device_sysfs.c
@@ -376,7 +376,7 @@ eject_store(struct device *d, struct device_attribute *attr,
 		return -EINVAL;
 
 	if ((!acpi_device->handler || !acpi_device->handler->hotplug.enabled)
-	    && !d->driver)
+	    && !device_is_bound(d))
 		return -ENODEV;
 
 	status = acpi_get_type(acpi_device->handle, &not_used);

Rafael sait in the thread where Pierre reported the problem that he has
a different fix in mind. So I guess it's not worth to send a v2.

Best regards
Uwe

Patch

diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index 9531dd0fef50..a5a8f82981ce 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -518,7 +518,7 @@  static void acpi_bus_notify(acpi_handle handle, u32 type, void *data)
 	if (!adev)
 		goto err;
 
-	if (adev->dev.driver) {
+	if (device_is_bound(&adev->dev)) {
 		struct acpi_driver *driver = to_acpi_driver(adev->dev.driver);
 
 		if (driver && driver->ops.notify &&