Re: [PATCH v1 3/7] ACPI: bus: Drop initialized flag from struct acpi_device_flags

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Removing the `initialized` flag causes the device enumeration check to remain true after a failed hot-remove eject, leading to a double-detach and a NULL pointer dereference (e.g., in `acpi_pci_root_remove()`) upon subsequent status checks.
--

commit 8f4c1e701ec6dd8150885a28ad18d087926defd7
Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

ACPI: bus: Drop initialized flag from struct acpi_device_flags

After previous changes, the ACPI device object "initialized" flag is
not particularly useful. It is set almost all the time and it is
only checked along with the "visited" flag, but checking the latter
is sufficient. Drop it accordingly.

> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 5aa3ecb000e14..019a43e3b5d7f 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -281,7 +281,6 @@ static int acpi_scan_check_and_detach(struct acpi_device *adev, void *p)
>  	 * that.
>  	 */
>  	acpi_device_set_power(adev, ACPI_STATE_D3_COLD);
> -	adev->flags.initialized = false;
>  
>  	/* For eject this is deferred to acpi_bus_post_eject() */
>  	if (!(flags & ACPI_SCAN_CHECK_FLAG_EJECT)) {

[Severity: High]
Could removing this state barrier cause a double-detach and a NULL pointer
dereference if a hot-remove eject fails?

When a hot-remove eject operation is initiated (ACPI_SCAN_CHECK_FLAG_EJECT),
acpi_scan_check_and_detach() executes handler->detach(adev).

For a PCI root device, acpi_pci_root_remove() frees the acpi_pci_root
structure and sets device->driver_data = NULL. At this stage, adev->handler
and adev->flags.visited are intentionally left uncleared because they are
deferred to acpi_bus_post_eject().

If the firmware eject (_EJ0) fails, acpi_scan_hot_remove() aborts and
acpi_bus_post_eject() is skipped. This leaves the device in the tree with
adev->handler attached and visited = 1.

Before this patch, adev->flags.initialized = false was unconditionally set
during the eject phase, so a subsequent status check would evaluate
acpi_device_enumerated() to false and safely abort.

By removing the initialized flag, the status check relies solely on visited.
This bypasses the early return and executes handler->detach(adev) a second
time.

Handlers like acpi_pci_root_remove() do not check if acpi_driver_data() is
NULL on a second pass, and will crash when dereferencing root->bus:

drivers/acpi/pci_root.c:acpi_pci_root_remove() {
    ...
    struct acpi_pci_root *root = acpi_driver_data(device);
    
    pci_lock_rescan_remove();
    pci_stop_root_bus(root->bus);
    ...
}

[ ... ]

> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index b0d9057ccb8ab..596fbd748e88d 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -654,7 +653,7 @@ void acpi_set_modalias(struct acpi_device *adev, const char *default_id,
>  
>  static inline bool acpi_device_enumerated(struct acpi_device *adev)
>  {
> -	return adev && adev->flags.initialized && adev->flags.visited;
> +	return adev && adev->flags.visited;
>  }

[Severity: High]
Does this change bypass the protection that initialized provided during failed
hot-remove operations as described above?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/3435655.aeNJFYEL58@rafael.j.wysocki?part=3