[dpdk-stable] [PATCH 1/3] lib/eal: fix vfio unmap that fails unexpectedly

[dpdk-stable] [PATCH 1/3] lib/eal: fix vfio unmap that fails unexpectedly

[Chaitanya Babu Talluri]

Unmap of multiple pages fails after a sequence of partial map/unmaps.
The scenario is that multiple maps are created in user_mem_maps,
after multiple map/unmap/remap sequences.

For an example,
Steps:
1. Map 3 pages together
2. Un-map page1
3. Re-map page 1
4. Un-map page 2
5. Re-map page 2
6. Un-map page 3
7. Re-map page 3
8. Un-map all pages

Unmap fails when there are duplicate entries in user_mem_maps.

The fix is to validate if the input VA, IOVA exists in
user_mem_maps before creating map.

Fixes: 73a63908 ("vfio: allow to map other memory regions")
Cc: stable@dpdk.org

Signed-off-by: Chaitanya Babu Talluri <tallurix.chaitanya.babu@intel.com>
---
 lib/librte_eal/linux/eal/eal_vfio.c | 46 +++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/lib/librte_eal/linux/eal/eal_vfio.c b/lib/librte_eal/linux/eal/eal_vfio.c
index 501c74f23..104912077 100644
--- a/lib/librte_eal/linux/eal/eal_vfio.c
+++ b/lib/librte_eal/linux/eal/eal_vfio.c
@@ -212,6 +212,41 @@ find_user_mem_map(struct user_mem_maps *user_mem_maps, uint64_t addr,
 	return NULL;
 }
 
+static int
+find_user_mem_map_overlap(struct user_mem_maps *user_mem_maps, uint64_t addr,
+		uint64_t iova, uint64_t len)
+{
+	uint64_t va_end = addr + len;
+	uint64_t iova_end = iova + len;
+	int i;
+
+	for (i = 0; i < user_mem_maps->n_maps; i++) {
+		struct user_mem_map *map = &user_mem_maps->maps[i];
+		uint64_t map_va_end = map->addr + map->len;
+		uint64_t map_iova_end = map->iova + map->len;
+
+		bool no_lo_va_overlap = addr < map->addr && va_end <= map->addr;
+		bool no_hi_va_overlap = addr >= map_va_end &&
+			va_end > map_va_end;
+		bool no_lo_iova_overlap = iova < map->iova &&
+			iova_end <= map->iova;
+		bool no_hi_iova_overlap = iova >= map_iova_end &&
+			iova_end > map_iova_end;
+
+		/* check input VA and iova is not within the
+		 * existing map's range
+		 */
+		if ((no_lo_va_overlap || no_hi_va_overlap) &&
+				(no_lo_iova_overlap || no_hi_iova_overlap))
+			continue;
+		else
+			/* map overlaps */
+			return 1;
+	}
+	/* map doesn't overlap */
+	return 0;
+}
+
 /* this will sort all user maps, and merge/compact any adjacent maps */
 static void
 compact_user_maps(struct user_mem_maps *user_mem_maps)
@@ -1732,6 +1767,17 @@ container_dma_map(struct vfio_config *vfio_cfg, uint64_t vaddr, uint64_t iova,
 		ret = -1;
 		goto out;
 	}
+
+	/* check whether vaddr and iova exists in user_mem_maps */
+	ret = find_user_mem_map_overlap(user_mem_maps, vaddr, iova, len);
+	if (ret) {
+		RTE_LOG(ERR, EAL, "Mapping overlaps with a previously "
+				"existing mapping\n");
+		rte_errno = EEXIST;
+		ret = -1;
+		goto out;
+	}
+
 	/* map the entry */
 	if (vfio_dma_mem_map(vfio_cfg, vaddr, iova, len, 1)) {
 		/* technically, this will fail if there are currently no devices
-- 
2.17.2

[dpdk-stable] [PATCH 2/3] lib/eal: fix vfio unmap that succeeds unexpectedly

[Chaitanya Babu Talluri]

Un-map of page with valid virtual address and
another page's IOVA succeeds unexpectedly.
An entry in user_mem_maps can refer multiple pages.
Currently in such case to unmap single page, VA
and IOVA related to entry in user_mem_maps is
checked but not based on page (based on the
page size), this is the cause.

The solution is that in find_user_mem_maps,
check whether user input iova is in relation with
input virtual address of the page which is to be
unmapped.

Fixes: 73a6390859 ("vfio: allow to map other memory regions")
Cc: stable@dpdk.org

Signed-off-by: Chaitanya Babu Talluri <tallurix.chaitanya.babu@intel.com>
---
 lib/librte_eal/linux/eal/eal_vfio.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/lib/librte_eal/linux/eal/eal_vfio.c b/lib/librte_eal/linux/eal/eal_vfio.c
index 104912077..04c284cb2 100644
--- a/lib/librte_eal/linux/eal/eal_vfio.c
+++ b/lib/librte_eal/linux/eal/eal_vfio.c
@@ -184,13 +184,13 @@ find_user_mem_map(struct user_mem_maps *user_mem_maps, uint64_t addr,
 		uint64_t iova, uint64_t len)
 {
 	uint64_t va_end = addr + len;
-	uint64_t iova_end = iova + len;
 	int i;
 
 	for (i = 0; i < user_mem_maps->n_maps; i++) {
 		struct user_mem_map *map = &user_mem_maps->maps[i];
 		uint64_t map_va_end = map->addr + map->len;
-		uint64_t map_iova_end = map->iova + map->len;
+		uint64_t diff_addr_len = addr - map->addr;
+		uint64_t expected_iova = map->iova + diff_addr_len;
 
 		/* check start VA */
 		if (addr < map->addr || addr >= map_va_end)
@@ -199,11 +199,10 @@ find_user_mem_map(struct user_mem_maps *user_mem_maps, uint64_t addr,
 		if (va_end <= map->addr || va_end > map_va_end)
 			continue;
 
-		/* check start IOVA */
-		if (iova < map->iova || iova >= map_iova_end)
-			continue;
-		/* check if IOVA end is within boundaries */
-		if (iova_end <= map->iova || iova_end > map_iova_end)
+		/* check whether user input iova is in sync with
+		 * user_mem_map entry's iova
+		 */
+		if (expected_iova != iova)
 			continue;
 
 		/* we've found our map */
-- 
2.17.2

Re: [dpdk-stable] [PATCH 1/3] lib/eal: fix vfio unmap that fails unexpectedly

[Burakov, Anatoly]

On 21-Aug-19 2:02 PM, Chaitanya Babu Talluri wrote:
> Unmap of multiple pages fails after a sequence of partial map/unmaps.
> The scenario is that multiple maps are created in user_mem_maps,
> after multiple map/unmap/remap sequences.
> 
> For an example,
> Steps:
> 1. Map 3 pages together
> 2. Un-map page1
> 3. Re-map page 1
> 4. Un-map page 2
> 5. Re-map page 2
> 6. Un-map page 3
> 7. Re-map page 3
> 8. Un-map all pages

I don't think this description is correct in relation to what is being 
fixed here. The code attempts to prevent overlaps, but there are no 
overlaps in the above example - none of the above operations would 
trigger the added code.

-- 
Thanks,
Anatoly

Re: [dpdk-stable] [PATCH 2/3] lib/eal: fix vfio unmap that succeeds unexpectedly

[Burakov, Anatoly]

On 21-Aug-19 2:02 PM, Chaitanya Babu Talluri wrote:
> Un-map of page with valid virtual address and
> another page's IOVA succeeds unexpectedly.
> An entry in user_mem_maps can refer multiple pages.
> Currently in such case to unmap single page, VA
> and IOVA related to entry in user_mem_maps is
> checked but not based on page (based on the
> page size), this is the cause.
> 
> The solution is that in find_user_mem_maps,
> check whether user input iova is in relation with
> input virtual address of the page which is to be
> unmapped.

The description could be clearer. Suggested rewording:

Unmapping page with a VA that is found in the list of current mappings 
will succeed even if the IOVA for the chunk that is being unmapped, is 
mismatched. Fix it by checking if IOVA address matches the expected IOVA 
address exactly.

-- 
Thanks,
Anatoly

[dpdk-stable] [PATCH v2 1/3] lib/eal: fix vfio unmap that fails unexpectedly

[Chaitanya Babu Talluri]

Unmap fails when there are duplicate entries in user_mem_maps.

The fix is to validate if the input VA, IOVA exists or
overlaps in user_mem_maps before creating map.

Fixes: 73a63908 ("vfio: allow to map other memory regions")
Cc: stable@dpdk.org

Signed-off-by: Chaitanya Babu Talluri <tallurix.chaitanya.babu@intel.com>
---
 lib/librte_eal/linux/eal/eal_vfio.c | 46 +++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/lib/librte_eal/linux/eal/eal_vfio.c b/lib/librte_eal/linux/eal/eal_vfio.c
index 501c74f23..104912077 100644
--- a/lib/librte_eal/linux/eal/eal_vfio.c
+++ b/lib/librte_eal/linux/eal/eal_vfio.c
@@ -212,6 +212,41 @@ find_user_mem_map(struct user_mem_maps *user_mem_maps, uint64_t addr,
 	return NULL;
 }
 
+static int
+find_user_mem_map_overlap(struct user_mem_maps *user_mem_maps, uint64_t addr,
+		uint64_t iova, uint64_t len)
+{
+	uint64_t va_end = addr + len;
+	uint64_t iova_end = iova + len;
+	int i;
+
+	for (i = 0; i < user_mem_maps->n_maps; i++) {
+		struct user_mem_map *map = &user_mem_maps->maps[i];
+		uint64_t map_va_end = map->addr + map->len;
+		uint64_t map_iova_end = map->iova + map->len;
+
+		bool no_lo_va_overlap = addr < map->addr && va_end <= map->addr;
+		bool no_hi_va_overlap = addr >= map_va_end &&
+			va_end > map_va_end;
+		bool no_lo_iova_overlap = iova < map->iova &&
+			iova_end <= map->iova;
+		bool no_hi_iova_overlap = iova >= map_iova_end &&
+			iova_end > map_iova_end;
+
+		/* check input VA and iova is not within the
+		 * existing map's range
+		 */
+		if ((no_lo_va_overlap || no_hi_va_overlap) &&
+				(no_lo_iova_overlap || no_hi_iova_overlap))
+			continue;
+		else
+			/* map overlaps */
+			return 1;
+	}
+	/* map doesn't overlap */
+	return 0;
+}
+
 /* this will sort all user maps, and merge/compact any adjacent maps */
 static void
 compact_user_maps(struct user_mem_maps *user_mem_maps)
@@ -1732,6 +1767,17 @@ container_dma_map(struct vfio_config *vfio_cfg, uint64_t vaddr, uint64_t iova,
 		ret = -1;
 		goto out;
 	}
+
+	/* check whether vaddr and iova exists in user_mem_maps */
+	ret = find_user_mem_map_overlap(user_mem_maps, vaddr, iova, len);
+	if (ret) {
+		RTE_LOG(ERR, EAL, "Mapping overlaps with a previously "
+				"existing mapping\n");
+		rte_errno = EEXIST;
+		ret = -1;
+		goto out;
+	}
+
 	/* map the entry */
 	if (vfio_dma_mem_map(vfio_cfg, vaddr, iova, len, 1)) {
 		/* technically, this will fail if there are currently no devices
-- 
2.17.2

[dpdk-stable] [PATCH v2 2/3] lib/eal: fix vfio unmap that succeeds unexpectedly

[Chaitanya Babu Talluri]

Unmapping page with a VA that is found in the list
of current mappings will succeed even if the IOVA
for the chunk that is being unmapped,is mismatched.

Fix it by checking if IOVA address matches the
expected IOVA address exactly.

Fixes: 73a6390859 ("vfio: allow to map other memory regions")
Cc: stable@dpdk.org

Signed-off-by: Chaitanya Babu Talluri <tallurix.chaitanya.babu@intel.com>
---
 lib/librte_eal/linux/eal/eal_vfio.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/lib/librte_eal/linux/eal/eal_vfio.c b/lib/librte_eal/linux/eal/eal_vfio.c
index 104912077..04c284cb2 100644
--- a/lib/librte_eal/linux/eal/eal_vfio.c
+++ b/lib/librte_eal/linux/eal/eal_vfio.c
@@ -184,13 +184,13 @@ find_user_mem_map(struct user_mem_maps *user_mem_maps, uint64_t addr,
 		uint64_t iova, uint64_t len)
 {
 	uint64_t va_end = addr + len;
-	uint64_t iova_end = iova + len;
 	int i;
 
 	for (i = 0; i < user_mem_maps->n_maps; i++) {
 		struct user_mem_map *map = &user_mem_maps->maps[i];
 		uint64_t map_va_end = map->addr + map->len;
-		uint64_t map_iova_end = map->iova + map->len;
+		uint64_t diff_addr_len = addr - map->addr;
+		uint64_t expected_iova = map->iova + diff_addr_len;
 
 		/* check start VA */
 		if (addr < map->addr || addr >= map_va_end)
@@ -199,11 +199,10 @@ find_user_mem_map(struct user_mem_maps *user_mem_maps, uint64_t addr,
 		if (va_end <= map->addr || va_end > map_va_end)
 			continue;
 
-		/* check start IOVA */
-		if (iova < map->iova || iova >= map_iova_end)
-			continue;
-		/* check if IOVA end is within boundaries */
-		if (iova_end <= map->iova || iova_end > map_iova_end)
+		/* check whether user input iova is in sync with
+		 * user_mem_map entry's iova
+		 */
+		if (expected_iova != iova)
 			continue;
 
 		/* we've found our map */
-- 
2.17.2