Re: [PATCH v3] vfio: fix deadlock between group lock and kvm lock

[Alex Williamson]

On Fri, 3 Feb 2023 12:29:01 -0500
Matthew Rosato <mjrosato@xxxxxxxxxxxxx> wrote:

> On 2/3/23 10:19 AM, Alex Williamson wrote:
> > On Fri, 3 Feb 2023 14:54:44 +0000
> > "Liu, Yi L" <yi.l.liu@xxxxxxxxx> wrote:
> >   
> >>> From: Alex Williamson <alex.williamson@xxxxxxxxxx>
> >>> Sent: Friday, February 3, 2023 9:50 PM
> >>>
> >>> On Fri, 3 Feb 2023 13:32:09 +0000
> >>> "Liu, Yi L" <yi.l.liu@xxxxxxxxx> wrote:
> >>>     
> >>>>> From: Tian, Kevin <kevin.tian@xxxxxxxxx>
> >>>>> Sent: Friday, February 3, 2023 10:00 AM
> >>>>>    
> >>>>>> From: Alex Williamson <alex.williamson@xxxxxxxxxx>
> >>>>>> Sent: Friday, February 3, 2023 7:13 AM
> >>>>>>
> >>>>>> On Thu, 2 Feb 2023 23:04:10 +0000
> >>>>>> "Tian, Kevin" <kevin.tian@xxxxxxxxx> wrote:
> >>>>>>    
> >>>>>>>> From: Alex Williamson <alex.williamson@xxxxxxxxxx>
> >>>>>>>> Sent: Friday, February 3, 2023 3:42 AM
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> LGTM.  I'm not sure moving the functions to vfio_main really buys    
> >>> us    
> >>>>>>>> anything since we're making so much use of group fields.  The cdev
> >>>>>>>> approach will necessarily be different, so the bulk of the get code    
> >>> will    
> >>>>>>>> likely need to move back to group.c anyway.
> >>>>>>>>    
> >>>>>>>
> >>>>>>> well my last comment was based on Matthew's v2 where the get    
> >>> code    
> >>>>>>> gets a kvm passed in instead of implicitly retrieving group ref_lock
> >>>>>>> internally. In that case the get/put helpers only contain device logic
> >>>>>>> thus fit in vfio_main.c.
> >>>>>>>
> >>>>>>> with v3 then they have to be in group.c since we don't want to use
> >>>>>>> group fields in vfio_main.c.
> >>>>>>>
> >>>>>>> but I still think v2 of the helpers is slightly better. The only difference
> >>>>>>> between cdev and group when handling this race is using different
> >>>>>>> ref_lock. the symbol get/put part is exactly same. So even if we
> >>>>>>> merge v3 like this, very likely Yi has to change it back to v2 style
> >>>>>>> to share the get/put helpers while just leaving the ref_lock part
> >>>>>>> handled differently between the two path.    
> >>>>>>
> >>>>>> I'm not really a fan of the asymmetry of the v2 version where the get
> >>>>>> helper needs to be called under the new kvm_ref_lock, but the put
> >>>>>> helper does not.  Having the get helper handle that makes the caller
> >>>>>> much cleaner.  Thanks,
> >>>>>>    
> >>>>>
> >>>>> What about passing the lock pointer into the helper? it's still slightly
> >>>>> asymmetry as the put helper doesn't carry the lock pointer but it
> >>>>> could also be interpreted as if the pointer has been saved in the get
> >>>>> then if it needs to be referenced by the put there is no need to pass
> >>>>> it in again.    
> >>>>
> >>>> For cdev, I may modify vfio_device_get_kvm_safe() to accept
> >>>> struct kvm and let its caller hold a kvm_ref_lock (field within
> >>>> struct vfio_device_file). Meanwhile, the group path holds
> >>>> the group->kvm_ref_lock before invoking vfio_device_get_kvm_safe().
> >>>> vfio_device_get_kvm_safe() just includes the symbol get/put and
> >>>> the device->kvm and put_kvm set.    
> >>>
> >>> Sounds a lot like v2 :-\     
> >>
> >> Yes, like v2. 😊
> >>  
> >>> I'd look more towards group and cdev specific
> >>> helpers that handle the locking so that the callers aren't exposed to
> >>> the asymmetry of get vs put, and reduce a new
> >>> _vfio_device_get_kvm_safe() in common code that only does the symbol
> >>> work.  Thanks,    
> >>
> >> If so, looks like Matthew needs a v4. I'm waiting for the final version
> >> of this patch and sending a new cdev series based on it. wish to see
> >> it soon ^_^.  
> > 
> > cdev support is a future feature, why does it become a requirement for
> > a fix to the current base?  The refactoring could also happen in the
> > cdev series.  Thanks,
> > 
> > Alex
> >   
> 
> FWIW, that would bloat the fix by a bit and it's already a decent-sized fix...  I took a quick stab and such a v4 would end up with a total of 120 insertions(+), 15 deletions(-).  See below for a diff of what I tried on top of v3. The idea would be to move the serialization and setting/clearing of device->kvm into group/cdev and use device->kvm as the value within vfio_device_get_kvm_safe for getting the kvm ref.  Wrappers in group/cdev would then be responsible for backing that device->kvm setting out on ref failure.
> Each side (group/cdev) can wrap their own serialization / load device->kvm from the appropriate location / handle the failed get / clear device->kvm when done (since get doesn't set it, put shouldn't clear it).
> 
> If Alex wants, I can wrap something like this into a v4 -- Or, if we don't want to include this kind of infrastructure in the fix, then Yi please feel free to use it as a starting point for what you need in cdev.
> 
> Thanks,
> Matt
> 
> diff --git a/drivers/vfio/group.c b/drivers/vfio/group.c
> index 7fed4233ca23..261af23975ae 100644
> --- a/drivers/vfio/group.c
> +++ b/drivers/vfio/group.c
> @@ -154,6 +154,31 @@ static int vfio_group_ioctl_set_container(struct vfio_group *group,
>  	return ret;
>  }
>  
> +static void vfio_device_group_get_kvm(struct vfio_device *device)
> +{
> +	lockdep_assert_held(&device->dev_set->lock);
> +
> +	spin_lock(&device->group->kvm_ref_lock);
> +
> +	if (!device->group->kvm)
> +		goto unlock;
> +
> +	device->kvm = device->group->kvm;
> +	if (!vfio_device_get_kvm_safe(device))

I'd probably go back to making this:

void _vfio_device_get_kvm_safe(struct vfio_device *device, struct kvm *kvm);

so the vfio_main function would handle setting and clearing
device->kvm.  That way we could also move the lockdep into the
vfio_main functions.  Once we do that, there's no reason to have a
group vs cdev put function and we end up with only a wrapper on the get
function, which should really never be used directly, so we prefix it
with an underscore.

At that point (see incremental diff below), it's about a wash.  Current v3:

 drivers/vfio/group.c     |   32 +++++++++++++----
 drivers/vfio/vfio.h      |   14 +++++++
 drivers/vfio/vfio_main.c |   70 +++++++++++++++++++++++++++++++++++----
 include/linux/vfio.h     |    2 -
 4 files changed, 103 insertions(+), 15 deletions(-)

Folding in below:

 drivers/vfio/group.c     |   44 ++++++++++++++++++++++-----
 drivers/vfio/vfio.h      |   15 +++++++++
 drivers/vfio/vfio_main.c |   63 ++++++++++++++++++++++++++++++++++-----
 include/linux/vfio.h     |    2 -
 4 files changed, 109 insertions(+), 15 deletions(-)

Unfortunately it seems I've talked myself into the answer that we
should maybe just pre-enable cdev by not adding a group reference in
vfio_main.  Thanks,

Alex

diff --git a/drivers/vfio/group.c b/drivers/vfio/group.c
index 7fed4233ca23..98621ac082f0 100644
--- a/drivers/vfio/group.c
+++ b/drivers/vfio/group.c
@@ -154,6 +154,18 @@ static int vfio_group_ioctl_set_container(struct vfio_group *group,
 	return ret;
 }
 
+static void vfio_device_group_get_kvm_safe(struct vfio_device *device)
+{
+	spin_lock(&device->group->kvm_ref_lock);
+	if (!device->group->kvm)
+		goto unlock;
+
+	_vfio_device_get_kvm_safe(device, device->group->kvm);
+
+unlock:
+	spin_unlock(&device->group->kvm_ref_lock);
+}
+
 static int vfio_device_group_open(struct vfio_device *device)
 {
 	int ret;
@@ -173,7 +185,7 @@ static int vfio_device_group_open(struct vfio_device *device)
 	 * the pointer in the device for use by drivers.
 	 */
 	if (device->open_count == 0)
-		vfio_device_get_kvm_safe(device);
+		vfio_device_group_get_kvm_safe(device);
 
 	ret = vfio_device_open(device, device->group->iommufd, device->kvm);
 
diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h
index 20d715b0a3a8..24d6cd285945 100644
--- a/drivers/vfio/vfio.h
+++ b/drivers/vfio/vfio.h
@@ -253,10 +253,11 @@ enum { vfio_noiommu = false };
 #endif
 
 #ifdef CONFIG_HAVE_KVM
-void vfio_device_get_kvm_safe(struct vfio_device *device);
+void _vfio_device_get_kvm_safe(struct vfio_device *device, struct kvm *kvm);
 void vfio_device_put_kvm(struct vfio_device *device);
 #else
-static inline void vfio_device_get_kvm_safe(struct vfio_device *device)
+static inline void _vfio_device_get_kvm_safe(struct vfio_device *device,
+					     struct kvm *kvm)
 {
 }
 
diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
index 4762550e9f42..00d4d5167d6c 100644
--- a/drivers/vfio/vfio_main.c
+++ b/drivers/vfio/vfio_main.c
@@ -342,7 +342,7 @@ void vfio_unregister_group_dev(struct vfio_device *device)
 EXPORT_SYMBOL_GPL(vfio_unregister_group_dev);
 
 #ifdef CONFIG_HAVE_KVM
-void vfio_device_get_kvm_safe(struct vfio_device *device)
+void _vfio_device_get_kvm_safe(struct vfio_device *device, struct kvm *kvm)
 {
 	void (*pfn)(struct kvm *kvm);
 	bool (*fn)(struct kvm *kvm);
@@ -350,32 +350,25 @@ void vfio_device_get_kvm_safe(struct vfio_device *device)
 
 	lockdep_assert_held(&device->dev_set->lock);
 
-	spin_lock(&device->group->kvm_ref_lock);
-	if (!device->group->kvm)
-		goto unlock;
-
 	pfn = symbol_get(kvm_put_kvm);
 	if (WARN_ON(!pfn))
-		goto unlock;
+		return;
 
 	fn = symbol_get(kvm_get_kvm_safe);
 	if (WARN_ON(!fn)) {
 		symbol_put(kvm_put_kvm);
-		goto unlock;
+		return;
 	}
 
 	ret = fn(device->group->kvm);
 	symbol_put(kvm_get_kvm_safe);
 	if (!ret) {
 		symbol_put(kvm_put_kvm);
-		goto unlock;
+		return;
 	}
 
 	device->put_kvm = pfn;
-	device->kvm = device->group->kvm;
-
-unlock:
-	spin_unlock(&device->group->kvm_ref_lock);
+	device->kvm = kvm;
 }
 
 void vfio_device_put_kvm(struct vfio_device *device)