[PATCH v2] vfio/type1: Fix vfio_find_dma_valid return

Fix vfio_find_dma_valid to return WAITED on success if it was necessary
to wait which mean iommu lock was dropped and reacquired.  This allows
vfio_iommu_type1_pin_pages to recheck vaddr_invalid_count and possibly
avoid the checking the validity of every vaddr in its list.

Signed-off-by: Anthony Yznaga <redacted>
---
v2:
  use Alex Williamson's simplified fix

 drivers/vfio/vfio_iommu_type1.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 0b4f7c174c7a..0e9217687f5c 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c

@@ -612,17 +612,17 @@ static int vfio_wait(struct vfio_iommu *iommu)
 static int vfio_find_dma_valid(struct vfio_iommu *iommu, dma_addr_t start,
 			       size_t size, struct vfio_dma **dma_p)
 {
-	int ret;
+	int ret = 0;
 
 	do {
 		*dma_p = vfio_find_dma(iommu, start, size);
 		if (!*dma_p)
-			ret = -EINVAL;
+			return -EINVAL;
 		else if (!(*dma_p)->vaddr_invalid)
-			ret = 0;
+			return ret;
 		else
 			ret = vfio_wait(iommu);
-	} while (ret > 0);
+	} while (ret == WAITED);
 
 	return ret;
 }

-- 
1.8.3.1

On 8/20/2021 4:39 PM, Anthony Yznaga wrote:

Fix vfio_find_dma_valid to return WAITED on success if it was necessary
to wait which mean iommu lock was dropped and reacquired.  This allows
vfio_iommu_type1_pin_pages to recheck vaddr_invalid_count and possibly
avoid the checking the validity of every vaddr in its list.

Signed-off-by: Anthony Yznaga <redacted>
---
v2:
  use Alex Williamson's simplified fix

Hi Anthony, thanks for finding and fixing this.  I suggest you modify the commit 
message to describe the bug.  Something like:

  vfio_find_dma_valid is defined to return WAITED on success if it was
  necessary to wait.  However, the loop forgets the WAITED value returned
  by vfio_wait() and returns 0 in a later iteration.  Fix it.

With that, 
Reviewed-by: Steve Sistare <redacted>

- Steve

On 8/23/21 7:58 AM, Steven Sistare wrote:

On 8/20/2021 4:39 PM, Anthony Yznaga wrote:

quoted

Fix vfio_find_dma_valid to return WAITED on success if it was necessary
to wait which mean iommu lock was dropped and reacquired.  This allows
vfio_iommu_type1_pin_pages to recheck vaddr_invalid_count and possibly
avoid the checking the validity of every vaddr in its list.

Signed-off-by: Anthony Yznaga <redacted>
---
v2:
  use Alex Williamson's simplified fix

Hi Anthony, thanks for finding and fixing this.  I suggest you modify the commit 
message to describe the bug.  Something like:

  vfio_find_dma_valid is defined to return WAITED on success if it was
  necessary to wait.  However, the loop forgets the WAITED value returned
  by vfio_wait() and returns 0 in a later iteration.  Fix it.

With that, 
Reviewed-by: Steve Sistare <redacted>

Thank, Steve.  I'll send out a v3 with the updated commit message.

Anthony

- Steve

quoted

 drivers/vfio/vfio_iommu_type1.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 0b4f7c174c7a..0e9217687f5c 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c

@@ -612,17 +612,17 @@ static int vfio_wait(struct vfio_iommu *iommu)
 static int vfio_find_dma_valid(struct vfio_iommu *iommu, dma_addr_t start,
 			       size_t size, struct vfio_dma **dma_p)
 {
-	int ret;
+	int ret = 0;
 
 	do {
 		*dma_p = vfio_find_dma(iommu, start, size);
 		if (!*dma_p)
-			ret = -EINVAL;
+			return -EINVAL;
 		else if (!(*dma_p)->vaddr_invalid)
-			ret = 0;
+			return ret;
 		else
 			ret = vfio_wait(iommu);
-	} while (ret > 0);
+	} while (ret == WAITED);
 
 	return ret;
 }