Date:   Wed, 19 May 2021 21:08:15 -0400
From:   Tony Krowiak <akrowiak@...ux.ibm.com>
To:     Jason Gunthorpe <jgg@...dia.com>
Cc:     linux-s390@...r.kernel.org, linux-kernel@...r.kernel.org,
        borntraeger@...ibm.com, cohuck@...hat.com,
        pasic@...ux.vnet.ibm.com, jjherne@...ux.ibm.com,
        alex.williamson@...hat.com, kwankhede@...dia.com
Subject: Re: [PATCH v3 2/2] s390/vfio-ap: control access to PQAP(AQIC)
 interception handler



On 5/19/21 7:22 PM, Jason Gunthorpe wrote:
> On Wed, May 19, 2021 at 07:04:46PM -0400, Tony Krowiak wrote:
>>
>> On 5/19/21 12:16 PM, Jason Gunthorpe wrote:
>>> On Wed, May 19, 2021 at 11:39:21AM -0400, Tony Krowiak wrote:
>>>
>>>> @@ -287,13 +289,17 @@ static int handle_pqap(struct kvm_vcpu *vcpu)
>>>>    	if (!(vcpu->arch.sie_block->eca & ECA_AIV))
>>>>    		return -EOPNOTSUPP;
>>>> -	apqn = vcpu->run->s.regs.gprs[0] & 0xffff;
>>>> -	mutex_lock(&matrix_dev->lock);
>>>> +	rcu_read_lock();
>>>> +	pqap_module_hook = rcu_dereference(vcpu->kvm->arch.crypto.pqap_hook);
>>>> +	if (!pqap_module_hook) {
>>>> +		rcu_read_unlock();
>>>> +		goto set_status;
>>>> +	}
>>>> -	if (!vcpu->kvm->arch.crypto.pqap_hook)
>>>> -		goto out_unlock;
>>>> -	matrix_mdev = container_of(vcpu->kvm->arch.crypto.pqap_hook,
>>>> -				   struct ap_matrix_mdev, pqap_hook);
>>>> +	matrix_mdev = pqap_module_hook->data;
>>>> +	rcu_read_unlock();
>>>> +	mutex_lock(&matrix_dev->lock);
>>> The matrix_mdev pointer was extracted from the pqap_module_hook,
>>> but now there is nothing protecting it since the rcu was dropped and
>>> it gets freed in vfio_ap_mdev_remove.
>> Therein lies the rub. We can't hold the rcu_read_lock across the
>> entire time that the interception is being processed because of
>> wait conditions in the interception handler. Regardless of whether
>> the pointer to the matrix_mdev is retrieved as the container of
>> or extracted from the pqap_hook, there is nothing protecting it
>> and there appears to be no way to do so using RCU.
> RCU is a lock that should only be used for highly performance
> sensitive read work loads. It eliminates one atomic from a lock, but
> if you go on to immediately do something like try_module_get, which
> has an atomic inside, the whole thing is pointless (assuming
> try_module_get was even the right thing to do)
>
> Use a simple sleepable rwsem around the whole thing and forget about
> the module_get. Hold the write side when NULL'ing the pointer.
>
>>> And, again, module locking doesn't prevent vfio_ap_mdev_remove() from
>>> being called. None of these patches should be combining module locking
>>> with RCU.
>> Is there any other way besides user interaction with the mdev's
>> sysfs remove interface for the remove callback to get invoked?
> There are more options after my re-organizing series.
>
> But I'm not sure why you ask in response to my point about module
> locking? Module locking is not really related to sysfs.

I don't know why you keep harping on module locking. I didn't
write the original code, so I really have no idea why that was
incorporated. The question wasn't in response to module
locking, it's just something that popped into my head at the
time because based on my observations, the remove callback
did not get invoked in response to a user removing the mdev
via the sysfs interface until the mdev fd was closed by the
guest.


>
>> If I try to remove the mdev using the sysfs interface while the
>> mdev fd is still open by the guest, the remove hangs until the
>> fd is closed.
> Yes, it will wait when the vfio_device is being destroyed.
>
>> That being the case, the mdev release callback will get invoked
>> prior to the remove callback being invoked which renders this whole
>> debate moot. What am I missing here?
> AFAICT the control flow is such that release can immediately move on
> to remove on the same CPU without an additional synchronization. So
> the kfree can still race with the above.

I'd have to look into it more, but my initial thought is that this is
not true. If what I observed is true - i.e., the mdev remove callback
won't get invoked until the mdev fd is closed by the guest - then the
matrix_mdev will never get freed while the pqap_hook is not NULL
because the mdev release callback - invoked when the mdev fd is
closed - nullifies it. That is why I asked whether there is any other
way that the mdev remove callback gets invoked other than
when a user removes it via the sysfs remove attribute. Keep in mind,
the matrix_mdev is removed under the matrix_dev->lock. If retrieve
the matix_mdev in the interception handler from the matrix_dev->mdev_list
under the matrix_dev->lock - instead of from the pqap_hook or the
container_of macro - and exit the handler if it has been removed,
then I think the race issue can be avoided.

>
> But the more I look at this the wonkier it is.. The real issue is not
> the matrix_mdev, it is the whole vcpu->kvm->arch.crypto.pqap_hook

Actually, the matrix_mdev is the issue. If the handle_pqap
callback is invoked after the matrix_mdev is freed, then
because we retrieve it from the pqap_hook structure or
even if we retrieve it from the enclosing container, the
interception handler is screwed.

>
> This is nonesense too:
>
> 	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);
>
> It should have a lock around it of some kind, not a
> try_module_get. module_get is not la lock.

As I said earlier, I don't know why the author did this. My best guess
is that he wanted to ensure that the module was still loaded; otherwise,
the data structures contained therein - for example, the pqap_hook
and matrix_mdev that contains it - would be gonzo.

>
> And that lock should interact with loading the hook in the first
> place:
> 		kvm->arch.crypto.pqap_hook = &matrix_mdev->pqap_hook;
>
> And of course NULLin'g the hooke should be synchronous with the lock.
>
> There should be no owner for something like this. unregistering the
> function pointer should fully fence all access.
>
> The simple solution in sketch is just this:
>
> diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
> index 9928f785c6773a..f70386452367dd 100644
> --- a/arch/s390/kvm/priv.c
> +++ b/arch/s390/kvm/priv.c
> @@ -657,13 +657,12 @@ 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;
> +	if (down_read_trylock(&vcpu->kvm->arch.crypto.rwsem) &&
> +	    vcpu->kvm->arch.crypto.pqap_hook) {
>   		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);
> +		up_read(&vcpu->kv->arch.crypto.rwsem);
>   		return ret;
>   	}

Since reading the various review comments related to this patch,
I started working on something along the lines of that which
you propose. I will post a new set of patches, probably tomorrow.

>   	/*
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index b2c7e10dfdcdcf..64c89f6a711e94 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -352,8 +352,7 @@ static int vfio_ap_mdev_create(struct mdev_device *mdev)
>   	vfio_ap_matrix_init(&matrix_dev->info, &matrix_mdev->matrix);
>   	init_waitqueue_head(&matrix_mdev->wait_for_kvm);
>   	mdev_set_drvdata(mdev, matrix_mdev);
> -	matrix_mdev->pqap_hook.hook = handle_pqap;
> -	matrix_mdev->pqap_hook.owner = THIS_MODULE;
> +	down_write(&&vcpu->kvm->arch.crypto.rwsem);
>   	mutex_lock(&matrix_dev->lock);
>   	list_add(&matrix_mdev->node, &matrix_dev->mdev_list);
>   	mutex_unlock(&matrix_dev->lock);
> @@ -1132,7 +1131,9 @@ static int vfio_ap_mdev_set_kvm(struct ap_matrix_mdev *matrix_mdev,
>   					  matrix_mdev->matrix.aqm,
>   					  matrix_mdev->matrix.adm);
>   		mutex_lock(&matrix_dev->lock);
> +		down_write(&kvm->arch.crypto.rwsem);
>   		kvm->arch.crypto.pqap_hook = &matrix_mdev->pqap_hook;
> +		up_write(&kvm->arch.crypto.rwsem);
>   		matrix_mdev->kvm = kvm;
>   		matrix_mdev->kvm_busy = false;
>   		wake_up_all(&matrix_mdev->wait_for_kvm);
> @@ -1202,7 +1203,9 @@ static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev)
>   		kvm_arch_crypto_clear_masks(matrix_mdev->kvm);
>   		mutex_lock(&matrix_dev->lock);
>   		vfio_ap_mdev_reset_queues(matrix_mdev->mdev);
> +		down_write(&matrix_mdev->kvm->arch.crypto.rwsem);
>   		matrix_mdev->kvm->arch.crypto.pqap_hook = NULL;
> +		up_write(&matrix_mdev->kvm->arch.crypto.rwsem);
>   		kvm_put_kvm(matrix_mdev->kvm);
>   		matrix_mdev->kvm = NULL;
>   		matrix_mdev->kvm_busy = false;
>
>
> Jason

Powered by blists - more mailing lists