From d3e6faf8405e1e527fba5c55edfa147e57c6ae0d Mon Sep 17 00:00:00 2001 From: David Marchand Date: Fri, 9 May 2014 15:15:56 +0200 Subject: [PATCH] pci: rework interrupt fd init and fix fd leak A fd leak happens in pci_map_resource when multiple bars are mapped. Fix this by closing fd unconditionnally in this function and open the intr_handle fd in pci_uio_map_resource instead. Signed-off-by: David Marchand Acked-by: Anatoly Burakov Acked-by: Neil Horman --- lib/librte_eal/bsdapp/eal/eal_pci.c | 60 +++++++++++----------- lib/librte_eal/linuxapp/eal/eal_pci.c | 71 +++++++++++++-------------- 2 files changed, 62 insertions(+), 69 deletions(-) diff --git a/lib/librte_eal/bsdapp/eal/eal_pci.c b/lib/librte_eal/bsdapp/eal/eal_pci.c index a8945e4a97..94ae461167 100644 --- a/lib/librte_eal/bsdapp/eal/eal_pci.c +++ b/lib/librte_eal/bsdapp/eal/eal_pci.c @@ -119,8 +119,8 @@ pci_unbind_kernel_driver(struct rte_pci_device *dev) /* map a particular resource from a file */ static void * -pci_map_resource(struct rte_pci_device *dev, void *requested_addr, - const char *devname, off_t offset, size_t size) +pci_map_resource(void *requested_addr, const char *devname, off_t offset, + size_t size) { int fd; void *mapaddr; @@ -130,7 +130,7 @@ pci_map_resource(struct rte_pci_device *dev, void *requested_addr, */ fd = open(devname, O_RDWR); if (fd < 0) { - RTE_LOG(ERR, EAL, "Cannot open %s: %s\n", + RTE_LOG(ERR, EAL, "Cannot open %s: %s\n", devname, strerror(errno)); goto fail; } @@ -138,35 +138,21 @@ pci_map_resource(struct rte_pci_device *dev, void *requested_addr, /* Map the PCI memory resource of device */ mapaddr = mmap(requested_addr, size, PROT_READ | PROT_WRITE, MAP_SHARED, fd, offset); + close(fd); if (mapaddr == MAP_FAILED || (requested_addr != NULL && mapaddr != requested_addr)) { RTE_LOG(ERR, EAL, "%s(): cannot mmap(%s(%d), %p, 0x%lx, 0x%lx):" - " %s (%p)\n", __func__, devname, fd, requested_addr, + " %s (%p)\n", __func__, devname, fd, requested_addr, (unsigned long)size, (unsigned long)offset, strerror(errno), mapaddr); - close(fd); goto fail; } - if (rte_eal_process_type() == RTE_PROC_PRIMARY) { - /* save fd if in primary process */ - dev->intr_handle.fd = fd; - dev->intr_handle.type = RTE_INTR_HANDLE_UIO; - } else { - /* fd is not needed in slave process, close it */ - dev->intr_handle.fd = -1; - dev->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN; - close(fd); - } - RTE_LOG(DEBUG, EAL, " PCI memory mapped at %p\n", mapaddr); return mapaddr; fail: - dev->intr_handle.fd = -1; - dev->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN; - return NULL; } @@ -179,19 +165,19 @@ pci_uio_map_secondary(struct rte_pci_device *dev) { size_t i; struct uio_resource *uio_res; - + TAILQ_FOREACH(uio_res, uio_res_list, next) { - + /* skip this element if it doesn't match our PCI address */ if (memcmp(&uio_res->pci_addr, &dev->addr, sizeof(dev->addr))) continue; - + for (i = 0; i != uio_res->nb_maps; i++) { - if (pci_map_resource(dev, uio_res->maps[i].addr, - uio_res->path, - (off_t)uio_res->maps[i].offset, - (size_t)uio_res->maps[i].size) != - uio_res->maps[i].addr) { + if (pci_map_resource(uio_res->maps[i].addr, + uio_res->path, + (off_t)uio_res->maps[i].offset, + (size_t)uio_res->maps[i].size) + != uio_res->maps[i].addr) { RTE_LOG(ERR, EAL, "Cannot mmap device resource\n"); return (-1); @@ -219,6 +205,7 @@ pci_uio_map_resource(struct rte_pci_device *dev) struct uio_map *maps; dev->intr_handle.fd = -1; + dev->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN; /* secondary processes - use already recorded details */ if (rte_eal_process_type() != RTE_PROC_PRIMARY) @@ -233,6 +220,15 @@ pci_uio_map_resource(struct rte_pci_device *dev) return -1; } + /* save fd if in primary process */ + dev->intr_handle.fd = open(devname, O_RDWR); + if (dev->intr_handle.fd < 0) { + RTE_LOG(ERR, EAL, "Cannot open %s: %s\n", + devname, strerror(errno)); + return -1; + } + dev->intr_handle.type = RTE_INTR_HANDLE_UIO; + /* allocate the mapping details for secondary processes*/ if ((uio_res = rte_zmalloc("UIO_RES", sizeof (*uio_res), 0)) == NULL) { RTE_LOG(ERR, EAL, @@ -246,7 +242,7 @@ pci_uio_map_resource(struct rte_pci_device *dev) /* Map all BARs */ pagesz = sysconf(_SC_PAGESIZE); - + maps = uio_res->maps; for (i = uio_res->nb_maps = 0; i != PCI_MAX_RESOURCE; i++) { @@ -254,16 +250,16 @@ pci_uio_map_resource(struct rte_pci_device *dev) /* skip empty BAR */ if ((phaddr = dev->mem_resource[i].phys_addr) == 0) continue; - + /* if matching map is found, then use it */ offset = i * pagesz; maps[j].offset = offset; maps[j].phaddr = dev->mem_resource[i].phys_addr; maps[j].size = dev->mem_resource[i].len; if (maps[j].addr != NULL || - (mapaddr = pci_map_resource(dev, - NULL, devname, (off_t)offset, - (size_t)maps[j].size)) == NULL) { + (mapaddr = pci_map_resource(NULL, devname, (off_t)offset, + (size_t)maps[j].size) + ) == NULL) { rte_free(uio_res); return (-1); } diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c b/lib/librte_eal/linuxapp/eal/eal_pci.c index c006cf5cb3..451fbd24f7 100644 --- a/lib/librte_eal/linuxapp/eal/eal_pci.c +++ b/lib/librte_eal/linuxapp/eal/eal_pci.c @@ -305,8 +305,8 @@ pci_switch_module(struct rte_pci_driver *dr, struct rte_pci_device *dev, /* map a particular resource from a file */ static void * -pci_map_resource(struct rte_pci_device *dev, void *requested_addr, - const char *devname, off_t offset, size_t size) +pci_map_resource(void *requested_addr, const char *devname, off_t offset, + size_t size) { int fd; void *mapaddr; @@ -317,8 +317,7 @@ pci_map_resource(struct rte_pci_device *dev, void *requested_addr, * appear, so we wait some time before returning an error */ unsigned n; - fd = dev->intr_handle.fd; - for (n = 0; n < UIO_DEV_WAIT_TIMEOUT*10 && fd < 0; n++) { + for (n = 0; n < UIO_DEV_WAIT_TIMEOUT*10; n++) { errno = 0; if ((fd = open(devname, O_RDWR)) < 0 && errno != ENOENT) break; @@ -331,7 +330,7 @@ pci_map_resource(struct rte_pci_device *dev, void *requested_addr, fd = open(devname, O_RDWR); #endif if (fd < 0) { - RTE_LOG(ERR, EAL, "Cannot open %s: %s\n", + RTE_LOG(ERR, EAL, "Cannot open %s: %s\n", devname, strerror(errno)); goto fail; } @@ -339,34 +338,21 @@ pci_map_resource(struct rte_pci_device *dev, void *requested_addr, /* Map the PCI memory resource of device */ mapaddr = mmap(requested_addr, size, PROT_READ | PROT_WRITE, MAP_SHARED, fd, offset); + close(fd); if (mapaddr == MAP_FAILED || (requested_addr != NULL && mapaddr != requested_addr)) { RTE_LOG(ERR, EAL, "%s(): cannot mmap(%s(%d), %p, 0x%lx, 0x%lx):" - " %s (%p)\n", __func__, devname, fd, requested_addr, + " %s (%p)\n", __func__, devname, fd, requested_addr, (unsigned long)size, (unsigned long)offset, strerror(errno), mapaddr); - close(fd); goto fail; } - if (rte_eal_process_type() == RTE_PROC_PRIMARY) { - /* save fd if in primary process */ - dev->intr_handle.fd = fd; - dev->intr_handle.type = RTE_INTR_HANDLE_UIO; - } else { - /* fd is not needed in slave process, close it */ - dev->intr_handle.fd = -1; - dev->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN; - close(fd); - } RTE_LOG(DEBUG, EAL, " PCI memory mapped at %p\n", mapaddr); return mapaddr; fail: - dev->intr_handle.fd = -1; - dev->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN; - return NULL; } @@ -436,19 +422,19 @@ pci_uio_map_secondary(struct rte_pci_device *dev) { size_t i; struct uio_resource *uio_res; - + TAILQ_FOREACH(uio_res, uio_res_list, next) { - + /* skip this element if it doesn't match our PCI address */ if (memcmp(&uio_res->pci_addr, &dev->addr, sizeof(dev->addr))) continue; - + for (i = 0; i != uio_res->nb_maps; i++) { - if (pci_map_resource(dev, uio_res->maps[i].addr, - uio_res->path, - (off_t)uio_res->maps[i].offset, - (size_t)uio_res->maps[i].size) != - uio_res->maps[i].addr) { + if (pci_map_resource(uio_res->maps[i].addr, + uio_res->path, + (off_t)uio_res->maps[i].offset, + (size_t)uio_res->maps[i].size) + != uio_res->maps[i].addr) { RTE_LOG(ERR, EAL, "Cannot mmap device resource\n"); return (-1); @@ -596,6 +582,7 @@ pci_uio_map_resource(struct rte_pci_device *dev) struct uio_map *maps; dev->intr_handle.fd = -1; + dev->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN; /* secondary processes - use already recorded details */ if (rte_eal_process_type() != RTE_PROC_PRIMARY) @@ -608,6 +595,16 @@ pci_uio_map_resource(struct rte_pci_device *dev) "skipping\n", loc->domain, loc->bus, loc->devid, loc->function); return -1; } + rte_snprintf(devname, sizeof(devname), "/dev/uio%u", uio_num); + + /* save fd if in primary process */ + dev->intr_handle.fd = open(devname, O_RDWR); + if (dev->intr_handle.fd < 0) { + RTE_LOG(ERR, EAL, "Cannot open %s: %s\n", + devname, strerror(errno)); + return -1; + } + dev->intr_handle.type = RTE_INTR_HANDLE_UIO; /* allocate the mapping details for secondary processes*/ if ((uio_res = rte_zmalloc("UIO_RES", sizeof (*uio_res), 0)) == NULL) { @@ -616,7 +613,6 @@ pci_uio_map_resource(struct rte_pci_device *dev) return (-1); } - rte_snprintf(devname, sizeof(devname), "/dev/uio%u", uio_num); rte_snprintf(uio_res->path, sizeof(uio_res->path), "%s", devname); memcpy(&uio_res->pci_addr, &dev->addr, sizeof(uio_res->pci_addr)); @@ -632,30 +628,31 @@ pci_uio_map_resource(struct rte_pci_device *dev) /* Map all BARs */ pagesz = sysconf(_SC_PAGESIZE); - + maps = uio_res->maps; for (i = 0; i != PCI_MAX_RESOURCE; i++) { - + /* skip empty BAR */ if ((phaddr = dev->mem_resource[i].phys_addr) == 0) continue; - + for (j = 0; j != nb_maps && (phaddr != maps[j].phaddr || dev->mem_resource[i].len != maps[j].size); j++) ; - + /* if matching map is found, then use it */ if (j != nb_maps) { offset = j * pagesz; if (maps[j].addr != NULL || - (mapaddr = pci_map_resource(dev, - NULL, devname, (off_t)offset, - (size_t)maps[j].size)) == NULL) { + (mapaddr = pci_map_resource(NULL, devname, + (off_t)offset, + (size_t)maps[j].size) + ) == NULL) { rte_free(uio_res); return (-1); } - + maps[j].addr = mapaddr; maps[j].offset = offset; dev->mem_resource[i].addr = mapaddr; -- 2.20.1