[PATCH v2 11/11] net/tap: track device by ifindex instead of name

[Stephen Hemminger <stephen@networkplumber.org>]

Remove name and remote_iface strings from pmd_internals.
The interface name can change at any time due to udev or admin
action, making cached names unreliable.

Resolve ifindex once at creation via TUNGETIFF on the keep-alive
fd. For per-queue opens, recover the name on demand with
if_indextoname(). In tap_netns_change(), use TUNGETIFF to get
the current kernel name before resolving the new ifindex,
fixing a bug where a rename during namespace move would lose
the device.

Log messages now use dev->device->name (DPDK vdev name) or
the ifindex, following the pattern established by the rtap PMD.

Bugzilla ID: 1880

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/tap/rte_eth_tap.c | 194 +++++++++++++++++++---------------
 drivers/net/tap/rte_eth_tap.h |   2 -
 2 files changed, 108 insertions(+), 88 deletions(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index deb1d72382..f5712055ee 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -130,6 +130,9 @@ static int tap_intr_handle_set(struct rte_eth_dev *dev, int set);
  * @param[in] pmd
  *   Pointer to private structure.
  *
+ * @param[in] ifname
+ *   Name of the TUN/TAP interface to open or create.
+ *
  * @param[in] is_keepalive
  *   Keepalive flag
  *
@@ -140,7 +143,8 @@ static int tap_intr_handle_set(struct rte_eth_dev *dev, int set);
  *   -1 on failure, fd on success
  */
 static int
-tun_alloc(struct pmd_internals *pmd, int is_keepalive, int persistent)
+tun_alloc(struct pmd_internals *pmd, const char *ifname,
+	  int is_keepalive, int persistent)
 {
 	struct ifreq ifr;
 #ifdef IFF_MULTI_QUEUE
@@ -157,7 +161,7 @@ tun_alloc(struct pmd_internals *pmd, int is_keepalive, int persistent)
 	ifr.ifr_flags = (pmd->type == ETH_TUNTAP_TYPE_TAP) ?
 		IFF_TAP : (IFF_TUN | IFF_POINTOPOINT);
 
-	strlcpy(ifr.ifr_name, pmd->name, IFNAMSIZ);
+	strlcpy(ifr.ifr_name, ifname, IFNAMSIZ);
 
 	fd = open(TUN_TAP_DEV_PATH, O_RDWR);
 	if (fd < 0) {
@@ -199,12 +203,7 @@ tun_alloc(struct pmd_internals *pmd, int is_keepalive, int persistent)
 		goto error;
 	}
 
-	/*
-	 * Name passed to kernel might be wildcard like dtun%d
-	 * and need to find the resulting device.
-	 */
-	TAP_LOG(DEBUG, "Device name is '%s'", ifr.ifr_name);
-	strlcpy(pmd->name, ifr.ifr_name, RTE_ETH_NAME_MAX_LEN);
+	TAP_LOG(DEBUG, "Opened TUN/TAP '%s'", ifr.ifr_name);
 
 	if (is_keepalive) {
 		/*
@@ -879,8 +878,6 @@ tap_dev_stop(struct rte_eth_dev *dev)
 static int
 tap_dev_configure(struct rte_eth_dev *dev)
 {
-	struct pmd_internals *pmd = dev->data->dev_private;
-
 	if (dev->data->nb_rx_queues != dev->data->nb_tx_queues) {
 		TAP_LOG(ERR,
 			"%s: number of rx queues %d must be equal to number of tx queues %d",
@@ -890,12 +887,6 @@ tap_dev_configure(struct rte_eth_dev *dev)
 		return -1;
 	}
 
-	TAP_LOG(INFO, "%s: %s: TX configured queues number: %u",
-		dev->device->name, pmd->name, dev->data->nb_tx_queues);
-
-	TAP_LOG(INFO, "%s: %s: RX configured queues number: %u",
-		dev->device->name, pmd->name, dev->data->nb_rx_queues);
-
 	return 0;
 }
 
@@ -1371,17 +1362,18 @@ tap_mac_set(struct rte_eth_dev *dev, struct rte_ether_addr *mac_addr)
 	return 0;
 }
 
-static int tap_carrier_set(struct pmd_internals *pmd, int carrier)
+static int tap_carrier_set(struct rte_eth_dev *dev, int carrier)
 {
+	struct pmd_internals *pmd = dev->data->dev_private;
 #ifdef TUNSETCARRIER
 	int ret = ioctl(pmd->ka_fd, TUNSETCARRIER, &carrier);
 	if (ret < 0) {
 		TAP_LOG(ERR, "%s: ioctl(TUNSETCARRIER) failed: %s",
-			pmd->name, strerror(errno));
+			dev->device->name, strerror(errno));
 		return ret;
 	}
 #else
-	(void)pmd;
+	(void)dev;
 	(void)carrier;
 #endif
 	return 0;
@@ -1407,9 +1399,9 @@ tap_gso_ctx_setup(struct rte_gso_ctx *gso_ctx, struct rte_eth_dev *dev)
 				dev->device->name);
 		if (ret < 0 || ret >= (int)sizeof(pool_name)) {
 			TAP_LOG(ERR,
-				"%s: failed to create mbuf pool name for device %s,"
+				"%s: failed to create mbuf pool name,"
 				"device name too long or output error, ret: %d",
-				pmd->name, dev->device->name, ret);
+				dev->device->name, ret);
 			return -ENAMETOOLONG;
 		}
 		pmd->gso_ctx_mp = rte_pktmbuf_pool_create(pool_name,
@@ -1418,8 +1410,8 @@ tap_gso_ctx_setup(struct rte_gso_ctx *gso_ctx, struct rte_eth_dev *dev)
 			SOCKET_ID_ANY);
 		if (!pmd->gso_ctx_mp) {
 			TAP_LOG(ERR,
-				"%s: failed to create mbuf pool for device %s",
-				pmd->name, dev->device->name);
+				"%s: failed to create mbuf pool",
+				dev->device->name);
 			return -1;
 		}
 	}
@@ -1450,18 +1442,27 @@ tap_setup_queue(struct rte_eth_dev *dev,
 	fd = process_private->fds[qid];
 	if (fd != -1) {
 		/* fd for this queue already exists */
-		TAP_LOG(DEBUG, "%s: fd %d for %s queue qid %d exists",
-			pmd->name, fd, dir, qid);
+		TAP_LOG(DEBUG, "%s: fd %d for %s queue %d exists",
+			dev->device->name, fd, dir, qid);
 		gso_ctx = NULL;
 	} else {
-		fd = tun_alloc(pmd, 0, 0);
+		char ifname[IFNAMSIZ];
+
+		if (if_indextoname(pmd->if_index, ifname) == NULL) {
+			TAP_LOG(ERR, "%s: ifindex %d not found",
+				dev->device->name, pmd->if_index);
+			return -1;
+		}
+
+		fd = tun_alloc(pmd, ifname, 0, 0);
 		if (fd < 0) {
-			TAP_LOG(ERR, "%s: tun_alloc() failed.", pmd->name);
+			TAP_LOG(ERR, "%s: tun_alloc() failed",
+				dev->device->name);
 			return -1;
 		}
 
-		TAP_LOG(DEBUG, "%s: add %s queue for qid %d fd %d",
-			pmd->name, dir, qid, fd);
+		TAP_LOG(DEBUG, "%s: add %s queue %d fd %d",
+			dev->device->name, dir, qid, fd);
 
 		process_private->fds[qid] = fd;
 	}
@@ -1555,12 +1556,12 @@ tap_rx_queue_setup(struct rte_eth_dev *dev,
 	}
 
 	/* set carrier after creating at least one rxq */
-	ret = tap_carrier_set(internals, 1);
+	ret = tap_carrier_set(dev, 1);
 	if (ret < 0)
 		goto error;
 
-	TAP_LOG(DEBUG, "  RX TUNTAP device name %s, qid %d on fd %d",
-		internals->name, rx_queue_id,
+	TAP_LOG(DEBUG, "  RX %s qid %d on fd %d",
+		dev->device->name, rx_queue_id,
 		process_private->fds[rx_queue_id]);
 
 	return 0;
@@ -1603,8 +1604,8 @@ tap_tx_queue_setup(struct rte_eth_dev *dev,
 	if (ret == -1)
 		return -1;
 	TAP_LOG(DEBUG,
-		"  TX TUNTAP device name %s, qid %d on fd %d csum %s",
-		internals->name, tx_queue_id,
+		"  TX %s qid %d on fd %d csum %s",
+		dev->device->name, tx_queue_id,
 		process_private->fds[tx_queue_id],
 		txq->csum ? "on" : "off");
 
@@ -1647,40 +1648,46 @@ tap_netns_change(struct rte_eth_dev *dev)
 {
 	struct pmd_internals *pmd = dev->data->dev_private;
 #ifdef TUNGETDEVNETNS
-	int netns_fd, orig_netns_fd, new_nlsk_fd;
+	struct ifreq ifr = { 0 };
+	int netns_fd, orig_netns_fd, new_nlsk_fd, new_ifindex;
 
 	netns_fd = ioctl(pmd->ka_fd, TUNGETDEVNETNS);
 	if (netns_fd < 0) {
-		TAP_LOG(INFO, "%s: interface deleted", pmd->name);
+		TAP_LOG(INFO, "ifindex %d: interface deleted",
+			pmd->if_index);
 		return 0;
 	}
 
-	/* Interface was moved to another namespace */
-	pmd->if_index = 0;
-
 	/* Save current namespace */
 	orig_netns_fd = open("/proc/self/ns/net", O_RDONLY);
 	if (orig_netns_fd < 0) {
-		TAP_LOG(ERR, "%s: failed to open original netns: %s",
-			pmd->name, strerror(errno));
+		TAP_LOG(ERR, "ifindex %d: failed to open original netns: %s",
+			pmd->if_index, strerror(errno));
 		close(netns_fd);
 		return -1;
 	}
 
 	/* Switch to new namespace */
 	if (setns(netns_fd, CLONE_NEWNET) < 0) {
-		TAP_LOG(ERR, "%s: failed to enter new netns: %s",
-			pmd->name, strerror(errno));
+		TAP_LOG(ERR, "ifindex %d: failed to enter new netns: %s",
+			pmd->if_index, strerror(errno));
 		close(netns_fd);
 		close(orig_netns_fd);
 		return -1;
 	}
 
 	/*
-	 * Update ifindex by querying interface name.
-	 * The interface now has a new ifindex in the new namespace.
+	 * Get the current name from the TUN fd and resolve to the new
+	 * ifindex. TUNGETIFF always returns the current kernel name
+	 * regardless of any renames.
 	 */
-	pmd->if_index = if_nametoindex(pmd->name);
+	if (ioctl(pmd->ka_fd, TUNGETIFF, &ifr) < 0) {
+		TAP_LOG(ERR, "ifindex %d: TUNGETIFF failed: %s",
+			pmd->if_index, strerror(errno));
+		new_ifindex = 0;
+	} else {
+		new_ifindex = if_nametoindex(ifr.ifr_name);
+	}
 
 	/* Recreate netlink socket in new namespace */
 	new_nlsk_fd = tap_nl_init(0);
@@ -1688,32 +1695,31 @@ tap_netns_change(struct rte_eth_dev *dev)
 	/* Recreate LSC interrupt netlink socket in new namespace */
 	rte_intr_callback_unregister_pending(pmd->intr_handle, tap_dev_intr_handler, dev, NULL);
 	if (tap_lsc_intr_handle_set(dev, 1) < 0)
-		TAP_LOG(WARNING, "%s: failed to recreate LSC interrupt socket",
-			pmd->name);
+		TAP_LOG(WARNING, "ifindex %d: failed to recreate LSC socket",
+			pmd->if_index);
 
 	/* Force carrier back after switching netns */
-	tap_carrier_set(pmd, 1);
+	tap_carrier_set(dev, 1);
 
 	/* Switch back to original namespace */
 	if (setns(orig_netns_fd, CLONE_NEWNET) < 0)
-		TAP_LOG(ERR, "%s: failed to return to original netns: %s",
-			pmd->name, strerror(errno));
+		TAP_LOG(ERR, "ifindex %d: failed to return to original netns: %s",
+			pmd->if_index, strerror(errno));
 
 	close(orig_netns_fd);
 	close(netns_fd);
 
-	if (pmd->if_index == 0) {
-		TAP_LOG(WARNING, "%s: interface moved to another namespace, "
-			"failed to get new ifindex",
-			pmd->name);
+	if (new_ifindex == 0) {
+		TAP_LOG(WARNING, "ifindex %d: moved to new namespace, "
+			"failed to get new ifindex", pmd->if_index);
 		if (new_nlsk_fd >= 0)
 			close(new_nlsk_fd);
 		return -1;
 	}
 
 	if (new_nlsk_fd < 0) {
-		TAP_LOG(WARNING, "%s: failed to recreate netlink socket in new namespace",
-			pmd->name);
+		TAP_LOG(WARNING, "ifindex %d: failed to recreate netlink socket",
+			pmd->if_index);
 		return -1;
 	}
 
@@ -1721,12 +1727,13 @@ tap_netns_change(struct rte_eth_dev *dev)
 	if (pmd->nlsk_fd >= 0)
 		tap_nl_final(pmd->nlsk_fd);
 	pmd->nlsk_fd = new_nlsk_fd;
+	pmd->if_index = new_ifindex;
 
-	TAP_LOG(INFO, "%s: interface moved to another namespace, new ifindex: %u",
-		pmd->name, pmd->if_index);
+	TAP_LOG(INFO, "interface moved to new namespace, new ifindex: %u",
+		pmd->if_index);
 #else
-	TAP_LOG(WARNING, "%s: interface deleted or moved to another namespace",
-		pmd->name);
+	TAP_LOG(WARNING, "ifindex %d: interface deleted or moved",
+		pmd->if_index);
 #endif
 
 	return 0;
@@ -2017,7 +2024,6 @@ eth_dev_tap_create(struct rte_vdev_device *vdev, const char *tap_name,
 	pmd = dev->data->dev_private;
 	dev->process_private = process_private;
 	pmd->dev = dev;
-	strlcpy(pmd->name, tap_name, sizeof(pmd->name));
 	pmd->type = type;
 	pmd->ka_fd = -1;
 	pmd->nlsk_fd = -1;
@@ -2069,12 +2075,34 @@ eth_dev_tap_create(struct rte_vdev_device *vdev, const char *tap_name,
 	 * This keep-alive file descriptor will guarantee that the TUN device
 	 * exists even when all of its queues are closed
 	 */
-	pmd->ka_fd = tun_alloc(pmd, 1, persist);
+	pmd->ka_fd = tun_alloc(pmd, tap_name, 1, persist);
 	if (pmd->ka_fd == -1) {
 		TAP_LOG(ERR, "Unable to create %s interface", tuntap_name);
 		goto error_exit;
 	}
-	TAP_LOG(DEBUG, "allocated %s", pmd->name);
+
+	/*
+	 * Get the kernel-assigned name from the TUN fd and resolve the
+	 * ifindex. From this point on, the device is tracked by ifindex
+	 * which is stable even if the interface is renamed.
+	 */
+	{
+		struct ifreq ifr = { 0 };
+
+		if (ioctl(pmd->ka_fd, TUNGETIFF, &ifr) < 0) {
+			TAP_LOG(ERR, "Unable to get interface name: %s",
+				strerror(errno));
+			goto error_exit;
+		}
+		pmd->if_index = if_nametoindex(ifr.ifr_name);
+		if (pmd->if_index == 0) {
+			TAP_LOG(ERR, "Unable to get ifindex for '%s'",
+				ifr.ifr_name);
+			goto error_exit;
+		}
+		TAP_LOG(DEBUG, "Created '%s' ifindex %d",
+			ifr.ifr_name, pmd->if_index);
+	}
 
 	/*
 	 * Create netlink socket for interface control.
@@ -2082,13 +2110,8 @@ eth_dev_tap_create(struct rte_vdev_device *vdev, const char *tap_name,
 	 */
 	pmd->nlsk_fd = tap_nl_init(0);
 	if (pmd->nlsk_fd == -1) {
-		TAP_LOG(ERR, "%s: failed to create netlink socket.", pmd->name);
-		goto error_exit;
-	}
-
-	pmd->if_index = if_nametoindex(pmd->name);
-	if (!pmd->if_index) {
-		TAP_LOG(ERR, "%s: failed to get if_index.", pmd->name);
+		TAP_LOG(ERR, "ifindex %d: failed to create netlink socket",
+			pmd->if_index);
 		goto error_exit;
 	}
 
@@ -2111,13 +2134,13 @@ eth_dev_tap_create(struct rte_vdev_device *vdev, const char *tap_name,
 	 * - implicit rules
 	 */
 	if (qdisc_create_multiq(pmd->nlsk_fd, pmd->if_index) < 0) {
-		TAP_LOG(ERR, "%s: failed to create multiq qdisc.",
-			pmd->name);
+		TAP_LOG(ERR, "ifindex %d: failed to create multiq qdisc",
+			pmd->if_index);
 		goto disable_rte_flow;
 	}
 	if (qdisc_create_ingress(pmd->nlsk_fd, pmd->if_index) < 0) {
-		TAP_LOG(ERR, "%s: failed to create ingress qdisc.",
-			pmd->name);
+		TAP_LOG(ERR, "ifindex %d: failed to create ingress qdisc",
+			pmd->if_index);
 		goto disable_rte_flow;
 	}
 
@@ -2126,11 +2149,10 @@ eth_dev_tap_create(struct rte_vdev_device *vdev, const char *tap_name,
 	if (strlen(remote_iface)) {
 		pmd->remote_if_index = if_nametoindex(remote_iface);
 		if (!pmd->remote_if_index) {
-			TAP_LOG(ERR, "%s: failed to get %s if_index.",
-				pmd->name, remote_iface);
+			TAP_LOG(ERR, "ifindex %d: failed to get ifindex for remote '%s'",
+				pmd->if_index, remote_iface);
 			goto error_remote;
 		}
-		strlcpy(pmd->remote_iface, remote_iface, RTE_ETH_NAME_MAX_LEN);
 
 		/* Save state of remote device */
 		if (tap_nl_get_flags(pmd->nlsk_fd, pmd->remote_if_index,
@@ -2139,14 +2161,14 @@ eth_dev_tap_create(struct rte_vdev_device *vdev, const char *tap_name,
 
 		/* Replicate remote MAC address */
 		if (tap_nl_get_mac(pmd->nlsk_fd, pmd->remote_if_index, &pmd->eth_addr) < 0) {
-			TAP_LOG(ERR, "%s: failed to get %s MAC address.",
-				pmd->name, pmd->remote_iface);
+			TAP_LOG(ERR, "ifindex %d: failed to get remote MAC",
+				pmd->if_index);
 			goto error_remote;
 		}
 
 		if (tap_nl_set_mac(pmd->nlsk_fd, pmd->if_index, &pmd->eth_addr) < 0) {
-			TAP_LOG(ERR, "%s: failed to set %s MAC address.",
-				pmd->name, remote_iface);
+			TAP_LOG(ERR, "ifindex %d: failed to set local MAC",
+				pmd->if_index);
 			goto error_remote;
 		}
 
@@ -2158,8 +2180,8 @@ eth_dev_tap_create(struct rte_vdev_device *vdev, const char *tap_name,
 		qdisc_flush(pmd->nlsk_fd, pmd->remote_if_index);
 		if (qdisc_create_ingress(pmd->nlsk_fd,
 					 pmd->remote_if_index) < 0) {
-			TAP_LOG(ERR, "%s: failed to create ingress qdisc.",
-				pmd->remote_iface);
+			TAP_LOG(ERR, "remote ifindex %d: failed to create ingress qdisc",
+				pmd->remote_if_index);
 			goto error_remote;
 		}
 		LIST_INIT(&pmd->implicit_flows);
@@ -2168,8 +2190,8 @@ eth_dev_tap_create(struct rte_vdev_device *vdev, const char *tap_name,
 		    tap_flow_implicit_create(pmd, TAP_REMOTE_BROADCAST) < 0 ||
 		    tap_flow_implicit_create(pmd, TAP_REMOTE_BROADCASTV6) < 0) {
 			TAP_LOG(ERR,
-				"%s: failed to create implicit rules.",
-				pmd->name);
+				"ifindex %d: failed to create implicit rules",
+				pmd->if_index);
 			goto error_remote;
 		}
 	}
diff --git a/drivers/net/tap/rte_eth_tap.h b/drivers/net/tap/rte_eth_tap.h
index 218ee1b811..74ad0b9253 100644
--- a/drivers/net/tap/rte_eth_tap.h
+++ b/drivers/net/tap/rte_eth_tap.h
@@ -68,8 +68,6 @@ struct tx_queue {
 
 struct pmd_internals {
 	struct rte_eth_dev *dev;          /* Ethernet device. */
-	char remote_iface[RTE_ETH_NAME_MAX_LEN]; /* Remote netdevice name */
-	char name[RTE_ETH_NAME_MAX_LEN];  /* Internal Tap device name */
 	int type;                         /* Type field - TUN|TAP */
 	int persist;			  /* 1 if keep link up, else 0 */
 	struct rte_ether_addr eth_addr;   /* Mac address of the device port */
-- 
2.51.0