[PATCH v2 0/2] s390/pci: Fix missing pci_dev_put() in zpci_report_status()

[PATCH v2 0/2] s390/pci: Fix missing pci_dev_put() in zpci_report_status()

[Niklas Schnelle]

Hi All,

This series fixes two issues found in zpci_report_status() by
LLM review. A missing pci_dev_put() which could prevent proper removal
on hot unplug and a too short device lock critical section.

Thanks,
Niklas

Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
---
Changes in v2:
- Added a second fix to address Sashiko's finding of having
  to hold the device lock during the call to zpci_report_status()
- Add R-bs from Matthew and Farhan
- Link to v1: https://lore.kernel.org/r/20260818-fix_zpci_report_status_pdev_leak-v1-1-576520f4d068@linux.ibm.com

---
Niklas Schnelle (2):
      s390/pci: Fix missing pci_dev_put() in zpci_report_status()
      s390/pci: Extend device lock to cover zpci_report_status()

 arch/s390/pci/pci_event.c  | 2 +-
 arch/s390/pci/pci_report.c | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)
---
base-commit: 8d3ae59288f1e7d58d76558a6ee96d533bc5019f
change-id: 20260817-fix_zpci_report_status_pdev_leak-1b101c1fe021

Best regards,
-- 
Niklas Schnelle

[PATCH v2 1/2] s390/pci: Fix missing pci_dev_put() in zpci_report_status()

[Niklas Schnelle]

In zpci_report_status() a reference to the struct pci_dev associated
with the struct zpci_dev being reported about is acquired using
pci_get_slot(). This reference needs to be dropped with pci_dev_put()
but this call is missing thus leaking the reference. On subsequent hot
unplug this will cause the struct pci_dev to not be released leaking
memory and potentially prevent reattach.

Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
Cc: stable@vger.kernel.org
Fixes: 4ec6054e7321 ("s390/pci: Report PCI error recovery results via SCLP")
Reviewed-by: Farhan Ali <alifm@linux.ibm.com>
Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
---
 arch/s390/pci/pci_report.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/s390/pci/pci_report.c b/arch/s390/pci/pci_report.c
index 7030f7052926..76b8afc146fe 100644
--- a/arch/s390/pci/pci_report.c
+++ b/arch/s390/pci/pci_report.c
@@ -151,6 +151,7 @@ int zpci_report_status(struct zpci_dev *zdev, const char *operation, const char
 	else
 		pr_info("Reported PCI device status\n");
 
+	pci_dev_put(pdev);
 	free_page((unsigned long)report);
 
 	return ret;

-- 
2.53.0

Re: [PATCH v2 1/2] s390/pci: Fix missing pci_dev_put() in zpci_report_status()

[sashiko-bot]

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

Pre-existing issues:
- [High] Lockless TOCTOU and Use-After-Free when accessing `pdev->dev.driver` and `driver->name`.
--

--- Patch [1]: [PATCH v2 1/2] s390/pci: Fix missing pci_dev_put() in zpci_report_status() ---
commit 81196a1544a6f30ec9381d6aeaf28e9992efad5f
Author: Niklas Schnelle <schnelle@linux.ibm.com>

    s390/pci: Fix missing pci_dev_put() in zpci_report_status()
    
    In zpci_report_status() a reference to the struct pci_dev associated
    with the struct zpci_dev being reported about is acquired using
    pci_get_slot(). This reference needs to be dropped with pci_dev_put()
    but this call is missing thus leaking the reference. On subsequent hot
    unplug this will cause the struct pci_dev to not be released leaking
    memory and potentially prevent reattach.

> diff --git a/arch/s390/pci/pci_report.c b/arch/s390/pci/pci_report.c
> index 7030f7052926a..76b8afc146fea 100644
> --- a/arch/s390/pci/pci_report.c
> +++ b/arch/s390/pci/pci_report.c
> @@ -151,6 +151,7 @@ int zpci_report_status(struct zpci_dev *zdev, const char *operation, const char
>  	else
>  		pr_info("Reported PCI device status\n");
>  
> +	pci_dev_put(pdev);

[Severity: High]
This isn't a bug introduced by this patch, but while we are working with
pdev in zpci_report_status(), is it safe that earlier in this function
we access pdev->dev.driver locklessly?

arch/s390/pci/pci_report.c:zpci_report_status() {
...
	if (pdev)
		driver = to_pci_driver(pdev->dev.driver);
...
	buf += scnprintf(buf, end - buf, "driver: %s\n", (driver) ? driver->name : "n/a");
...
}

If a concurrent driver unbind (e.g., via sysfs) clears pdev->dev.driver while
zpci_report_status() is executing, the to_pci_driver() macro could read
it during a race. The to_pci_driver() macro evaluates its argument twice.
If pdev->dev.driver becomes NULL between these evaluations, it subtracts
the offset from NULL, yielding a large negative pointer.

Can this dereference crash the kernel if driver is a negative pointer from
the race?

Additionally, even if the macro evaluates safely, accessing driver->name
without holding the device lock or preventing module unload could result
in a use-after-free if the driver module is concurrently unloaded.

Could this be triggered by an administrator unbinding a PCI driver
concurrently with a PCI error recovery event?

>  	free_page((unsigned long)report);
>  
>  	return ret;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260819-fix_zpci_report_status_pdev_leak-v2-0-da0c4aa7c0ca@linux.ibm.com?part=1

[PATCH v2 2/2] s390/pci: Extend device lock to cover zpci_report_status()

[Niklas Schnelle]

As zpci_report_status() accesses the device's driver the device lock
needs to be held. Extend the critical section to cover the
zpci_report_status() call.

Cc: stable@vger.kernel.org
Fixes: 4ec6054e7321 ("s390/pci: Report PCI error recovery results via SCLP")
Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
---
 arch/s390/pci/pci_event.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/s390/pci/pci_event.c b/arch/s390/pci/pci_event.c
index 839bd91c056e..48c150d6ba81 100644
--- a/arch/s390/pci/pci_event.c
+++ b/arch/s390/pci/pci_event.c
@@ -256,8 +256,8 @@ static pci_ers_result_t zpci_event_attempt_error_recovery(struct pci_dev *pdev)
 		driver->err_handler->resume(pdev);
 	pci_uevent_ers(pdev, PCI_ERS_RESULT_RECOVERED);
 out_unlock:
-	device_unlock(&pdev->dev);
 	zpci_report_status(zdev, "recovery", status_str);
+	device_unlock(&pdev->dev);
 
 	return ers_res;
 }

-- 
2.53.0

Re: [PATCH v2 2/2] s390/pci: Extend device lock to cover zpci_report_status()

[sashiko-bot]

> As zpci_report_status() accesses the device's driver the device lock
> needs to be held. Extend the critical section to cover the
> zpci_report_status() call.
> 
> Cc: stable@vger.kernel.org
> Fixes: 4ec6054e7321 ("s390/pci: Report PCI error recovery results via SCLP")
> Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260819-fix_zpci_report_status_pdev_leak-v2-0-da0c4aa7c0ca@linux.ibm.com?part=2