[PATCH] PCI: octep: handle an unsuccessful hotplug scan

[PATCH] PCI: octep: handle an unsuccessful hotplug scan

[Slavin Liu]

pci_scan_single_device() can return NULL if no device is found or
allocation fails. octep_hp_enable_pdev() passes that pointer to
pci_bus_add_device() and then dereferences it for a debug message.

Return -ENODEV when the scan fails and propagate it through the hotplug
enable_slot callback. This leaves the slot disabled and allows a later
retry without reporting a successful enable to userspace. Log the
failure for callers that cannot return an error to userspace. The
scoped mutex guard releases the slot lock on each return path.

Detected by static analysis and reviewed with AI-assisted source auditing.

Fixes: e434e54d3ffc ("PCI: hotplug: Add OCTEON PCI hotplug controller driver")
Assisted-by: LLM
Signed-off-by: Slavin Liu <bolin.liu@seu.edu.cn>
---
 drivers/pci/hotplug/octep_hp.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/hotplug/octep_hp.c b/drivers/pci/hotplug/octep_hp.c
index a0a7f9ccb8fa..a57969185022 100644
--- a/drivers/pci/hotplug/octep_hp.c
+++ b/drivers/pci/hotplug/octep_hp.c
@@ -66,24 +66,31 @@ struct octep_hp_controller {
 	spinlock_t hp_cmd_lock; /* Protects hp_cmd_list */
 };
 
-static void octep_hp_enable_pdev(struct octep_hp_controller *hp_ctrl,
-				 struct octep_hp_slot *hp_slot)
+static int octep_hp_enable_pdev(struct octep_hp_controller *hp_ctrl,
+				struct octep_hp_slot *hp_slot)
 {
 	guard(mutex)(&hp_ctrl->slot_lock);
 	if (hp_slot->hp_pdev) {
 		pci_dbg(hp_slot->hp_pdev, "Slot %s is already enabled\n",
 			hotplug_slot_name(&hp_slot->slot));
-		return;
+		return 0;
 	}
 
 	/* Scan the device and add it to the bus */
 	hp_slot->hp_pdev = pci_scan_single_device(hp_ctrl->pdev->bus,
 						  hp_slot->hp_devfn);
+	if (!hp_slot->hp_pdev) {
+		pci_err(hp_ctrl->pdev, "Failed to scan slot %s\n",
+			hotplug_slot_name(&hp_slot->slot));
+		return -ENODEV;
+	}
 	pci_bus_assign_resources(hp_ctrl->pdev->bus);
 	pci_bus_add_device(hp_slot->hp_pdev);
 
 	dev_dbg(&hp_slot->hp_pdev->dev, "Enabled slot %s\n",
 		hotplug_slot_name(&hp_slot->slot));
+
+	return 0;
 }
 
 static void octep_hp_disable_pdev(struct octep_hp_controller *hp_ctrl,
@@ -109,8 +116,7 @@ static int octep_hp_enable_slot(struct hotplug_slot *slot)
 	struct octep_hp_slot *hp_slot =
 		container_of(slot, struct octep_hp_slot, slot);
 
-	octep_hp_enable_pdev(hp_slot->ctrl, hp_slot);
-	return 0;
+	return octep_hp_enable_pdev(hp_slot->ctrl, hp_slot);
 }
 
 static int octep_hp_disable_slot(struct hotplug_slot *slot)

Re: [PATCH] PCI: octep: handle an unsuccessful hotplug scan

[sashiko-bot]

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

Pre-existing issues:
- [High] Calling hotplug_slot_name() after deregistering the slot causes a NULL pointer dereference.
- [High] PCI bus topology changes in octep_hp_enable_pdev are made without holding the global pci_lock_rescan_remove().
- [High] Lockless modification and traversal of hp_ctrl->slot_list leads to Data Race and Use-After-Free.
- [High] Iterating over pdev->bus->devices during probe is performed without holding pci_bus_sem.
--

commit 4181e29e1dc190e4bfd9a170a79184523f6d175e
Author: Slavin Liu <bolin.liu@seu.edu.cn>

PCI: octep: handle an unsuccessful hotplug scan

pci_scan_single_device() can return NULL if no device is found or
allocation fails. Return -ENODEV when the scan fails and propagate
it through the hotplug enable_slot callback. The scoped mutex guard
releases the slot lock on each return path.

> diff --git a/drivers/pci/hotplug/octep_hp.c b/drivers/pci/hotplug/octep_hp.c
> index a0a7f9ccb8fa3..a57969185022c 100644
> --- a/drivers/pci/hotplug/octep_hp.c
> +++ b/drivers/pci/hotplug/octep_hp.c
> @@ -66,24 +66,31 @@ struct octep_hp_controller {
[ ... ]
>  	/* Scan the device and add it to the bus */
>  	hp_slot->hp_pdev = pci_scan_single_device(hp_ctrl->pdev->bus,
>  						  hp_slot->hp_devfn);
> +	if (!hp_slot->hp_pdev) {
> +		pci_err(hp_ctrl->pdev, "Failed to scan slot %s\n",
> +			hotplug_slot_name(&hp_slot->slot));

[Severity: High]
This is a pre-existing issue, but this newly added error logging expands the
bug by triggering a NULL pointer dereference if the scan fails during
deregistration.

When devm executes octep_hp_deregister_slot() during driver unbind or probe
failure, it calls pci_hp_deregister(&hp_slot->slot), which sets
slot->pci_slot to NULL.

Immediately after, it calls octep_hp_enable_pdev(). If the scan fails,
hotplug_slot_name(&hp_slot->slot) is called, which dereferences the now-NULL
slot->pci_slot pointer, causing a kernel oops. Can we avoid using
hotplug_slot_name() here or ensure it is safe to call?

> +		return -ENODEV;
> +	}
>  	pci_bus_assign_resources(hp_ctrl->pdev->bus);
>  	pci_bus_add_device(hp_slot->hp_pdev);

[Severity: High]
This isn't a bug introduced by this patch, but does this function modify
global PCI bus topology without holding pci_lock_rescan_remove()?

When octep_hp_enable_pdev() calls pci_scan_single_device(),
pci_bus_assign_resources(), and pci_bus_add_device(), it modifies global
PCI lists and structures. The PCI core requires callers to serialize such
changes using pci_lock_rescan_remove().

If this lock isn't held, can concurrent PCI operations like sysfs remove or
other hotplug events corrupt the PCI structures?

[ ... ]

[Severity: High]
This is a pre-existing issue, but does lockless modification and traversal
of hp_ctrl->slot_list lead to a use-after-free or data race?

In octep_hp_deregister_slot(), the list is modified without a lock:

drivers/pci/hotplug/octep_hp.c:octep_hp_deregister_slot() {
    ...
	pci_hp_deregister(&hp_slot->slot);
	octep_hp_enable_pdev(hp_ctrl, hp_slot);
	list_del(&hp_slot->list);
	kfree(hp_slot);
}

Concurrently, the workqueue iterates this list locklessly:

drivers/pci/hotplug/octep_hp.c:octep_hp_cmd_handler() {
    ...
	list_for_each_entry(hp_slot, &hp_ctrl->slot_list, list) {
    ...
}

Because devres executes cleanup actions in LIFO order,
octep_hp_deregister_slot() runs during driver unbind before
octep_hp_irq_cleanup() has disabled interrupts and flushed the workqueue.
If a hotplug interrupt fires during unbind, could the workqueue traverse
a freed hp_slot?

[Severity: High]
This is a pre-existing issue, but does iterating over pdev->bus->devices
during probe require holding the read/write lock?

In octep_hp_pci_probe():

drivers/pci/hotplug/octep_hp.c:octep_hp_pci_probe() {
    ...
	list_for_each_entry_safe(tmp_pdev, next, &pdev->bus->devices, bus_list) {
		if (tmp_pdev == pdev)
			continue;
    ...
}

Although probe is serialized by the driver core for this specific device,
the bus itself is globally shared. A concurrent sysfs removal or hotplug
event for a sibling device on the same bus could remove an entry from
bus->devices while the loop is iterating. Should this be protected with
down_read(&pci_bus_sem)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260913125118.109777-1-bolin.liu@seu.edu.cn?part=1