Re: [PATCH] ACPI: PCI: Clear driver_data on all error paths in acpi_pci_root_add()
Chen Pei <[email protected]>
| Newsgroups | org.kernel.vger.linux-cxl,dev.linux.lists.sashiko-reviews,org.kernel.vger.linux-acpi,org.kernel.vger.linux-kernel,org.kernel.vger.linux-pci |
|---|---|
| Message-ID | <[email protected]> |
Thanks for the review. > Pre-existing issues: > - [High] The acpi_pci_root_remove() function frees the acpi_pci_root > object but fails to clear device->driver_data, leaving a dangling > pointer. Agreed. acpi_pci_root_remove() calls kfree(root) without resetting device->driver_data, which leaves exactly the same dangling pointer this patch removes from the add() error paths. I'll fix it in v2 by also clearing device->driver_data in acpi_pci_root_remove(), so the add and remove paths leave driver_data consistent. > - [High] Lockless access to `device->driver_data` causes a TOCTOU > Use-After-Free race during hot-add failure and hot-remove. I don't think this is reachable, so I'll leave the locking model unchanged: - driver_data is published early (device->driver_data = root), but the PCI hierarchy below this root bridge is not created until pci_acpi_scan_root() and pci_bus_add_devices(), both of which run only on the success path, after the dmar_device_add() error goto. During the error window there are no child PCI devices yet, so an AER or CXL consumer under this bridge has no way to obtain the handle and call acpi_pci_find_root() against a half-initialized root. - The early publish of driver_data is pre-existing; this patch does not change it, it only makes the error-path cleanup consistent. - ACPI .add and .remove are serialized by acpi_scan_lock, so add cannot race remove. If there is a concrete caller that can reach acpi_pci_find_root() for this handle during the add-failure window, I'm happy to revisit. Adding RCU or locking to the find_root() fast path otherwise seems unwarranted for this fix. I'll send a v2 with the acpi_pci_root_remove() fix. Thanks, Pei