Re: [PATCH v3 2/3] s390/pci: Rework __zpci_event_availability() to remove conditional locking

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

Pre-existing issues:
- [Critical] zpci_remove_reserved_devices() leaves zdev linked to a stack-allocated list, leading to Use-After-Scope stack memory corruption.
- [High] zpci_remove_reserved_devices() modifies device state without holding state_lock, causing data races and lockdep panics.
--

--- Patch [2]: [PATCH v3 2/3] s390/pci: Rework __zpci_event_availability() to remove conditional locking ---
commit 75e2f03936eebee8914ba9e93f94ec0e2bac2ac9
Author: Heiko Carstens <[email protected]>

s390/pci: Rework __zpci_event_availability() to remove conditional locking
    
This patch reworks __zpci_event_availability() to avoid taking locks
conditionally, resolving a compiler warning from static context analysis.
The event handling is split into smaller, dedicated functions.

> diff --git a/arch/s390/pci/pci_event.c b/arch/s390/pci/pci_event.c
> index bead4ed5d4abe..0a9eecb62bd1a 100644
> --- a/arch/s390/pci/pci_event.c
> +++ b/arch/s390/pci/pci_event.c
[ ... ]
> -static void __zpci_event_availability(struct zpci_ccdf_avail *ccdf)
> +static bool zpci_event_avail_any_device(struct zpci_ccdf_avail *ccdf)
>  {
> -	struct zpci_dev *zdev = get_zdev_by_fid(ccdf->fid);
> -	bool existing_zdev = !!zdev;
> -	enum zpci_state state;
> -
> -	zpci_dbg(3, "avl fid:%x, fh:%x, pec:%x\n",
> -		 ccdf->fid, ccdf->fh, ccdf->pec);
> +	/* 0x0306 - No handle or fid stored */
> +	if (ccdf->pec != 0x0306)
> +		return false;
> +	/* 0x308 or 0x302 for multiple devices */
> +	zpci_remove_reserved_devices();

[Severity: High]
This is a pre-existing issue, but does zpci_remove_reserved_devices() call
zpci_device_reserved() without holding the state_lock?

Looking at zpci_remove_reserved_devices() in arch/s390/pci/pci.c:

void zpci_remove_reserved_devices(void)
{
        ...
        list_for_each_entry_safe(zdev, tmp, &remove, entry)
                zpci_device_reserved(zdev);
}

And zpci_device_reserved() asserts this lock:

void zpci_device_reserved(struct zpci_dev *zdev)
{
        lockdep_assert_held(&zdev->state_lock);
        ...

Will this result in a lockdep warning and potential data races on the
device state during a 0x0306 PCI event?


[Severity: Critical]
This is also a pre-existing issue, but can this lead to memory corruption
if the device reference count doesn't drop to zero?

In arch/s390/pci/pci.c:zpci_remove_reserved_devices():

void zpci_remove_reserved_devices(void)
{
        ...
        LIST_HEAD(remove);

        spin_lock(&zpci_list_lock);
        list_for_each_entry_safe(zdev, tmp, &zpci_list, entry) {
                if (...)
                        list_move_tail(&zdev->entry, &remove);
        }
        ...
}

The devices are moved to a local stack-allocated list (remove).

When zpci_device_reserved(zdev) is called, it drops one reference. If
another thread currently holds a reference (for example, via
get_zdev_by_fid()), the device is not freed immediately. When
zpci_remove_reserved_devices() returns, the stack frame is destroyed, but
the device's entry remains linked to this destroyed stack memory.

Later, when the final reference is dropped, zpci_release_device() will call
list_del(&zdev->entry), writing to the invalid stack memory. Is there a
safe way to handle this list manipulation?

> +	zpci_scan_devices();
> +	return true;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2