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

[Farhan Ali]

On 7/23/2026 8:19 AM, Matthew Rosato wrote:
> 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?

Yes, this becomes unnecessary and actually overwriting the fib->fmt0.isc 
causes a reference count leak in the cleanup path as we use 
fib->fmt0.isc when calling kvm_s390_gisc_unregister().


> 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.'

Ack, will add that.


> >   	/* 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.

Yeah, will do. I agree, that its not strictly necessary just makes the 
code a little less messier having to drop and re-acquire. FWIW we also 
acquire the lock before doing a kvm_zpci_clear_airq() in the disable path.

Thanks

Farhan

> 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);