Re: [PATCH v2 2/6] ACPI: scan: Stop calling acpi_bus_init_power() early

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [High] Global mutex init_power_lock in acpi_bus_init_power causes deadlocks when power initialization of one device indirectly triggers initialization of another.
- [High] The poisoned ACPI_D_STATE_DISABLED state is silently overwritten by acpi_device_update_power() for devices without power resources.
- [High] Leaving device->flags.power_manageable = 1 when power initialization fails aborts system suspend.
- [High] TOCTOU race in acpi_device_set_power allows bypassing the disabled power state safety check.

Pre-existing issues:
- [High] Unreleased ACPI power resource references if acpi_dev_pm_explicit_set() fails.
--

commit 6095d5c97ad32b03d70b1c09e3f9a5916aa85e11
Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

    ACPI: scan: Stop calling acpi_bus_init_power() early

    There is a problem, introduced by commit 9d9bcae47fd5 ("ACPI: delay
    enumeration of devices with a _DEP pointing to an INT3472 device")
    inadvertently, that devices with missing dependencies may be put
    into power state D0 prematurely [1].

    Namely, acpi_bus_init_power() called by acpi_bus_get_power_flags()
    during the early initialization of ACPI device objects, may discover
    that all of the power resources needed by the given device to be in
    power state D0 are initially on, so it will reference count those
    power resources and transition the device into D0.

    Link: https://lore.kernel.org/linux-acpi/20260820-acpi-power-resource-ref-fix-v2-1-29818173ea13@linux.spacemit.com/ [1]

> diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
> index 4269735aadde5..e13096cfd7909 100644
> --- a/drivers/acpi/device_pm.c
> +++ b/drivers/acpi/device_pm.c
[ ... ]
> @@ -157,6 +159,15 @@ int acpi_device_set_power(struct acpi_device *device, int state)
>  	    || (state < ACPI_STATE_D0) || (state > ACPI_STATE_D3_COLD))
>  		return -EINVAL;
>  
> +	if (device->power.state == ACPI_D_STATE_DISABLED)
> +		return -ENXIO;
> +
> +	if (device->power.state == ACPI_STATE_UNKNOWN) {
> +		result = acpi_bus_init_power(device);
> +		if (result)
> +			return result;
> +	}
> +

[Severity: High]
Since device->power.state is checked outside of the init_power_lock, can a
TOCTOU race occur here?

If two threads enter acpi_device_set_power() concurrently for a device in
ACPI_STATE_UNKNOWN:

1. Thread 1 calls acpi_bus_init_power(), fails, and poisons the state to
ACPI_D_STATE_DISABLED.
2. Thread 2 acquires init_power_lock in acpi_bus_init_power(), sees the
state is no longer ACPI_STATE_UNKNOWN, and returns 0 (success).
3. Thread 2 then proceeds to apply power transitions to a device that Thread
1 just poisoned.

[ ... ]
> @@ -293,20 +304,11 @@ int acpi_bus_set_power(acpi_handle handle, int state)
>  }
>  EXPORT_SYMBOL(acpi_bus_set_power);
>  
> -int acpi_bus_init_power(struct acpi_device *device)
> +static int acpi_device_init_power(struct acpi_device *device)
>  {

[Severity: High]
This is a pre-existing issue, but does this function leak power resource
references?

If acpi_power_on_resources() succeeds but the subsequent call to
acpi_dev_pm_explicit_set(device, state) fails, it returns the error
immediately without dropping the power resource references acquired just
before.

[ ... ]
> @@ -340,9 +342,43 @@ int acpi_bus_init_power(struct acpi_device *device)
>  		state = ACPI_STATE_D0;
>  	}
>  	device->power.state = state;
> +
> +	acpi_handle_debug(device->handle, "Initial power state: %s\n",
> +			  acpi_power_state_string(state));
> +
>  	return 0;
>  }
>  
> +int acpi_bus_init_power(struct acpi_device *device)
> +{
> +	static DEFINE_MUTEX(init_power_lock);
> +	int result;
> +
> +	/*
> +	 * This is done to prevent power state initialization from being carried
> +	 * out twice in parallel for the same device (not impossible, but very
> +	 * unlikely).
> +	 */
> +	guard(mutex)(&init_power_lock);

[Severity: High]
Can this global non-recursive mutex cause deadlocks?

If acpi_device_init_power() (called while holding this lock) evaluates ACPI
methods (e.g., _ON) that access an ACPI Operation Region, it can trigger a
runtime resume of the controller device. If the controller device's power
state is ACPI_STATE_UNKNOWN, its runtime resume path (e.g.,
acpi_device_set_power()) will recursively call acpi_bus_init_power(), leading
to a deadlock on this static mutex.

> +
> +	if (device->power.state != ACPI_STATE_UNKNOWN)
> +		return 0;
> +
> +	/*
> +	 * The ACPI device power state can be only initialized once.  If this
> +	 * fails, ACPI power management will not be used for the device going
> +	 * forward.
> +	 */
> +	result = acpi_device_init_power(device);
> +	if (result) {
> +		device->power.state = ACPI_D_STATE_DISABLED;

[Severity: High]
Is this disabled state fully protected from being overwritten by other
functions?

If acpi_bus_init_power() fails, the state is set to ACPI_D_STATE_DISABLED.
Later, if acpi_device_update_power() is called, it sees the state is not
ACPI_STATE_UNKNOWN, bypasses the initialization check, and calls
acpi_device_get_power(). If the device has no power resources, it executes
device->power.state = state;, unconditionally wiping out the
ACPI_D_STATE_DISABLED marker.

> +		acpi_handle_info(device->handle,
> +			"Failed to determine initial power state, ACPI PM disabled\n");
> +	}
> +
> +	return result;
> +}
> +
>  /**
>   * acpi_device_fix_up_power - Force device with missing _PSC into D0.
>   * @device: Device object whose power state is to be fixed up.
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index f48715ed827c7..4586f1798685f 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
[ ... ]
> @@ -2364,16 +2361,10 @@ static int acpi_bus_attach(struct acpi_device *device, void *first_pass)
>  
>  	acpi_ec_register_opregions(device);
>  
> -	if (!device->flags.initialized) {
> -		device->flags.power_manageable =
> -			device->power.states[ACPI_STATE_D0].flags.valid;
> -		if (acpi_bus_init_power(device))
> -			device->flags.power_manageable = 0;
> +	acpi_bus_init_power(device);

[Severity: High]
Does ignoring the return value of acpi_bus_init_power() here break system
suspend?

By no longer clearing device->flags.power_manageable when initialization
fails, the device remains power manageable but its state is marked
ACPI_D_STATE_DISABLED. During system suspend, acpi_dev_pm_low_power() will
see it is power manageable and call acpi_device_set_power(). Encountering
ACPI_D_STATE_DISABLED, acpi_device_set_power() returns -ENXIO, which
propagates to the PM core and aborts the suspend process entirely.

>  
> -		device->flags.initialized = true;
> -	} else if (device->flags.visited) {
> +	if (device->flags.visited)
>  		goto ok;
> -	}
>  
>  	ret = acpi_scan_attach_handler(device);
>  	if (ret < 0)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/5144065.31r3eYUQgx@rafael.j.wysocki?part=2