[PATCH 0/4] BSD PCI Fixes

[PATCH 0/4] BSD PCI Fixes

[Jake Freeland]

Hi there,

The following patchset includes a number of fixes I've made over the
past year relating to the PCI driver. Most of these changes target
the FreeBSD platform.

Let me know if you have any feedback. Thanks.

Jake Freeland (4):
  bus/pci: Use force-noreplace flag when mapping PCI resources
  bus/pci/bsd: Map resources at EAL baseaddr
  bus/pci/bsd: Eliminate potential overflow
  bus/pci/bsd: Fix device existence check

 drivers/bus/pci/bsd/pci.c        | 32 ++++++++++++++++++++++++--------
 drivers/bus/pci/pci_common_uio.c |  4 +++-
 lib/eal/common/eal_private.h     |  7 ++++++-
 lib/eal/include/rte_eal_paging.h |  7 ++++++-
 lib/eal/unix/eal_unix_memory.c   |  8 +++++++-
 5 files changed, 46 insertions(+), 12 deletions(-)

--
2.47.2

[PATCH 1/4] bus/pci: Use force-noreplace flag when mapping PCI resources

[Jake Freeland]

When mapping PCI resources in secondary processes, use the
RTE_MAP_FORCE_ADDRESS_NOREPLACE flag to indicate that the
mapping must be made at the provided address.

Without this flag, the kernel might return a different address for
the mapping, even if the requested region was available.

Signed-off-by: Jake Freeland <jfree@FreeBSD.org>
---
 drivers/bus/pci/pci_common_uio.c | 4 +++-
 lib/eal/common/eal_private.h     | 7 ++++++-
 lib/eal/include/rte_eal_paging.h | 7 ++++++-
 lib/eal/unix/eal_unix_memory.c   | 8 +++++++-
 4 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/drivers/bus/pci/pci_common_uio.c b/drivers/bus/pci/pci_common_uio.c
index 30503bd23a..71974e9f56 100644
--- a/drivers/bus/pci/pci_common_uio.c
+++ b/drivers/bus/pci/pci_common_uio.c
@@ -10,6 +10,7 @@
 #include <sys/mman.h>
 
 #include <rte_eal.h>
+#include <rte_eal_paging.h>
 #include <rte_pci.h>
 #include <rte_bus_pci.h>
 #include <rte_tailq.h>
@@ -58,7 +59,8 @@ pci_uio_map_secondary(struct rte_pci_device *dev)
 
 			void *mapaddr = pci_map_resource(uio_res->maps[i].addr,
 					fd, (off_t)uio_res->maps[i].offset,
-					(size_t)uio_res->maps[i].size, 0);
+					(size_t)uio_res->maps[i].size,
+					RTE_MAP_FORCE_ADDRESS_NOREPLACE);
 
 			/* fd is not needed in secondary process, close it */
 			close(fd);
diff --git a/lib/eal/common/eal_private.h b/lib/eal/common/eal_private.h
index 04ba8ddb86..aaeb140eaf 100644
--- a/lib/eal/common/eal_private.h
+++ b/lib/eal/common/eal_private.h
@@ -211,7 +211,12 @@ enum eal_mem_reserve_flags {
 	 * @see RTE_MAP_FORCE_ADDRESS for description of possible consequences
 	 *      (although implementations are not required to use it).
 	 */
-	EAL_RESERVE_FORCE_ADDRESS = 1 << 1
+	EAL_RESERVE_FORCE_ADDRESS = 1 << 1,
+	/**
+	 * Force reserving memory at the requested address, but fail if a
+	 * preexisting mapping collides with the request.
+	 */
+	EAL_RESERVE_FORCE_ADDRESS_NOREPLACE = 1 << 2,
 };
 
 /**
diff --git a/lib/eal/include/rte_eal_paging.h b/lib/eal/include/rte_eal_paging.h
index c60317d0f5..7b1983b615 100644
--- a/lib/eal/include/rte_eal_paging.h
+++ b/lib/eal/include/rte_eal_paging.h
@@ -34,7 +34,12 @@ enum rte_map_flags {
 	 * may remove all other mappings in the requested region. However,
 	 * it is not required to do so, thus mapping with this flag may fail.
 	 */
-	RTE_MAP_FORCE_ADDRESS = 1 << 3
+	RTE_MAP_FORCE_ADDRESS = 1 << 3,
+	/**
+	 * Force mapping to the requested address, but fail if a preexisting
+	 * mapping collides with the request.
+	 */
+	RTE_MAP_FORCE_ADDRESS_NOREPLACE = 1 << 4,
 };
 
 /**
diff --git a/lib/eal/unix/eal_unix_memory.c b/lib/eal/unix/eal_unix_memory.c
index c540f1e838..61e914b8db 100644
--- a/lib/eal/unix/eal_unix_memory.c
+++ b/lib/eal/unix/eal_unix_memory.c
@@ -17,11 +17,13 @@
 #ifdef RTE_EXEC_ENV_LINUX
 #define EAL_DONTDUMP MADV_DONTDUMP
 #define EAL_DODUMP   MADV_DODUMP
+#define EAL_FIXED_NOREPLACE MAP_FIXED_NOREPLACE
 #elif defined RTE_EXEC_ENV_FREEBSD
 #define EAL_DONTDUMP MADV_NOCORE
 #define EAL_DODUMP   MADV_CORE
+#define EAL_FIXED_NOREPLACE (MAP_FIXED | MAP_EXCL)
 #else
-#error "madvise doesn't support this OS"
+#error "EAL doesn't support this OS"
 #endif
 
 static void *
@@ -68,6 +70,8 @@ eal_mem_reserve(void *requested_addr, size_t size, int flags)
 
 	if (flags & EAL_RESERVE_FORCE_ADDRESS)
 		sys_flags |= MAP_FIXED;
+	if (flags & EAL_RESERVE_FORCE_ADDRESS_NOREPLACE)
+		sys_flags |= EAL_FIXED_NOREPLACE;
 
 	return mem_map(requested_addr, size, PROT_NONE, sys_flags, -1, 0);
 }
@@ -124,6 +128,8 @@ rte_mem_map(void *requested_addr, size_t size, int prot, int flags,
 		sys_flags |= MAP_PRIVATE;
 	if (flags & RTE_MAP_FORCE_ADDRESS)
 		sys_flags |= MAP_FIXED;
+	if (flags & RTE_MAP_FORCE_ADDRESS_NOREPLACE)
+		sys_flags |= EAL_FIXED_NOREPLACE;
 
 	return mem_map(requested_addr, size, sys_prot, sys_flags, fd, offset);
 }
-- 
2.47.2

[PATCH 2/4] bus/pci/bsd: Map resources at EAL baseaddr

[Jake Freeland]

Provide the EAL base address as a hint to mmap(2), so device resources
are not mapped where malloc(3) et al. make allocations.

This makes mapping conflicts less likely for secondary processes that
make memory allocations before initializing EAL.

Signed-off-by: Jake Freeland <jfree@FreeBSD.org>
---
 drivers/bus/pci/bsd/pci.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/bus/pci/bsd/pci.c b/drivers/bus/pci/bsd/pci.c
index 5e2e09d5a4..0581daf130 100644
--- a/drivers/bus/pci/bsd/pci.c
+++ b/drivers/bus/pci/bsd/pci.c
@@ -182,7 +182,15 @@ pci_uio_map_resource_by_index(struct rte_pci_device *dev, int res_idx,
 
 	/* if matching map is found, then use it */
 	offset = res_idx * pagesz;
-	mapaddr = pci_map_resource(NULL, fd, (off_t)offset,
+
+	/*
+	 * Use baseaddr as a hint to avoid mapping resources where
+	 * malloc(3) et al. usually make allocations. This reduces
+	 * mapping conflicts in secondary processes that make
+	 * memory allocations before initializing EAL.
+	 */
+	mapaddr = pci_map_resource((void *)rte_eal_get_baseaddr(),
+			fd, (off_t)offset,
 			(size_t)dev->mem_resource[res_idx].len, 0);
 	close(fd);
 	if (mapaddr == NULL)
-- 
2.47.2

[PATCH 3/4] bus/pci/bsd: Eliminate potential overflow

[Jake Freeland]

When calling rte_pci_write_config(), use memcpy(3) to copy @len bytes
of @buf into local memory instead of casting it to a uint32_t pointer
and dereferencing it. This prevents us from reading data outside of
@buf in the case that @buf has a length less than 32 bits.

Signed-off-by: Jake Freeland <jfree@FreeBSD.org>
---
 drivers/bus/pci/bsd/pci.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/bus/pci/bsd/pci.c b/drivers/bus/pci/bsd/pci.c
index 0581daf130..c64cd2c86c 100644
--- a/drivers/bus/pci/bsd/pci.c
+++ b/drivers/bus/pci/bsd/pci.c
@@ -467,7 +467,6 @@ int rte_pci_write_config(const struct rte_pci_device *dev,
 			.pc_func = dev->addr.function,
 		},
 		.pi_reg = offset,
-		.pi_data = *(const uint32_t *)buf,
 		.pi_width = len,
 	};
 
@@ -476,7 +475,7 @@ int rte_pci_write_config(const struct rte_pci_device *dev,
 		goto error;
 	}
 
-	memcpy(&pi.pi_data, buf, len);
+	memcpy(&pi.pi_data, buf, MIN(len, sizeof(pi.pi_data)));
 
 	fd = open("/dev/pci", O_RDWR);
 	if (fd < 0) {
-- 
2.47.2

[PATCH 4/4] bus/pci/bsd: Fix device existence check

[Jake Freeland]

Use open(2) instead of access(2) to check for the existence of the target
device. This avoids a possible race condition where the the device file is
removed after a successful call to access(2) but before open(2).

This also fixes any potential bugs associated with passing open(2)-style
flags into access(2). i.e. access(2) does not formally support the O_RDWR
flag.

Signed-off-by: Jake Freeland <jfree@FreeBSD.org>
---
 drivers/bus/pci/bsd/pci.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/bus/pci/bsd/pci.c b/drivers/bus/pci/bsd/pci.c
index c64cd2c86c..9bce9df503 100644
--- a/drivers/bus/pci/bsd/pci.c
+++ b/drivers/bus/pci/bsd/pci.c
@@ -106,20 +106,29 @@ pci_uio_alloc_resource(struct rte_pci_device *dev,
 {
 	char devname[PATH_MAX]; /* contains the /dev/uioX */
 	struct rte_pci_addr *loc;
+	int fd;
 
 	loc = &dev->addr;
 
 	snprintf(devname, sizeof(devname), "/dev/uio@pci:%u:%u:%u",
 			dev->addr.bus, dev->addr.devid, dev->addr.function);
 
-	if (access(devname, O_RDWR) < 0) {
-		PCI_LOG(WARNING, "  "PCI_PRI_FMT" not managed by UIO driver, skipping",
-			loc->domain, loc->bus, loc->devid, loc->function);
-		return 1;
+	fd = open(devname, O_RDWR);
+	if (fd < 0) {
+		if (errno == ENOENT) {
+			PCI_LOG(WARNING, PCI_PRI_FMT" not managed by UIO "
+					"driver, skipping", loc->domain,
+					loc->bus, loc->devid, loc->function);
+			return 1;
+		}
+		PCI_LOG(ERR, "Failed to open device file for " PCI_PRI_FMT
+				" (%s)", loc->domain, loc->bus, loc->devid,
+				loc->function, devname);
+		return -1;
 	}
 
 	/* save fd if in primary process */
-	if (rte_intr_fd_set(dev->intr_handle, open(devname, O_RDWR))) {
+	if (rte_intr_fd_set(dev->intr_handle, fd)) {
 		PCI_LOG(WARNING, "Failed to save fd");
 		goto error;
 	}
-- 
2.47.2