All of lore.kernel.org
 help / color / mirror / Atom feed
From: David Marchand <david.marchand-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>
To: dev-VfR2kkLFssw@public.gmane.org
Subject: [PATCH 4/7] pci: rework interrupt fd init and fix fd leak
Date: Mon, 28 Apr 2014 15:19:44 +0200	[thread overview]
Message-ID: <1398691187-4918-5-git-send-email-david.marchand@6wind.com> (raw)
In-Reply-To: <1398691187-4918-1-git-send-email-david.marchand-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>

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 <david.marchand-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>
---
 lib/librte_eal/bsdapp/eal/eal_pci.c   |   60 +++++++++++++--------------
 lib/librte_eal/linuxapp/eal/eal_pci.c |   73 ++++++++++++++++-----------------
 2 files changed, 63 insertions(+), 70 deletions(-)

diff --git a/lib/librte_eal/bsdapp/eal/eal_pci.c b/lib/librte_eal/bsdapp/eal/eal_pci.c
index fbb281f..5e53e07 100644
--- a/lib/librte_eal/bsdapp/eal/eal_pci.c
+++ b/lib/librte_eal/bsdapp/eal/eal_pci.c
@@ -118,8 +118,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;
@@ -129,7 +129,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;
 	}
@@ -137,35 +137,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;
 }
 
@@ -178,19 +164,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);
@@ -218,6 +204,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)
@@ -232,6 +219,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,
@@ -245,7 +241,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++) {
 
@@ -253,16 +249,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 64c2130..2c3b914 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));
 
@@ -625,35 +621,36 @@ pci_uio_map_resource(struct rte_pci_device *dev)
 			sizeof (uio_res->maps) / sizeof (uio_res->maps[0])))
 			< 0)
 		return (nb_maps);
- 
+
 	uio_res->nb_maps = nb_maps;
 
 	/* 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;
-- 
1.7.10.4

  parent reply	other threads:[~2014-04-28 13:19 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-04-28 13:19 [PATCH 0/7] pci cleanup David Marchand
     [not found] ` <1398691187-4918-1-git-send-email-david.marchand-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>
2014-04-28 13:19   ` [PATCH 1/7] pci: fix potential mem leak David Marchand
     [not found]     ` <1398691187-4918-2-git-send-email-david.marchand-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>
2014-05-01 15:00       ` Burakov, Anatoly
     [not found]         ` <C6ECDF3AB251BE4894318F4E45123697592AAEE3-kPTMFJFq+rF9qrmMLTLiibfspsVTdybXVpNB7YpNyf8@public.gmane.org>
2014-05-05  8:14           ` David Marchand
2014-04-28 13:19   ` [PATCH 2/7] pci: align bsd implementation on linux David Marchand
2014-04-28 13:19   ` [PATCH 3/7] pci: remove virtio-uio workaround David Marchand
2014-04-28 13:19   ` David Marchand [this message]
2014-04-28 13:19   ` [PATCH 5/7] pci: pci_switch_module cleanup David Marchand
2014-04-28 13:19   ` [PATCH 6/7] pci: move RTE_PCI_DRV_FORCE_UNBIND handling out of #ifdef David Marchand
2014-04-28 13:19   ` [PATCH 7/7] pci: remove deprecated RTE_EAL_UNBIND_PORTS option David Marchand
2014-04-29  9:19   ` [PATCH 0/7] pci cleanup Burakov, Anatoly
2014-04-29 11:00   ` Neil Horman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1398691187-4918-5-git-send-email-david.marchand@6wind.com \
    --to=david.marchand-pdr9zngts4eavxtiumwx3w@public.gmane.org \
    --cc=dev-VfR2kkLFssw@public.gmane.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.