* [PATCH] kvm/vfio: Fix potential deadlock on vfio group_lock
@ 2023-01-20 15:05 ` Yi Liu
0 siblings, 0 replies; 26+ messages in thread
From: Yi Liu @ 2023-01-20 15:05 UTC (permalink / raw)
To: alex.williamson, pbonzini, mjrosato
Cc: yi.l.liu, jgg, kevin.tian, cohuck, farman, pmorel, borntraeger,
frankja, imbrenda, david, akrowiak, jjherne, pasic, zhenyuw,
zhi.a.wang, seanjc, linux-s390, kvm, intel-gvt-dev, intel-gfx,
linux-kernel
Currently it is possible that the final put of a KVM reference comes from
vfio during its device close operation. This occurs while the vfio group
lock is held; however, if the vfio device is still in the kvm device list,
then the following call chain could result in a deadlock:
VFIO holds group->group_lock/group_rwsem
-> kvm_put_kvm
-> kvm_destroy_vm
-> kvm_destroy_devices
-> kvm_vfio_destroy
-> kvm_vfio_file_set_kvm
-> vfio_file_set_kvm
-> try to hold group->group_lock/group_rwsem
The key function is the kvm_destroy_devices() which triggers destroy cb
of kvm_device_ops. It calls back to vfio and try to hold group_lock. So
if this path doesn't call back to vfio, this dead lock would be fixed.
Actually, there is a way for it. KVM provides another point to free the
kvm-vfio device which is the point when the device file descriptor is
closed. This can be achieved by providing the release cb instead of the
destroy cb. Also rename kvm_vfio_destroy() to be kvm_vfio_release().
/*
* Destroy is responsible for freeing dev.
*
* Destroy may be called before or after destructors are called
* on emulated I/O regions, depending on whether a reference is
* held by a vcpu or other kvm component that gets destroyed
* after the emulated I/O.
*/
void (*destroy)(struct kvm_device *dev);
/*
* Release is an alternative method to free the device. It is
* called when the device file descriptor is closed. Once
* release is called, the destroy method will not be called
* anymore as the device is removed from the device list of
* the VM. kvm->lock is held.
*/
void (*release)(struct kvm_device *dev);
Fixes: 421cfe6596f6 ("vfio: remove VFIO_GROUP_NOTIFY_SET_KVM")
Reported-by: Alex Williamson <alex.williamson@redhat.com>
Suggested-by: Kevin Tian <kevin.tian@intel.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
virt/kvm/vfio.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c
index 495ceabffe88..e94f3ea718e5 100644
--- a/virt/kvm/vfio.c
+++ b/virt/kvm/vfio.c
@@ -336,7 +336,7 @@ static int kvm_vfio_has_attr(struct kvm_device *dev,
return -ENXIO;
}
-static void kvm_vfio_destroy(struct kvm_device *dev)
+static void kvm_vfio_release(struct kvm_device *dev)
{
struct kvm_vfio *kv = dev->private;
struct kvm_vfio_group *kvg, *tmp;
@@ -363,7 +363,7 @@ static int kvm_vfio_create(struct kvm_device *dev, u32 type);
static struct kvm_device_ops kvm_vfio_ops = {
.name = "kvm-vfio",
.create = kvm_vfio_create,
- .destroy = kvm_vfio_destroy,
+ .release = kvm_vfio_release,
.set_attr = kvm_vfio_set_attr,
.has_attr = kvm_vfio_has_attr,
};
--
2.34.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [Intel-gfx] [PATCH] kvm/vfio: Fix potential deadlock on vfio group_lock
@ 2023-01-20 15:05 ` Yi Liu
0 siblings, 0 replies; 26+ messages in thread
From: Yi Liu @ 2023-01-20 15:05 UTC (permalink / raw)
To: alex.williamson, pbonzini, mjrosato
Cc: akrowiak, jjherne, yi.l.liu, frankja, pmorel, david, imbrenda,
seanjc, intel-gfx, cohuck, farman, linux-kernel, pasic, jgg, kvm,
linux-s390, borntraeger, intel-gvt-dev
Currently it is possible that the final put of a KVM reference comes from
vfio during its device close operation. This occurs while the vfio group
lock is held; however, if the vfio device is still in the kvm device list,
then the following call chain could result in a deadlock:
VFIO holds group->group_lock/group_rwsem
-> kvm_put_kvm
-> kvm_destroy_vm
-> kvm_destroy_devices
-> kvm_vfio_destroy
-> kvm_vfio_file_set_kvm
-> vfio_file_set_kvm
-> try to hold group->group_lock/group_rwsem
The key function is the kvm_destroy_devices() which triggers destroy cb
of kvm_device_ops. It calls back to vfio and try to hold group_lock. So
if this path doesn't call back to vfio, this dead lock would be fixed.
Actually, there is a way for it. KVM provides another point to free the
kvm-vfio device which is the point when the device file descriptor is
closed. This can be achieved by providing the release cb instead of the
destroy cb. Also rename kvm_vfio_destroy() to be kvm_vfio_release().
/*
* Destroy is responsible for freeing dev.
*
* Destroy may be called before or after destructors are called
* on emulated I/O regions, depending on whether a reference is
* held by a vcpu or other kvm component that gets destroyed
* after the emulated I/O.
*/
void (*destroy)(struct kvm_device *dev);
/*
* Release is an alternative method to free the device. It is
* called when the device file descriptor is closed. Once
* release is called, the destroy method will not be called
* anymore as the device is removed from the device list of
* the VM. kvm->lock is held.
*/
void (*release)(struct kvm_device *dev);
Fixes: 421cfe6596f6 ("vfio: remove VFIO_GROUP_NOTIFY_SET_KVM")
Reported-by: Alex Williamson <alex.williamson@redhat.com>
Suggested-by: Kevin Tian <kevin.tian@intel.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
virt/kvm/vfio.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c
index 495ceabffe88..e94f3ea718e5 100644
--- a/virt/kvm/vfio.c
+++ b/virt/kvm/vfio.c
@@ -336,7 +336,7 @@ static int kvm_vfio_has_attr(struct kvm_device *dev,
return -ENXIO;
}
-static void kvm_vfio_destroy(struct kvm_device *dev)
+static void kvm_vfio_release(struct kvm_device *dev)
{
struct kvm_vfio *kv = dev->private;
struct kvm_vfio_group *kvg, *tmp;
@@ -363,7 +363,7 @@ static int kvm_vfio_create(struct kvm_device *dev, u32 type);
static struct kvm_device_ops kvm_vfio_ops = {
.name = "kvm-vfio",
.create = kvm_vfio_create,
- .destroy = kvm_vfio_destroy,
+ .release = kvm_vfio_release,
.set_attr = kvm_vfio_set_attr,
.has_attr = kvm_vfio_has_attr,
};
--
2.34.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* RE: [PATCH] kvm/vfio: Fix potential deadlock on vfio group_lock
2023-01-20 15:05 ` [Intel-gfx] " Yi Liu
@ 2023-01-20 15:08 ` Liu, Yi L
-1 siblings, 0 replies; 26+ messages in thread
From: Liu, Yi L @ 2023-01-20 15:08 UTC (permalink / raw)
To: alex.williamson, pbonzini, mjrosato
Cc: jgg, Tian, Kevin, cohuck, farman, pmorel, borntraeger, frankja,
imbrenda, david, akrowiak, jjherne, pasic, zhenyuw, Wang, Zhi A,
Christopherson,,
Sean, linux-s390, kvm, intel-gvt-dev, intel-gfx, linux-kernel
> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Friday, January 20, 2023 11:05 PM
>
> Currently it is possible that the final put of a KVM reference comes from
> vfio during its device close operation. This occurs while the vfio group
> lock is held; however, if the vfio device is still in the kvm device list,
> then the following call chain could result in a deadlock:
>
> VFIO holds group->group_lock/group_rwsem
> -> kvm_put_kvm
> -> kvm_destroy_vm
> -> kvm_destroy_devices
> -> kvm_vfio_destroy
> -> kvm_vfio_file_set_kvm
> -> vfio_file_set_kvm
> -> try to hold group->group_lock/group_rwsem
>
> The key function is the kvm_destroy_devices() which triggers destroy cb
> of kvm_device_ops. It calls back to vfio and try to hold group_lock. So
> if this path doesn't call back to vfio, this dead lock would be fixed.
> Actually, there is a way for it. KVM provides another point to free the
> kvm-vfio device which is the point when the device file descriptor is
> closed. This can be achieved by providing the release cb instead of the
> destroy cb. Also rename kvm_vfio_destroy() to be kvm_vfio_release().
>
> /*
> * Destroy is responsible for freeing dev.
> *
> * Destroy may be called before or after destructors are called
> * on emulated I/O regions, depending on whether a reference is
> * held by a vcpu or other kvm component that gets destroyed
> * after the emulated I/O.
> */
> void (*destroy)(struct kvm_device *dev);
>
> /*
> * Release is an alternative method to free the device. It is
> * called when the device file descriptor is closed. Once
> * release is called, the destroy method will not be called
> * anymore as the device is removed from the device list of
> * the VM. kvm->lock is held.
> */
> void (*release)(struct kvm_device *dev);
>
> Fixes: 421cfe6596f6 ("vfio: remove VFIO_GROUP_NOTIFY_SET_KVM")
> Reported-by: Alex Williamson <alex.williamson@redhat.com>
> Suggested-by: Kevin Tian <kevin.tian@intel.com>
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
More background can be found in Mathew's work.
https://lore.kernel.org/kvm/20230114000351.115444-1-mjrosato@linux.ibm.com/T/#u
Regards,
Yi Liu
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Intel-gfx] [PATCH] kvm/vfio: Fix potential deadlock on vfio group_lock
@ 2023-01-20 15:08 ` Liu, Yi L
0 siblings, 0 replies; 26+ messages in thread
From: Liu, Yi L @ 2023-01-20 15:08 UTC (permalink / raw)
To: alex.williamson, pbonzini, mjrosato
Cc: akrowiak, jjherne, imbrenda, frankja, pmorel, david,
Christopherson, ,
Sean, intel-gfx, cohuck, farman, linux-kernel, pasic, jgg, kvm,
linux-s390, borntraeger, intel-gvt-dev
> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Friday, January 20, 2023 11:05 PM
>
> Currently it is possible that the final put of a KVM reference comes from
> vfio during its device close operation. This occurs while the vfio group
> lock is held; however, if the vfio device is still in the kvm device list,
> then the following call chain could result in a deadlock:
>
> VFIO holds group->group_lock/group_rwsem
> -> kvm_put_kvm
> -> kvm_destroy_vm
> -> kvm_destroy_devices
> -> kvm_vfio_destroy
> -> kvm_vfio_file_set_kvm
> -> vfio_file_set_kvm
> -> try to hold group->group_lock/group_rwsem
>
> The key function is the kvm_destroy_devices() which triggers destroy cb
> of kvm_device_ops. It calls back to vfio and try to hold group_lock. So
> if this path doesn't call back to vfio, this dead lock would be fixed.
> Actually, there is a way for it. KVM provides another point to free the
> kvm-vfio device which is the point when the device file descriptor is
> closed. This can be achieved by providing the release cb instead of the
> destroy cb. Also rename kvm_vfio_destroy() to be kvm_vfio_release().
>
> /*
> * Destroy is responsible for freeing dev.
> *
> * Destroy may be called before or after destructors are called
> * on emulated I/O regions, depending on whether a reference is
> * held by a vcpu or other kvm component that gets destroyed
> * after the emulated I/O.
> */
> void (*destroy)(struct kvm_device *dev);
>
> /*
> * Release is an alternative method to free the device. It is
> * called when the device file descriptor is closed. Once
> * release is called, the destroy method will not be called
> * anymore as the device is removed from the device list of
> * the VM. kvm->lock is held.
> */
> void (*release)(struct kvm_device *dev);
>
> Fixes: 421cfe6596f6 ("vfio: remove VFIO_GROUP_NOTIFY_SET_KVM")
> Reported-by: Alex Williamson <alex.williamson@redhat.com>
> Suggested-by: Kevin Tian <kevin.tian@intel.com>
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
More background can be found in Mathew's work.
https://lore.kernel.org/kvm/20230114000351.115444-1-mjrosato@linux.ibm.com/T/#u
Regards,
Yi Liu
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] kvm/vfio: Fix potential deadlock on vfio group_lock
2023-01-20 15:08 ` [Intel-gfx] " Liu, Yi L
@ 2023-01-20 15:45 ` Matthew Rosato
-1 siblings, 0 replies; 26+ messages in thread
From: Matthew Rosato @ 2023-01-20 15:45 UTC (permalink / raw)
To: Liu, Yi L, alex.williamson, pbonzini
Cc: jgg, Tian, Kevin, cohuck, farman, pmorel, borntraeger, frankja,
imbrenda, david, akrowiak, jjherne, pasic, zhenyuw, Wang, Zhi A,
Christopherson, ,
Sean, linux-s390, kvm, intel-gvt-dev, intel-gfx, linux-kernel
On 1/20/23 10:08 AM, Liu, Yi L wrote:
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Sent: Friday, January 20, 2023 11:05 PM
>>
>> Currently it is possible that the final put of a KVM reference comes from
>> vfio during its device close operation. This occurs while the vfio group
>> lock is held; however, if the vfio device is still in the kvm device list,
>> then the following call chain could result in a deadlock:
>>
>> VFIO holds group->group_lock/group_rwsem
>> -> kvm_put_kvm
>> -> kvm_destroy_vm
>> -> kvm_destroy_devices
>> -> kvm_vfio_destroy
>> -> kvm_vfio_file_set_kvm
>> -> vfio_file_set_kvm
>> -> try to hold group->group_lock/group_rwsem
>>
>> The key function is the kvm_destroy_devices() which triggers destroy cb
>> of kvm_device_ops. It calls back to vfio and try to hold group_lock. So
>> if this path doesn't call back to vfio, this dead lock would be fixed.
>> Actually, there is a way for it. KVM provides another point to free the
>> kvm-vfio device which is the point when the device file descriptor is
>> closed. This can be achieved by providing the release cb instead of the
>> destroy cb. Also rename kvm_vfio_destroy() to be kvm_vfio_release().
>>
>> /*
>> * Destroy is responsible for freeing dev.
>> *
>> * Destroy may be called before or after destructors are called
>> * on emulated I/O regions, depending on whether a reference is
>> * held by a vcpu or other kvm component that gets destroyed
>> * after the emulated I/O.
>> */
>> void (*destroy)(struct kvm_device *dev);
>>
>> /*
>> * Release is an alternative method to free the device. It is
>> * called when the device file descriptor is closed. Once
>> * release is called, the destroy method will not be called
>> * anymore as the device is removed from the device list of
>> * the VM. kvm->lock is held.
>> */
>> void (*release)(struct kvm_device *dev);
>>
>> Fixes: 421cfe6596f6 ("vfio: remove VFIO_GROUP_NOTIFY_SET_KVM")
>> Reported-by: Alex Williamson <alex.williamson@redhat.com>
>> Suggested-by: Kevin Tian <kevin.tian@intel.com>
>> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>
> More background can be found in Mathew's work.
> https://lore.kernel.org/kvm/20230114000351.115444-1-mjrosato@linux.ibm.com/T/#u
>
Thanks Yi.
Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
One small nit: There is a comment at the very end of kvm_vfio_release on the kfree(dev) that still references .destroy, this should be updated to .release
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Intel-gfx] [PATCH] kvm/vfio: Fix potential deadlock on vfio group_lock
@ 2023-01-20 15:45 ` Matthew Rosato
0 siblings, 0 replies; 26+ messages in thread
From: Matthew Rosato @ 2023-01-20 15:45 UTC (permalink / raw)
To: Liu, Yi L, alex.williamson, pbonzini
Cc: akrowiak, jjherne, imbrenda, frankja, pmorel, david,
Christopherson, ,
Sean, intel-gfx, cohuck, farman, linux-kernel, pasic, jgg, kvm,
linux-s390, borntraeger, intel-gvt-dev
On 1/20/23 10:08 AM, Liu, Yi L wrote:
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Sent: Friday, January 20, 2023 11:05 PM
>>
>> Currently it is possible that the final put of a KVM reference comes from
>> vfio during its device close operation. This occurs while the vfio group
>> lock is held; however, if the vfio device is still in the kvm device list,
>> then the following call chain could result in a deadlock:
>>
>> VFIO holds group->group_lock/group_rwsem
>> -> kvm_put_kvm
>> -> kvm_destroy_vm
>> -> kvm_destroy_devices
>> -> kvm_vfio_destroy
>> -> kvm_vfio_file_set_kvm
>> -> vfio_file_set_kvm
>> -> try to hold group->group_lock/group_rwsem
>>
>> The key function is the kvm_destroy_devices() which triggers destroy cb
>> of kvm_device_ops. It calls back to vfio and try to hold group_lock. So
>> if this path doesn't call back to vfio, this dead lock would be fixed.
>> Actually, there is a way for it. KVM provides another point to free the
>> kvm-vfio device which is the point when the device file descriptor is
>> closed. This can be achieved by providing the release cb instead of the
>> destroy cb. Also rename kvm_vfio_destroy() to be kvm_vfio_release().
>>
>> /*
>> * Destroy is responsible for freeing dev.
>> *
>> * Destroy may be called before or after destructors are called
>> * on emulated I/O regions, depending on whether a reference is
>> * held by a vcpu or other kvm component that gets destroyed
>> * after the emulated I/O.
>> */
>> void (*destroy)(struct kvm_device *dev);
>>
>> /*
>> * Release is an alternative method to free the device. It is
>> * called when the device file descriptor is closed. Once
>> * release is called, the destroy method will not be called
>> * anymore as the device is removed from the device list of
>> * the VM. kvm->lock is held.
>> */
>> void (*release)(struct kvm_device *dev);
>>
>> Fixes: 421cfe6596f6 ("vfio: remove VFIO_GROUP_NOTIFY_SET_KVM")
>> Reported-by: Alex Williamson <alex.williamson@redhat.com>
>> Suggested-by: Kevin Tian <kevin.tian@intel.com>
>> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>
> More background can be found in Mathew's work.
> https://lore.kernel.org/kvm/20230114000351.115444-1-mjrosato@linux.ibm.com/T/#u
>
Thanks Yi.
Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
One small nit: There is a comment at the very end of kvm_vfio_release on the kfree(dev) that still references .destroy, this should be updated to .release
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Intel-gfx] [PATCH] kvm/vfio: Fix potential deadlock on vfio group_lock
2023-01-20 15:45 ` [Intel-gfx] " Matthew Rosato
@ 2023-01-20 15:49 ` Alex Williamson
-1 siblings, 0 replies; 26+ messages in thread
From: Alex Williamson @ 2023-01-20 15:49 UTC (permalink / raw)
To: Matthew Rosato
Cc: kvm, david, farman, imbrenda, linux-s390, Liu, Yi L, frankja,
pasic, jgg, borntraeger, jjherne, intel-gfx, intel-gvt-dev,
akrowiak, pmorel, Christopherson, ,
Sean, cohuck, linux-kernel, pbonzini
On Fri, 20 Jan 2023 10:45:40 -0500
Matthew Rosato <mjrosato@linux.ibm.com> wrote:
> On 1/20/23 10:08 AM, Liu, Yi L wrote:
> >> From: Liu, Yi L <yi.l.liu@intel.com>
> >> Sent: Friday, January 20, 2023 11:05 PM
> >>
> >> Currently it is possible that the final put of a KVM reference comes from
> >> vfio during its device close operation. This occurs while the vfio group
> >> lock is held; however, if the vfio device is still in the kvm device list,
> >> then the following call chain could result in a deadlock:
> >>
> >> VFIO holds group->group_lock/group_rwsem
> >> -> kvm_put_kvm
> >> -> kvm_destroy_vm
> >> -> kvm_destroy_devices
> >> -> kvm_vfio_destroy
> >> -> kvm_vfio_file_set_kvm
> >> -> vfio_file_set_kvm
> >> -> try to hold group->group_lock/group_rwsem
> >>
> >> The key function is the kvm_destroy_devices() which triggers destroy cb
> >> of kvm_device_ops. It calls back to vfio and try to hold group_lock. So
> >> if this path doesn't call back to vfio, this dead lock would be fixed.
> >> Actually, there is a way for it. KVM provides another point to free the
> >> kvm-vfio device which is the point when the device file descriptor is
> >> closed. This can be achieved by providing the release cb instead of the
> >> destroy cb. Also rename kvm_vfio_destroy() to be kvm_vfio_release().
> >>
> >> /*
> >> * Destroy is responsible for freeing dev.
> >> *
> >> * Destroy may be called before or after destructors are called
> >> * on emulated I/O regions, depending on whether a reference is
> >> * held by a vcpu or other kvm component that gets destroyed
> >> * after the emulated I/O.
> >> */
> >> void (*destroy)(struct kvm_device *dev);
> >>
> >> /*
> >> * Release is an alternative method to free the device. It is
> >> * called when the device file descriptor is closed. Once
> >> * release is called, the destroy method will not be called
> >> * anymore as the device is removed from the device list of
> >> * the VM. kvm->lock is held.
> >> */
> >> void (*release)(struct kvm_device *dev);
> >>
> >> Fixes: 421cfe6596f6 ("vfio: remove VFIO_GROUP_NOTIFY_SET_KVM")
> >> Reported-by: Alex Williamson <alex.williamson@redhat.com>
> >> Suggested-by: Kevin Tian <kevin.tian@intel.com>
> >> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> >> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> >
> > More background can be found in Mathew's work.
> > https://lore.kernel.org/kvm/20230114000351.115444-1-mjrosato@linux.ibm.com/T/#u
> >
>
> Thanks Yi.
>
> Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
>
> One small nit: There is a comment at the very end of
> kvm_vfio_release on the kfree(dev) that still references .destroy,
> this should be updated to .release
I've fixed this locally, s/destroy/release/ in that comment. Thanks,
Alex
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] kvm/vfio: Fix potential deadlock on vfio group_lock
@ 2023-01-20 15:49 ` Alex Williamson
0 siblings, 0 replies; 26+ messages in thread
From: Alex Williamson @ 2023-01-20 15:49 UTC (permalink / raw)
To: Matthew Rosato
Cc: Liu, Yi L, pbonzini, jgg, Tian, Kevin, cohuck, farman, pmorel,
borntraeger, frankja, imbrenda, david, akrowiak, jjherne, pasic,
zhenyuw, Wang, Zhi A, Christopherson, ,
Sean, linux-s390, kvm, intel-gvt-dev, intel-gfx, linux-kernel
On Fri, 20 Jan 2023 10:45:40 -0500
Matthew Rosato <mjrosato@linux.ibm.com> wrote:
> On 1/20/23 10:08 AM, Liu, Yi L wrote:
> >> From: Liu, Yi L <yi.l.liu@intel.com>
> >> Sent: Friday, January 20, 2023 11:05 PM
> >>
> >> Currently it is possible that the final put of a KVM reference comes from
> >> vfio during its device close operation. This occurs while the vfio group
> >> lock is held; however, if the vfio device is still in the kvm device list,
> >> then the following call chain could result in a deadlock:
> >>
> >> VFIO holds group->group_lock/group_rwsem
> >> -> kvm_put_kvm
> >> -> kvm_destroy_vm
> >> -> kvm_destroy_devices
> >> -> kvm_vfio_destroy
> >> -> kvm_vfio_file_set_kvm
> >> -> vfio_file_set_kvm
> >> -> try to hold group->group_lock/group_rwsem
> >>
> >> The key function is the kvm_destroy_devices() which triggers destroy cb
> >> of kvm_device_ops. It calls back to vfio and try to hold group_lock. So
> >> if this path doesn't call back to vfio, this dead lock would be fixed.
> >> Actually, there is a way for it. KVM provides another point to free the
> >> kvm-vfio device which is the point when the device file descriptor is
> >> closed. This can be achieved by providing the release cb instead of the
> >> destroy cb. Also rename kvm_vfio_destroy() to be kvm_vfio_release().
> >>
> >> /*
> >> * Destroy is responsible for freeing dev.
> >> *
> >> * Destroy may be called before or after destructors are called
> >> * on emulated I/O regions, depending on whether a reference is
> >> * held by a vcpu or other kvm component that gets destroyed
> >> * after the emulated I/O.
> >> */
> >> void (*destroy)(struct kvm_device *dev);
> >>
> >> /*
> >> * Release is an alternative method to free the device. It is
> >> * called when the device file descriptor is closed. Once
> >> * release is called, the destroy method will not be called
> >> * anymore as the device is removed from the device list of
> >> * the VM. kvm->lock is held.
> >> */
> >> void (*release)(struct kvm_device *dev);
> >>
> >> Fixes: 421cfe6596f6 ("vfio: remove VFIO_GROUP_NOTIFY_SET_KVM")
> >> Reported-by: Alex Williamson <alex.williamson@redhat.com>
> >> Suggested-by: Kevin Tian <kevin.tian@intel.com>
> >> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> >> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> >
> > More background can be found in Mathew's work.
> > https://lore.kernel.org/kvm/20230114000351.115444-1-mjrosato@linux.ibm.com/T/#u
> >
>
> Thanks Yi.
>
> Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
>
> One small nit: There is a comment at the very end of
> kvm_vfio_release on the kfree(dev) that still references .destroy,
> this should be updated to .release
I've fixed this locally, s/destroy/release/ in that comment. Thanks,
Alex
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] kvm/vfio: Fix potential deadlock on vfio group_lock
2023-01-20 15:05 ` [Intel-gfx] " Yi Liu
@ 2023-01-20 17:58 ` Alex Williamson
-1 siblings, 0 replies; 26+ messages in thread
From: Alex Williamson @ 2023-01-20 17:58 UTC (permalink / raw)
To: Yi Liu
Cc: pbonzini, mjrosato, jgg, kevin.tian, cohuck, farman, pmorel,
borntraeger, frankja, imbrenda, david, akrowiak, jjherne, pasic,
zhenyuw, zhi.a.wang, seanjc, linux-s390, kvm, intel-gvt-dev,
intel-gfx, linux-kernel
On Fri, 20 Jan 2023 07:05:28 -0800
Yi Liu <yi.l.liu@intel.com> wrote:
> Currently it is possible that the final put of a KVM reference comes from
> vfio during its device close operation. This occurs while the vfio group
> lock is held; however, if the vfio device is still in the kvm device list,
> then the following call chain could result in a deadlock:
>
> VFIO holds group->group_lock/group_rwsem
> -> kvm_put_kvm
> -> kvm_destroy_vm
> -> kvm_destroy_devices
> -> kvm_vfio_destroy
> -> kvm_vfio_file_set_kvm
> -> vfio_file_set_kvm
> -> try to hold group->group_lock/group_rwsem
>
> The key function is the kvm_destroy_devices() which triggers destroy cb
> of kvm_device_ops. It calls back to vfio and try to hold group_lock. So
> if this path doesn't call back to vfio, this dead lock would be fixed.
> Actually, there is a way for it. KVM provides another point to free the
> kvm-vfio device which is the point when the device file descriptor is
> closed. This can be achieved by providing the release cb instead of the
> destroy cb. Also rename kvm_vfio_destroy() to be kvm_vfio_release().
>
> /*
> * Destroy is responsible for freeing dev.
> *
> * Destroy may be called before or after destructors are called
> * on emulated I/O regions, depending on whether a reference is
> * held by a vcpu or other kvm component that gets destroyed
> * after the emulated I/O.
> */
> void (*destroy)(struct kvm_device *dev);
>
> /*
> * Release is an alternative method to free the device. It is
> * called when the device file descriptor is closed. Once
> * release is called, the destroy method will not be called
> * anymore as the device is removed from the device list of
> * the VM. kvm->lock is held.
> */
> void (*release)(struct kvm_device *dev);
>
> Fixes: 421cfe6596f6 ("vfio: remove VFIO_GROUP_NOTIFY_SET_KVM")
> Reported-by: Alex Williamson <alex.williamson@redhat.com>
> Suggested-by: Kevin Tian <kevin.tian@intel.com>
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> ---
> virt/kvm/vfio.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c
> index 495ceabffe88..e94f3ea718e5 100644
> --- a/virt/kvm/vfio.c
> +++ b/virt/kvm/vfio.c
> @@ -336,7 +336,7 @@ static int kvm_vfio_has_attr(struct kvm_device *dev,
> return -ENXIO;
> }
>
> -static void kvm_vfio_destroy(struct kvm_device *dev)
> +static void kvm_vfio_release(struct kvm_device *dev)
> {
> struct kvm_vfio *kv = dev->private;
> struct kvm_vfio_group *kvg, *tmp;
> @@ -363,7 +363,7 @@ static int kvm_vfio_create(struct kvm_device *dev, u32 type);
> static struct kvm_device_ops kvm_vfio_ops = {
> .name = "kvm-vfio",
> .create = kvm_vfio_create,
> - .destroy = kvm_vfio_destroy,
> + .release = kvm_vfio_release,
> .set_attr = kvm_vfio_set_attr,
> .has_attr = kvm_vfio_has_attr,
> };
Applied to vfio for-linus branch for v6.2, along with Matthew's R-b,
the comment update, and the extra reference link. Once we get a
linux-next build I'll send a pull request, along with Matthew's
reserved region fix. Thanks,
Alex
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Intel-gfx] [PATCH] kvm/vfio: Fix potential deadlock on vfio group_lock
@ 2023-01-20 17:58 ` Alex Williamson
0 siblings, 0 replies; 26+ messages in thread
From: Alex Williamson @ 2023-01-20 17:58 UTC (permalink / raw)
To: Yi Liu
Cc: mjrosato, david, imbrenda, linux-s390, kvm, pasic, jgg,
borntraeger, jjherne, farman, intel-gfx, intel-gvt-dev, frankja,
akrowiak, pmorel, seanjc, cohuck, linux-kernel, pbonzini
On Fri, 20 Jan 2023 07:05:28 -0800
Yi Liu <yi.l.liu@intel.com> wrote:
> Currently it is possible that the final put of a KVM reference comes from
> vfio during its device close operation. This occurs while the vfio group
> lock is held; however, if the vfio device is still in the kvm device list,
> then the following call chain could result in a deadlock:
>
> VFIO holds group->group_lock/group_rwsem
> -> kvm_put_kvm
> -> kvm_destroy_vm
> -> kvm_destroy_devices
> -> kvm_vfio_destroy
> -> kvm_vfio_file_set_kvm
> -> vfio_file_set_kvm
> -> try to hold group->group_lock/group_rwsem
>
> The key function is the kvm_destroy_devices() which triggers destroy cb
> of kvm_device_ops. It calls back to vfio and try to hold group_lock. So
> if this path doesn't call back to vfio, this dead lock would be fixed.
> Actually, there is a way for it. KVM provides another point to free the
> kvm-vfio device which is the point when the device file descriptor is
> closed. This can be achieved by providing the release cb instead of the
> destroy cb. Also rename kvm_vfio_destroy() to be kvm_vfio_release().
>
> /*
> * Destroy is responsible for freeing dev.
> *
> * Destroy may be called before or after destructors are called
> * on emulated I/O regions, depending on whether a reference is
> * held by a vcpu or other kvm component that gets destroyed
> * after the emulated I/O.
> */
> void (*destroy)(struct kvm_device *dev);
>
> /*
> * Release is an alternative method to free the device. It is
> * called when the device file descriptor is closed. Once
> * release is called, the destroy method will not be called
> * anymore as the device is removed from the device list of
> * the VM. kvm->lock is held.
> */
> void (*release)(struct kvm_device *dev);
>
> Fixes: 421cfe6596f6 ("vfio: remove VFIO_GROUP_NOTIFY_SET_KVM")
> Reported-by: Alex Williamson <alex.williamson@redhat.com>
> Suggested-by: Kevin Tian <kevin.tian@intel.com>
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> ---
> virt/kvm/vfio.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c
> index 495ceabffe88..e94f3ea718e5 100644
> --- a/virt/kvm/vfio.c
> +++ b/virt/kvm/vfio.c
> @@ -336,7 +336,7 @@ static int kvm_vfio_has_attr(struct kvm_device *dev,
> return -ENXIO;
> }
>
> -static void kvm_vfio_destroy(struct kvm_device *dev)
> +static void kvm_vfio_release(struct kvm_device *dev)
> {
> struct kvm_vfio *kv = dev->private;
> struct kvm_vfio_group *kvg, *tmp;
> @@ -363,7 +363,7 @@ static int kvm_vfio_create(struct kvm_device *dev, u32 type);
> static struct kvm_device_ops kvm_vfio_ops = {
> .name = "kvm-vfio",
> .create = kvm_vfio_create,
> - .destroy = kvm_vfio_destroy,
> + .release = kvm_vfio_release,
> .set_attr = kvm_vfio_set_attr,
> .has_attr = kvm_vfio_has_attr,
> };
Applied to vfio for-linus branch for v6.2, along with Matthew's R-b,
the comment update, and the extra reference link. Once we get a
linux-next build I'll send a pull request, along with Matthew's
reserved region fix. Thanks,
Alex
^ permalink raw reply [flat|nested] 26+ messages in thread
* [Intel-gfx] ✓ Fi.CI.BAT: success for kvm/vfio: Fix potential deadlock on vfio group_lock
2023-01-20 15:05 ` [Intel-gfx] " Yi Liu
` (2 preceding siblings ...)
(?)
@ 2023-01-20 19:25 ` Patchwork
-1 siblings, 0 replies; 26+ messages in thread
From: Patchwork @ 2023-01-20 19:25 UTC (permalink / raw)
To: Liu, Yi L; +Cc: intel-gfx
[-- Attachment #1: Type: text/plain, Size: 3328 bytes --]
== Series Details ==
Series: kvm/vfio: Fix potential deadlock on vfio group_lock
URL : https://patchwork.freedesktop.org/series/113156/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_12618 -> Patchwork_113156v1
====================================================
Summary
-------
**SUCCESS**
No regressions found.
External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113156v1/index.html
Participating hosts (36 -> 36)
------------------------------
Additional (1): fi-bsw-kefka
Missing (1): fi-snb-2520m
Known issues
------------
Here are the changes found in Patchwork_113156v1 that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@prime_vgem@basic-fence-flip:
- fi-bsw-kefka: NOTRUN -> [SKIP][1] ([fdo#109271]) +26 similar issues
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113156v1/fi-bsw-kefka/igt@prime_vgem@basic-fence-flip.html
#### Possible fixes ####
* igt@i915_selftest@live@gt_heartbeat:
- {bat-jsl-1}: [DMESG-FAIL][2] ([i915#5334]) -> [PASS][3]
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12618/bat-jsl-1/igt@i915_selftest@live@gt_heartbeat.html
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113156v1/bat-jsl-1/igt@i915_selftest@live@gt_heartbeat.html
* igt@i915_selftest@live@slpc:
- {bat-rpls-1}: [DMESG-FAIL][4] ([i915#6367]) -> [PASS][5]
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12618/bat-rpls-1/igt@i915_selftest@live@slpc.html
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113156v1/bat-rpls-1/igt@i915_selftest@live@slpc.html
* igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions:
- fi-bsw-n3050: [FAIL][6] ([i915#6298]) -> [PASS][7]
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12618/fi-bsw-n3050/igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions.html
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113156v1/fi-bsw-n3050/igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions.html
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
[i915#4817]: https://gitlab.freedesktop.org/drm/intel/issues/4817
[i915#5334]: https://gitlab.freedesktop.org/drm/intel/issues/5334
[i915#5591]: https://gitlab.freedesktop.org/drm/intel/issues/5591
[i915#6298]: https://gitlab.freedesktop.org/drm/intel/issues/6298
[i915#6367]: https://gitlab.freedesktop.org/drm/intel/issues/6367
[i915#7443]: https://gitlab.freedesktop.org/drm/intel/issues/7443
Build changes
-------------
* Linux: CI_DRM_12618 -> Patchwork_113156v1
CI-20190529: 20190529
CI_DRM_12618: 7ba8ff20ba23bc940e928ffe3a9054225fff418e @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_7129: 7816773163a1b0d248dd9dd34d14e632ad8903be @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
Patchwork_113156v1: 7ba8ff20ba23bc940e928ffe3a9054225fff418e @ git://anongit.freedesktop.org/gfx-ci/linux
### Linux commits
b10222112eb6 kvm/vfio: Fix potential deadlock on vfio group_lock
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113156v1/index.html
[-- Attachment #2: Type: text/html, Size: 3826 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* [Intel-gfx] ✓ Fi.CI.IGT: success for kvm/vfio: Fix potential deadlock on vfio group_lock
2023-01-20 15:05 ` [Intel-gfx] " Yi Liu
` (3 preceding siblings ...)
(?)
@ 2023-01-21 20:16 ` Patchwork
-1 siblings, 0 replies; 26+ messages in thread
From: Patchwork @ 2023-01-21 20:16 UTC (permalink / raw)
To: Liu, Yi L; +Cc: intel-gfx
[-- Attachment #1: Type: text/plain, Size: 22489 bytes --]
== Series Details ==
Series: kvm/vfio: Fix potential deadlock on vfio group_lock
URL : https://patchwork.freedesktop.org/series/113156/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_12618_full -> Patchwork_113156v1_full
====================================================
Summary
-------
**SUCCESS**
No regressions found.
External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113156v1/index.html
Participating hosts (12 -> 10)
------------------------------
Missing (2): pig-skl-6260u pig-kbl-iris
Known issues
------------
Here are the changes found in Patchwork_113156v1_full that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@gem_exec_fair@basic-none@vcs0:
- shard-glk: [PASS][1] -> [FAIL][2] ([i915#2842])
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12618/shard-glk2/igt@gem_exec_fair@basic-none@vcs0.html
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113156v1/shard-glk9/igt@gem_exec_fair@basic-none@vcs0.html
* igt@gem_lmem_swapping@heavy-verify-multi:
- shard-glk: NOTRUN -> [SKIP][3] ([fdo#109271] / [i915#4613])
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113156v1/shard-glk9/igt@gem_lmem_swapping@heavy-verify-multi.html
* igt@gem_userptr_blits@vma-merge:
- shard-glk: NOTRUN -> [FAIL][4] ([i915#3318])
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113156v1/shard-glk9/igt@gem_userptr_blits@vma-merge.html
* igt@kms_ccs@pipe-a-ccs-on-another-bo-y_tiled_gen12_rc_ccs_cc:
- shard-glk: NOTRUN -> [SKIP][5] ([fdo#109271] / [i915#3886]) +1 similar issue
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113156v1/shard-glk9/igt@kms_ccs@pipe-a-ccs-on-another-bo-y_tiled_gen12_rc_ccs_cc.html
* igt@kms_cdclk@mode-transition:
- shard-glk: NOTRUN -> [SKIP][6] ([fdo#109271]) +46 similar issues
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113156v1/shard-glk9/igt@kms_cdclk@mode-transition.html
* igt@kms_cursor_legacy@flip-vs-cursor@atomic-transitions:
- shard-glk: [PASS][7] -> [FAIL][8] ([i915#2346])
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12618/shard-glk6/igt@kms_cursor_legacy@flip-vs-cursor@atomic-transitions.html
[8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113156v1/shard-glk3/igt@kms_cursor_legacy@flip-vs-cursor@atomic-transitions.html
* igt@kms_dsc@dsc-with-bpc-formats:
- shard-glk: NOTRUN -> [SKIP][9] ([fdo#109271] / [i915#7205])
[9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113156v1/shard-glk9/igt@kms_dsc@dsc-with-bpc-formats.html
* igt@kms_flip@2x-flip-vs-expired-vblank-interruptible@ab-hdmi-a1-hdmi-a2:
- shard-glk: [PASS][10] -> [FAIL][11] ([i915#79])
[10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12618/shard-glk3/igt@kms_flip@2x-flip-vs-expired-vblank-interruptible@ab-hdmi-a1-hdmi-a2.html
[11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113156v1/shard-glk5/igt@kms_flip@2x-flip-vs-expired-vblank-interruptible@ab-hdmi-a1-hdmi-a2.html
* igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-mmap-wc:
- shard-glk: [PASS][12] -> [DMESG-FAIL][13] ([i915#118])
[12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12618/shard-glk7/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-mmap-wc.html
[13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113156v1/shard-glk2/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-mmap-wc.html
* igt@kms_psr2_sf@cursor-plane-move-continuous-exceed-fully-sf:
- shard-glk: NOTRUN -> [SKIP][14] ([fdo#109271] / [i915#658])
[14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113156v1/shard-glk9/igt@kms_psr2_sf@cursor-plane-move-continuous-exceed-fully-sf.html
* igt@sysfs_clients@sema-50:
- shard-glk: NOTRUN -> [SKIP][15] ([fdo#109271] / [i915#2994])
[15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113156v1/shard-glk9/igt@sysfs_clients@sema-50.html
#### Possible fixes ####
* igt@drm_fdinfo@virtual-idle:
- {shard-rkl}: [FAIL][16] ([i915#7742]) -> [PASS][17] +2 similar issues
[16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12618/shard-rkl-6/igt@drm_fdinfo@virtual-idle.html
[17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113156v1/shard-rkl-2/igt@drm_fdinfo@virtual-idle.html
* igt@gem_eio@suspend:
- {shard-rkl}: [FAIL][18] ([i915#7052]) -> [PASS][19]
[18]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12618/shard-rkl-4/igt@gem_eio@suspend.html
[19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113156v1/shard-rkl-2/igt@gem_eio@suspend.html
* igt@gem_exec_fair@basic-pace-share@rcs0:
- shard-glk: [FAIL][20] ([i915#2842]) -> [PASS][21]
[20]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12618/shard-glk8/igt@gem_exec_fair@basic-pace-share@rcs0.html
[21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113156v1/shard-glk1/igt@gem_exec_fair@basic-pace-share@rcs0.html
* igt@gem_exec_fair@basic-pace@rcs0:
- {shard-rkl}: [FAIL][22] ([i915#2842]) -> [PASS][23] +2 similar issues
[22]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12618/shard-rkl-1/igt@gem_exec_fair@basic-pace@rcs0.html
[23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113156v1/shard-rkl-5/igt@gem_exec_fair@basic-pace@rcs0.html
* igt@gem_exec_reloc@basic-write-read-active:
- {shard-rkl}: [SKIP][24] ([i915#3281]) -> [PASS][25] +4 similar issues
[24]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12618/shard-rkl-1/igt@gem_exec_reloc@basic-write-read-active.html
[25]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113156v1/shard-rkl-5/igt@gem_exec_reloc@basic-write-read-active.html
* igt@gem_exec_schedule@semaphore-power:
- {shard-rkl}: [SKIP][26] ([i915#7276]) -> [PASS][27]
[26]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12618/shard-rkl-1/igt@gem_exec_schedule@semaphore-power.html
[27]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113156v1/shard-rkl-5/igt@gem_exec_schedule@semaphore-power.html
* igt@gem_mmap_wc@set-cache-level:
- {shard-tglu}: [SKIP][28] ([i915#1850]) -> [PASS][29]
[28]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12618/shard-tglu-6/igt@gem_mmap_wc@set-cache-level.html
[29]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113156v1/shard-tglu-5/igt@gem_mmap_wc@set-cache-level.html
* igt@gem_partial_pwrite_pread@writes-after-reads-snoop:
- {shard-rkl}: [SKIP][30] ([i915#3282]) -> [PASS][31] +1 similar issue
[30]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12618/shard-rkl-1/igt@gem_partial_pwrite_pread@writes-after-reads-snoop.html
[31]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113156v1/shard-rkl-5/igt@gem_partial_pwrite_pread@writes-after-reads-snoop.html
* igt@gen9_exec_parse@valid-registers:
- {shard-rkl}: [SKIP][32] ([i915#2527]) -> [PASS][33]
[32]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12618/shard-rkl-1/igt@gen9_exec_parse@valid-registers.html
[33]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113156v1/shard-rkl-5/igt@gen9_exec_parse@valid-registers.html
* igt@i915_pm_rpm@system-suspend-modeset:
- {shard-rkl}: [SKIP][34] ([fdo#109308]) -> [PASS][35]
[34]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12618/shard-rkl-3/igt@i915_pm_rpm@system-suspend-modeset.html
[35]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113156v1/shard-rkl-6/igt@i915_pm_rpm@system-suspend-modeset.html
* igt@i915_pm_sseu@full-enable:
- {shard-rkl}: [SKIP][36] ([i915#4387]) -> [PASS][37]
[36]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12618/shard-rkl-1/igt@i915_pm_sseu@full-enable.html
[37]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113156v1/shard-rkl-5/igt@i915_pm_sseu@full-enable.html
* igt@kms_atomic@atomic_plane_damage:
- {shard-rkl}: [SKIP][38] ([i915#4098]) -> [PASS][39]
[38]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12618/shard-rkl-5/igt@kms_atomic@atomic_plane_damage.html
[39]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113156v1/shard-rkl-6/igt@kms_atomic@atomic_plane_damage.html
* igt@kms_big_fb@y-tiled-max-hw-stride-64bpp-rotate-0:
- {shard-tglu}: [SKIP][40] ([i915#7651]) -> [PASS][41] +8 similar issues
[40]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12618/shard-tglu-6/igt@kms_big_fb@y-tiled-max-hw-stride-64bpp-rotate-0.html
[41]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113156v1/shard-tglu-5/igt@kms_big_fb@y-tiled-max-hw-stride-64bpp-rotate-0.html
* igt@kms_frontbuffer_tracking@fbc-1p-primscrn-pri-shrfb-draw-mmap-cpu:
- {shard-tglu}: [SKIP][42] ([i915#1849]) -> [PASS][43]
[42]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12618/shard-tglu-6/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-pri-shrfb-draw-mmap-cpu.html
[43]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113156v1/shard-tglu-5/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-pri-shrfb-draw-mmap-cpu.html
* igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-mmap-gtt:
- {shard-rkl}: [SKIP][44] ([i915#1849] / [i915#4098]) -> [PASS][45] +9 similar issues
[44]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12618/shard-rkl-5/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-mmap-gtt.html
[45]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113156v1/shard-rkl-6/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-mmap-gtt.html
* igt@kms_plane@pixel-format@pipe-b-planes:
- {shard-rkl}: [SKIP][46] ([i915#1849]) -> [PASS][47] +3 similar issues
[46]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12618/shard-rkl-5/igt@kms_plane@pixel-format@pipe-b-planes.html
[47]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113156v1/shard-rkl-6/igt@kms_plane@pixel-format@pipe-b-planes.html
* igt@kms_psr@primary_mmap_gtt:
- {shard-rkl}: [SKIP][48] ([i915#1072]) -> [PASS][49] +1 similar issue
[48]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12618/shard-rkl-5/igt@kms_psr@primary_mmap_gtt.html
[49]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113156v1/shard-rkl-6/igt@kms_psr@primary_mmap_gtt.html
* igt@kms_rotation_crc@primary-y-tiled-reflect-x-0:
- {shard-rkl}: [SKIP][50] ([i915#1845] / [i915#4098]) -> [PASS][51] +10 similar issues
[50]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12618/shard-rkl-5/igt@kms_rotation_crc@primary-y-tiled-reflect-x-0.html
[51]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113156v1/shard-rkl-6/igt@kms_rotation_crc@primary-y-tiled-reflect-x-0.html
* igt@kms_universal_plane@cursor-fb-leak-pipe-a:
- {shard-tglu}: [SKIP][52] ([fdo#109274]) -> [PASS][53]
[52]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12618/shard-tglu-6/igt@kms_universal_plane@cursor-fb-leak-pipe-a.html
[53]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113156v1/shard-tglu-5/igt@kms_universal_plane@cursor-fb-leak-pipe-a.html
* igt@kms_vblank@pipe-c-wait-forked:
- {shard-tglu}: [SKIP][54] ([i915#1845] / [i915#7651]) -> [PASS][55]
[54]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12618/shard-tglu-6/igt@kms_vblank@pipe-c-wait-forked.html
[55]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113156v1/shard-tglu-5/igt@kms_vblank@pipe-c-wait-forked.html
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[IGT#2]: https://gitlab.freedesktop.org/drm/igt-gpu-tools/issues/2
[fdo#103375]: https://bugs.freedesktop.org/show_bug.cgi?id=103375
[fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
[fdo#109274]: https://bugs.freedesktop.org/show_bug.cgi?id=109274
[fdo#109279]: https://bugs.freedesktop.org/show_bug.cgi?id=109279
[fdo#109280]: https://bugs.freedesktop.org/show_bug.cgi?id=109280
[fdo#109283]: https://bugs.freedesktop.org/show_bug.cgi?id=109283
[fdo#109289]: https://bugs.freedesktop.org/show_bug.cgi?id=109289
[fdo#109291]: https://bugs.freedesktop.org/show_bug.cgi?id=109291
[fdo#109295]: https://bugs.freedesktop.org/show_bug.cgi?id=109295
[fdo#109302]: https://bugs.freedesktop.org/show_bug.cgi?id=109302
[fdo#109303]: https://bugs.freedesktop.org/show_bug.cgi?id=109303
[fdo#109307]: https://bugs.freedesktop.org/show_bug.cgi?id=109307
[fdo#109308]: https://bugs.freedesktop.org/show_bug.cgi?id=109308
[fdo#109312]: https://bugs.freedesktop.org/show_bug.cgi?id=109312
[fdo#109313]: https://bugs.freedesktop.org/show_bug.cgi?id=109313
[fdo#109314]: https://bugs.freedesktop.org/show_bug.cgi?id=109314
[fdo#109315]: https://bugs.freedesktop.org/show_bug.cgi?id=109315
[fdo#109506]: https://bugs.freedesktop.org/show_bug.cgi?id=109506
[fdo#109642]: https://bugs.freedesktop.org/show_bug.cgi?id=109642
[fdo#110189]: https://bugs.freedesktop.org/show_bug.cgi?id=110189
[fdo#110723]: https://bugs.freedesktop.org/show_bug.cgi?id=110723
[fdo#111068]: https://bugs.freedesktop.org/show_bug.cgi?id=111068
[fdo#111614]: https://bugs.freedesktop.org/show_bug.cgi?id=111614
[fdo#111615]: https://bugs.freedesktop.org/show_bug.cgi?id=111615
[fdo#111644]: https://bugs.freedesktop.org/show_bug.cgi?id=111644
[fdo#111656]: https://bugs.freedesktop.org/show_bug.cgi?id=111656
[fdo#111825]: https://bugs.freedesktop.org/show_bug.cgi?id=111825
[fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
[fdo#112054]: https://bugs.freedesktop.org/show_bug.cgi?id=112054
[fdo#112283]: https://bugs.freedesktop.org/show_bug.cgi?id=112283
[i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
[i915#118]: https://gitlab.freedesktop.org/drm/intel/issues/118
[i915#132]: https://gitlab.freedesktop.org/drm/intel/issues/132
[i915#1397]: https://gitlab.freedesktop.org/drm/intel/issues/1397
[i915#1722]: https://gitlab.freedesktop.org/drm/intel/issues/1722
[i915#1755]: https://gitlab.freedesktop.org/drm/intel/issues/1755
[i915#1825]: https://gitlab.freedesktop.org/drm/intel/issues/1825
[i915#1839]: https://gitlab.freedesktop.org/drm/intel/issues/1839
[i915#1845]: https://gitlab.freedesktop.org/drm/intel/issues/1845
[i915#1849]: https://gitlab.freedesktop.org/drm/intel/issues/1849
[i915#1850]: https://gitlab.freedesktop.org/drm/intel/issues/1850
[i915#1902]: https://gitlab.freedesktop.org/drm/intel/issues/1902
[i915#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190
[i915#2346]: https://gitlab.freedesktop.org/drm/intel/issues/2346
[i915#2437]: https://gitlab.freedesktop.org/drm/intel/issues/2437
[i915#2527]: https://gitlab.freedesktop.org/drm/intel/issues/2527
[i915#2575]: https://gitlab.freedesktop.org/drm/intel/issues/2575
[i915#2582]: https://gitlab.freedesktop.org/drm/intel/issues/2582
[i915#2587]: https://gitlab.freedesktop.org/drm/intel/issues/2587
[i915#2658]: https://gitlab.freedesktop.org/drm/intel/issues/2658
[i915#2672]: https://gitlab.freedesktop.org/drm/intel/issues/2672
[i915#2681]: https://gitlab.freedesktop.org/drm/intel/issues/2681
[i915#2842]: https://gitlab.freedesktop.org/drm/intel/issues/2842
[i915#2856]: https://gitlab.freedesktop.org/drm/intel/issues/2856
[i915#2920]: https://gitlab.freedesktop.org/drm/intel/issues/2920
[i915#2994]: https://gitlab.freedesktop.org/drm/intel/issues/2994
[i915#3116]: https://gitlab.freedesktop.org/drm/intel/issues/3116
[i915#315]: https://gitlab.freedesktop.org/drm/intel/issues/315
[i915#3281]: https://gitlab.freedesktop.org/drm/intel/issues/3281
[i915#3282]: https://gitlab.freedesktop.org/drm/intel/issues/3282
[i915#3297]: https://gitlab.freedesktop.org/drm/intel/issues/3297
[i915#3299]: https://gitlab.freedesktop.org/drm/intel/issues/3299
[i915#3318]: https://gitlab.freedesktop.org/drm/intel/issues/3318
[i915#3359]: https://gitlab.freedesktop.org/drm/intel/issues/3359
[i915#3458]: https://gitlab.freedesktop.org/drm/intel/issues/3458
[i915#3539]: https://gitlab.freedesktop.org/drm/intel/issues/3539
[i915#3546]: https://gitlab.freedesktop.org/drm/intel/issues/3546
[i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
[i915#3591]: https://gitlab.freedesktop.org/drm/intel/issues/3591
[i915#3637]: https://gitlab.freedesktop.org/drm/intel/issues/3637
[i915#3638]: https://gitlab.freedesktop.org/drm/intel/issues/3638
[i915#3689]: https://gitlab.freedesktop.org/drm/intel/issues/3689
[i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
[i915#3734]: https://gitlab.freedesktop.org/drm/intel/issues/3734
[i915#3742]: https://gitlab.freedesktop.org/drm/intel/issues/3742
[i915#3743]: https://gitlab.freedesktop.org/drm/intel/issues/3743
[i915#3826]: https://gitlab.freedesktop.org/drm/intel/issues/3826
[i915#3840]: https://gitlab.freedesktop.org/drm/intel/issues/3840
[i915#3886]: https://gitlab.freedesktop.org/drm/intel/issues/3886
[i915#3938]: https://gitlab.freedesktop.org/drm/intel/issues/3938
[i915#3955]: https://gitlab.freedesktop.org/drm/intel/issues/3955
[i915#4070]: https://gitlab.freedesktop.org/drm/intel/issues/4070
[i915#4077]: https://gitlab.freedesktop.org/drm/intel/issues/4077
[i915#4079]: https://gitlab.freedesktop.org/drm/intel/issues/4079
[i915#4083]: https://gitlab.freedesktop.org/drm/intel/issues/4083
[i915#4098]: https://gitlab.freedesktop.org/drm/intel/issues/4098
[i915#4103]: https://gitlab.freedesktop.org/drm/intel/issues/4103
[i915#4270]: https://gitlab.freedesktop.org/drm/intel/issues/4270
[i915#433]: https://gitlab.freedesktop.org/drm/intel/issues/433
[i915#4349]: https://gitlab.freedesktop.org/drm/intel/issues/4349
[i915#4387]: https://gitlab.freedesktop.org/drm/intel/issues/4387
[i915#4391]: https://gitlab.freedesktop.org/drm/intel/issues/4391
[i915#4538]: https://gitlab.freedesktop.org/drm/intel/issues/4538
[i915#4565]: https://gitlab.freedesktop.org/drm/intel/issues/4565
[i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
[i915#4767]: https://gitlab.freedesktop.org/drm/intel/issues/4767
[i915#4771]: https://gitlab.freedesktop.org/drm/intel/issues/4771
[i915#4812]: https://gitlab.freedesktop.org/drm/intel/issues/4812
[i915#4818]: https://gitlab.freedesktop.org/drm/intel/issues/4818
[i915#4833]: https://gitlab.freedesktop.org/drm/intel/issues/4833
[i915#4852]: https://gitlab.freedesktop.org/drm/intel/issues/4852
[i915#4860]: https://gitlab.freedesktop.org/drm/intel/issues/4860
[i915#4879]: https://gitlab.freedesktop.org/drm/intel/issues/4879
[i915#4884]: https://gitlab.freedesktop.org/drm/intel/issues/4884
[i915#4885]: https://gitlab.freedesktop.org/drm/intel/issues/4885
[i915#5176]: https://gitlab.freedesktop.org/drm/intel/issues/5176
[i915#5235]: https://gitlab.freedesktop.org/drm/intel/issues/5235
[i915#5286]: https://gitlab.freedesktop.org/drm/intel/issues/5286
[i915#5288]: https://gitlab.freedesktop.org/drm/intel/issues/5288
[i915#5289]: https://gitlab.freedesktop.org/drm/intel/issues/5289
[i915#5325]: https://gitlab.freedesktop.org/drm/intel/issues/5325
[i915#5327]: https://gitlab.freedesktop.org/drm/intel/issues/5327
[i915#533]: https://gitlab.freedesktop.org/drm/intel/issues/533
[i915#5439]: https://gitlab.freedesktop.org/drm/intel/issues/5439
[i915#5461]: https://gitlab.freedesktop.org/drm/intel/issues/5461
[i915#5563]: https://gitlab.freedesktop.org/drm/intel/issues/5563
[i915#6095]: https://gitlab.freedesktop.org/drm/intel/issues/6095
[i915#6117]: https://gitlab.freedesktop.org/drm/intel/issues/6117
[i915#6230]: https://gitlab.freedesktop.org/drm/intel/issues/6230
[i915#6247]: https://gitlab.freedesktop.org/drm/intel/issues/6247
[i915#6335]: https://gitlab.freedesktop.org/drm/intel/issues/6335
[i915#6412]: https://gitlab.freedesktop.org/drm/intel/issues/6412
[i915#6433]: https://gitlab.freedesktop.org/drm/intel/issues/6433
[i915#6493]: https://gitlab.freedesktop.org/drm/intel/issues/6493
[i915#6497]: https://gitlab.freedesktop.org/drm/intel/issues/6497
[i915#6524]: https://gitlab.freedesktop.org/drm/intel/issues/6524
[i915#658]: https://gitlab.freedesktop.org/drm/intel/issues/658
[i915#6621]: https://gitlab.freedesktop.org/drm/intel/issues/6621
[i915#6768]: https://gitlab.freedesktop.org/drm/intel/issues/6768
[i915#6944]: https://gitlab.freedesktop.org/drm/intel/issues/6944
[i915#6946]: https://gitlab.freedesktop.org/drm/intel/issues/6946
[i915#7037]: https://gitlab.freedesktop.org/drm/intel/issues/7037
[i915#7052]: https://gitlab.freedesktop.org/drm/intel/issues/7052
[i915#7116]: https://gitlab.freedesktop.org/drm/intel/issues/7116
[i915#7118]: https://gitlab.freedesktop.org/drm/intel/issues/7118
[i915#7128]: https://gitlab.freedesktop.org/drm/intel/issues/7128
[i915#7178]: https://gitlab.freedesktop.org/drm/intel/issues/7178
[i915#7205]: https://gitlab.freedesktop.org/drm/intel/issues/7205
[i915#7276]: https://gitlab.freedesktop.org/drm/intel/issues/7276
[i915#7294]: https://gitlab.freedesktop.org/drm/intel/issues/7294
[i915#7561]: https://gitlab.freedesktop.org/drm/intel/issues/7561
[i915#7582]: https://gitlab.freedesktop.org/drm/intel/issues/7582
[i915#7651]: https://gitlab.freedesktop.org/drm/intel/issues/7651
[i915#7697]: https://gitlab.freedesktop.org/drm/intel/issues/7697
[i915#7707]: https://gitlab.freedesktop.org/drm/intel/issues/7707
[i915#7711]: https://gitlab.freedesktop.org/drm/intel/issues/7711
[i915#7742]: https://gitlab.freedesktop.org/drm/intel/issues/7742
[i915#7828]: https://gitlab.freedesktop.org/drm/intel/issues/7828
[i915#79]: https://gitlab.freedesktop.org/drm/intel/issues/79
Build changes
-------------
* Linux: CI_DRM_12618 -> Patchwork_113156v1
* Piglit: piglit_4509 -> None
CI-20190529: 20190529
CI_DRM_12618: 7ba8ff20ba23bc940e928ffe3a9054225fff418e @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_7129: 7816773163a1b0d248dd9dd34d14e632ad8903be @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
Patchwork_113156v1: 7ba8ff20ba23bc940e928ffe3a9054225fff418e @ git://anongit.freedesktop.org/gfx-ci/linux
piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113156v1/index.html
[-- Attachment #2: Type: text/html, Size: 16316 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] kvm/vfio: Fix potential deadlock on vfio group_lock
2023-01-20 15:05 ` [Intel-gfx] " Yi Liu
@ 2023-01-31 14:27 ` Anthony Krowiak
-1 siblings, 0 replies; 26+ messages in thread
From: Anthony Krowiak @ 2023-01-31 14:27 UTC (permalink / raw)
To: Yi Liu, alex.williamson, pbonzini, mjrosato
Cc: jgg, kevin.tian, cohuck, farman, pmorel, borntraeger, frankja,
imbrenda, david, jjherne, pasic, zhenyuw, zhi.a.wang, seanjc,
linux-s390, kvm, intel-gvt-dev, intel-gfx, linux-kernel
I encountered a lockdep splat while running some regression tests today
(see below). I suspected it might be this patch so I reverted it,
rebuilt the kernel and ran the regression tests again; this time, the
test ran cleanly. It looks like this patch may not have fixed the
problem for which it was intended. Here is the relevant dmesg output:
[ 579.471402] hades[1099]: Start test run
[ 579.473486] hades[1099]: Start
'tests.test_vfio_ap.VfioAPAssignMdevToGuestTest.runTest' test
[ 579.505804] vfio_ap matrix: MDEV: Registered
[ 579.604024] vfio_ap_mdev 529654a9-bea4-461a-b64d-9d9c63df0deb: Adding
to iommu group 0
[ 585.043898] ======================================================
[ 585.043900] WARNING: possible circular locking dependency detected
[ 585.043902] 6.2.0-rc6-00057-g41c03ba9beea-dirty #18 Not tainted
[ 585.043904] ------------------------------------------------------
[ 585.043905] CPU 0/KVM/1173 is trying to acquire lock:
[ 585.043907] 000000008cfb24b0 (&group->group_lock){+.+.}-{3:3}, at:
vfio_file_set_kvm+0x50/0x68 [vfio]
[ 585.043919]
but task is already holding lock:
[ 585.043920] 00000000b2960ba0 (&kvm->lock){+.+.}-{3:3}, at:
kvm_device_release+0x4a/0xb8 [kvm]
[ 585.043960]
which lock already depends on the new lock.
[ 585.043962]
the existing dependency chain (in reverse order) is:
[ 585.043963]
-> #3 (&kvm->lock){+.+.}-{3:3}:
[ 585.043967] __lock_acquire+0x3e2/0x750
[ 585.043974] lock_acquire.part.0+0xe2/0x250
[ 585.043977] lock_acquire+0xac/0x1d0
[ 585.043980] __mutex_lock+0x9e/0x868
[ 585.043985] mutex_lock_nested+0x32/0x40
[ 585.043988] vfio_ap_mdev_open_device+0x9a/0x198 [vfio_ap]
[ 585.043991] vfio_device_open+0x122/0x168 [vfio]
[ 585.043995] vfio_device_open_file+0x64/0x120 [vfio]
[ 585.043999] vfio_group_fops_unl_ioctl+0xd4/0x1e0 [vfio]
[ 585.044002] __s390x_sys_ioctl+0xc0/0x100
[ 585.044007] do_syscall+0xee/0x118
[ 585.044032] __do_syscall+0xd2/0x120
[ 585.044035] system_call+0x82/0xb0
[ 585.044037]
-> #2 (&matrix_dev->guests_lock){+.+.}-{3:3}:
[ 585.044041] __lock_acquire+0x3e2/0x750
[ 585.044044] lock_acquire.part.0+0xe2/0x250
[ 585.044047] lock_acquire+0xac/0x1d0
[ 585.044049] __mutex_lock+0x9e/0x868
[ 585.044052] mutex_lock_nested+0x32/0x40
[ 585.044054] vfio_ap_mdev_open_device+0x8c/0x198 [vfio_ap]
[ 585.044057] vfio_device_open+0x122/0x168 [vfio]
[ 585.044060] vfio_device_open_file+0x64/0x120 [vfio]
[ 585.044064] vfio_group_fops_unl_ioctl+0xd4/0x1e0 [vfio]
[ 585.044068] __s390x_sys_ioctl+0xc0/0x100
[ 585.044070] do_syscall+0xee/0x118
[ 585.044072] __do_syscall+0xd2/0x120
[ 585.044074] system_call+0x82/0xb0
[ 585.044076]
-> #1 (&new_dev_set->lock){+.+.}-{3:3}:
[ 585.044080] __lock_acquire+0x3e2/0x750
[ 585.044082] lock_acquire.part.0+0xe2/0x250
[ 585.044085] lock_acquire+0xac/0x1d0
[ 585.044088] __mutex_lock+0x9e/0x868
[ 585.044090] mutex_lock_nested+0x32/0x40
[ 585.044093] vfio_device_open+0x3e/0x168 [vfio]
[ 585.044096] vfio_device_open_file+0x64/0x120 [vfio]
[ 585.044100] vfio_group_fops_unl_ioctl+0xd4/0x1e0 [vfio]
[ 585.044104] __s390x_sys_ioctl+0xc0/0x100
[ 585.044106] do_syscall+0xee/0x118
[ 585.044108] __do_syscall+0xd2/0x120
[ 585.044110] system_call+0x82/0xb0
[ 585.044112]
-> #0 (&group->group_lock){+.+.}-{3:3}:
[ 585.044115] check_prev_add+0xd4/0xf10
[ 585.044118] validate_chain+0x698/0x8e8
[ 585.044120] __lock_acquire+0x3e2/0x750
[ 585.044123] lock_acquire.part.0+0xe2/0x250
[ 585.044125] lock_acquire+0xac/0x1d0
[ 585.044128] __mutex_lock+0x9e/0x868
[ 585.044130] mutex_lock_nested+0x32/0x40
[ 585.044133] vfio_file_set_kvm+0x50/0x68 [vfio]
[ 585.044137] kvm_vfio_release+0x5e/0xf8 [kvm]
[ 585.044156] kvm_device_release+0x90/0xb8 [kvm]
[ 585.044175] __fput+0xaa/0x2a0
[ 585.044180] task_work_run+0x76/0xd0
[ 585.044183] do_exit+0x248/0x538
[ 585.044186] do_group_exit+0x40/0xb0
[ 585.044188] get_signal+0x614/0x698
[ 585.044192] arch_do_signal_or_restart+0x58/0x370
[ 585.044195] exit_to_user_mode_loop+0xe8/0x1b8
[ 585.044200] exit_to_user_mode_prepare+0x164/0x190
[ 585.044203] __do_syscall+0xd2/0x120
[ 585.044205] system_call+0x82/0xb0
[ 585.044207]
other info that might help us debug this:
[ 585.044209] Chain exists of:
&group->group_lock --> &matrix_dev->guests_lock -->
&kvm->lock
[ 585.044213] Possible unsafe locking scenario:
[ 585.044214] CPU0 CPU1
[ 585.044216] ---- ----
[ 585.044217] lock(&kvm->lock);
[ 585.044219] lock(&matrix_dev->guests_lock);
[ 585.044221] lock(&kvm->lock);
[ 585.044223] lock(&group->group_lock);
[ 585.044225]
*** DEADLOCK ***
[ 585.044227] 1 lock held by CPU 0/KVM/1173:
[ 585.044228] #0: 00000000b2960ba0 (&kvm->lock){+.+.}-{3:3}, at:
kvm_device_release+0x4a/0xb8 [kvm]
[ 585.044251]
stack backtrace:
[ 585.044253] CPU: 3 PID: 1173 Comm: CPU 0/KVM Not tainted
6.2.0-rc6-00057-g41c03ba9beea-dirty #18
[ 585.044256] Hardware name: IBM 8561 T01 772 (LPAR)
[ 585.044257] Call Trace:
[ 585.044258] [<000000011a818936>] dump_stack_lvl+0x8e/0xc8
[ 585.044261] [<0000000119aca3f2>] check_noncircular+0x132/0x158
[ 585.044264] [<0000000119acba44>] check_prev_add+0xd4/0xf10
[ 585.044267] [<0000000119accf18>] validate_chain+0x698/0x8e8
[ 585.044270] [<0000000119ace70a>] __lock_acquire+0x3e2/0x750
[ 585.044273] [<0000000119acf682>] lock_acquire.part.0+0xe2/0x250
[ 585.044276] [<0000000119acf89c>] lock_acquire+0xac/0x1d0
[ 585.044279] [<000000011a823c66>] __mutex_lock+0x9e/0x868
[ 585.044282] [<000000011a824462>] mutex_lock_nested+0x32/0x40
[ 585.044285] [<000003ff7fbcd6a0>] vfio_file_set_kvm+0x50/0x68 [vfio]
[ 585.044289] [<000003ff7feacab6>] kvm_vfio_release+0x5e/0xf8 [kvm]
[ 585.044308] [<000003ff7fea6d58>] kvm_device_release+0x90/0xb8 [kvm]
[ 585.044328] [<0000000119dbb83a>] __fput+0xaa/0x2a0
[ 585.044331] [<0000000119a67c66>] task_work_run+0x76/0xd0
[ 585.044333] [<0000000119a3ec18>] do_exit+0x248/0x538
[ 585.044335] [<0000000119a3f0c8>] do_group_exit+0x40/0xb0
[ 585.044338] [<0000000119a50dec>] get_signal+0x614/0x698
[ 585.044340] [<00000001199ea030>] arch_do_signal_or_restart+0x58/0x370
[ 585.044343] [<0000000119b0bb50>] exit_to_user_mode_loop+0xe8/0x1b8
[ 585.044346] [<0000000119b0bd84>] exit_to_user_mode_prepare+0x164/0x190
[ 585.044349] [<000000011a818d2a>] __do_syscall+0xd2/0x120
[ 585.044351] [<000000011a82c462>] system_call+0x82/0xb0
[ 585.044354] INFO: lockdep is turned off.
[ 610.595528] vfio_ap_mdev 529654a9-bea4-461a-b64d-9d9c63df0deb:
Removing from iommu group 0
[ 610.604408] vfio_ap matrix: MDEV: Unregistering
[ 610.826074] hades[1099]: Stop
'tests.test_vfio_ap.VfioAPAssignMdevToGuestTest.runTest'
On 1/20/23 10:05 AM, Yi Liu wrote:
> Currently it is possible that the final put of a KVM reference comes from
> vfio during its device close operation. This occurs while the vfio group
> lock is held; however, if the vfio device is still in the kvm device list,
> then the following call chain could result in a deadlock:
>
> VFIO holds group->group_lock/group_rwsem
> -> kvm_put_kvm
> -> kvm_destroy_vm
> -> kvm_destroy_devices
> -> kvm_vfio_destroy
> -> kvm_vfio_file_set_kvm
> -> vfio_file_set_kvm
> -> try to hold group->group_lock/group_rwsem
>
> The key function is the kvm_destroy_devices() which triggers destroy cb
> of kvm_device_ops. It calls back to vfio and try to hold group_lock. So
> if this path doesn't call back to vfio, this dead lock would be fixed.
> Actually, there is a way for it. KVM provides another point to free the
> kvm-vfio device which is the point when the device file descriptor is
> closed. This can be achieved by providing the release cb instead of the
> destroy cb. Also rename kvm_vfio_destroy() to be kvm_vfio_release().
>
> /*
> * Destroy is responsible for freeing dev.
> *
> * Destroy may be called before or after destructors are called
> * on emulated I/O regions, depending on whether a reference is
> * held by a vcpu or other kvm component that gets destroyed
> * after the emulated I/O.
> */
> void (*destroy)(struct kvm_device *dev);
>
> /*
> * Release is an alternative method to free the device. It is
> * called when the device file descriptor is closed. Once
> * release is called, the destroy method will not be called
> * anymore as the device is removed from the device list of
> * the VM. kvm->lock is held.
> */
> void (*release)(struct kvm_device *dev);
>
> Fixes: 421cfe6596f6 ("vfio: remove VFIO_GROUP_NOTIFY_SET_KVM")
> Reported-by: Alex Williamson <alex.williamson@redhat.com>
> Suggested-by: Kevin Tian <kevin.tian@intel.com>
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> ---
> virt/kvm/vfio.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c
> index 495ceabffe88..e94f3ea718e5 100644
> --- a/virt/kvm/vfio.c
> +++ b/virt/kvm/vfio.c
> @@ -336,7 +336,7 @@ static int kvm_vfio_has_attr(struct kvm_device *dev,
> return -ENXIO;
> }
>
> -static void kvm_vfio_destroy(struct kvm_device *dev)
> +static void kvm_vfio_release(struct kvm_device *dev)
> {
> struct kvm_vfio *kv = dev->private;
> struct kvm_vfio_group *kvg, *tmp;
> @@ -363,7 +363,7 @@ static int kvm_vfio_create(struct kvm_device *dev, u32 type);
> static struct kvm_device_ops kvm_vfio_ops = {
> .name = "kvm-vfio",
> .create = kvm_vfio_create,
> - .destroy = kvm_vfio_destroy,
> + .release = kvm_vfio_release,
> .set_attr = kvm_vfio_set_attr,
> .has_attr = kvm_vfio_has_attr,
> };
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Intel-gfx] [PATCH] kvm/vfio: Fix potential deadlock on vfio group_lock
@ 2023-01-31 14:27 ` Anthony Krowiak
0 siblings, 0 replies; 26+ messages in thread
From: Anthony Krowiak @ 2023-01-31 14:27 UTC (permalink / raw)
To: Yi Liu, alex.williamson, pbonzini, mjrosato
Cc: jjherne, linux-s390, imbrenda, frankja, pmorel, david, seanjc,
intel-gfx, cohuck, farman, linux-kernel, pasic, jgg, kvm,
borntraeger, intel-gvt-dev
I encountered a lockdep splat while running some regression tests today
(see below). I suspected it might be this patch so I reverted it,
rebuilt the kernel and ran the regression tests again; this time, the
test ran cleanly. It looks like this patch may not have fixed the
problem for which it was intended. Here is the relevant dmesg output:
[ 579.471402] hades[1099]: Start test run
[ 579.473486] hades[1099]: Start
'tests.test_vfio_ap.VfioAPAssignMdevToGuestTest.runTest' test
[ 579.505804] vfio_ap matrix: MDEV: Registered
[ 579.604024] vfio_ap_mdev 529654a9-bea4-461a-b64d-9d9c63df0deb: Adding
to iommu group 0
[ 585.043898] ======================================================
[ 585.043900] WARNING: possible circular locking dependency detected
[ 585.043902] 6.2.0-rc6-00057-g41c03ba9beea-dirty #18 Not tainted
[ 585.043904] ------------------------------------------------------
[ 585.043905] CPU 0/KVM/1173 is trying to acquire lock:
[ 585.043907] 000000008cfb24b0 (&group->group_lock){+.+.}-{3:3}, at:
vfio_file_set_kvm+0x50/0x68 [vfio]
[ 585.043919]
but task is already holding lock:
[ 585.043920] 00000000b2960ba0 (&kvm->lock){+.+.}-{3:3}, at:
kvm_device_release+0x4a/0xb8 [kvm]
[ 585.043960]
which lock already depends on the new lock.
[ 585.043962]
the existing dependency chain (in reverse order) is:
[ 585.043963]
-> #3 (&kvm->lock){+.+.}-{3:3}:
[ 585.043967] __lock_acquire+0x3e2/0x750
[ 585.043974] lock_acquire.part.0+0xe2/0x250
[ 585.043977] lock_acquire+0xac/0x1d0
[ 585.043980] __mutex_lock+0x9e/0x868
[ 585.043985] mutex_lock_nested+0x32/0x40
[ 585.043988] vfio_ap_mdev_open_device+0x9a/0x198 [vfio_ap]
[ 585.043991] vfio_device_open+0x122/0x168 [vfio]
[ 585.043995] vfio_device_open_file+0x64/0x120 [vfio]
[ 585.043999] vfio_group_fops_unl_ioctl+0xd4/0x1e0 [vfio]
[ 585.044002] __s390x_sys_ioctl+0xc0/0x100
[ 585.044007] do_syscall+0xee/0x118
[ 585.044032] __do_syscall+0xd2/0x120
[ 585.044035] system_call+0x82/0xb0
[ 585.044037]
-> #2 (&matrix_dev->guests_lock){+.+.}-{3:3}:
[ 585.044041] __lock_acquire+0x3e2/0x750
[ 585.044044] lock_acquire.part.0+0xe2/0x250
[ 585.044047] lock_acquire+0xac/0x1d0
[ 585.044049] __mutex_lock+0x9e/0x868
[ 585.044052] mutex_lock_nested+0x32/0x40
[ 585.044054] vfio_ap_mdev_open_device+0x8c/0x198 [vfio_ap]
[ 585.044057] vfio_device_open+0x122/0x168 [vfio]
[ 585.044060] vfio_device_open_file+0x64/0x120 [vfio]
[ 585.044064] vfio_group_fops_unl_ioctl+0xd4/0x1e0 [vfio]
[ 585.044068] __s390x_sys_ioctl+0xc0/0x100
[ 585.044070] do_syscall+0xee/0x118
[ 585.044072] __do_syscall+0xd2/0x120
[ 585.044074] system_call+0x82/0xb0
[ 585.044076]
-> #1 (&new_dev_set->lock){+.+.}-{3:3}:
[ 585.044080] __lock_acquire+0x3e2/0x750
[ 585.044082] lock_acquire.part.0+0xe2/0x250
[ 585.044085] lock_acquire+0xac/0x1d0
[ 585.044088] __mutex_lock+0x9e/0x868
[ 585.044090] mutex_lock_nested+0x32/0x40
[ 585.044093] vfio_device_open+0x3e/0x168 [vfio]
[ 585.044096] vfio_device_open_file+0x64/0x120 [vfio]
[ 585.044100] vfio_group_fops_unl_ioctl+0xd4/0x1e0 [vfio]
[ 585.044104] __s390x_sys_ioctl+0xc0/0x100
[ 585.044106] do_syscall+0xee/0x118
[ 585.044108] __do_syscall+0xd2/0x120
[ 585.044110] system_call+0x82/0xb0
[ 585.044112]
-> #0 (&group->group_lock){+.+.}-{3:3}:
[ 585.044115] check_prev_add+0xd4/0xf10
[ 585.044118] validate_chain+0x698/0x8e8
[ 585.044120] __lock_acquire+0x3e2/0x750
[ 585.044123] lock_acquire.part.0+0xe2/0x250
[ 585.044125] lock_acquire+0xac/0x1d0
[ 585.044128] __mutex_lock+0x9e/0x868
[ 585.044130] mutex_lock_nested+0x32/0x40
[ 585.044133] vfio_file_set_kvm+0x50/0x68 [vfio]
[ 585.044137] kvm_vfio_release+0x5e/0xf8 [kvm]
[ 585.044156] kvm_device_release+0x90/0xb8 [kvm]
[ 585.044175] __fput+0xaa/0x2a0
[ 585.044180] task_work_run+0x76/0xd0
[ 585.044183] do_exit+0x248/0x538
[ 585.044186] do_group_exit+0x40/0xb0
[ 585.044188] get_signal+0x614/0x698
[ 585.044192] arch_do_signal_or_restart+0x58/0x370
[ 585.044195] exit_to_user_mode_loop+0xe8/0x1b8
[ 585.044200] exit_to_user_mode_prepare+0x164/0x190
[ 585.044203] __do_syscall+0xd2/0x120
[ 585.044205] system_call+0x82/0xb0
[ 585.044207]
other info that might help us debug this:
[ 585.044209] Chain exists of:
&group->group_lock --> &matrix_dev->guests_lock -->
&kvm->lock
[ 585.044213] Possible unsafe locking scenario:
[ 585.044214] CPU0 CPU1
[ 585.044216] ---- ----
[ 585.044217] lock(&kvm->lock);
[ 585.044219] lock(&matrix_dev->guests_lock);
[ 585.044221] lock(&kvm->lock);
[ 585.044223] lock(&group->group_lock);
[ 585.044225]
*** DEADLOCK ***
[ 585.044227] 1 lock held by CPU 0/KVM/1173:
[ 585.044228] #0: 00000000b2960ba0 (&kvm->lock){+.+.}-{3:3}, at:
kvm_device_release+0x4a/0xb8 [kvm]
[ 585.044251]
stack backtrace:
[ 585.044253] CPU: 3 PID: 1173 Comm: CPU 0/KVM Not tainted
6.2.0-rc6-00057-g41c03ba9beea-dirty #18
[ 585.044256] Hardware name: IBM 8561 T01 772 (LPAR)
[ 585.044257] Call Trace:
[ 585.044258] [<000000011a818936>] dump_stack_lvl+0x8e/0xc8
[ 585.044261] [<0000000119aca3f2>] check_noncircular+0x132/0x158
[ 585.044264] [<0000000119acba44>] check_prev_add+0xd4/0xf10
[ 585.044267] [<0000000119accf18>] validate_chain+0x698/0x8e8
[ 585.044270] [<0000000119ace70a>] __lock_acquire+0x3e2/0x750
[ 585.044273] [<0000000119acf682>] lock_acquire.part.0+0xe2/0x250
[ 585.044276] [<0000000119acf89c>] lock_acquire+0xac/0x1d0
[ 585.044279] [<000000011a823c66>] __mutex_lock+0x9e/0x868
[ 585.044282] [<000000011a824462>] mutex_lock_nested+0x32/0x40
[ 585.044285] [<000003ff7fbcd6a0>] vfio_file_set_kvm+0x50/0x68 [vfio]
[ 585.044289] [<000003ff7feacab6>] kvm_vfio_release+0x5e/0xf8 [kvm]
[ 585.044308] [<000003ff7fea6d58>] kvm_device_release+0x90/0xb8 [kvm]
[ 585.044328] [<0000000119dbb83a>] __fput+0xaa/0x2a0
[ 585.044331] [<0000000119a67c66>] task_work_run+0x76/0xd0
[ 585.044333] [<0000000119a3ec18>] do_exit+0x248/0x538
[ 585.044335] [<0000000119a3f0c8>] do_group_exit+0x40/0xb0
[ 585.044338] [<0000000119a50dec>] get_signal+0x614/0x698
[ 585.044340] [<00000001199ea030>] arch_do_signal_or_restart+0x58/0x370
[ 585.044343] [<0000000119b0bb50>] exit_to_user_mode_loop+0xe8/0x1b8
[ 585.044346] [<0000000119b0bd84>] exit_to_user_mode_prepare+0x164/0x190
[ 585.044349] [<000000011a818d2a>] __do_syscall+0xd2/0x120
[ 585.044351] [<000000011a82c462>] system_call+0x82/0xb0
[ 585.044354] INFO: lockdep is turned off.
[ 610.595528] vfio_ap_mdev 529654a9-bea4-461a-b64d-9d9c63df0deb:
Removing from iommu group 0
[ 610.604408] vfio_ap matrix: MDEV: Unregistering
[ 610.826074] hades[1099]: Stop
'tests.test_vfio_ap.VfioAPAssignMdevToGuestTest.runTest'
On 1/20/23 10:05 AM, Yi Liu wrote:
> Currently it is possible that the final put of a KVM reference comes from
> vfio during its device close operation. This occurs while the vfio group
> lock is held; however, if the vfio device is still in the kvm device list,
> then the following call chain could result in a deadlock:
>
> VFIO holds group->group_lock/group_rwsem
> -> kvm_put_kvm
> -> kvm_destroy_vm
> -> kvm_destroy_devices
> -> kvm_vfio_destroy
> -> kvm_vfio_file_set_kvm
> -> vfio_file_set_kvm
> -> try to hold group->group_lock/group_rwsem
>
> The key function is the kvm_destroy_devices() which triggers destroy cb
> of kvm_device_ops. It calls back to vfio and try to hold group_lock. So
> if this path doesn't call back to vfio, this dead lock would be fixed.
> Actually, there is a way for it. KVM provides another point to free the
> kvm-vfio device which is the point when the device file descriptor is
> closed. This can be achieved by providing the release cb instead of the
> destroy cb. Also rename kvm_vfio_destroy() to be kvm_vfio_release().
>
> /*
> * Destroy is responsible for freeing dev.
> *
> * Destroy may be called before or after destructors are called
> * on emulated I/O regions, depending on whether a reference is
> * held by a vcpu or other kvm component that gets destroyed
> * after the emulated I/O.
> */
> void (*destroy)(struct kvm_device *dev);
>
> /*
> * Release is an alternative method to free the device. It is
> * called when the device file descriptor is closed. Once
> * release is called, the destroy method will not be called
> * anymore as the device is removed from the device list of
> * the VM. kvm->lock is held.
> */
> void (*release)(struct kvm_device *dev);
>
> Fixes: 421cfe6596f6 ("vfio: remove VFIO_GROUP_NOTIFY_SET_KVM")
> Reported-by: Alex Williamson <alex.williamson@redhat.com>
> Suggested-by: Kevin Tian <kevin.tian@intel.com>
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> ---
> virt/kvm/vfio.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c
> index 495ceabffe88..e94f3ea718e5 100644
> --- a/virt/kvm/vfio.c
> +++ b/virt/kvm/vfio.c
> @@ -336,7 +336,7 @@ static int kvm_vfio_has_attr(struct kvm_device *dev,
> return -ENXIO;
> }
>
> -static void kvm_vfio_destroy(struct kvm_device *dev)
> +static void kvm_vfio_release(struct kvm_device *dev)
> {
> struct kvm_vfio *kv = dev->private;
> struct kvm_vfio_group *kvg, *tmp;
> @@ -363,7 +363,7 @@ static int kvm_vfio_create(struct kvm_device *dev, u32 type);
> static struct kvm_device_ops kvm_vfio_ops = {
> .name = "kvm-vfio",
> .create = kvm_vfio_create,
> - .destroy = kvm_vfio_destroy,
> + .release = kvm_vfio_release,
> .set_attr = kvm_vfio_set_attr,
> .has_attr = kvm_vfio_has_attr,
> };
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] kvm/vfio: Fix potential deadlock on vfio group_lock
2023-01-31 14:27 ` [Intel-gfx] " Anthony Krowiak
@ 2023-01-31 14:34 ` Jason Gunthorpe
-1 siblings, 0 replies; 26+ messages in thread
From: Jason Gunthorpe @ 2023-01-31 14:34 UTC (permalink / raw)
To: Anthony Krowiak
Cc: Yi Liu, alex.williamson, pbonzini, mjrosato, kevin.tian, cohuck,
farman, pmorel, borntraeger, frankja, imbrenda, david, jjherne,
pasic, zhenyuw, zhi.a.wang, seanjc, linux-s390, kvm,
intel-gvt-dev, intel-gfx, linux-kernel
On Tue, Jan 31, 2023 at 09:27:54AM -0500, Anthony Krowiak wrote:
> I encountered a lockdep splat while running some regression tests today (see
> below). I suspected it might be this patch so I reverted it, rebuilt the
> kernel and ran the regression tests again; this time, the test ran cleanly.
> It looks like this patch may not have fixed the problem for which it was
> intended. Here is the relevant dmesg output:
Well, it fixes the deadlock it intended to fix and created another one
:)
It means device drivers cannot obtain the kvm lock from their open
functions in this new model.
Why does ap need to touch kvm->lock? (via get_update_locks_for_kvm)
Maybe you should split that lock and have a dedicated apcb lock?
Jason
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Intel-gfx] [PATCH] kvm/vfio: Fix potential deadlock on vfio group_lock
@ 2023-01-31 14:34 ` Jason Gunthorpe
0 siblings, 0 replies; 26+ messages in thread
From: Jason Gunthorpe @ 2023-01-31 14:34 UTC (permalink / raw)
To: Anthony Krowiak
Cc: mjrosato, david, farman, imbrenda, linux-s390, Yi Liu, kvm,
pasic, borntraeger, intel-gfx, intel-gvt-dev, frankja, jjherne,
pmorel, seanjc, cohuck, linux-kernel, pbonzini
On Tue, Jan 31, 2023 at 09:27:54AM -0500, Anthony Krowiak wrote:
> I encountered a lockdep splat while running some regression tests today (see
> below). I suspected it might be this patch so I reverted it, rebuilt the
> kernel and ran the regression tests again; this time, the test ran cleanly.
> It looks like this patch may not have fixed the problem for which it was
> intended. Here is the relevant dmesg output:
Well, it fixes the deadlock it intended to fix and created another one
:)
It means device drivers cannot obtain the kvm lock from their open
functions in this new model.
Why does ap need to touch kvm->lock? (via get_update_locks_for_kvm)
Maybe you should split that lock and have a dedicated apcb lock?
Jason
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] kvm/vfio: Fix potential deadlock on vfio group_lock
2023-01-31 14:27 ` [Intel-gfx] " Anthony Krowiak
@ 2023-01-31 14:35 ` Matthew Rosato
-1 siblings, 0 replies; 26+ messages in thread
From: Matthew Rosato @ 2023-01-31 14:35 UTC (permalink / raw)
To: Anthony Krowiak, Yi Liu, alex.williamson, pbonzini
Cc: jgg, kevin.tian, cohuck, farman, pmorel, borntraeger, frankja,
imbrenda, david, jjherne, pasic, zhenyuw, zhi.a.wang, seanjc,
linux-s390, kvm, intel-gvt-dev, intel-gfx, linux-kernel
On 1/31/23 9:27 AM, Anthony Krowiak wrote:
> I encountered a lockdep splat while running some regression tests today (see below). I suspected it might be this patch so I reverted it, rebuilt the kernel and ran the regression tests again; this time, the test ran cleanly. It looks like this patch may not have fixed the problem for which it was intended. Here is the relevant dmesg output:
>
Damn it, the config I used to test must not have had lockdep enabled.
It looks like the issue is that .destroy did not used to acquire the kvm lock prior to calling into vfio_file_set_kvm whereas now .release does. This means .release expects a hierarchy of kvm->lock ... vfio->group_lock but your .open_device does vfio->group_lock ... kvm->lock.
I can reproduce something similar for vfio_pci_zdev.
> [ 579.471402] hades[1099]: Start test run
> [ 579.473486] hades[1099]: Start 'tests.test_vfio_ap.VfioAPAssignMdevToGuestTest.runTest' test
> [ 579.505804] vfio_ap matrix: MDEV: Registered
> [ 579.604024] vfio_ap_mdev 529654a9-bea4-461a-b64d-9d9c63df0deb: Adding to iommu group 0
>
> [ 585.043898] ======================================================
> [ 585.043900] WARNING: possible circular locking dependency detected
> [ 585.043902] 6.2.0-rc6-00057-g41c03ba9beea-dirty #18 Not tainted
> [ 585.043904] ------------------------------------------------------
> [ 585.043905] CPU 0/KVM/1173 is trying to acquire lock:
> [ 585.043907] 000000008cfb24b0 (&group->group_lock){+.+.}-{3:3}, at: vfio_file_set_kvm+0x50/0x68 [vfio]
> [ 585.043919]
> but task is already holding lock:
> [ 585.043920] 00000000b2960ba0 (&kvm->lock){+.+.}-{3:3}, at: kvm_device_release+0x4a/0xb8 [kvm]
> [ 585.043960]
> which lock already depends on the new lock.
>
> [ 585.043962]
> the existing dependency chain (in reverse order) is:
> [ 585.043963]
> -> #3 (&kvm->lock){+.+.}-{3:3}:
> [ 585.043967] __lock_acquire+0x3e2/0x750
> [ 585.043974] lock_acquire.part.0+0xe2/0x250
> [ 585.043977] lock_acquire+0xac/0x1d0
> [ 585.043980] __mutex_lock+0x9e/0x868
> [ 585.043985] mutex_lock_nested+0x32/0x40
> [ 585.043988] vfio_ap_mdev_open_device+0x9a/0x198 [vfio_ap]
> [ 585.043991] vfio_device_open+0x122/0x168 [vfio]
> [ 585.043995] vfio_device_open_file+0x64/0x120 [vfio]
> [ 585.043999] vfio_group_fops_unl_ioctl+0xd4/0x1e0 [vfio]
> [ 585.044002] __s390x_sys_ioctl+0xc0/0x100
> [ 585.044007] do_syscall+0xee/0x118
> [ 585.044032] __do_syscall+0xd2/0x120
> [ 585.044035] system_call+0x82/0xb0
> [ 585.044037]
> -> #2 (&matrix_dev->guests_lock){+.+.}-{3:3}:
> [ 585.044041] __lock_acquire+0x3e2/0x750
> [ 585.044044] lock_acquire.part.0+0xe2/0x250
> [ 585.044047] lock_acquire+0xac/0x1d0
> [ 585.044049] __mutex_lock+0x9e/0x868
> [ 585.044052] mutex_lock_nested+0x32/0x40
> [ 585.044054] vfio_ap_mdev_open_device+0x8c/0x198 [vfio_ap]
> [ 585.044057] vfio_device_open+0x122/0x168 [vfio]
> [ 585.044060] vfio_device_open_file+0x64/0x120 [vfio]
> [ 585.044064] vfio_group_fops_unl_ioctl+0xd4/0x1e0 [vfio]
> [ 585.044068] __s390x_sys_ioctl+0xc0/0x100
> [ 585.044070] do_syscall+0xee/0x118
> [ 585.044072] __do_syscall+0xd2/0x120
> [ 585.044074] system_call+0x82/0xb0
> [ 585.044076]
> -> #1 (&new_dev_set->lock){+.+.}-{3:3}:
> [ 585.044080] __lock_acquire+0x3e2/0x750
> [ 585.044082] lock_acquire.part.0+0xe2/0x250
> [ 585.044085] lock_acquire+0xac/0x1d0
> [ 585.044088] __mutex_lock+0x9e/0x868
> [ 585.044090] mutex_lock_nested+0x32/0x40
> [ 585.044093] vfio_device_open+0x3e/0x168 [vfio]
> [ 585.044096] vfio_device_open_file+0x64/0x120 [vfio]
> [ 585.044100] vfio_group_fops_unl_ioctl+0xd4/0x1e0 [vfio]
> [ 585.044104] __s390x_sys_ioctl+0xc0/0x100
> [ 585.044106] do_syscall+0xee/0x118
> [ 585.044108] __do_syscall+0xd2/0x120
> [ 585.044110] system_call+0x82/0xb0
> [ 585.044112]
> -> #0 (&group->group_lock){+.+.}-{3:3}:
> [ 585.044115] check_prev_add+0xd4/0xf10
> [ 585.044118] validate_chain+0x698/0x8e8
> [ 585.044120] __lock_acquire+0x3e2/0x750
> [ 585.044123] lock_acquire.part.0+0xe2/0x250
> [ 585.044125] lock_acquire+0xac/0x1d0
> [ 585.044128] __mutex_lock+0x9e/0x868
> [ 585.044130] mutex_lock_nested+0x32/0x40
> [ 585.044133] vfio_file_set_kvm+0x50/0x68 [vfio]
> [ 585.044137] kvm_vfio_release+0x5e/0xf8 [kvm]
> [ 585.044156] kvm_device_release+0x90/0xb8 [kvm]
> [ 585.044175] __fput+0xaa/0x2a0
> [ 585.044180] task_work_run+0x76/0xd0
> [ 585.044183] do_exit+0x248/0x538
> [ 585.044186] do_group_exit+0x40/0xb0
> [ 585.044188] get_signal+0x614/0x698
> [ 585.044192] arch_do_signal_or_restart+0x58/0x370
> [ 585.044195] exit_to_user_mode_loop+0xe8/0x1b8
> [ 585.044200] exit_to_user_mode_prepare+0x164/0x190
> [ 585.044203] __do_syscall+0xd2/0x120
> [ 585.044205] system_call+0x82/0xb0
> [ 585.044207]
> other info that might help us debug this:
>
> [ 585.044209] Chain exists of:
> &group->group_lock --> &matrix_dev->guests_lock --> &kvm->lock
>
> [ 585.044213] Possible unsafe locking scenario:
>
> [ 585.044214] CPU0 CPU1
> [ 585.044216] ---- ----
> [ 585.044217] lock(&kvm->lock);
> [ 585.044219] lock(&matrix_dev->guests_lock);
> [ 585.044221] lock(&kvm->lock);
> [ 585.044223] lock(&group->group_lock);
> [ 585.044225]
> *** DEADLOCK ***
>
> [ 585.044227] 1 lock held by CPU 0/KVM/1173:
> [ 585.044228] #0: 00000000b2960ba0 (&kvm->lock){+.+.}-{3:3}, at: kvm_device_release+0x4a/0xb8 [kvm]
> [ 585.044251]
> stack backtrace:
> [ 585.044253] CPU: 3 PID: 1173 Comm: CPU 0/KVM Not tainted 6.2.0-rc6-00057-g41c03ba9beea-dirty #18
> [ 585.044256] Hardware name: IBM 8561 T01 772 (LPAR)
> [ 585.044257] Call Trace:
> [ 585.044258] [<000000011a818936>] dump_stack_lvl+0x8e/0xc8
> [ 585.044261] [<0000000119aca3f2>] check_noncircular+0x132/0x158
> [ 585.044264] [<0000000119acba44>] check_prev_add+0xd4/0xf10
> [ 585.044267] [<0000000119accf18>] validate_chain+0x698/0x8e8
> [ 585.044270] [<0000000119ace70a>] __lock_acquire+0x3e2/0x750
> [ 585.044273] [<0000000119acf682>] lock_acquire.part.0+0xe2/0x250
> [ 585.044276] [<0000000119acf89c>] lock_acquire+0xac/0x1d0
> [ 585.044279] [<000000011a823c66>] __mutex_lock+0x9e/0x868
> [ 585.044282] [<000000011a824462>] mutex_lock_nested+0x32/0x40
> [ 585.044285] [<000003ff7fbcd6a0>] vfio_file_set_kvm+0x50/0x68 [vfio]
> [ 585.044289] [<000003ff7feacab6>] kvm_vfio_release+0x5e/0xf8 [kvm]
> [ 585.044308] [<000003ff7fea6d58>] kvm_device_release+0x90/0xb8 [kvm]
> [ 585.044328] [<0000000119dbb83a>] __fput+0xaa/0x2a0
> [ 585.044331] [<0000000119a67c66>] task_work_run+0x76/0xd0
> [ 585.044333] [<0000000119a3ec18>] do_exit+0x248/0x538
> [ 585.044335] [<0000000119a3f0c8>] do_group_exit+0x40/0xb0
> [ 585.044338] [<0000000119a50dec>] get_signal+0x614/0x698
> [ 585.044340] [<00000001199ea030>] arch_do_signal_or_restart+0x58/0x370
> [ 585.044343] [<0000000119b0bb50>] exit_to_user_mode_loop+0xe8/0x1b8
> [ 585.044346] [<0000000119b0bd84>] exit_to_user_mode_prepare+0x164/0x190
> [ 585.044349] [<000000011a818d2a>] __do_syscall+0xd2/0x120
> [ 585.044351] [<000000011a82c462>] system_call+0x82/0xb0
> [ 585.044354] INFO: lockdep is turned off.
> [ 610.595528] vfio_ap_mdev 529654a9-bea4-461a-b64d-9d9c63df0deb: Removing from iommu group 0
> [ 610.604408] vfio_ap matrix: MDEV: Unregistering
> [ 610.826074] hades[1099]: Stop 'tests.test_vfio_ap.VfioAPAssignMdevToGuestTest.runTest'
>
> On 1/20/23 10:05 AM, Yi Liu wrote:
>> Currently it is possible that the final put of a KVM reference comes from
>> vfio during its device close operation. This occurs while the vfio group
>> lock is held; however, if the vfio device is still in the kvm device list,
>> then the following call chain could result in a deadlock:
>>
>> VFIO holds group->group_lock/group_rwsem
>> -> kvm_put_kvm
>> -> kvm_destroy_vm
>> -> kvm_destroy_devices
>> -> kvm_vfio_destroy
>> -> kvm_vfio_file_set_kvm
>> -> vfio_file_set_kvm
>> -> try to hold group->group_lock/group_rwsem
>>
>> The key function is the kvm_destroy_devices() which triggers destroy cb
>> of kvm_device_ops. It calls back to vfio and try to hold group_lock. So
>> if this path doesn't call back to vfio, this dead lock would be fixed.
>> Actually, there is a way for it. KVM provides another point to free the
>> kvm-vfio device which is the point when the device file descriptor is
>> closed. This can be achieved by providing the release cb instead of the
>> destroy cb. Also rename kvm_vfio_destroy() to be kvm_vfio_release().
>>
>> /*
>> * Destroy is responsible for freeing dev.
>> *
>> * Destroy may be called before or after destructors are called
>> * on emulated I/O regions, depending on whether a reference is
>> * held by a vcpu or other kvm component that gets destroyed
>> * after the emulated I/O.
>> */
>> void (*destroy)(struct kvm_device *dev);
>>
>> /*
>> * Release is an alternative method to free the device. It is
>> * called when the device file descriptor is closed. Once
>> * release is called, the destroy method will not be called
>> * anymore as the device is removed from the device list of
>> * the VM. kvm->lock is held.
>> */
>> void (*release)(struct kvm_device *dev);
>>
>> Fixes: 421cfe6596f6 ("vfio: remove VFIO_GROUP_NOTIFY_SET_KVM")
>> Reported-by: Alex Williamson <alex.williamson@redhat.com>
>> Suggested-by: Kevin Tian <kevin.tian@intel.com>
>> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>> ---
>> virt/kvm/vfio.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c
>> index 495ceabffe88..e94f3ea718e5 100644
>> --- a/virt/kvm/vfio.c
>> +++ b/virt/kvm/vfio.c
>> @@ -336,7 +336,7 @@ static int kvm_vfio_has_attr(struct kvm_device *dev,
>> return -ENXIO;
>> }
>> -static void kvm_vfio_destroy(struct kvm_device *dev)
>> +static void kvm_vfio_release(struct kvm_device *dev)
>> {
>> struct kvm_vfio *kv = dev->private;
>> struct kvm_vfio_group *kvg, *tmp;
>> @@ -363,7 +363,7 @@ static int kvm_vfio_create(struct kvm_device *dev, u32 type);
>> static struct kvm_device_ops kvm_vfio_ops = {
>> .name = "kvm-vfio",
>> .create = kvm_vfio_create,
>> - .destroy = kvm_vfio_destroy,
>> + .release = kvm_vfio_release,
>> .set_attr = kvm_vfio_set_attr,
>> .has_attr = kvm_vfio_has_attr,
>> };
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Intel-gfx] [PATCH] kvm/vfio: Fix potential deadlock on vfio group_lock
@ 2023-01-31 14:35 ` Matthew Rosato
0 siblings, 0 replies; 26+ messages in thread
From: Matthew Rosato @ 2023-01-31 14:35 UTC (permalink / raw)
To: Anthony Krowiak, Yi Liu, alex.williamson, pbonzini
Cc: jjherne, linux-s390, imbrenda, frankja, pmorel, david, seanjc,
intel-gfx, cohuck, farman, linux-kernel, pasic, jgg, kvm,
borntraeger, intel-gvt-dev
On 1/31/23 9:27 AM, Anthony Krowiak wrote:
> I encountered a lockdep splat while running some regression tests today (see below). I suspected it might be this patch so I reverted it, rebuilt the kernel and ran the regression tests again; this time, the test ran cleanly. It looks like this patch may not have fixed the problem for which it was intended. Here is the relevant dmesg output:
>
Damn it, the config I used to test must not have had lockdep enabled.
It looks like the issue is that .destroy did not used to acquire the kvm lock prior to calling into vfio_file_set_kvm whereas now .release does. This means .release expects a hierarchy of kvm->lock ... vfio->group_lock but your .open_device does vfio->group_lock ... kvm->lock.
I can reproduce something similar for vfio_pci_zdev.
> [ 579.471402] hades[1099]: Start test run
> [ 579.473486] hades[1099]: Start 'tests.test_vfio_ap.VfioAPAssignMdevToGuestTest.runTest' test
> [ 579.505804] vfio_ap matrix: MDEV: Registered
> [ 579.604024] vfio_ap_mdev 529654a9-bea4-461a-b64d-9d9c63df0deb: Adding to iommu group 0
>
> [ 585.043898] ======================================================
> [ 585.043900] WARNING: possible circular locking dependency detected
> [ 585.043902] 6.2.0-rc6-00057-g41c03ba9beea-dirty #18 Not tainted
> [ 585.043904] ------------------------------------------------------
> [ 585.043905] CPU 0/KVM/1173 is trying to acquire lock:
> [ 585.043907] 000000008cfb24b0 (&group->group_lock){+.+.}-{3:3}, at: vfio_file_set_kvm+0x50/0x68 [vfio]
> [ 585.043919]
> but task is already holding lock:
> [ 585.043920] 00000000b2960ba0 (&kvm->lock){+.+.}-{3:3}, at: kvm_device_release+0x4a/0xb8 [kvm]
> [ 585.043960]
> which lock already depends on the new lock.
>
> [ 585.043962]
> the existing dependency chain (in reverse order) is:
> [ 585.043963]
> -> #3 (&kvm->lock){+.+.}-{3:3}:
> [ 585.043967] __lock_acquire+0x3e2/0x750
> [ 585.043974] lock_acquire.part.0+0xe2/0x250
> [ 585.043977] lock_acquire+0xac/0x1d0
> [ 585.043980] __mutex_lock+0x9e/0x868
> [ 585.043985] mutex_lock_nested+0x32/0x40
> [ 585.043988] vfio_ap_mdev_open_device+0x9a/0x198 [vfio_ap]
> [ 585.043991] vfio_device_open+0x122/0x168 [vfio]
> [ 585.043995] vfio_device_open_file+0x64/0x120 [vfio]
> [ 585.043999] vfio_group_fops_unl_ioctl+0xd4/0x1e0 [vfio]
> [ 585.044002] __s390x_sys_ioctl+0xc0/0x100
> [ 585.044007] do_syscall+0xee/0x118
> [ 585.044032] __do_syscall+0xd2/0x120
> [ 585.044035] system_call+0x82/0xb0
> [ 585.044037]
> -> #2 (&matrix_dev->guests_lock){+.+.}-{3:3}:
> [ 585.044041] __lock_acquire+0x3e2/0x750
> [ 585.044044] lock_acquire.part.0+0xe2/0x250
> [ 585.044047] lock_acquire+0xac/0x1d0
> [ 585.044049] __mutex_lock+0x9e/0x868
> [ 585.044052] mutex_lock_nested+0x32/0x40
> [ 585.044054] vfio_ap_mdev_open_device+0x8c/0x198 [vfio_ap]
> [ 585.044057] vfio_device_open+0x122/0x168 [vfio]
> [ 585.044060] vfio_device_open_file+0x64/0x120 [vfio]
> [ 585.044064] vfio_group_fops_unl_ioctl+0xd4/0x1e0 [vfio]
> [ 585.044068] __s390x_sys_ioctl+0xc0/0x100
> [ 585.044070] do_syscall+0xee/0x118
> [ 585.044072] __do_syscall+0xd2/0x120
> [ 585.044074] system_call+0x82/0xb0
> [ 585.044076]
> -> #1 (&new_dev_set->lock){+.+.}-{3:3}:
> [ 585.044080] __lock_acquire+0x3e2/0x750
> [ 585.044082] lock_acquire.part.0+0xe2/0x250
> [ 585.044085] lock_acquire+0xac/0x1d0
> [ 585.044088] __mutex_lock+0x9e/0x868
> [ 585.044090] mutex_lock_nested+0x32/0x40
> [ 585.044093] vfio_device_open+0x3e/0x168 [vfio]
> [ 585.044096] vfio_device_open_file+0x64/0x120 [vfio]
> [ 585.044100] vfio_group_fops_unl_ioctl+0xd4/0x1e0 [vfio]
> [ 585.044104] __s390x_sys_ioctl+0xc0/0x100
> [ 585.044106] do_syscall+0xee/0x118
> [ 585.044108] __do_syscall+0xd2/0x120
> [ 585.044110] system_call+0x82/0xb0
> [ 585.044112]
> -> #0 (&group->group_lock){+.+.}-{3:3}:
> [ 585.044115] check_prev_add+0xd4/0xf10
> [ 585.044118] validate_chain+0x698/0x8e8
> [ 585.044120] __lock_acquire+0x3e2/0x750
> [ 585.044123] lock_acquire.part.0+0xe2/0x250
> [ 585.044125] lock_acquire+0xac/0x1d0
> [ 585.044128] __mutex_lock+0x9e/0x868
> [ 585.044130] mutex_lock_nested+0x32/0x40
> [ 585.044133] vfio_file_set_kvm+0x50/0x68 [vfio]
> [ 585.044137] kvm_vfio_release+0x5e/0xf8 [kvm]
> [ 585.044156] kvm_device_release+0x90/0xb8 [kvm]
> [ 585.044175] __fput+0xaa/0x2a0
> [ 585.044180] task_work_run+0x76/0xd0
> [ 585.044183] do_exit+0x248/0x538
> [ 585.044186] do_group_exit+0x40/0xb0
> [ 585.044188] get_signal+0x614/0x698
> [ 585.044192] arch_do_signal_or_restart+0x58/0x370
> [ 585.044195] exit_to_user_mode_loop+0xe8/0x1b8
> [ 585.044200] exit_to_user_mode_prepare+0x164/0x190
> [ 585.044203] __do_syscall+0xd2/0x120
> [ 585.044205] system_call+0x82/0xb0
> [ 585.044207]
> other info that might help us debug this:
>
> [ 585.044209] Chain exists of:
> &group->group_lock --> &matrix_dev->guests_lock --> &kvm->lock
>
> [ 585.044213] Possible unsafe locking scenario:
>
> [ 585.044214] CPU0 CPU1
> [ 585.044216] ---- ----
> [ 585.044217] lock(&kvm->lock);
> [ 585.044219] lock(&matrix_dev->guests_lock);
> [ 585.044221] lock(&kvm->lock);
> [ 585.044223] lock(&group->group_lock);
> [ 585.044225]
> *** DEADLOCK ***
>
> [ 585.044227] 1 lock held by CPU 0/KVM/1173:
> [ 585.044228] #0: 00000000b2960ba0 (&kvm->lock){+.+.}-{3:3}, at: kvm_device_release+0x4a/0xb8 [kvm]
> [ 585.044251]
> stack backtrace:
> [ 585.044253] CPU: 3 PID: 1173 Comm: CPU 0/KVM Not tainted 6.2.0-rc6-00057-g41c03ba9beea-dirty #18
> [ 585.044256] Hardware name: IBM 8561 T01 772 (LPAR)
> [ 585.044257] Call Trace:
> [ 585.044258] [<000000011a818936>] dump_stack_lvl+0x8e/0xc8
> [ 585.044261] [<0000000119aca3f2>] check_noncircular+0x132/0x158
> [ 585.044264] [<0000000119acba44>] check_prev_add+0xd4/0xf10
> [ 585.044267] [<0000000119accf18>] validate_chain+0x698/0x8e8
> [ 585.044270] [<0000000119ace70a>] __lock_acquire+0x3e2/0x750
> [ 585.044273] [<0000000119acf682>] lock_acquire.part.0+0xe2/0x250
> [ 585.044276] [<0000000119acf89c>] lock_acquire+0xac/0x1d0
> [ 585.044279] [<000000011a823c66>] __mutex_lock+0x9e/0x868
> [ 585.044282] [<000000011a824462>] mutex_lock_nested+0x32/0x40
> [ 585.044285] [<000003ff7fbcd6a0>] vfio_file_set_kvm+0x50/0x68 [vfio]
> [ 585.044289] [<000003ff7feacab6>] kvm_vfio_release+0x5e/0xf8 [kvm]
> [ 585.044308] [<000003ff7fea6d58>] kvm_device_release+0x90/0xb8 [kvm]
> [ 585.044328] [<0000000119dbb83a>] __fput+0xaa/0x2a0
> [ 585.044331] [<0000000119a67c66>] task_work_run+0x76/0xd0
> [ 585.044333] [<0000000119a3ec18>] do_exit+0x248/0x538
> [ 585.044335] [<0000000119a3f0c8>] do_group_exit+0x40/0xb0
> [ 585.044338] [<0000000119a50dec>] get_signal+0x614/0x698
> [ 585.044340] [<00000001199ea030>] arch_do_signal_or_restart+0x58/0x370
> [ 585.044343] [<0000000119b0bb50>] exit_to_user_mode_loop+0xe8/0x1b8
> [ 585.044346] [<0000000119b0bd84>] exit_to_user_mode_prepare+0x164/0x190
> [ 585.044349] [<000000011a818d2a>] __do_syscall+0xd2/0x120
> [ 585.044351] [<000000011a82c462>] system_call+0x82/0xb0
> [ 585.044354] INFO: lockdep is turned off.
> [ 610.595528] vfio_ap_mdev 529654a9-bea4-461a-b64d-9d9c63df0deb: Removing from iommu group 0
> [ 610.604408] vfio_ap matrix: MDEV: Unregistering
> [ 610.826074] hades[1099]: Stop 'tests.test_vfio_ap.VfioAPAssignMdevToGuestTest.runTest'
>
> On 1/20/23 10:05 AM, Yi Liu wrote:
>> Currently it is possible that the final put of a KVM reference comes from
>> vfio during its device close operation. This occurs while the vfio group
>> lock is held; however, if the vfio device is still in the kvm device list,
>> then the following call chain could result in a deadlock:
>>
>> VFIO holds group->group_lock/group_rwsem
>> -> kvm_put_kvm
>> -> kvm_destroy_vm
>> -> kvm_destroy_devices
>> -> kvm_vfio_destroy
>> -> kvm_vfio_file_set_kvm
>> -> vfio_file_set_kvm
>> -> try to hold group->group_lock/group_rwsem
>>
>> The key function is the kvm_destroy_devices() which triggers destroy cb
>> of kvm_device_ops. It calls back to vfio and try to hold group_lock. So
>> if this path doesn't call back to vfio, this dead lock would be fixed.
>> Actually, there is a way for it. KVM provides another point to free the
>> kvm-vfio device which is the point when the device file descriptor is
>> closed. This can be achieved by providing the release cb instead of the
>> destroy cb. Also rename kvm_vfio_destroy() to be kvm_vfio_release().
>>
>> /*
>> * Destroy is responsible for freeing dev.
>> *
>> * Destroy may be called before or after destructors are called
>> * on emulated I/O regions, depending on whether a reference is
>> * held by a vcpu or other kvm component that gets destroyed
>> * after the emulated I/O.
>> */
>> void (*destroy)(struct kvm_device *dev);
>>
>> /*
>> * Release is an alternative method to free the device. It is
>> * called when the device file descriptor is closed. Once
>> * release is called, the destroy method will not be called
>> * anymore as the device is removed from the device list of
>> * the VM. kvm->lock is held.
>> */
>> void (*release)(struct kvm_device *dev);
>>
>> Fixes: 421cfe6596f6 ("vfio: remove VFIO_GROUP_NOTIFY_SET_KVM")
>> Reported-by: Alex Williamson <alex.williamson@redhat.com>
>> Suggested-by: Kevin Tian <kevin.tian@intel.com>
>> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>> ---
>> virt/kvm/vfio.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c
>> index 495ceabffe88..e94f3ea718e5 100644
>> --- a/virt/kvm/vfio.c
>> +++ b/virt/kvm/vfio.c
>> @@ -336,7 +336,7 @@ static int kvm_vfio_has_attr(struct kvm_device *dev,
>> return -ENXIO;
>> }
>> -static void kvm_vfio_destroy(struct kvm_device *dev)
>> +static void kvm_vfio_release(struct kvm_device *dev)
>> {
>> struct kvm_vfio *kv = dev->private;
>> struct kvm_vfio_group *kvg, *tmp;
>> @@ -363,7 +363,7 @@ static int kvm_vfio_create(struct kvm_device *dev, u32 type);
>> static struct kvm_device_ops kvm_vfio_ops = {
>> .name = "kvm-vfio",
>> .create = kvm_vfio_create,
>> - .destroy = kvm_vfio_destroy,
>> + .release = kvm_vfio_release,
>> .set_attr = kvm_vfio_set_attr,
>> .has_attr = kvm_vfio_has_attr,
>> };
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] kvm/vfio: Fix potential deadlock on vfio group_lock
2023-01-31 14:34 ` [Intel-gfx] " Jason Gunthorpe
@ 2023-01-31 14:46 ` Anthony Krowiak
-1 siblings, 0 replies; 26+ messages in thread
From: Anthony Krowiak @ 2023-01-31 14:46 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Yi Liu, alex.williamson, pbonzini, mjrosato, kevin.tian, cohuck,
farman, pmorel, borntraeger, frankja, imbrenda, david, jjherne,
pasic, zhenyuw, zhi.a.wang, seanjc, linux-s390, kvm,
intel-gvt-dev, intel-gfx, linux-kernel
On 1/31/23 9:34 AM, Jason Gunthorpe wrote:
> On Tue, Jan 31, 2023 at 09:27:54AM -0500, Anthony Krowiak wrote:
>> I encountered a lockdep splat while running some regression tests today (see
>> below). I suspected it might be this patch so I reverted it, rebuilt the
>> kernel and ran the regression tests again; this time, the test ran cleanly.
>> It looks like this patch may not have fixed the problem for which it was
>> intended. Here is the relevant dmesg output:
> Well, it fixes the deadlock it intended to fix and created another one
> :)
>
> It means device drivers cannot obtain the kvm lock from their open
> functions in this new model.
>
> Why does ap need to touch kvm->lock? (via get_update_locks_for_kvm)
We need the kvm->lock because we take the vCPUs out of SIE in order to
dynamically change values in the APCB.
>
> Maybe you should split that lock and have a dedicated apcb lock?
I don't think that would suffice for taking the vCPUs out of SIE.
>
> Jason
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Intel-gfx] [PATCH] kvm/vfio: Fix potential deadlock on vfio group_lock
@ 2023-01-31 14:46 ` Anthony Krowiak
0 siblings, 0 replies; 26+ messages in thread
From: Anthony Krowiak @ 2023-01-31 14:46 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: mjrosato, david, farman, imbrenda, linux-s390, Yi Liu, kvm,
pasic, borntraeger, intel-gfx, intel-gvt-dev, frankja, jjherne,
pmorel, seanjc, cohuck, linux-kernel, pbonzini
On 1/31/23 9:34 AM, Jason Gunthorpe wrote:
> On Tue, Jan 31, 2023 at 09:27:54AM -0500, Anthony Krowiak wrote:
>> I encountered a lockdep splat while running some regression tests today (see
>> below). I suspected it might be this patch so I reverted it, rebuilt the
>> kernel and ran the regression tests again; this time, the test ran cleanly.
>> It looks like this patch may not have fixed the problem for which it was
>> intended. Here is the relevant dmesg output:
> Well, it fixes the deadlock it intended to fix and created another one
> :)
>
> It means device drivers cannot obtain the kvm lock from their open
> functions in this new model.
>
> Why does ap need to touch kvm->lock? (via get_update_locks_for_kvm)
We need the kvm->lock because we take the vCPUs out of SIE in order to
dynamically change values in the APCB.
>
> Maybe you should split that lock and have a dedicated apcb lock?
I don't think that would suffice for taking the vCPUs out of SIE.
>
> Jason
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] kvm/vfio: Fix potential deadlock on vfio group_lock
2023-01-31 14:46 ` [Intel-gfx] " Anthony Krowiak
@ 2023-01-31 14:48 ` Jason Gunthorpe
-1 siblings, 0 replies; 26+ messages in thread
From: Jason Gunthorpe @ 2023-01-31 14:48 UTC (permalink / raw)
To: Anthony Krowiak
Cc: Yi Liu, alex.williamson, pbonzini, mjrosato, kevin.tian, cohuck,
farman, pmorel, borntraeger, frankja, imbrenda, david, jjherne,
pasic, zhenyuw, zhi.a.wang, seanjc, linux-s390, kvm,
intel-gvt-dev, intel-gfx, linux-kernel
On Tue, Jan 31, 2023 at 09:46:18AM -0500, Anthony Krowiak wrote:
> > Maybe you should split that lock and have a dedicated apcb lock?
>
> I don't think that would suffice for taking the vCPUs out of SIE.
Then I think we have to keep this patch and also do Matthew's patch to
keep kvm refs inside vfio as well.
Jason
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Intel-gfx] [PATCH] kvm/vfio: Fix potential deadlock on vfio group_lock
@ 2023-01-31 14:48 ` Jason Gunthorpe
0 siblings, 0 replies; 26+ messages in thread
From: Jason Gunthorpe @ 2023-01-31 14:48 UTC (permalink / raw)
To: Anthony Krowiak
Cc: mjrosato, david, farman, imbrenda, linux-s390, Yi Liu, kvm,
pasic, borntraeger, intel-gfx, intel-gvt-dev, frankja, jjherne,
pmorel, seanjc, cohuck, linux-kernel, pbonzini
On Tue, Jan 31, 2023 at 09:46:18AM -0500, Anthony Krowiak wrote:
> > Maybe you should split that lock and have a dedicated apcb lock?
>
> I don't think that would suffice for taking the vCPUs out of SIE.
Then I think we have to keep this patch and also do Matthew's patch to
keep kvm refs inside vfio as well.
Jason
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Intel-gfx] [PATCH] kvm/vfio: Fix potential deadlock on vfio group_lock
2023-01-31 14:48 ` [Intel-gfx] " Jason Gunthorpe
@ 2023-01-31 15:00 ` Matthew Rosato
-1 siblings, 0 replies; 26+ messages in thread
From: Matthew Rosato @ 2023-01-31 15:00 UTC (permalink / raw)
To: Jason Gunthorpe, Anthony Krowiak
Cc: jjherne, linux-s390, farman, Yi Liu, frankja, pmorel, david,
imbrenda, seanjc, intel-gfx, cohuck, linux-kernel, pasic, kvm,
pbonzini, borntraeger, intel-gvt-dev
On 1/31/23 9:48 AM, Jason Gunthorpe wrote:
> On Tue, Jan 31, 2023 at 09:46:18AM -0500, Anthony Krowiak wrote:
>
>>> Maybe you should split that lock and have a dedicated apcb lock?
>>
>> I don't think that would suffice for taking the vCPUs out of SIE.
>
> Then I think we have to keep this patch and also do Matthew's patch to
> keep kvm refs inside vfio as well.
>
I don't think keeping kvm refs inside vfio solves this issue though -- Even if we handle the kvm_put_kvm asynchronously within vfio as previously proposed, kvm_vfio_release will eventually get called and it gets called with the kvm->lock already held, then proceeds to call vfio_file_set_kvm which gets the group->lock. That order conflicts with the hierarchy used by the driver during open_device of vfio->group_lock ... kvm->lock.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] kvm/vfio: Fix potential deadlock on vfio group_lock
@ 2023-01-31 15:00 ` Matthew Rosato
0 siblings, 0 replies; 26+ messages in thread
From: Matthew Rosato @ 2023-01-31 15:00 UTC (permalink / raw)
To: Jason Gunthorpe, Anthony Krowiak
Cc: Yi Liu, alex.williamson, pbonzini, kevin.tian, cohuck, farman,
pmorel, borntraeger, frankja, imbrenda, david, jjherne, pasic,
zhenyuw, zhi.a.wang, seanjc, linux-s390, kvm, intel-gvt-dev,
intel-gfx, linux-kernel
On 1/31/23 9:48 AM, Jason Gunthorpe wrote:
> On Tue, Jan 31, 2023 at 09:46:18AM -0500, Anthony Krowiak wrote:
>
>>> Maybe you should split that lock and have a dedicated apcb lock?
>>
>> I don't think that would suffice for taking the vCPUs out of SIE.
>
> Then I think we have to keep this patch and also do Matthew's patch to
> keep kvm refs inside vfio as well.
>
I don't think keeping kvm refs inside vfio solves this issue though -- Even if we handle the kvm_put_kvm asynchronously within vfio as previously proposed, kvm_vfio_release will eventually get called and it gets called with the kvm->lock already held, then proceeds to call vfio_file_set_kvm which gets the group->lock. That order conflicts with the hierarchy used by the driver during open_device of vfio->group_lock ... kvm->lock.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Intel-gfx] [PATCH] kvm/vfio: Fix potential deadlock on vfio group_lock
2023-01-31 15:00 ` Matthew Rosato
@ 2023-01-31 15:12 ` Jason Gunthorpe
-1 siblings, 0 replies; 26+ messages in thread
From: Jason Gunthorpe @ 2023-01-31 15:12 UTC (permalink / raw)
To: Matthew Rosato
Cc: kvm, david, farman, imbrenda, linux-s390, Yi Liu, frankja, pasic,
borntraeger, jjherne, intel-gfx, intel-gvt-dev, Anthony Krowiak,
pmorel, seanjc, cohuck, linux-kernel, pbonzini
On Tue, Jan 31, 2023 at 10:00:14AM -0500, Matthew Rosato wrote:
> On 1/31/23 9:48 AM, Jason Gunthorpe wrote:
> > On Tue, Jan 31, 2023 at 09:46:18AM -0500, Anthony Krowiak wrote:
> >
> >>> Maybe you should split that lock and have a dedicated apcb lock?
> >>
> >> I don't think that would suffice for taking the vCPUs out of SIE.
> >
> > Then I think we have to keep this patch and also do Matthew's patch to
> > keep kvm refs inside vfio as well.
> >
>
> I don't think keeping kvm refs inside vfio solves this issue though
> -- Even if we handle the kvm_put_kvm asynchronously within vfio as
> previously proposed, kvm_vfio_release will eventually get called and
> it gets called with the kvm->lock already held, then proceeds to
> call vfio_file_set_kvm which gets the group->lock. That order
> conflicts with the hierarchy used by the driver during open_device
> of vfio->group_lock ... kvm->lock.
The group lock is held by vfio_file_set_kvm() only because we don't
have a refcount and we have to hold it across the open call to keep
the pointer alive.
With proper refcounting you'd split this to a spinlock and hold it
only while obtaining the get ref for the open thread.
Jason
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] kvm/vfio: Fix potential deadlock on vfio group_lock
@ 2023-01-31 15:12 ` Jason Gunthorpe
0 siblings, 0 replies; 26+ messages in thread
From: Jason Gunthorpe @ 2023-01-31 15:12 UTC (permalink / raw)
To: Matthew Rosato
Cc: Anthony Krowiak, Yi Liu, alex.williamson, pbonzini, kevin.tian,
cohuck, farman, pmorel, borntraeger, frankja, imbrenda, david,
jjherne, pasic, zhenyuw, zhi.a.wang, seanjc, linux-s390, kvm,
intel-gvt-dev, intel-gfx, linux-kernel
On Tue, Jan 31, 2023 at 10:00:14AM -0500, Matthew Rosato wrote:
> On 1/31/23 9:48 AM, Jason Gunthorpe wrote:
> > On Tue, Jan 31, 2023 at 09:46:18AM -0500, Anthony Krowiak wrote:
> >
> >>> Maybe you should split that lock and have a dedicated apcb lock?
> >>
> >> I don't think that would suffice for taking the vCPUs out of SIE.
> >
> > Then I think we have to keep this patch and also do Matthew's patch to
> > keep kvm refs inside vfio as well.
> >
>
> I don't think keeping kvm refs inside vfio solves this issue though
> -- Even if we handle the kvm_put_kvm asynchronously within vfio as
> previously proposed, kvm_vfio_release will eventually get called and
> it gets called with the kvm->lock already held, then proceeds to
> call vfio_file_set_kvm which gets the group->lock. That order
> conflicts with the hierarchy used by the driver during open_device
> of vfio->group_lock ... kvm->lock.
The group lock is held by vfio_file_set_kvm() only because we don't
have a refcount and we have to hold it across the open call to keep
the pointer alive.
With proper refcounting you'd split this to a spinlock and hold it
only while obtaining the get ref for the open thread.
Jason
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2023-01-31 15:15 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-20 15:05 [PATCH] kvm/vfio: Fix potential deadlock on vfio group_lock Yi Liu
2023-01-20 15:05 ` [Intel-gfx] " Yi Liu
2023-01-20 15:08 ` Liu, Yi L
2023-01-20 15:08 ` [Intel-gfx] " Liu, Yi L
2023-01-20 15:45 ` Matthew Rosato
2023-01-20 15:45 ` [Intel-gfx] " Matthew Rosato
2023-01-20 15:49 ` Alex Williamson
2023-01-20 15:49 ` Alex Williamson
2023-01-20 17:58 ` Alex Williamson
2023-01-20 17:58 ` [Intel-gfx] " Alex Williamson
2023-01-20 19:25 ` [Intel-gfx] ✓ Fi.CI.BAT: success for " Patchwork
2023-01-21 20:16 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork
2023-01-31 14:27 ` [PATCH] " Anthony Krowiak
2023-01-31 14:27 ` [Intel-gfx] " Anthony Krowiak
2023-01-31 14:34 ` Jason Gunthorpe
2023-01-31 14:34 ` [Intel-gfx] " Jason Gunthorpe
2023-01-31 14:46 ` Anthony Krowiak
2023-01-31 14:46 ` [Intel-gfx] " Anthony Krowiak
2023-01-31 14:48 ` Jason Gunthorpe
2023-01-31 14:48 ` [Intel-gfx] " Jason Gunthorpe
2023-01-31 15:00 ` Matthew Rosato
2023-01-31 15:00 ` Matthew Rosato
2023-01-31 15:12 ` [Intel-gfx] " Jason Gunthorpe
2023-01-31 15:12 ` Jason Gunthorpe
2023-01-31 14:35 ` Matthew Rosato
2023-01-31 14:35 ` [Intel-gfx] " Matthew Rosato
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.