Re: [PATCH v2 2/3] ppc/pnv: Refactor PNV PCI hotplug driver

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unprotected state modifications and TOCTOU race in slot enable/disable paths.
- [High] Lockless access to pci_bus->devices list from hardirq context.
--

commit cf3f614d9d225b084eb92a8c44dcbe6b7a0d3932
Author: Aditya Gupta <adityag@linux.ibm.com>

ppc/pnv: Refactor PNV PCI hotplug driver

This patch refactors the PNV PCI hotplug driver to separate PCIe and OpenCAPI
slot operations into backend_ops. It isolates PCIe-specific code so future
updates can edit specific ops instead of common code.

> diff --git a/drivers/pci/hotplug/pnv_php.c b/drivers/pci/hotplug/pnv_php.c
> index d0f5e8ad1f716..997412eea4863 100644
> --- a/drivers/pci/hotplug/pnv_php.c
> +++ b/drivers/pci/hotplug/pnv_php.c

[ ... ]

> @@ -972,7 +1026,7 @@ static void pnv_php_event_handler(struct work_struct *work)
>  		pnv_php_enable_slot(&php_slot->slot);
>  	} else {
>  		pnv_php_disable_slot(&php_slot->slot);
> -		pnv_php_detect_clear_suprise_removal_freeze(php_slot);
> +		pnv_php_detect_clear_surprise_removal_freeze(php_slot);
>  	}
>  
>  	kfree(event);

[Severity: High]
This isn't a bug introduced by this patch, but can this sequence lead to a
race condition?

Looking at pnv_php_disable_slot(), it checks php_slot->state without holding
any synchronization locks. If a sysfs write and a surprise hotplug event (via
this event handler workqueue) trigger concurrently, can both threads pass the
state check and proceed to tear down the slot?

It seems this could lead to double-unregistering and calling pnv_php_release()
twice, which would result in a double list_del() on php_slot->link and a
kernel panic.

[ ... ]

> @@ -1055,7 +1109,7 @@ static irqreturn_t pnv_php_interrupt(int irq, void *data)
>  	return IRQ_HANDLED;
>  }

[Severity: High]
This is a pre-existing issue, but does the interrupt handler have a lockless
access issue here?

In pnv_php_interrupt(), the handler accesses php_slot->bus->devices via
list_first_entry_or_null() without holding pci_bus_sem or taking a reference
on the returned device.

Concurrently, pnv_php_disable_slot() calls pci_hp_remove_devices() which
unbinds and frees the child devices. Since the root slot IRQ is intentionally
left active during this removal, if the interrupt fires and resolves the child
device just before the hotplug core frees it, could this cause a use-after-free
in hardirq context when passing the pointer to pci_dev_to_eeh_dev()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260527180816.2749186-1-adityag@linux.ibm.com?part=2