env: add spdk_pci_device_unclaim()

spdk_pci_device_claim() could create a file on the
filesystem that couldn't be deleted programatically.
It could only be overwritten - e.g. by another spdk
instance - but this didn't really work if that
another instance had less privileges and hence no
access to the previous file.

This is exactly the case we're seeing on our CI when
running SPDK as non-root. In general it's a good idea
not to leave any leftover files, so now we'll delete
the pci claim file when the spdk process exits.

spdk_pci_device_claim() used to return a file descriptor
that could be simply closed to "un-claim" the device.
It'll now return only a return code. The fd will be
stored inside spdk_pci_device and will be closed either
when user calls the newly introduced spdk_pci_device_unclaim(),
or when the device is detached.

We'll still need to clean up those files somewhere in
our test scripts (probably ./setup.sh cleanup) to
clean up after crashed processes or so - but we don't
necessarily want to run such scripts inside the autotest
whenever a non-root spdk is about to be started.

Change-Id: I797e079417bb56491013cc5b92f0f0d14f451d18
Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/467107
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>

include/spdk/env.h

@@ -669,6 +669,8 @@ struct spdk_pci_device {
 	struct _spdk_pci_device_internal {
 		struct spdk_pci_driver		*driver;
 		bool				attached;
+		/* optional fd for exclusive access to this device on this process */
+		int				claim_fd;
 		bool				pending_removal;
 		/* The device was successfully removed on a DPDK interrupt thread,
 		 * but to prevent data races we couldn't remove it from the global
@@ -895,13 +897,23 @@ int spdk_pci_device_get_serial_number(struct spdk_pci_device *dev, char *sn, siz
  * As long as this file remains open with the lock acquired, other processes will
  * not be able to successfully call this function on the same PCI device.
  *
- * \param pci_addr PCI address of the device to claim
+ * The device can be un-claimed by the owning process with spdk_pci_device_unclaim().
+ * It will be also unclaimed automatically when detached.
  *
- * \return -1 if the device has already been claimed, an fd otherwise. This fd
- * should be closed when the application no longer needs access to the PCI device
- * (including when it is hot removed).
+ * \param dev PCI device to claim.
+ *
+ * \return -EACCES if the device has already been claimed,
+ *	   negative errno on unexpected errors,
+ *	   0 on success.
  */
-int spdk_pci_device_claim(const struct spdk_pci_addr *pci_addr);
+int spdk_pci_device_claim(struct spdk_pci_device *dev);
+
+/**
+ * Undo spdk_pci_device_claim().
+ *
+ * \param dev PCI device to unclaim.
+ */
+void spdk_pci_device_unclaim(struct spdk_pci_device *dev);
 
 /**
  * Release all resources associated with the given device and detach it. As long

lib/env_dpdk/pci.c

@@ -342,6 +342,7 @@ spdk_pci_device_init(struct rte_pci_driver *_drv,
 	dev->detach = spdk_detach_rte;
 
 	dev->internal.driver = driver;
+	dev->internal.claim_fd = -1;
 
 	if (driver->cb_fn != NULL) {
 		rc = driver->cb_fn(driver->cb_arg, dev);
@@ -387,6 +388,11 @@ void
 spdk_pci_device_detach(struct spdk_pci_device *dev)
 {
 	assert(dev->internal.attached);
+
+	if (dev->internal.claim_fd >= 0) {
+		spdk_pci_device_unclaim(dev);
+	}
+
 	dev->internal.attached = false;
 	dev->detach(dev);
 
@@ -728,7 +734,7 @@ spdk_pci_addr_compare(const struct spdk_pci_addr *a1, const struct spdk_pci_addr
 
 #ifdef __linux__
 int
-spdk_pci_device_claim(const struct spdk_pci_addr *pci_addr)
+spdk_pci_device_claim(struct spdk_pci_device *dev)
 {
 	int dev_fd;
 	char dev_name[64];
@@ -741,20 +747,19 @@ spdk_pci_device_claim(const struct spdk_pci_addr *pci_addr)
 		.l_len = 0,
 	};
 
-	snprintf(dev_name, sizeof(dev_name), "/tmp/spdk_pci_lock_%04x:%02x:%02x.%x", pci_addr->domain,
-		 pci_addr->bus,
-		 pci_addr->dev, pci_addr->func);
+	snprintf(dev_name, sizeof(dev_name), "/tmp/spdk_pci_lock_%04x:%02x:%02x.%x",
+		 dev->addr.domain, dev->addr.bus, dev->addr.dev, dev->addr.func);
 
 	dev_fd = open(dev_name, O_RDWR | O_CREAT, S_IRUSR | S_IWUSR);
 	if (dev_fd == -1) {
 		fprintf(stderr, "could not open %s\n", dev_name);
-		return -1;
+		return -errno;
 	}
 
 	if (ftruncate(dev_fd, sizeof(int)) != 0) {
 		fprintf(stderr, "could not truncate %s\n", dev_name);
 		close(dev_fd);
-		return -1;
+		return -errno;
 	}
 
 	dev_map = mmap(NULL, sizeof(int), PROT_READ | PROT_WRITE,
@@ -762,7 +767,7 @@ spdk_pci_device_claim(const struct spdk_pci_addr *pci_addr)
 	if (dev_map == MAP_FAILED) {
 		fprintf(stderr, "could not mmap dev %s (%d)\n", dev_name, errno);
 		close(dev_fd);
-		return -1;
+		return -errno;
 	}
 
 	if (fcntl(dev_fd, F_SETLK, &pcidev_lock) != 0) {
@@ -771,23 +776,44 @@ spdk_pci_device_claim(const struct spdk_pci_addr *pci_addr)
 			" process %d has claimed it\n", dev_name, pid);
 		munmap(dev_map, sizeof(int));
 		close(dev_fd);
-		return -1;
+		/* F_SETLK returns unspecified errnos, normalize them */
+		return -EACCES;
 	}
 
 	*(int *)dev_map = (int)getpid();
 	munmap(dev_map, sizeof(int));
+	dev->internal.claim_fd = dev_fd;
 	/* Keep dev_fd open to maintain the lock. */
-	return dev_fd;
+	return 0;
+}
+
+void
+spdk_pci_device_unclaim(struct spdk_pci_device *dev)
+{
+	char dev_name[64];
+
+	snprintf(dev_name, sizeof(dev_name), "/tmp/spdk_pci_lock_%04x:%02x:%02x.%x",
+		 dev->addr.domain, dev->addr.bus, dev->addr.dev, dev->addr.func);
+
+	close(dev->internal.claim_fd);
+	dev->internal.claim_fd = -1;
+	unlink(dev_name);
 }
 #endif /* __linux__ */
 
 #ifdef __FreeBSD__
 int
-spdk_pci_device_claim(const struct spdk_pci_addr *pci_addr)
+spdk_pci_device_claim(struct spdk_pci_device *dev)
 {
 	/* TODO */
 	return 0;
 }
+
+void
+spdk_pci_device_unclaim(struct spdk_pci_device *dev)
+{
+	/* TODO */
+}
 #endif /* __FreeBSD__ */
 
 int

lib/nvme/nvme_pcie.c

@@ -39,6 +39,7 @@
 #include "spdk/stdinc.h"
 #include "spdk/env.h"
 #include "spdk/likely.h"
+#include "spdk/string.h"
 #include "nvme_internal.h"
 #include "nvme_uevent.h"
 
@@ -101,9 +102,6 @@ struct nvme_pcie_ctrlr {
 	/* Opaque handle to associated PCI device. */
 	struct spdk_pci_device *devhandle;
 
-	/* File descriptor returned from spdk_pci_device_claim().  Closed when ctrlr is detached. */
-	int claim_fd;
-
 	/* Flag to indicate the MMIO register has been remapped */
 	bool is_remapped;
 };
@@ -808,25 +806,20 @@ struct spdk_nvme_ctrlr *nvme_pcie_ctrlr_construct(const struct spdk_nvme_transpo
 	union spdk_nvme_cap_register cap;
 	union spdk_nvme_vs_register vs;
 	uint32_t cmd_reg;
-	int rc, claim_fd;
+	int rc;
 	struct spdk_pci_id pci_id;
-	struct spdk_pci_addr pci_addr;
 
-	if (spdk_pci_addr_parse(&pci_addr, trid->traddr)) {
-		SPDK_ERRLOG("could not parse pci address\n");
-		return NULL;
-	}
-
-	claim_fd = spdk_pci_device_claim(&pci_addr);
-	if (claim_fd < 0) {
-		SPDK_ERRLOG("could not claim device %s\n", trid->traddr);
+	rc = spdk_pci_device_claim(pci_dev);
+	if (rc < 0) {
+		SPDK_ERRLOG("could not claim device %s (%s)\n",
+			    trid->traddr, spdk_strerror(-rc));
 		return NULL;
 	}
 
 	pctrlr = spdk_zmalloc(sizeof(struct nvme_pcie_ctrlr), 64, NULL,
 			      SPDK_ENV_SOCKET_ID_ANY, SPDK_MALLOC_SHARE);
 	if (pctrlr == NULL) {
-		close(claim_fd);
+		spdk_pci_device_unclaim(pci_dev);
 		SPDK_ERRLOG("could not allocate ctrlr\n");
 		return NULL;
 	}
@@ -836,19 +829,18 @@ struct spdk_nvme_ctrlr *nvme_pcie_ctrlr_construct(const struct spdk_nvme_transpo
 	pctrlr->ctrlr.trid.trtype = SPDK_NVME_TRANSPORT_PCIE;
 	pctrlr->devhandle = devhandle;
 	pctrlr->ctrlr.opts = *opts;
-	pctrlr->claim_fd = claim_fd;
 	memcpy(&pctrlr->ctrlr.trid, trid, sizeof(pctrlr->ctrlr.trid));
 
 	rc = nvme_ctrlr_construct(&pctrlr->ctrlr);
 	if (rc != 0) {
-		close(claim_fd);
+		spdk_pci_device_unclaim(pci_dev);
 		spdk_free(pctrlr);
 		return NULL;
 	}
 
 	rc = nvme_pcie_ctrlr_allocate_bars(pctrlr);
 	if (rc != 0) {
-		close(claim_fd);
+		spdk_pci_device_unclaim(pci_dev);
 		spdk_free(pctrlr);
 		return NULL;
 	}
@@ -860,14 +852,14 @@ struct spdk_nvme_ctrlr *nvme_pcie_ctrlr_construct(const struct spdk_nvme_transpo
 
 	if (nvme_ctrlr_get_cap(&pctrlr->ctrlr, &cap)) {
 		SPDK_ERRLOG("get_cap() failed\n");
-		close(claim_fd);
+		spdk_pci_device_unclaim(pci_dev);
 		spdk_free(pctrlr);
 		return NULL;
 	}
 
 	if (nvme_ctrlr_get_vs(&pctrlr->ctrlr, &vs)) {
 		SPDK_ERRLOG("get_vs() failed\n");
-		close(claim_fd);
+		spdk_pci_device_unclaim(pci_dev);
 		spdk_free(pctrlr);
 		return NULL;
 	}
@@ -938,8 +930,6 @@ nvme_pcie_ctrlr_destruct(struct spdk_nvme_ctrlr *ctrlr)
 	struct nvme_pcie_ctrlr *pctrlr = nvme_pcie_ctrlr(ctrlr);
 	struct spdk_pci_device *devhandle = nvme_ctrlr_proc_get_devhandle(ctrlr);
 
-	close(pctrlr->claim_fd);
-
 	if (ctrlr->adminq) {
 		nvme_pcie_qpair_destroy(ctrlr->adminq);
 	}
@@ -951,6 +941,7 @@ nvme_pcie_ctrlr_destruct(struct spdk_nvme_ctrlr *ctrlr)
 	nvme_pcie_ctrlr_free_bars(pctrlr);
 
 	if (devhandle) {
+		spdk_pci_device_unclaim(devhandle);
 		spdk_pci_device_detach(devhandle);
 	}
 

module/copy/ioat/copy_engine_ioat.c

@@ -256,7 +256,7 @@ probe_cb(void *cb_ctx, struct spdk_pci_device *pci_dev)
 	}
 
 	/* Claim the device in case conflict with other process */
-	if (spdk_pci_device_claim(&pci_addr) < 0) {
+	if (spdk_pci_device_claim(pci_dev) < 0) {
 		return false;
 	}
 

test/env/pci/pci_ut.c

@@ -38,25 +38,19 @@
 #include "env_dpdk/pci.c"
 
 static void
-pci_claim_test(void)
+pci_claim_test(struct spdk_pci_device *dev)
 {
 	int rc = 0;
 	pid_t childPid;
 	int status, ret;
-	struct spdk_pci_addr pci_addr;
 
-	pci_addr.domain = 0x0;
-	pci_addr.bus = 0x5;
-	pci_addr.dev = 0x4;
-	pci_addr.func = 1;
-
-	rc = spdk_pci_device_claim(&pci_addr);
+	rc = spdk_pci_device_claim(dev);
 	CU_ASSERT(rc >= 0);
 
 	childPid = fork();
 	CU_ASSERT(childPid >= 0);
 	if (childPid == 0) {
-		ret = spdk_pci_device_claim(&pci_addr);
+		ret = spdk_pci_device_claim(dev);
 		CU_ASSERT(ret == -1);
 		exit(0);
 	} else {
@@ -206,6 +200,9 @@ pci_hook_test(void)
 	rc = spdk_pci_device_map_bar(&ut_dev.pci, 1, &bar0_vaddr, &bar0_paddr, &bar0_size);
 	CU_ASSERT(rc != 0);
 
+	/* test spdk_pci_device_claim() */
+	pci_claim_test(&ut_dev.pci);
+
 	/* detach and verify our callback was called */
 	spdk_pci_device_detach(&ut_dev.pci);
 	CU_ASSERT(!ut_dev.attached);
@@ -235,7 +232,6 @@ int main(int argc, char **argv)
 	}
 
 	if (
-		CU_add_test(suite, "pci_claim", pci_claim_test) == NULL ||
 		CU_add_test(suite, "pci_hook", pci_hook_test) == NULL
 	) {
 		CU_cleanup_registry();

test/unit/lib/nvme/nvme_pcie.c/nvme_pcie_ut.c

@@ -153,7 +153,13 @@ spdk_pci_device_cfg_write32(struct spdk_pci_device *dev, uint32_t value, uint32_
 }
 
 int
-spdk_pci_device_claim(const struct spdk_pci_addr *pci_addr)
+spdk_pci_device_claim(struct spdk_pci_device *dev)
+{
+	abort();
+}
+
+void
+spdk_pci_device_unclaim(struct spdk_pci_device *dev)
 {
 	abort();
 }