Re: [PATCH v15 5/5] s390/pci: Fix circular/recursive deadlocks in PCI-bus and -device release

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] pci: mixing goto and cleanup helpers in __zpci_event_error and pci_base_init
--

commit 703530c4b98b53581e1625ffc110fa9701e163ee
Author: Benjamin Block <bblock@linux.ibm.com>

s390/pci: Fix circular/recursive deadlocks in PCI-bus and -device release

When removing PCI device or PCI bus objects there are a couple of
call-chains where it is possible that the kernel runs into a circular
deadlock involving the central pci_rescan_remove_lock.

> diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
> index d64c544b32b0c..4fc3e3dc87ad1 100644
> --- a/arch/s390/pci/pci.c
> +++ b/arch/s390/pci/pci.c
[ ... ]
> @@ -1208,7 +1212,8 @@ static int __init pci_base_init(void)
>  	if (rc)
>  		goto out_irq;
>  
> -	rc = zpci_scan_devices();
> +	scoped_guard(pci_rescan_remove)
> +		rc = zpci_scan_devices();
>  	if (rc)
>  		goto out_find;
>  

[Severity: Medium]
Is it intentional to mix goto-based error handling with scope-based cleanup
helpers here?

The pci_base_init() function contains multiple goto statements for error
handling, and this patch introduces a scoped_guard() into the same routine.
The cleanup.h guidelines state that usage of goto and cleanup helpers should
never be mixed in the same function.

[ ... ]
> diff --git a/arch/s390/pci/pci_event.c b/arch/s390/pci/pci_event.c
> index f317a1465dad8..268f435677ae3 100644
> --- a/arch/s390/pci/pci_event.c
> +++ b/arch/s390/pci/pci_event.c
[ ... ]
> @@ -382,6 +382,7 @@ static void __zpci_event_error(struct zpci_ccdf_err *ccdf)
>  	pci_dev_put(pdev);
>  no_pdev:
>  	mutex_unlock(&zdev->state_lock);
> +	guard(pci_rescan_remove)();
>  	zpci_zdev_put(zdev);
>  }
>  

[Severity: Medium]
Can this introduction of guard() into __zpci_event_error() also cause issues
with mixed cleanup paradigms?

In __zpci_event_error(), there are goto statements used for error handling,
and this patch introduces a guard() right at the cleanup label. Mixing goto
with cleanup.h helpers in the same function goes against the subsystem
guidelines.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1790267348.git.bblock@linux.ibm.com?part=5