Re: [PATCH 1/4] s390/vfio-ap: Fix leak of pinned NIB and registered NISC in vfio_ap_irq_enable()

On 8/24/26 12:57 PM, Matthew Rosato wrote:
> On 8/24/26 9:58 AM, Anthony Krowiak wrote:
>> The vfio_ap_irq_enable() function executes the PQAP(AQIC) instruction to
>> enable interrupts for an AP queue. A switch statement is used to examine
>> the status response code returned from the instruction to determine
>> whether it succeeded or failed and react accordingly. For the default case,
>> the vfio_ap_irq_disable function is invoked to disable interrupts for the
>> queue and clean up the AQIC resources (i.e., unpin the NIB and unregister
>> the NISC). There are a number of problems with this:
>>
>> 1. Neither the q->saved_iova nor q->saved_isc has been set, so the
>>     AQIC resources - assuming those values have been previously set - will
>>     be the NIB and NISC resources from a prior call; the NIB and NISC from
>>     the current call are therefore leaked.
>>
>> 2. Interrupts may never have been enabled. Sending a disable instruction to
>>     a queue that the hardware just told you is in a bad state (CHECKSTOPPED,
>>     DECONFIGURED, Q_NOT_AVAIL) is at best wasted work and at worst generates
>>     a further WARN_ONCE from inside vfio_ap_irq_disable's own default.
>>
>> 3. The hardware just rejected the new ap_aqic() enable attempt with an
>>     unexpected status. Disabling a previously-working IRQ config - assuming
>>     that is even possible - as a reaction to a failed enable attempt does
>>     not make sense; it is actively destructive, tearing down something that
>>     was working for no valid reason.
>>
>> The fix is to unregister the NISC and an unpin the NIB in the default case
>> of the switch statement.
>>
>> Fixes: ec89b55e3bce7 ("s390: ap: implement PAPQ AQIC interception in kernel")
>> Cc: [email protected]
>> Signed-off-by: Anthony Krowiak <[email protected]>
>> ---
>>   drivers/s390/crypto/vfio_ap_ops.c | 11 ++++++++---
>>   1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> index 940c0ff668be..a46bf381ab72 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -503,9 +503,14 @@ static struct ap_queue_status vfio_ap_irq_enable(struct vfio_ap_queue *q,
>>   		vfio_unpin_pages(&q->matrix_mdev->vdev, nib, 1);
>>   		break;
>>   	default:
>> -		pr_warn("%s: apqn %04x: response: %02x\n", __func__, q->apqn,
>> -			status.response_code);
>> -		vfio_ap_irq_disable(q);
>> +		pr_warn("%s: PQAP(AQIC) failed with response code %02x for apqn %04x\n",
>> +			__func__, status.response_code, q->apqn);
> LGTM, except Sashiko mentions the pr_warn here that you are updating has
> a pre-existing issue.
>
> Since you're touching it already, do you think it makes sense to switch
> to pr_warn_ratelimited with this patch?

I think it makes more sense to switch to VFIO_AP_DBF_WARN which all other
guest-triggered warning paths in  this function already use.

>
>> +		/* We could not modify IRQ settings: clear new configuration */
>> +		ret = kvm_s390_gisc_unregister(kvm, isc);
>> +		if (ret)
>> +			VFIO_AP_DBF_WARN("%s: kvm_s390_gisc_unregister: rc=%d isc=%d, apqn=%#04x\n",
>> +					 __func__, ret, isc, q->apqn);
>> +		vfio_unpin_pages(&q->matrix_mdev->vdev, nib, 1);
>>   		break;
>>   	}
>>