[PATCH 2/2] drm/i915/gvt: Fix inconsistent locks holding sequence

[Zhenyu Wang]

On 2017.06.20 10:39:56 +0000, Dong, Chuanxiao wrote:
> > On 2017.06.19 17:04:11 +0800, Chuanxiao Dong wrote:
> > > There are two kinds of locking sequence.
> > >
> > > One is in the thread which is started by vfio ioctl to do the iommu
> > > unmapping. The locking sequence is:
> > > 	down_read(&group_lock) ----> mutex_lock(&cached_lock)
> > >
> > > The other is in the vfio release thread which will unpin all the
> > > cached pages. The lock sequence is:
> > > 	mutex_lock(&cached_lock) ---> down_read(&group_lock)
> > >
> > > And, the cache_lock is used to protect the rb tree of the cache node
> > > and doing vfio unpin doesn't require this lock. Move the vfio unpin
> > > out of the cache_lock protected region.
> > >
> > > Fixes: f30437c5e7bf ("drm/i915/gvt: add KVMGT support")
> > > Signed-off-by: Chuanxiao Dong <chuanxiao.dong at intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/gvt/kvmgt.c | 25 ++++++++++++-------------
> > >  1 file changed, 12 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c
> > > b/drivers/gpu/drm/i915/gvt/kvmgt.c
> > > index 88a4d8f..1405c60 100644
> > > --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
> > > +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
> > > @@ -192,13 +192,6 @@ static void gvt_cache_add(struct intel_vgpu
> > *vgpu, gfn_t gfn,
> > >  	kfree(new);
> > >  }
> > >
> > > -static void __gvt_cache_remove_entry(struct intel_vgpu *vgpu,
> > > -				struct gvt_dma *entry)
> > > -{
> > > -	rb_erase(&entry->node, &vgpu->vdev.cache);
> > > -	kfree(entry);
> > > -}
> > > -
> > >  static void intel_vgpu_unpin_work(struct work_struct *work)  {
> > >  	struct intel_vgpu *vgpu = container_of(work, struct intel_vgpu, @@
> > > -260,16 +253,22 @@ static void gvt_cache_destroy(struct intel_vgpu
> > *vgpu)
> > >  	struct device *dev = mdev_dev(vgpu->vdev.mdev);
> > >  	unsigned long gfn;
> > >
> > > -	mutex_lock(&vgpu->vdev.cache_lock);
> > > -	while ((node = rb_first(&vgpu->vdev.cache))) {
> > > +	do {
> > > +		mutex_lock(&vgpu->vdev.cache_lock);
> > > +		node = rb_first(&vgpu->vdev.cache);
> > > +		if (!node) {
> > > +			mutex_unlock(&vgpu->vdev.cache_lock);
> > > +			break;
> > > +		}
> > >  		dma = rb_entry(node, struct gvt_dma, node);
> > >  		gvt_dma_unmap_iova(vgpu, dma->iova);
> > > -		gfn = dma->gfn;
> > > +		rb_erase(&dma->node, &vgpu->vdev.cache);
> > > +		mutex_unlock(&vgpu->vdev.cache_lock);
> > >
> > > +		gfn = dma->gfn;
> > >  		vfio_unpin_pages(dev, &gfn, 1);
> > > -		__gvt_cache_remove_entry(vgpu, dma);
> > > -	}
> > > -	mutex_unlock(&vgpu->vdev.cache_lock);
> > > +		kfree(dma);
> > > +	} while (1);
> > >  }
> > 
> > For me, I might like to change as
> > 
> > while ((node = rb_first(...))) {
> >       mutex_lock(&cache_lock);
> 
> In this case, rb_first() won't be protected by cache_lock for getting the node.  We may still want this to be protected.
> 

oh, yeah, you're right, might just use for(;;) though.

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/intel-gvt-dev/attachments/20170620/313a5cfe/attachment.sig>