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

[Matthew Rosato <mjrosato@linux.ibm.com>]

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: stable@vger.kernel.org
> Signed-off-by: Anthony Krowiak <akrowiak@linux.ibm.com>
> ---
>  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?

> +		/* 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;
>  	}
>