[v2,1/2] vfio iommu type1: Fix size argument to vfio_find_dma() during DMA UNMAP.

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).
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(-)

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;

On 12/6/2016 11:08 PM, Alex Williamson wrote:
> 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,
> 

Makes sense. Updating the description of both patch.

Thanks,
Kirti

> 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;
>