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
