Re: [PATCH v13 11/22] vfio iommu: Add blocking notifier to notify DMA_UNMAP

[Alex Williamson]

On Tue, 15 Nov 2016 20:16:12 -0700
Alex Williamson <alex.williamson@xxxxxxxxxx> wrote:

> On Wed, 16 Nov 2016 08:16:15 +0530
> Kirti Wankhede <kwankhede@xxxxxxxxxx> wrote:
> 
> > On 11/16/2016 3:49 AM, Alex Williamson wrote:  
> > > On Tue, 15 Nov 2016 20:59:54 +0530
> > > Kirti Wankhede <kwankhede@xxxxxxxxxx> wrote:
> > >     
> > ...
> >   
> > >> @@ -854,7 +857,28 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
> > >>  		 */
> > >>  		if (dma->task->mm != current->mm)
> > >>  			break;
> > >> +
> > >>  		unmapped += dma->size;
> > >> +
> > >> +		if (iommu->external_domain && !RB_EMPTY_ROOT(&dma->pfn_list)) {
> > >> +			struct vfio_iommu_type1_dma_unmap nb_unmap;
> > >> +
> > >> +			nb_unmap.iova = dma->iova;
> > >> +			nb_unmap.size = dma->size;
> > >> +
> > >> +			/*
> > >> +			 * Notifier callback would call vfio_unpin_pages() which
> > >> +			 * would acquire iommu->lock. Release lock here and
> > >> +			 * reacquire it again.
> > >> +			 */
> > >> +			mutex_unlock(&iommu->lock);
> > >> +			blocking_notifier_call_chain(&iommu->notifier,
> > >> +						    VFIO_IOMMU_NOTIFY_DMA_UNMAP,
> > >> +						    &nb_unmap);
> > >> +			mutex_lock(&iommu->lock);
> > >> +			if (WARN_ON(!RB_EMPTY_ROOT(&dma->pfn_list)))
> > >> +				break;
> > >> +		}    
> > > 
> > > 
> > > Why exactly do we need to notify per vfio_dma rather than per unmap
> > > request?  If we do the latter we can send the notify first, limiting us
> > > to races where a page is pinned between the notify and the locking,
> > > whereas here, even our dma pointer is suspect once we re-acquire the
> > > lock, we don't technically know if another unmap could have removed
> > > that already.  Perhaps something like this (untested):
> > >     
> > 
> > There are checks to validate unmap request, like v2 check and who is
> > calling unmap and is it allowed for that task to unmap. Before these
> > checks its not sure that unmap region range which asked for would be
> > unmapped all. Notify call should be at the place where its sure that the
> > range provided to notify call is definitely going to be removed. My
> > change do that.  
> 
> Ok, but that does solve the problem.  What about this (untested):

s/does/does not/

BTW, I like how the retries here fill the gap in my previous proposal
where we could still race re-pinning.  We've given it an honest shot or
someone is not participating if we've retried 10 times.  I don't
understand why the test for iommu->external_domain was there, clearly
if the list is not empty, we need to notify.  Thanks,

Alex

> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index ee9a680..50cafdf 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -782,9 +782,9 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
>  			     struct vfio_iommu_type1_dma_unmap *unmap)
>  {
>  	uint64_t mask;
> -	struct vfio_dma *dma;
> +	struct vfio_dma *dma, *dma_last = NULL;
>  	size_t unmapped = 0;
> -	int ret = 0;
> +	int ret = 0, retries;
>  
>  	mask = ((uint64_t)1 << __ffs(vfio_pgsize_bitmap(iommu))) - 1;
>  
> @@ -794,7 +794,7 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
>  		return -EINVAL;
>  
>  	WARN_ON(mask & PAGE_MASK);
> -
> +again:
>  	mutex_lock(&iommu->lock);
>  
>  	/*
> @@ -851,11 +851,16 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
>  		if (dma->task->mm != current->mm)
>  			break;
>  
> -		unmapped += dma->size;
> -
> -		if (iommu->external_domain && !RB_EMPTY_ROOT(&dma->pfn_list)) {
> +		if (!RB_EMPTY_ROOT(&dma->pfn_list)) {
>  			struct vfio_iommu_type1_dma_unmap nb_unmap;
>  
> +			if (dma_last == dma) {
> +				BUG_ON(++retries > 10);
> +			} else {
> +				dma_last = dma;
> +				retries = 0;
> +			}
> +
>  			nb_unmap.iova = dma->iova;
>  			nb_unmap.size = dma->size;
>  
> @@ -868,11 +873,11 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
>  			blocking_notifier_call_chain(&iommu->notifier,
>  						    VFIO_IOMMU_NOTIFY_DMA_UNMAP,
>  						    &nb_unmap);
> -			mutex_lock(&iommu->lock);
> -			if (WARN_ON(!RB_EMPTY_ROOT(&dma->pfn_list)))
> -				break;
> +			goto again:
>  		}
> +		unmapped += dma->size;
>  		vfio_remove_dma(iommu, dma);
> +
>  	}
>  
>  unlock: