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

[Farhan Ali <alifm@linux.ibm.com>]

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@linux.ibm.com>
>> ---
>>   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@linux.ibm.com>
>
>> +		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);