Date: Tue, 6 Dec 2016 10:38:59 -0700
From: Alex Williamson <alex.williamson@redhat.com>
To: Kirti Wankhede <kwankhede@nvidia.com>
Cc: <pbonzini@redhat.com>, <kraxel@redhat.com>, <cjia@nvidia.com>,
        <qemu-devel@nongnu.org>, <kvm@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 1/2] vfio iommu type1: Fix size argument to
 vfio_find_dma() during DMA UNMAP.
Message-ID: <20161206103859.2fd91db0@t450s.home>
In-Reply-To: <1481044410-23862-1-git-send-email-kwankhede@nvidia.com>
References: <1481044410-23862-1-git-send-email-kwankhede@nvidia.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2250
Lines: 50

On Tue, 6 Dec 2016 22:43:30 +0530
Kirti Wankhede <kwankhede@nvidia.com> wrote:

> vfio_dma keeps track of address range from (dma->iova + 0) to
> (dma->iova + dma->size - 1), while vfio_find_dma() search logic looks for
> range from (dma->iova + 1) to (dma->iova + dma->size).

This is not true.  The issue is with the non-inclusive address at the
end of a range being incompatible with passing zero for the range
size.  Passing zero for the range size is a bit of a hack and doing
such triggers a corner case in the end of range detection where we test
(start + size <= dma->iova).  Using <= here covers the non-inclusive
range end, ie. if the range was start=0x0, size=0x1000, the range is
actually 0x0-0xfff.  Thus if start+size is 0x1000, it should not be
considered part of a range beginning at 0x1000.  So there's no
incompatibility as implied in the statement above, it's more that using
zero for the size isn't compatible with matching the start address of
an existing vfio_dma.  Thanks,

Alex

> In vfio_find_dma(), when the start address is equal to dma->iova and size
> is 0, check for the end of search range makes it to take wrong side of
> RB-tree. That fails the search even though the address is present in
> mapped dma ranges. Due to this, in vfio_dma_do_unmap(), while checking
> boundary conditions, size should be set to 1 for verifying start address
> of unmap range.
> vfio_find_dma() is also used to verify last address in unmap range with
> size = 0, but in that case address to be searched is calculated with
> size - 1 and so it works correctly.
> 
> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
> Signed-off-by: Neo Jia <cjia@nvidia.com>
> ---
>  drivers/vfio/vfio_iommu_type1.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index a28fbddb505c..8e9e94ccb2ff 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -826,7 +826,7 @@ again:
>  	 * mappings within the range.
>  	 */
>  	if (iommu->v2) {
> -		dma = vfio_find_dma(iommu, unmap->iova, 0);
> +		dma = vfio_find_dma(iommu, unmap->iova, 1);
>  		if (dma && dma->iova != unmap->iova) {
>  			ret = -EINVAL;
>  			goto unlock;