Re: [PATCH v2] s390/vfio-ap: fix memory leak in mdev remove callback

[Halil Pasic]

On Mon, 10 May 2021 17:48:37 -0400
Tony Krowiak <akrowiak@xxxxxxxxxxxxx> wrote:

> The mdev remove callback for the vfio_ap device driver bails out with
> -EBUSY if the mdev is in use by a KVM guest. The intended purpose was
> to prevent the mdev from being removed while in use; however, returning a
> non-zero rc does not prevent removal. This could result in a memory leak
> of the resources allocated when the mdev was created. In addition, the
> KVM guest will still have access to the AP devices assigned to the mdev
> even though the mdev no longer exists.
> 
> To prevent this scenario, cleanup will be done - including unplugging the
> AP adapters, domains and control domains - regardless of whether the mdev
> is in use by a KVM guest or not.
> 
> Fixes: 258287c994de ("s390: vfio-ap: implement mediated device open callback")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Tony Krowiak <akrowiak@xxxxxxxxxxx>
> ---
>  drivers/s390/crypto/vfio_ap_ops.c | 13 ++-----------
>  1 file changed, 2 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index b2c7e10dfdcd..f90c9103dac2 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -26,6 +26,7 @@
> 
>  static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev);
>  static struct vfio_ap_queue *vfio_ap_find_queue(int apqn);
> +static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev);
> 
>  static int match_apqn(struct device *dev, const void *data)
>  {
> @@ -366,17 +367,7 @@ static int vfio_ap_mdev_remove(struct mdev_device *mdev)
>  	struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
> 
>  	mutex_lock(&matrix_dev->lock);
> -
> -	/*
> -	 * If the KVM pointer is in flux or the guest is running, disallow
> -	 * un-assignment of control domain.
> -	 */
> -	if (matrix_mdev->kvm_busy || matrix_mdev->kvm) {
> -		mutex_unlock(&matrix_dev->lock);
> -		return -EBUSY;
> -	}
> -
> -	vfio_ap_mdev_reset_queues(mdev);
> +	vfio_ap_mdev_unset_kvm(matrix_mdev);

>  	list_del(&matrix_mdev->node);
>  	kfree(matrix_mdev);

Are we at risk of handle_pqap() in arch/s390/kvm/priv.c using an
already freed pqap_hook (which is a member of the matrix_mdev pointee
that is freed just above my comment).

I'm aware of the fact that vfio_ap_mdev_unset_kvm() does a
matrix_mdev->kvm->arch.crypto.pqap_hook = NULL but that is
AFRICT not done under any lock relevant for handle_pqap(). I guess
the idea is, I guess, the check cited below 

static int handle_pqap(struct kvm_vcpu *vcpu)
[..]
        /*                                                                      
         * Verify that the hook callback is registered, lock the owner          
         * and call the hook.                                                   
         */                                                                     
        if (vcpu->kvm->arch.crypto.pqap_hook) {                                 
                if (!try_module_get(vcpu->kvm->arch.crypto.pqap_hook->owner))   
                        return -EOPNOTSUPP;                                     
                ret = vcpu->kvm->arch.crypto.pqap_hook->hook(vcpu);             
                module_put(vcpu->kvm->arch.crypto.pqap_hook->owner);            
                if (!ret && vcpu->run->s.regs.gprs[1] & 0x00ff0000)             
                        kvm_s390_set_psw_cc(vcpu, 3);                           
                return ret;                                                     
        }

is going to catch it, but I'm not sure it is guaranteed to catch it.
Opinions?

Regards,
Halil


>  	mdev_set_drvdata(mdev, NULL);