Re: [PATCH v5 9/9] s390/vfio-ap: Fix memory leak when queue removed from host AP config

[Anthony Krowiak]

On 8/12/26 5:12 PM, Matthew Rosato wrote:
> On 8/12/26 4:02 PM, Anthony Krowiak wrote:
> > When an adapter or domain is removed from the host's AP configuration, the
> > AP bus invokes vfio_ap_on_cfg_changed() to notify the vfio_ap device
> > driver. For each ap_matrix_mdev object to which the adapter or domain
> > is assigned, vfio_ap_mdev_hot_unplug_cfg() is called and removes the
> > adapter or domain from the matrix_mdev->shadow_apcb (i.e., the guest's AP
> > configuration) and hot unplugs it if a guest is using it. The new host
> > AP configuration (sans adapter or domain) is then stored in
> > matrix_dev->info.
> >
> > When the AP bus subsequently unbinds the physical queue devices associated
> > with the adapter or domain that has been removed, it invokes
> > vfio_ap_mdev_remove_queue() for each queue removed. At this point, the
> > adapter or domain will no longer be assigned to the
> > matrix_mdev->shadow_apcb or the matrix_dev->info object because they would
> > have been removed by vfio_ap_on_cfg_changed(). Consequently,
> > vfio_ap_mdev_reset_queue(q) is bypassed and kfree(q) is called
> > without executing vfio_ap_free_aqic_resources(). This indefinitely pins
> > guest memory (q->saved_iova) and leaks KVM GISC resources (q->saved_isc).
> >
> > Note that resetting the queue would fail with an invalid APQN error due to
> > the fact the queue is not longer in the host's AP configuration; however,
> > it is still necessary to free the AQIC resources. The fix here is to call
> > vfio_ap_free_aqic_resources if the adapter or domain is neither in
> > matrix_mdev->shadow_apcb or matrix_dev->info.
> >
> > Fixes: b9bd10c43456d ("s390/vfio-ap: do not reset queue removed from host config")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Anthony Krowiak <akrowiak@xxxxxxxxxxxxx>
> > Reviewed-by: Matthew Rosato <mjrosato@xxxxxxxxxxxxx>
> > ---
> >   drivers/s390/crypto/vfio_ap_ops.c | 5 ++++-
> >   1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> > index 1edd0b7a3cce..bd9d239caeba 100644
> > --- a/drivers/s390/crypto/vfio_ap_ops.c
> > +++ b/drivers/s390/crypto/vfio_ap_ops.c
> > @@ -2533,12 +2533,15 @@ void vfio_ap_mdev_remove_queue(struct ap_device *apdev)
> >   	/*
> >   	 * If the queue is not in the host's AP configuration, then resetting
> >   	 * it will fail with response code 01, (APQN not valid); so, let's make
> > -	 * sure it is in the host's config.
> > +	 * sure it is in the host's config. If it is not, then free the KVM GISC
> > +	 * resources.
> >   	 */
> >   	if (test_bit_inv(apid, (unsigned long *)matrix_dev->info.apm) &&
> >   	    test_bit_inv(apqi, (unsigned long *)matrix_dev->info.aqm)) {
> >   		vfio_ap_mdev_reset_queue(q);
> >   		flush_work(&q->reset_work);
> > +	} else {
> > +		vfio_ap_free_aqic_resources(q);
> Re: the Sashiko finding for this one:
>
> [Severity: High]
> Can this still trigger a WARN_ON and leak KVM GISC and pinned memory if
> a guest shuts down or the mdev is removed before the physical queue is
> unbound?
>
> I think Sashiko has a point here...  At first I thought it was a
> pre-existing issue (just another path that fails to free resources) but
> I think this patch actually makes the described thread of execution a
> bit worse:
>
> Before this patch you would leak resources on the described path.
>
> After this patch, you will leak the resources on that path, then reach
> this new else statement and could potentially now WARN about the null
> pointer(s) in vfio_ap_free_aqic_resources() and still not unregister the
> gisc and/or unpin the saved_iova due to the null pointers.

This is a legitimate concern. I will fix this via an individual patch since
this 9/9 patch will not be included in the series for the pull request.

>