Re: [PATCH] PCI: rpaphp: report first slot registration error

[sashiko-bot@kernel.org]

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

New issues:
- [High] Changing the return value to track `first_error` without adding rollback logic causes callers to fatally abort on partial successes, actively leaking successfully registered slots and major resources.

Pre-existing issues:
- [High] DLPAR remove only deregisters the first hotplug slot for a device node, leaking any remaining slots and risking a use-after-free.
- [High] Missing pci_lock_rescan_remove() when adding PCI devices during lockless slot enablement.
--

commit 5c0fcbf749d55b7d5f494a28af745273bc4f6af3
Author: Adriano Cordova <adrianox@gmail.com>

    PCI: rpaphp: report first slot registration error
    
    rpaphp_drc_add_slot() overwrites retval on every loop iteration, so a
    failure to register a slot is lost if a later slot registers
    successfully and the function returns success.
    
    Track the first error and return it, while still attempting to register
    the remaining slots.

> diff --git a/drivers/pci/hotplug/rpaphp_core.c b/drivers/pci/hotplug/rpaphp_core.c
> index 2316de0fd1982..af7241d526cf4 100644
> --- a/drivers/pci/hotplug/rpaphp_core.c
> +++ b/drivers/pci/hotplug/rpaphp_core.c
> @@ -376,6 +376,7 @@ static int rpaphp_drc_add_slot(struct device_node *dn)
>  {
>  	struct slot *slot;
>  	int retval = 0;
> +	int first_error = 0;
>  	int i;
>  	const __be32 *indexes, *names, *types, *power_domains;
>  	char *name, *type;

[Severity: High]
This is a pre-existing issue, but does dlpar_remove_pci_slot() incorrectly
assume a 1:1 mapping between device nodes and slots?

The loop in rpaphp_drc_add_slot() can register multiple hotplug slots pointing
to the same device node. During a DLPAR remove operation,
dlpar_remove_pci_slot() in drivers/pci/hotplug/rpadlpar_core.c calls
find_php_slot(dn), which returns only the first matching slot:

drivers/pci/hotplug/rpadlpar_core.c:dlpar_remove_pci_slot() {
    ...
    slot = find_php_slot(dn);
    if (slot) {
        pr_debug("PCI: Removing hotplug slot for %04x:%02x...\n",
                 pci_domain_nr(bus), bus->number);

        if (rpaphp_deregister_slot(slot)) {
    ...
}

Does this mean deregistering only the first slot will leave the remaining
slots permanently leaked, risking a use-after-free when they later try to
dereference a freed slot->bus?

[ ... ]

> @@ -407,16 +408,18 @@ static int rpaphp_drc_add_slot(struct device_node *dn)
>  		if (!retval)
>  			retval = rpaphp_register_slot(slot);

[Severity: High]
This is a pre-existing issue, but does calling rpaphp_enable_slot() just
before this registration modify the PCI bus devices list without holding the
required locks?

rpaphp_enable_slot() eventually calls pci_hp_add_devices(bus) locklessly,
but PCI rules require holding the pci_lock_rescan_remove() mutex when
modifying PCI bus structures:

drivers/pci/hotplug/rpaphp_pci.c:rpaphp_enable_slot() {
    ...
        if (list_empty(&bus->devices)) {
            pseries_eeh_init_edev_recursive(PCI_DN(slot->dn));
            pci_hp_add_devices(bus);
        }
    ...
}

Can this data race lead to list corruption or a use-after-free if it races
with a standard PCI sysfs rescan?

> -		if (retval)
> +		if (retval) {
> +			if (!first_error)
> +				first_error = retval;
>  			dealloc_slot_struct(slot);
> +		}
>  
>  		name += strlen(name) + 1;
>  		type += strlen(type) + 1;
>  	}
> -	dbg("%s - Exit: rc[%d]\n", __func__, retval);
> +	dbg("%s - Exit: rc[%d]\n", __func__, first_error);
>  
> -	/* XXX FIXME: reports a failure only if last entry in loop failed */
> -	return retval;
> +	return first_error;
>  }

[Severity: High]
Does returning first_error instead of 0 here cause callers to leak resources?

Because callers like dlpar_add_pci_slot() in
drivers/pci/hotplug/rpadlpar_core.c treat any error as a fatal failure and
immediately abort without rolling back prior allocations:

drivers/pci/hotplug/rpadlpar_core.c:dlpar_add_pci_slot() {
    ...
	if (rpaphp_add_slot(dn)) {
		printk(KERN_ERR "%s: unable to add hotplug slot %s\n",
			__func__, drc_name);
		return -EIO;
	}
    ...
}

If an early slot in a multi-slot device node fails to register here but a
later slot succeeds, does returning an error instead of 0 convert a previously
resilient partial success into a fatal abort that permanently leaks the
successfully registered slots and prior PHB structures?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260910121330.339150-1-adrianox@gmail.com?part=1