Re: [PATCH v4 5/6] KVM: s390: pci: Fix resource leak on IRQ registration failure

[Matthew Rosato]

On 7/22/26 1:06 PM, Farhan Ali wrote:
> Currently if kvm_zpci_set_airq() fails, kvm_s390_pci_aif_enable() returns
> the error code but doesn't do any resource cleanup thus leaking resources.

Nits: s/the/an/  ....  and 'resource cleanup, thus leaking' (add a comma)

> Fix this by cleaning up all the resources such as the GAITE, AIBV, AISB and
> unpinning any pinned pages.
> 
> Fixes: 3c5a1b6f0a18 ("KVM: s390: pci: provide routines for enabling/disabling interrupt forwarding")
> Signed-off-by: Farhan Ali <alifm@xxxxxxxxxxxxx>
> ---
>  arch/s390/kvm/pci.c | 29 +++++++++++++++++++++--------
>  1 file changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
> index 231a4236fc3c..d76b2c5484ac 100644
> --- a/arch/s390/kvm/pci.c
> +++ b/arch/s390/kvm/pci.c
> @@ -341,19 +341,32 @@ static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
>  	aift->kzdev[zdev->aisb] = zdev->kzdev;
>  	spin_unlock_irq(&aift->gait_lock);
>  
> -	/* Update guest FIB for re-issue */
> -	fib->fmt0.aisbo = zdev->aisb & 63;
> -	fib->fmt0.aisb = virt_to_phys(aift->sbv->vector) + (zdev->aisb / 64) * 8;
> -	fib->fmt0.isc = gisc;
> -

OK, I had the same problem as Christian here.

This is dead code because we don't re-use the fib, we make a new one in
kvm_zpci_set_airq() and fill it with values from the kzdev.  Right?

I agree adding something to the commit message to that effect would be
good, even something like 'While at it, remove dead code that stored FIB
values that were never referenced.'

>  	/* Save some guest fib values in the host for later use */
> -	zdev->kzdev->fib.fmt0.isc = fib->fmt0.isc;
> +	zdev->kzdev->fib.fmt0.isc = gisc;
>  	zdev->kzdev->fib.fmt0.aibv = fib->fmt0.aibv;
> -	mutex_unlock(&aift->aift_lock);
>  
>  	/* Issue the clp to setup the irq now */
>  	rc = kvm_zpci_set_airq(zdev);
> -	return rc;
> +	if (!rc) {
> +		mutex_unlock(&aift->aift_lock);

This is subtle; we are now holding the aift_lock a bit longer now (over
the MPCIFC re-issue) which I don't think is strictly necessary since we
aren't messing with any of the zpci_aift fields during that timeframe,
but I think is fine to do.  Maybe also worth a mention in the commit
message that the lock is held a bit longer in order to handle the error
case vs drop/re-acquire.

For the code itself:

Reviewed-by: Matthew Rosato <mjrosato@xxxxxxxxxxxxx>

> +		return rc;
> +	}
> +
> +	/* Start cleanup */
> +	zdev->kzdev->fib.fmt0.isc = 0;
> +	zdev->kzdev->fib.fmt0.aibv = 0;
> +
> +	spin_lock_irq(&aift->gait_lock);
> +	gaite->count--;
> +	gaite->aisb = 0;
> +	gaite->gisc = 0;
> +	gaite->aisbo = 0;
> +	gaite->gisa = 0;
> +	aift->kzdev[zdev->aisb] = NULL;
> +	spin_unlock_irq(&aift->gait_lock);
> +
> +	airq_iv_release(zdev->aibv);
> +	zdev->aibv = NULL;
>  
>  free_aisb:
>  	airq_iv_free_bit(aift->sbv, zdev->aisb);