Re: [PATCH v5 1/1] s390/vfio-ap: fix circular lockdep when setting/clearing crypto masks
On 3/25/21 4:32 PM, Halil Pasic wrote:
On Thu, 25 Mar 2021 08:46:40 -0400
Tony Krowiak wrote:
This patch fixes a lockdep splat introduced by commit f21916ec4826
("s390/vfio-ap: clean up vfio_ap resources when KVM pointer invalidated").
The lockdep splat only occurs when starting a Secure Execution guest.
Crypto virtualization (vfio_ap) is not yet supported for SE guests;
however, in order to avoid this problem when support becomes available,
this fix is being provided.
The circular locking dependency was introduced when the setting of the
masks in the guest's APCB was executed while holding the matrix_dev->lock.
While the lock is definitely needed to protect the setting/unsetting of the
matrix_mdev->kvm pointer, it is not necessarily critical for setting the
masks; so, the matrix_dev->lock will be released while the masks are being
set or cleared.
Keep in mind, however, that another process that takes the matrix_dev->lock
can get control while the masks in the guest's APCB are being set or
cleared as a result of the driver being notified that the KVM pointer
has been set or unset. This could result in invalid access to the
matrix_mdev->kvm pointer by the intervening process. To avoid this
scenario, two new fields are being added to the ap_matrix_mdev struct:
struct ap_matrix_mdev {
...
bool kvm_busy;
wait_queue_head_t wait_for_kvm;
...
};
The functions that handle notification that the KVM pointer value has
been set or cleared will set the kvm_busy flag to true until they are done
processing at which time they will set it to false and wake up the tasks on
the matrix_mdev->wait_for_kvm wait queue. Functions that require
access to matrix_mdev->kvm will sleep on the wait queue until they are
awakened at which time they can safely access the matrix_mdev->kvm
field.
Fixes: f21916ec4826 ("s390/vfio-ap: clean up vfio_ap resources when KVM pointer
invalidated")
Cc: sta...@vger.kernel.org
Signed-off-by: Tony Krowiak
Reviewed-by: Halil Pasic
I intend to give a couple of work-days to others, and if nobody objects
merge this. (I will wait till Tuesday.)
Thanks Halil.
I've tested it and it does silence the lockdep splat.
Regards,
Halil
Re: [PATCH v5 1/1] s390/vfio-ap: fix circular lockdep when setting/clearing crypto masks
On Thu, 25 Mar 2021 08:46:40 -0400
Tony Krowiak wrote:
> This patch fixes a lockdep splat introduced by commit f21916ec4826
> ("s390/vfio-ap: clean up vfio_ap resources when KVM pointer invalidated").
> The lockdep splat only occurs when starting a Secure Execution guest.
> Crypto virtualization (vfio_ap) is not yet supported for SE guests;
> however, in order to avoid this problem when support becomes available,
> this fix is being provided.
>
> The circular locking dependency was introduced when the setting of the
> masks in the guest's APCB was executed while holding the matrix_dev->lock.
> While the lock is definitely needed to protect the setting/unsetting of the
> matrix_mdev->kvm pointer, it is not necessarily critical for setting the
> masks; so, the matrix_dev->lock will be released while the masks are being
> set or cleared.
>
> Keep in mind, however, that another process that takes the matrix_dev->lock
> can get control while the masks in the guest's APCB are being set or
> cleared as a result of the driver being notified that the KVM pointer
> has been set or unset. This could result in invalid access to the
> matrix_mdev->kvm pointer by the intervening process. To avoid this
> scenario, two new fields are being added to the ap_matrix_mdev struct:
>
> struct ap_matrix_mdev {
> ...
> bool kvm_busy;
> wait_queue_head_t wait_for_kvm;
>...
> };
>
> The functions that handle notification that the KVM pointer value has
> been set or cleared will set the kvm_busy flag to true until they are done
> processing at which time they will set it to false and wake up the tasks on
> the matrix_mdev->wait_for_kvm wait queue. Functions that require
> access to matrix_mdev->kvm will sleep on the wait queue until they are
> awakened at which time they can safely access the matrix_mdev->kvm
> field.
>
> Fixes: f21916ec4826 ("s390/vfio-ap: clean up vfio_ap resources when KVM
> pointer invalidated")
> Cc: sta...@vger.kernel.org
> Signed-off-by: Tony Krowiak
Reviewed-by: Halil Pasic
I intend to give a couple of work-days to others, and if nobody objects
merge this. (I will wait till Tuesday.)
I've tested it and it does silence the lockdep splat.
Regards,
Halil
[PATCH v5 1/1] s390/vfio-ap: fix circular lockdep when setting/clearing crypto masks
This patch fixes a lockdep splat introduced by commit f21916ec4826
("s390/vfio-ap: clean up vfio_ap resources when KVM pointer invalidated").
The lockdep splat only occurs when starting a Secure Execution guest.
Crypto virtualization (vfio_ap) is not yet supported for SE guests;
however, in order to avoid this problem when support becomes available,
this fix is being provided.
The circular locking dependency was introduced when the setting of the
masks in the guest's APCB was executed while holding the matrix_dev->lock.
While the lock is definitely needed to protect the setting/unsetting of the
matrix_mdev->kvm pointer, it is not necessarily critical for setting the
masks; so, the matrix_dev->lock will be released while the masks are being
set or cleared.
Keep in mind, however, that another process that takes the matrix_dev->lock
can get control while the masks in the guest's APCB are being set or
cleared as a result of the driver being notified that the KVM pointer
has been set or unset. This could result in invalid access to the
matrix_mdev->kvm pointer by the intervening process. To avoid this
scenario, two new fields are being added to the ap_matrix_mdev struct:
struct ap_matrix_mdev {
...
bool kvm_busy;
wait_queue_head_t wait_for_kvm;
...
};
The functions that handle notification that the KVM pointer value has
been set or cleared will set the kvm_busy flag to true until they are done
processing at which time they will set it to false and wake up the tasks on
the matrix_mdev->wait_for_kvm wait queue. Functions that require
access to matrix_mdev->kvm will sleep on the wait queue until they are
awakened at which time they can safely access the matrix_mdev->kvm
field.
Fixes: f21916ec4826 ("s390/vfio-ap: clean up vfio_ap resources when KVM pointer
invalidated")
Cc: sta...@vger.kernel.org
Signed-off-by: Tony Krowiak
---
drivers/s390/crypto/vfio_ap_ops.c | 309 ++
drivers/s390/crypto/vfio_ap_private.h | 2 +
2 files changed, 215 insertions(+), 96 deletions(-)
diff --git a/drivers/s390/crypto/vfio_ap_ops.c
b/drivers/s390/crypto/vfio_ap_ops.c
index 1ffdd411201c..7deb0f9bb9ee 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -294,6 +294,19 @@ static int handle_pqap(struct kvm_vcpu *vcpu)
matrix_mdev = container_of(vcpu->kvm->arch.crypto.pqap_hook,
struct ap_matrix_mdev, pqap_hook);
+ /*
+* If the KVM pointer is in the process of being set, wait until the
+* process has completed.
+*/
+ wait_event_cmd(matrix_mdev->wait_for_kvm,
+ matrix_mdev->kvm_busy == false,
+ mutex_unlock(_dev->lock),
+ mutex_lock(_dev->lock));
+
+ /* If the there is no guest using the mdev, there is nothing to do */
+ if (!matrix_mdev->kvm)
+ goto out_unlock;
+
q = vfio_ap_get_queue(matrix_mdev, apqn);
if (!q)
goto out_unlock;
@@ -337,6 +350,7 @@ static int vfio_ap_mdev_create(struct kobject *kobj, struct
mdev_device *mdev)
matrix_mdev->mdev = mdev;
vfio_ap_matrix_init(_dev->info, _mdev->matrix);
+ init_waitqueue_head(_mdev->wait_for_kvm);
mdev_set_drvdata(mdev, matrix_mdev);
matrix_mdev->pqap_hook.hook = handle_pqap;
matrix_mdev->pqap_hook.owner = THIS_MODULE;
@@ -351,17 +365,23 @@ static int vfio_ap_mdev_remove(struct mdev_device *mdev)
{
struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
- if (matrix_mdev->kvm)
+ mutex_lock(_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(_dev->lock);
return -EBUSY;
+ }
- mutex_lock(_dev->lock);
vfio_ap_mdev_reset_queues(mdev);
list_del(_mdev->node);
- mutex_unlock(_dev->lock);
-
kfree(matrix_mdev);
mdev_set_drvdata(mdev, NULL);
atomic_inc(_dev->available_instances);
+ mutex_unlock(_dev->lock);
return 0;
}
@@ -606,24 +626,31 @@ static ssize_t assign_adapter_store(struct device *dev,
struct mdev_device *mdev = mdev_from_dev(dev);
struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
- /* If the guest is running, disallow assignment of adapter */
- if (matrix_mdev->kvm)
- return -EBUSY;
+ mutex_lock(_dev->lock);
+
+ /*
+* If the KVM pointer is in flux or the guest is running, disallow
+* un-assignment of adapter
+*/
+ if (matrix_mdev->kvm_busy || matrix_mdev->kvm) {
+ ret = -EBUSY;
+ goto done;
+ }
ret = kstrtoul(buf, 0, );
if (ret)
- return ret;
+ goto done;
-
