[dpdk-dev] [PATCH 0/6] net/tap: fixes and cleanups

[dpdk-dev] [PATCH 0/6] net/tap: fixes and cleanups

[Stephen Hemminger]

The tap device (used by vdev_netvsc on Azure) has a bug that
prevents it working with primary/secondary process model because
the device name generation assumed a single process.  The fix for
this is to have the kernel assign the device name (patch #5).

While investigating this, found a number of other small issues
that should be cleaned up as well.

Stephen Hemminger (6):
  net/tap: use strlcpy for interface name
  net/tap: allow full length names
  net/tap: check interface name in kvargs
  net/tap: lower the priority of log messages
  net/tap: let kernel choose tun device name
  net/tap: get rid of global tuntap_name

 drivers/net/tap/rte_eth_tap.c | 106 +++++++++++++++++++++-------------
 1 file changed, 65 insertions(+), 41 deletions(-)

-- 
2.20.1

[dpdk-dev] [PATCH 1/6] net/tap: use strlcpy for interface name

[Stephen Hemminger]

snprintf is not needed here, use strlcpy instead.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/tap/rte_eth_tap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 0ec030bef63c..1c00681ba0eb 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -148,7 +148,7 @@ tun_alloc(struct pmd_internals *pmd, int is_keepalive)
 	 */
 	ifr.ifr_flags = (pmd->type == ETH_TUNTAP_TYPE_TAP) ?
 		IFF_TAP : IFF_TUN | IFF_POINTOPOINT;
-	snprintf(ifr.ifr_name, IFNAMSIZ, "%s", pmd->name);
+	strlcpy(ifr.ifr_name, pmd->name, IFNAMSIZ);
 
 	TAP_LOG(DEBUG, "ifr_name '%s'", ifr.ifr_name);
 
-- 
2.20.1

[dpdk-dev] [PATCH 2/6] net/tap: allow full length names

[Stephen Hemminger]

The code for set_interface_name was incorrectly assuming that
space for null byte was necessary with snprintf/strlcpy.

Fixes: 02f96a0a82d1 ("net/tap: add TUN/TAP device PMD")
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/tap/rte_eth_tap.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 1c00681ba0eb..d7f77d664502 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -1892,10 +1892,10 @@ set_interface_name(const char *key __rte_unused,
 	char *name = (char *)extra_args;
 
 	if (value)
-		strlcpy(name, value, RTE_ETH_NAME_MAX_LEN - 1);
+		strlcpy(name, value, RTE_ETH_NAME_MAX_LEN);
 	else
-		snprintf(name, RTE_ETH_NAME_MAX_LEN - 1, "%s%d",
-			 DEFAULT_TAP_NAME, (tap_unit - 1));
+		snprintf(name, RTE_ETH_NAME_MAX_LEN, "%s%d",
+			 DEFAULT_TAP_NAME, tun_unit - 1);
 
 	return 0;
 }
-- 
2.20.1

[dpdk-dev] [PATCH 3/6] net/tap: check interface name in kvargs

[Stephen Hemminger]

If interface name is passed to remote or iface then check
the length and for invalid characters. This avoids problems where
name gets truncated or rejected by kernel.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/tap/rte_eth_tap.c | 37 +++++++++++++++++++++++++++++++----
 1 file changed, 33 insertions(+), 4 deletions(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index d7f77d664502..6a388eed0dd4 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -37,6 +37,7 @@
 #include <linux/if_tun.h>
 #include <linux/if_ether.h>
 #include <fcntl.h>
+#include <ctype.h>
 
 #include <tap_rss.h>
 #include <rte_eth_tap.h>
@@ -1884,6 +1885,23 @@ eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
 	return -EINVAL;
 }
 
+/* make sure name is a possible Linux network device name */
+static bool is_valid_iface(const char *name)
+{
+	if (*name == '\0')
+		return false;
+
+	if (strnlen(name, IFNAMSIZ) == IFNAMSIZ)
+		return false;
+
+	while (*name) {
+		if (*name == '/' || *name == ':' || isspace(*name))
+			return false;
+		name++;
+	}
+	return true;
+}
+
 static int
 set_interface_name(const char *key __rte_unused,
 		   const char *value,
@@ -1891,12 +1909,17 @@ set_interface_name(const char *key __rte_unused,
 {
 	char *name = (char *)extra_args;
 
-	if (value)
+	if (value) {
+		if (!is_valid_iface(value)) {
+			TAP_LOG(ERR, "TAP invalid remote interface name (%s)",
+				value);
+			return -1;
+		}
 		strlcpy(name, value, RTE_ETH_NAME_MAX_LEN);
-	else
+	} else {
 		snprintf(name, RTE_ETH_NAME_MAX_LEN, "%s%d",
 			 DEFAULT_TAP_NAME, tun_unit - 1);
-
+	}
 	return 0;
 }
 
@@ -1907,8 +1930,14 @@ set_remote_iface(const char *key __rte_unused,
 {
 	char *name = (char *)extra_args;
 
-	if (value)
+	if (value) {
+		if (!is_valid_iface(value)) {
+			TAP_LOG(ERR, "TAP invalid remote interface name (%s)",
+				value);
+			return -1;
+		}
 		strlcpy(name, value, RTE_ETH_NAME_MAX_LEN);
+	}
 
 	return 0;
 }
-- 
2.20.1

[dpdk-dev] [PATCH 4/6] net/tap: lower the priority of log messages

[Stephen Hemminger]

Any messages that normally occur during probe should be at DEBUG
level (not NOTICE). This reduces overall log clutter.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/tap/rte_eth_tap.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 6a388eed0dd4..50d6729cb609 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -2052,7 +2052,7 @@ rte_pmd_tun_probe(struct rte_vdev_device *dev)
 	}
 	pmd_link.link_speed = ETH_SPEED_NUM_10G;
 
-	TAP_LOG(NOTICE, "Initializing pmd_tun for %s as %s",
+	TAP_LOG(DEBUG, "Initializing pmd_tun for %s as %s",
 		name, tun_name);
 
 	ret = eth_dev_tap_create(dev, tun_name, remote_iface, 0,
@@ -2257,8 +2257,7 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
 	}
 	pmd_link.link_speed = speed;
 
-	TAP_LOG(NOTICE, "Initializing pmd_tap for %s as %s",
-		name, tap_name);
+	TAP_LOG(DEBUG, "Initializing pmd_tap for %s", name);
 
 	/* Register IPC feed callback */
 	if (!tap_devices_count) {
-- 
2.20.1

[dpdk-dev] [PATCH 5/6] net/tap: let kernel choose tun device name

[Stephen Hemminger]

Assigning tun and tap index in DPDK tap device driver is racy and
fails if used with primary/secondary process model. Instead, use the
kernel feature of device wildcarding where if a name with %d is used
the kernel will fill in the next available device.

Reported-by: hfli@netitest.com
Fixes: 02f96a0a82d1 ("net/tap: add TUN/TAP device PMD")
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/tap/rte_eth_tap.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 50d6729cb609..b836c1ae3d4e 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -79,9 +79,6 @@ static const char *valid_arguments[] = {
 	NULL
 };
 
-static unsigned int tap_unit;
-static unsigned int tun_unit;
-
 static char tuntap_name[8];
 
 static volatile uint32_t tap_trigger;	/* Rx trigger */
@@ -151,8 +148,6 @@ tun_alloc(struct pmd_internals *pmd, int is_keepalive)
 		IFF_TAP : IFF_TUN | IFF_POINTOPOINT;
 	strlcpy(ifr.ifr_name, pmd->name, IFNAMSIZ);
 
-	TAP_LOG(DEBUG, "ifr_name '%s'", ifr.ifr_name);
-
 	fd = open(TUN_TAP_DEV_PATH, O_RDWR);
 	if (fd < 0) {
 		TAP_LOG(ERR, "Unable to create %s interface", tuntap_name);
@@ -186,6 +181,13 @@ tun_alloc(struct pmd_internals *pmd, int is_keepalive)
 		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);
+
 	if (is_keepalive) {
 		/*
 		 * Detach the TUN/TAP keep-alive queue
@@ -1756,6 +1758,7 @@ eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
 		TAP_LOG(ERR, "Unable to create %s interface", tuntap_name);
 		goto error_exit;
 	}
+	TAP_LOG(INFO, "allocated %s", pmd->name);
 
 	ifr.ifr_mtu = dev->data->mtu;
 	if (tap_ioctl(pmd, SIOCSIFMTU, &ifr, 1, LOCAL_AND_REMOTE) < 0)
@@ -1917,8 +1920,8 @@ set_interface_name(const char *key __rte_unused,
 		}
 		strlcpy(name, value, RTE_ETH_NAME_MAX_LEN);
 	} else {
-		snprintf(name, RTE_ETH_NAME_MAX_LEN, "%s%d",
-			 DEFAULT_TAP_NAME, tun_unit - 1);
+		snprintf(name, RTE_ETH_NAME_MAX_LEN, "%s%%d",
+			 DEFAULT_TAP_NAME);
 	}
 	return 0;
 }
@@ -2031,8 +2034,8 @@ rte_pmd_tun_probe(struct rte_vdev_device *dev)
 		return 0;
 	}
 
-	snprintf(tun_name, sizeof(tun_name), "%s%u",
-		 DEFAULT_TUN_NAME, tun_unit++);
+	snprintf(tun_name, sizeof(tun_name), "%s%%d",
+		 DEFAULT_TUN_NAME);
 
 	if (params && (params[0] != '\0')) {
 		TAP_LOG(DEBUG, "parameters (%s)", params);
@@ -2052,17 +2055,15 @@ rte_pmd_tun_probe(struct rte_vdev_device *dev)
 	}
 	pmd_link.link_speed = ETH_SPEED_NUM_10G;
 
-	TAP_LOG(DEBUG, "Initializing pmd_tun for %s as %s",
-		name, tun_name);
+	TAP_LOG(DEBUG, "Initializing pmd_tun for %s", name);
 
 	ret = eth_dev_tap_create(dev, tun_name, remote_iface, 0,
-		ETH_TUNTAP_TYPE_TUN);
+				 ETH_TUNTAP_TYPE_TUN);
 
 leave:
 	if (ret == -1) {
 		TAP_LOG(ERR, "Failed to create pmd for %s as %s",
 			name, tun_name);
-		tun_unit--; /* Restore the unit number */
 	}
 	rte_kvargs_free(kvlist);
 
@@ -2218,8 +2219,8 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
 	}
 
 	speed = ETH_SPEED_NUM_10G;
-	snprintf(tap_name, sizeof(tap_name), "%s%u",
-		 DEFAULT_TAP_NAME, tap_unit++);
+	snprintf(tap_name, sizeof(tap_name),
+		 "%s%%d", DEFAULT_TAP_NAME);
 	memset(remote_iface, 0, RTE_ETH_NAME_MAX_LEN);
 
 	if (params && (params[0] != '\0')) {
@@ -2282,7 +2283,6 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
 				rte_mp_action_unregister(TAP_MP_KEY);
 			tap_devices_count--;
 		}
-		tap_unit--;		/* Restore the unit number */
 	}
 	rte_kvargs_free(kvlist);
 
-- 
2.20.1

[dpdk-dev] [PATCH 6/6] net/tap: get rid of global tuntap_name

[Stephen Hemminger]

Having a global variable which is set to "TUN" or "TAP" during
probe is a potential bug if probing is ever done in different
processes or contexts. Let's fix it now by using existing enum
that has type of connection.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/tap/rte_eth_tap.c | 30 +++++++++++++-----------------
 1 file changed, 13 insertions(+), 17 deletions(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index b836c1ae3d4e..0069930819c2 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -79,8 +79,6 @@ static const char *valid_arguments[] = {
 	NULL
 };
 
-static char tuntap_name[8];
-
 static volatile uint32_t tap_trigger;	/* Rx trigger */
 
 static struct rte_eth_link pmd_link = {
@@ -150,18 +148,17 @@ tun_alloc(struct pmd_internals *pmd, int is_keepalive)
 
 	fd = open(TUN_TAP_DEV_PATH, O_RDWR);
 	if (fd < 0) {
-		TAP_LOG(ERR, "Unable to create %s interface", tuntap_name);
+		TAP_LOG(ERR, "Unable to open %s interface", TUN_TAP_DEV_PATH);
 		goto error;
 	}
 
 #ifdef IFF_MULTI_QUEUE
 	/* Grab the TUN features to verify we can work multi-queue */
 	if (ioctl(fd, TUNGETFEATURES, &features) < 0) {
-		TAP_LOG(ERR, "%s unable to get TUN/TAP features",
-			tuntap_name);
+		TAP_LOG(ERR, "unable to get TUN/TAP features");
 		goto error;
 	}
-	TAP_LOG(DEBUG, "%s Features %08x", tuntap_name, features);
+	TAP_LOG(DEBUG, "%s Features %08x", TUN_TAP_DEV_PATH, features);
 
 	if (features & IFF_MULTI_QUEUE) {
 		TAP_LOG(DEBUG, "  Multi-queue support for %d queues",
@@ -1668,8 +1665,12 @@ static const struct eth_dev_ops ops = {
 	.filter_ctrl            = tap_dev_filter_ctrl,
 };
 
+static const char *tuntap_types[ETH_TUNTAP_TYPE_MAX] = {
+	"UNKNOWN", "TUN", "TAP"
+};
+
 static int
-eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
+eth_dev_tap_create(struct rte_vdev_device *vdev, const char *tap_name,
 		   char *remote_iface, struct ether_addr *mac_addr,
 		   enum rte_tuntap_type type)
 {
@@ -1677,12 +1678,12 @@ eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
 	struct rte_eth_dev *dev;
 	struct pmd_internals *pmd;
 	struct pmd_process_private *process_private;
+	const char *tuntap_name = tuntap_types[type];
 	struct rte_eth_dev_data *data;
 	struct ifreq ifr;
 	int i;
 
-	TAP_LOG(DEBUG, "%s device on numa %u",
-			tuntap_name, rte_socket_id());
+	TAP_LOG(DEBUG, "%s device on numa %u", tuntap_name, rte_socket_id());
 
 	dev = rte_eth_vdev_allocate(vdev, sizeof(*pmd));
 	if (!dev) {
@@ -2015,8 +2016,6 @@ rte_pmd_tun_probe(struct rte_vdev_device *dev)
 	char remote_iface[RTE_ETH_NAME_MAX_LEN];
 	struct rte_eth_dev *eth_dev;
 
-	strcpy(tuntap_name, "TUN");
-
 	name = rte_vdev_device_name(dev);
 	params = rte_vdev_device_args(dev);
 	memset(remote_iface, 0, RTE_ETH_NAME_MAX_LEN);
@@ -2181,8 +2180,6 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
 	struct rte_eth_dev *eth_dev;
 	int tap_devices_count_increased = 0;
 
-	strcpy(tuntap_name, "TAP");
-
 	name = rte_vdev_device_name(dev);
 	params = rte_vdev_device_args(dev);
 
@@ -2264,8 +2261,8 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
 	if (!tap_devices_count) {
 		ret = rte_mp_action_register(TAP_MP_KEY, tap_mp_sync_queues);
 		if (ret < 0) {
-			TAP_LOG(ERR, "%s: Failed to register IPC callback: %s",
-				tuntap_name, strerror(rte_errno));
+			TAP_LOG(ERR, "tap: Failed to register IPC callback: %s",
+				strerror(rte_errno));
 			goto leave;
 		}
 	}
@@ -2314,8 +2311,7 @@ rte_pmd_tap_remove(struct rte_vdev_device *dev)
 	process_private = eth_dev->process_private;
 
 	TAP_LOG(DEBUG, "Closing %s Ethernet device on numa %u",
-		(internals->type == ETH_TUNTAP_TYPE_TAP) ? "TAP" : "TUN",
-		rte_socket_id());
+		tuntap_types[internals->type], rte_socket_id());
 
 	if (internals->nlsk_fd) {
 		tap_flow_flush(eth_dev, NULL);
-- 
2.20.1

Re: [dpdk-dev] [PATCH 2/6] net/tap: allow full length names

[Wiles, Keith]

> On Jan 11, 2019, at 12:06 PM, Stephen Hemminger <stephen@networkplumber.org> wrote:
> 
> The code for set_interface_name was incorrectly assuming that
> space for null byte was necessary with snprintf/strlcpy.
> 
> Fixes: 02f96a0a82d1 ("net/tap: add TUN/TAP device PMD")
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
> drivers/net/tap/rte_eth_tap.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index 1c00681ba0eb..d7f77d664502 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -1892,10 +1892,10 @@ set_interface_name(const char *key __rte_unused,
> 	char *name = (char *)extra_args;
> 
> 	if (value)
> -		strlcpy(name, value, RTE_ETH_NAME_MAX_LEN - 1);
> +		strlcpy(name, value, RTE_ETH_NAME_MAX_LEN);
> 	else
> -		snprintf(name, RTE_ETH_NAME_MAX_LEN - 1, "%s%d",
> -			 DEFAULT_TAP_NAME, (tap_unit - 1));
> +		snprintf(name, RTE_ETH_NAME_MAX_LEN, "%s%d",
> +			 DEFAULT_TAP_NAME, tun_unit - 1);

Was this meant to change from tap_unit to tun_unit here, if so why?
> 
> 	return 0;
> }
> -- 
> 2.20.1
> 

Regards,
Keith

Re: [dpdk-dev] [PATCH 3/6] net/tap: check interface name in kvargs

[Wiles, Keith]

> On Jan 11, 2019, at 12:06 PM, Stephen Hemminger <stephen@networkplumber.org> wrote:
> 
> If interface name is passed to remote or iface then check
> the length and for invalid characters. This avoids problems where
> name gets truncated or rejected by kernel.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
> drivers/net/tap/rte_eth_tap.c | 37 +++++++++++++++++++++++++++++++----
> 1 file changed, 33 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index d7f77d664502..6a388eed0dd4 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -37,6 +37,7 @@
> #include <linux/if_tun.h>
> #include <linux/if_ether.h>
> #include <fcntl.h>
> +#include <ctype.h>
> 
> #include <tap_rss.h>
> #include <rte_eth_tap.h>
> @@ -1884,6 +1885,23 @@ eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
> 	return -EINVAL;
> }
> 
> +/* make sure name is a possible Linux network device name */
> +static bool is_valid_iface(const char *name)

I am sorry, but the function name must be on the next line as per the style. I know you do not like it, but that is what the style states.
> +{
> +	if (*name == '\0')
> +		return false;
> +
> +	if (strnlen(name, IFNAMSIZ) == IFNAMSIZ)
> +		return false;
> +
> +	while (*name) {
> +		if (*name == '/' || *name == ':' || isspace(*name))
> +			return false;
> +		name++;
> +	}
> +	return true;
> +}
> +
> static int
> set_interface_name(const char *key __rte_unused,
> 		   const char *value,
> @@ -1891,12 +1909,17 @@ set_interface_name(const char *key __rte_unused,
> {
> 	char *name = (char *)extra_args;
> 
> -	if (value)
> +	if (value) {
> +		if (!is_valid_iface(value)) {
> +			TAP_LOG(ERR, "TAP invalid remote interface name (%s)",
> +				value);
> +			return -1;
> +		}
> 		strlcpy(name, value, RTE_ETH_NAME_MAX_LEN);
> -	else
> +	} else {
> 		snprintf(name, RTE_ETH_NAME_MAX_LEN, "%s%d",
> 			 DEFAULT_TAP_NAME, tun_unit - 1);
> -
> +	}

This is also a one line else and the style states to remove the {}.
> 	return 0;
> }
> 
> @@ -1907,8 +1930,14 @@ set_remote_iface(const char *key __rte_unused,
> {
> 	char *name = (char *)extra_args;
> 
> -	if (value)
> +	if (value) {
> +		if (!is_valid_iface(value)) {
> +			TAP_LOG(ERR, "TAP invalid remote interface name (%s)",
> +				value);
> +			return -1;
> +		}
> 		strlcpy(name, value, RTE_ETH_NAME_MAX_LEN);
> +	}
> 
> 	return 0;
> }
> -- 
> 2.20.1
> 

I hate having to be the style police, but until we use something to validate the DPDK coding style (as I posted the uncrustify config last month) we have to keep the style the same or change the DPDK coding standard.

Regards,
Keith

Re: [dpdk-dev] [PATCH 5/6] net/tap: let kernel choose tun device name

[Wiles, Keith]

> On Jan 11, 2019, at 12:06 PM, Stephen Hemminger <stephen@networkplumber.org> wrote:
> 
> Assigning tun and tap index in DPDK tap device driver is racy and
> fails if used with primary/secondary process model. Instead, use the
> kernel feature of device wildcarding where if a name with %d is used
> the kernel will fill in the next available device.
> 
> Reported-by: hfli@netitest.com
> Fixes: 02f96a0a82d1 ("net/tap: add TUN/TAP device PMD")
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
> drivers/net/tap/rte_eth_tap.c | 32 ++++++++++++++++----------------
> 1 file changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index 50d6729cb609..b836c1ae3d4e 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -79,9 +79,6 @@ static const char *valid_arguments[] = {
> 	NULL
> };
> 
> -static unsigned int tap_unit;
> -static unsigned int tun_unit;
> -
> static char tuntap_name[8];
> 
> static volatile uint32_t tap_trigger;	/* Rx trigger */
> @@ -151,8 +148,6 @@ tun_alloc(struct pmd_internals *pmd, int is_keepalive)
> 		IFF_TAP : IFF_TUN | IFF_POINTOPOINT;
> 	strlcpy(ifr.ifr_name, pmd->name, IFNAMSIZ);
> 
> -	TAP_LOG(DEBUG, "ifr_name '%s'", ifr.ifr_name);
> -
> 	fd = open(TUN_TAP_DEV_PATH, O_RDWR);
> 	if (fd < 0) {
> 		TAP_LOG(ERR, "Unable to create %s interface", tuntap_name);
> @@ -186,6 +181,13 @@ tun_alloc(struct pmd_internals *pmd, int is_keepalive)
> 		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);
> +
> 	if (is_keepalive) {
> 		/*
> 		 * Detach the TUN/TAP keep-alive queue
> @@ -1756,6 +1758,7 @@ eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
> 		TAP_LOG(ERR, "Unable to create %s interface", tuntap_name);
> 		goto error_exit;
> 	}
> +	TAP_LOG(INFO, "allocated %s", pmd->name);

Is this one required to be INFO as you wanted to reduce log clutter? I am ok with it, but thought I would ask.
> 
> 	ifr.ifr_mtu = dev->data->mtu;
> 	if (tap_ioctl(pmd, SIOCSIFMTU, &ifr, 1, LOCAL_AND_REMOTE) < 0)
> @@ -1917,8 +1920,8 @@ set_interface_name(const char *key __rte_unused,
> 		}
> 		strlcpy(name, value, RTE_ETH_NAME_MAX_LEN);
> 	} else {
> -		snprintf(name, RTE_ETH_NAME_MAX_LEN, "%s%d",
> -			 DEFAULT_TAP_NAME, tun_unit - 1);
> +		snprintf(name, RTE_ETH_NAME_MAX_LEN, "%s%%d",
> +			 DEFAULT_TAP_NAME);

I guess a short comment would be nice for the reader to know that this is a kernel feature syntax.
> 	}
> 	return 0;
> }
> @@ -2031,8 +2034,8 @@ rte_pmd_tun_probe(struct rte_vdev_device *dev)
> 		return 0;
> 	}
> 
> -	snprintf(tun_name, sizeof(tun_name), "%s%u",
> -		 DEFAULT_TUN_NAME, tun_unit++);
> +	snprintf(tun_name, sizeof(tun_name), "%s%%d",
> +		 DEFAULT_TUN_NAME);

Same here.
> 
> 	if (params && (params[0] != '\0')) {
> 		TAP_LOG(DEBUG, "parameters (%s)", params);
> @@ -2052,17 +2055,15 @@ rte_pmd_tun_probe(struct rte_vdev_device *dev)
> 	}
> 	pmd_link.link_speed = ETH_SPEED_NUM_10G;
> 
> -	TAP_LOG(DEBUG, "Initializing pmd_tun for %s as %s",
> -		name, tun_name);
> +	TAP_LOG(DEBUG, "Initializing pmd_tun for %s", name);
> 
> 	ret = eth_dev_tap_create(dev, tun_name, remote_iface, 0,
> -		ETH_TUNTAP_TYPE_TUN);
> +				 ETH_TUNTAP_TYPE_TUN);
> 
> leave:
> 	if (ret == -1) {
> 		TAP_LOG(ERR, "Failed to create pmd for %s as %s",
> 			name, tun_name);
> -		tun_unit--; /* Restore the unit number */
> 	}
> 	rte_kvargs_free(kvlist);
> 
> @@ -2218,8 +2219,8 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
> 	}
> 
> 	speed = ETH_SPEED_NUM_10G;
> -	snprintf(tap_name, sizeof(tap_name), "%s%u",
> -		 DEFAULT_TAP_NAME, tap_unit++);
> +	snprintf(tap_name, sizeof(tap_name),
> +		 "%s%%d", DEFAULT_TAP_NAME);
> 	memset(remote_iface, 0, RTE_ETH_NAME_MAX_LEN);
> 
> 	if (params && (params[0] != '\0')) {
> @@ -2282,7 +2283,6 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
> 				rte_mp_action_unregister(TAP_MP_KEY);
> 			tap_devices_count--;
> 		}
> -		tap_unit--;		/* Restore the unit number */
> 	}
> 	rte_kvargs_free(kvlist);
> 
> -- 
> 2.20.1
> 

Regards,
Keith

Re: [dpdk-dev] [PATCH 0/6] net/tap: fixes and cleanups

[Wiles, Keith]

> On Jan 11, 2019, at 12:06 PM, Stephen Hemminger <stephen@networkplumber.org> wrote:
> 
> The tap device (used by vdev_netvsc on Azure) has a bug that
> prevents it working with primary/secondary process model because
> the device name generation assumed a single process.  The fix for
> this is to have the kernel assign the device name (patch #5).
> 
> While investigating this, found a number of other small issues
> that should be cleaned up as well.
> 
> Stephen Hemminger (6):
>  net/tap: use strlcpy for interface name
>  net/tap: allow full length names
>  net/tap: check interface name in kvargs
>  net/tap: lower the priority of log messages
>  net/tap: let kernel choose tun device name
>  net/tap: get rid of global tuntap_name
> 
> drivers/net/tap/rte_eth_tap.c | 106 +++++++++++++++++++++-------------
> 1 file changed, 65 insertions(+), 41 deletions(-)
> 
> -- 
> 2.20.1
> 

All of these look good except for a few comments. The styling changes I think need to be address, but if everyone is find with it then I  will be OK after a few more drinks.

Acked-by Keith Wiles <keith.wiles@intel.com>

Regards,
Keith

Re: [dpdk-dev] [PATCH 2/6] net/tap: allow full length names

[Stephen Hemminger]

On Fri, 11 Jan 2019 19:32:39 +0000
"Wiles, Keith" <keith.wiles@intel.com> wrote:

> > On Jan 11, 2019, at 12:06 PM, Stephen Hemminger <stephen@networkplumber.org> wrote:
> > 
> > The code for set_interface_name was incorrectly assuming that
> > space for null byte was necessary with snprintf/strlcpy.
> > 
> > Fixes: 02f96a0a82d1 ("net/tap: add TUN/TAP device PMD")
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > ---
> > drivers/net/tap/rte_eth_tap.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> > index 1c00681ba0eb..d7f77d664502 100644
> > --- a/drivers/net/tap/rte_eth_tap.c
> > +++ b/drivers/net/tap/rte_eth_tap.c
> > @@ -1892,10 +1892,10 @@ set_interface_name(const char *key __rte_unused,
> > 	char *name = (char *)extra_args;
> > 
> > 	if (value)
> > -		strlcpy(name, value, RTE_ETH_NAME_MAX_LEN - 1);
> > +		strlcpy(name, value, RTE_ETH_NAME_MAX_LEN);
> > 	else
> > -		snprintf(name, RTE_ETH_NAME_MAX_LEN - 1, "%s%d",
> > -			 DEFAULT_TAP_NAME, (tap_unit - 1));
> > +		snprintf(name, RTE_ETH_NAME_MAX_LEN, "%s%d",
> > +			 DEFAULT_TAP_NAME, tun_unit - 1);  
> 
> Was this meant to change from tap_unit to tun_unit here, if so why?

Accident. tun_unit disappears in later patch.

Re: [dpdk-dev] [PATCH 3/6] net/tap: check interface name in kvargs

[Stephen Hemminger]

On Fri, 11 Jan 2019 19:37:00 +0000
"Wiles, Keith" <keith.wiles@intel.com> wrote:

> > +/* make sure name is a possible Linux network device name */
> > +static bool is_valid_iface(const char *name)  
> 
> I am sorry, but the function name must be on the next line as per the style. I know you do not like it, but that is what the style states.

I am surprised because most of the DPDK follows kernel style.
And in kernel style one line is preferred if line is not too long.

Re: [dpdk-dev] [PATCH 2/6] net/tap: allow full length names

[Wiles, Keith]

> On Jan 11, 2019, at 1:48 PM, Stephen Hemminger <stephen@networkplumber.org> wrote:
> 
> On Fri, 11 Jan 2019 19:32:39 +0000
> "Wiles, Keith" <keith.wiles@intel.com> wrote:
> 
>>> On Jan 11, 2019, at 12:06 PM, Stephen Hemminger <stephen@networkplumber.org> wrote:
>>> 
>>> The code for set_interface_name was incorrectly assuming that
>>> space for null byte was necessary with snprintf/strlcpy.
>>> 
>>> Fixes: 02f96a0a82d1 ("net/tap: add TUN/TAP device PMD")
>>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>>> ---
>>> drivers/net/tap/rte_eth_tap.c | 6 +++---
>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>> 
>>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
>>> index 1c00681ba0eb..d7f77d664502 100644
>>> --- a/drivers/net/tap/rte_eth_tap.c
>>> +++ b/drivers/net/tap/rte_eth_tap.c
>>> @@ -1892,10 +1892,10 @@ set_interface_name(const char *key __rte_unused,
>>> 	char *name = (char *)extra_args;
>>> 
>>> 	if (value)
>>> -		strlcpy(name, value, RTE_ETH_NAME_MAX_LEN - 1);
>>> +		strlcpy(name, value, RTE_ETH_NAME_MAX_LEN);
>>> 	else
>>> -		snprintf(name, RTE_ETH_NAME_MAX_LEN - 1, "%s%d",
>>> -			 DEFAULT_TAP_NAME, (tap_unit - 1));
>>> +		snprintf(name, RTE_ETH_NAME_MAX_LEN, "%s%d",
>>> +			 DEFAULT_TAP_NAME, tun_unit - 1);  
>> 
>> Was this meant to change from tap_unit to tun_unit here, if so why?
> 
> Accident. tun_unit disappears in later patch.

OK, great

Regards,
Keith

Re: [dpdk-dev] [PATCH 3/6] net/tap: check interface name in kvargs

[Wiles, Keith]

> On Jan 11, 2019, at 1:49 PM, Stephen Hemminger <stephen@networkplumber.org> wrote:
> 
> On Fri, 11 Jan 2019 19:37:00 +0000
> "Wiles, Keith" <keith.wiles@intel.com> wrote:
> 
>>> +/* make sure name is a possible Linux network device name */
>>> +static bool is_valid_iface(const char *name)  
>> 
>> I am sorry, but the function name must be on the next line as per the style. I know you do not like it, but that is what the style states.
> 
> I am surprised because most of the DPDK follows kernel style.
> And in kernel style one line is preferred if line is not too long.

Last time I looked at the DPDK coding style it is not the case. We do not really follow Linux coding style only started with it and made changes :-(

Regards,
Keith

[dpdk-dev] [PATCH v2 0/6] net/tap: fixes and cleanups

[Stephen Hemminger]

The tap device (used by vdev_netvsc on Azure) has a bug that
prevents it working with primary/secondary process model because
the device name generation assumed a single process.  The fix for
this is to have the kernel assign the device name (patch #5).

While investigating this, found a number of other small issues
that should be cleaned up as well.

v2
  - encorporate Keith's style feedback and ack
  - use strlcpy instead of snprintf when copying "dtap%d"
    since it makes intent clearer
  - add another message cleanup

Stephen Hemminger (7):
  net/tap: use strlcpy for interface name
  net/tap: allow full length names
  net/tap: check interface name in kvargs
  net/tap: lower the priority of log messages
  net/tap: let kernel choose tun device name
  net/tap: get rid of global tuntap_name
  net/tap: don't print pointer in info message

 drivers/net/tap/rte_eth_tap.c | 118 +++++++++++++++++++++-------------
 1 file changed, 73 insertions(+), 45 deletions(-)

-- 
2.20.1

[dpdk-dev] [PATCH v2 1/7] net/tap: use strlcpy for interface name

[Stephen Hemminger]

snprintf is not needed here, use strlcpy instead.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by Keith Wiles <keith.wiles@intel.com>
---
 drivers/net/tap/rte_eth_tap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 0ec030bef63c..1c00681ba0eb 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -148,7 +148,7 @@ tun_alloc(struct pmd_internals *pmd, int is_keepalive)
 	 */
 	ifr.ifr_flags = (pmd->type == ETH_TUNTAP_TYPE_TAP) ?
 		IFF_TAP : IFF_TUN | IFF_POINTOPOINT;
-	snprintf(ifr.ifr_name, IFNAMSIZ, "%s", pmd->name);
+	strlcpy(ifr.ifr_name, pmd->name, IFNAMSIZ);
 
 	TAP_LOG(DEBUG, "ifr_name '%s'", ifr.ifr_name);
 
-- 
2.20.1

[dpdk-dev] [PATCH v2 2/7] net/tap: allow full length names

[Stephen Hemminger]

The code for set_interface_name was incorrectly assuming that
space for null byte was necessary with snprintf/strlcpy.

Fixes: 02f96a0a82d1 ("net/tap: add TUN/TAP device PMD")
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by Keith Wiles <keith.wiles@intel.com>
---
 drivers/net/tap/rte_eth_tap.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 1c00681ba0eb..11e402e42bd0 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -1892,10 +1892,10 @@ set_interface_name(const char *key __rte_unused,
 	char *name = (char *)extra_args;
 
 	if (value)
-		strlcpy(name, value, RTE_ETH_NAME_MAX_LEN - 1);
+		strlcpy(name, value, RTE_ETH_NAME_MAX_LEN);
 	else
-		snprintf(name, RTE_ETH_NAME_MAX_LEN - 1, "%s%d",
-			 DEFAULT_TAP_NAME, (tap_unit - 1));
+		snprintf(name, RTE_ETH_NAME_MAX_LEN, "%s%d",
+			 DEFAULT_TAP_NAME, tap_unit - 1);
 
 	return 0;
 }
-- 
2.20.1

[dpdk-dev] [PATCH v2 3/7] net/tap: check interface name in kvargs

[Stephen Hemminger]

If interface name is passed to remote or iface then check
the length and for invalid characters. This avoids problems where
name gets truncated or rejected by kernel.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by Keith Wiles <keith.wiles@intel.com>
---
 drivers/net/tap/rte_eth_tap.c | 38 +++++++++++++++++++++++++++++++----
 1 file changed, 34 insertions(+), 4 deletions(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 11e402e42bd0..d8e9ede7ac7c 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -37,6 +37,7 @@
 #include <linux/if_tun.h>
 #include <linux/if_ether.h>
 #include <fcntl.h>
+#include <ctype.h>
 
 #include <tap_rss.h>
 #include <rte_eth_tap.h>
@@ -1884,6 +1885,24 @@ eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
 	return -EINVAL;
 }
 
+/* make sure name is a possible Linux network device name */
+static bool
+is_valid_iface(const char *name)
+{
+	if (*name == '\0')
+		return false;
+
+	if (strnlen(name, IFNAMSIZ) == IFNAMSIZ)
+		return false;
+
+	while (*name) {
+		if (*name == '/' || *name == ':' || isspace(*name))
+			return false;
+		name++;
+	}
+	return true;
+}
+
 static int
 set_interface_name(const char *key __rte_unused,
 		   const char *value,
@@ -1891,12 +1910,17 @@ set_interface_name(const char *key __rte_unused,
 {
 	char *name = (char *)extra_args;
 
-	if (value)
+	if (value) {
+		if (!is_valid_iface(value)) {
+			TAP_LOG(ERR, "TAP invalid remote interface name (%s)",
+				value);
+			return -1;
+		}
 		strlcpy(name, value, RTE_ETH_NAME_MAX_LEN);
-	else
+	} else {
 		snprintf(name, RTE_ETH_NAME_MAX_LEN, "%s%d",
 			 DEFAULT_TAP_NAME, tap_unit - 1);
-
+	}
 	return 0;
 }
 
@@ -1907,8 +1931,14 @@ set_remote_iface(const char *key __rte_unused,
 {
 	char *name = (char *)extra_args;
 
-	if (value)
+	if (value) {
+		if (!is_valid_iface(value)) {
+			TAP_LOG(ERR, "TAP invalid remote interface name (%s)",
+				value);
+			return -1;
+		}
 		strlcpy(name, value, RTE_ETH_NAME_MAX_LEN);
+	}
 
 	return 0;
 }
-- 
2.20.1

[dpdk-dev] [PATCH v2 4/7] net/tap: lower the priority of log messages

[Stephen Hemminger]

Any messages that normally occur during probe should be at DEBUG
level (not NOTICE). This reduces overall log clutter.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by Keith Wiles <keith.wiles@intel.com>
---
 drivers/net/tap/rte_eth_tap.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index d8e9ede7ac7c..d699b4fb095d 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -2053,7 +2053,7 @@ rte_pmd_tun_probe(struct rte_vdev_device *dev)
 	}
 	pmd_link.link_speed = ETH_SPEED_NUM_10G;
 
-	TAP_LOG(NOTICE, "Initializing pmd_tun for %s as %s",
+	TAP_LOG(DEBUG, "Initializing pmd_tun for %s as %s",
 		name, tun_name);
 
 	ret = eth_dev_tap_create(dev, tun_name, remote_iface, 0,
@@ -2258,8 +2258,7 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
 	}
 	pmd_link.link_speed = speed;
 
-	TAP_LOG(NOTICE, "Initializing pmd_tap for %s as %s",
-		name, tap_name);
+	TAP_LOG(DEBUG, "Initializing pmd_tap for %s", name);
 
 	/* Register IPC feed callback */
 	if (!tap_devices_count) {
-- 
2.20.1

[dpdk-dev] [PATCH v2 5/7] net/tap: let kernel choose tun device name

[Stephen Hemminger]

Assigning tun and tap index in DPDK tap device driver is racy
and fails if used with primary/secondary. Instead use the kernel
feature of device wildcarding where if a name with %d is used
the kernel will fill in the next available device.

Reported-by: hfli@netitest.com
Fixes: 02f96a0a82d1 ("net/tap: add TUN/TAP device PMD")
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by Keith Wiles <keith.wiles@intel.com>
---
 drivers/net/tap/rte_eth_tap.c | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index d699b4fb095d..ed8483cb96bc 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -79,9 +79,6 @@ static const char *valid_arguments[] = {
 	NULL
 };
 
-static unsigned int tap_unit;
-static unsigned int tun_unit;
-
 static char tuntap_name[8];
 
 static volatile uint32_t tap_trigger;	/* Rx trigger */
@@ -151,8 +148,6 @@ tun_alloc(struct pmd_internals *pmd, int is_keepalive)
 		IFF_TAP : IFF_TUN | IFF_POINTOPOINT;
 	strlcpy(ifr.ifr_name, pmd->name, IFNAMSIZ);
 
-	TAP_LOG(DEBUG, "ifr_name '%s'", ifr.ifr_name);
-
 	fd = open(TUN_TAP_DEV_PATH, O_RDWR);
 	if (fd < 0) {
 		TAP_LOG(ERR, "Unable to create %s interface", tuntap_name);
@@ -186,6 +181,13 @@ tun_alloc(struct pmd_internals *pmd, int is_keepalive)
 		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);
+
 	if (is_keepalive) {
 		/*
 		 * Detach the TUN/TAP keep-alive queue
@@ -1756,6 +1758,7 @@ eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
 		TAP_LOG(ERR, "Unable to create %s interface", tuntap_name);
 		goto error_exit;
 	}
+	TAP_LOG(DEBUG, "allocated %s", pmd->name);
 
 	ifr.ifr_mtu = dev->data->mtu;
 	if (tap_ioctl(pmd, SIOCSIFMTU, &ifr, 1, LOCAL_AND_REMOTE) < 0)
@@ -1918,8 +1921,8 @@ set_interface_name(const char *key __rte_unused,
 		}
 		strlcpy(name, value, RTE_ETH_NAME_MAX_LEN);
 	} else {
-		snprintf(name, RTE_ETH_NAME_MAX_LEN, "%s%d",
-			 DEFAULT_TAP_NAME, tap_unit - 1);
+		/* use tap%d which causes kernel to choose next available */
+		strlcpy(name, DEFAULT_TAP_NAME "%d", RTE_ETH_NAME_MAX_LEN);
 	}
 	return 0;
 }
@@ -2032,8 +2035,8 @@ rte_pmd_tun_probe(struct rte_vdev_device *dev)
 		return 0;
 	}
 
-	snprintf(tun_name, sizeof(tun_name), "%s%u",
-		 DEFAULT_TUN_NAME, tun_unit++);
+	/* use tun%d which causes kernel to choose next available */
+	strlcpy(tun_name, DEFAULT_TUN_NAME "%d", RTE_ETH_NAME_MAX_LEN);
 
 	if (params && (params[0] != '\0')) {
 		TAP_LOG(DEBUG, "parameters (%s)", params);
@@ -2053,17 +2056,15 @@ rte_pmd_tun_probe(struct rte_vdev_device *dev)
 	}
 	pmd_link.link_speed = ETH_SPEED_NUM_10G;
 
-	TAP_LOG(DEBUG, "Initializing pmd_tun for %s as %s",
-		name, tun_name);
+	TAP_LOG(DEBUG, "Initializing pmd_tun for %s", name);
 
 	ret = eth_dev_tap_create(dev, tun_name, remote_iface, 0,
-		ETH_TUNTAP_TYPE_TUN);
+				 ETH_TUNTAP_TYPE_TUN);
 
 leave:
 	if (ret == -1) {
 		TAP_LOG(ERR, "Failed to create pmd for %s as %s",
 			name, tun_name);
-		tun_unit--; /* Restore the unit number */
 	}
 	rte_kvargs_free(kvlist);
 
@@ -2219,8 +2220,9 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
 	}
 
 	speed = ETH_SPEED_NUM_10G;
-	snprintf(tap_name, sizeof(tap_name), "%s%u",
-		 DEFAULT_TAP_NAME, tap_unit++);
+
+	/* use tap%d which causes kernel to choose next available */
+	strlcpy(tap_name, DEFAULT_TAP_NAME "%d", RTE_ETH_NAME_MAX_LEN);
 	memset(remote_iface, 0, RTE_ETH_NAME_MAX_LEN);
 
 	if (params && (params[0] != '\0')) {
@@ -2283,7 +2285,6 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
 				rte_mp_action_unregister(TAP_MP_KEY);
 			tap_devices_count--;
 		}
-		tap_unit--;		/* Restore the unit number */
 	}
 	rte_kvargs_free(kvlist);
 
-- 
2.20.1

[dpdk-dev] [PATCH v2 6/7] net/tap: get rid of global tuntap_name

[Stephen Hemminger]

Having a global variable which is set to "TUN" or "TAP" during
probe is a potential bug if probing is ever done in different
processes or contexts. Let's fix it now by using existing enum
that has type of connection.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by Keith Wiles <keith.wiles@intel.com>
---
 drivers/net/tap/rte_eth_tap.c | 30 +++++++++++++-----------------
 1 file changed, 13 insertions(+), 17 deletions(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index ed8483cb96bc..705c90dadee5 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -79,8 +79,6 @@ static const char *valid_arguments[] = {
 	NULL
 };
 
-static char tuntap_name[8];
-
 static volatile uint32_t tap_trigger;	/* Rx trigger */
 
 static struct rte_eth_link pmd_link = {
@@ -150,18 +148,17 @@ tun_alloc(struct pmd_internals *pmd, int is_keepalive)
 
 	fd = open(TUN_TAP_DEV_PATH, O_RDWR);
 	if (fd < 0) {
-		TAP_LOG(ERR, "Unable to create %s interface", tuntap_name);
+		TAP_LOG(ERR, "Unable to open %s interface", TUN_TAP_DEV_PATH);
 		goto error;
 	}
 
 #ifdef IFF_MULTI_QUEUE
 	/* Grab the TUN features to verify we can work multi-queue */
 	if (ioctl(fd, TUNGETFEATURES, &features) < 0) {
-		TAP_LOG(ERR, "%s unable to get TUN/TAP features",
-			tuntap_name);
+		TAP_LOG(ERR, "unable to get TUN/TAP features");
 		goto error;
 	}
-	TAP_LOG(DEBUG, "%s Features %08x", tuntap_name, features);
+	TAP_LOG(DEBUG, "%s Features %08x", TUN_TAP_DEV_PATH, features);
 
 	if (features & IFF_MULTI_QUEUE) {
 		TAP_LOG(DEBUG, "  Multi-queue support for %d queues",
@@ -1668,8 +1665,12 @@ static const struct eth_dev_ops ops = {
 	.filter_ctrl            = tap_dev_filter_ctrl,
 };
 
+static const char *tuntap_types[ETH_TUNTAP_TYPE_MAX] = {
+	"UNKNOWN", "TUN", "TAP"
+};
+
 static int
-eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
+eth_dev_tap_create(struct rte_vdev_device *vdev, const char *tap_name,
 		   char *remote_iface, struct ether_addr *mac_addr,
 		   enum rte_tuntap_type type)
 {
@@ -1677,12 +1678,12 @@ eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
 	struct rte_eth_dev *dev;
 	struct pmd_internals *pmd;
 	struct pmd_process_private *process_private;
+	const char *tuntap_name = tuntap_types[type];
 	struct rte_eth_dev_data *data;
 	struct ifreq ifr;
 	int i;
 
-	TAP_LOG(DEBUG, "%s device on numa %u",
-			tuntap_name, rte_socket_id());
+	TAP_LOG(DEBUG, "%s device on numa %u", tuntap_name, rte_socket_id());
 
 	dev = rte_eth_vdev_allocate(vdev, sizeof(*pmd));
 	if (!dev) {
@@ -2016,8 +2017,6 @@ rte_pmd_tun_probe(struct rte_vdev_device *dev)
 	char remote_iface[RTE_ETH_NAME_MAX_LEN];
 	struct rte_eth_dev *eth_dev;
 
-	strcpy(tuntap_name, "TUN");
-
 	name = rte_vdev_device_name(dev);
 	params = rte_vdev_device_args(dev);
 	memset(remote_iface, 0, RTE_ETH_NAME_MAX_LEN);
@@ -2182,8 +2181,6 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
 	struct rte_eth_dev *eth_dev;
 	int tap_devices_count_increased = 0;
 
-	strcpy(tuntap_name, "TAP");
-
 	name = rte_vdev_device_name(dev);
 	params = rte_vdev_device_args(dev);
 
@@ -2266,8 +2263,8 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
 	if (!tap_devices_count) {
 		ret = rte_mp_action_register(TAP_MP_KEY, tap_mp_sync_queues);
 		if (ret < 0) {
-			TAP_LOG(ERR, "%s: Failed to register IPC callback: %s",
-				tuntap_name, strerror(rte_errno));
+			TAP_LOG(ERR, "tap: Failed to register IPC callback: %s",
+				strerror(rte_errno));
 			goto leave;
 		}
 	}
@@ -2316,8 +2313,7 @@ rte_pmd_tap_remove(struct rte_vdev_device *dev)
 	process_private = eth_dev->process_private;
 
 	TAP_LOG(DEBUG, "Closing %s Ethernet device on numa %u",
-		(internals->type == ETH_TUNTAP_TYPE_TAP) ? "TAP" : "TUN",
-		rte_socket_id());
+		tuntap_types[internals->type], rte_socket_id());
 
 	if (internals->nlsk_fd) {
 		tap_flow_flush(eth_dev, NULL);
-- 
2.20.1

[dpdk-dev] [PATCH v2 7/7] net/tap: don't print pointer in info message

[Stephen Hemminger]

Printing pointer in log is uninformative (unless in a debugger),
instead print the assigned kernel device name which correlates
well with what TAP is doing.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/tap/rte_eth_tap.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 705c90dadee5..586c8a952df9 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -853,6 +853,8 @@ 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 > RTE_PMD_TAP_MAX_QUEUES) {
 		TAP_LOG(ERR,
 			"%s: number of rx queues %d exceeds max num of queues %d",
@@ -870,11 +872,11 @@ tap_dev_configure(struct rte_eth_dev *dev)
 		return -1;
 	}
 
-	TAP_LOG(INFO, "%s: %p: TX configured queues number: %u",
-		dev->device->name, (void *)dev, dev->data->nb_tx_queues);
+	TAP_LOG(INFO, "%s: %s: TX configured queues number: %u",
+		dev->device->name, pmd->name, dev->data->nb_tx_queues);
 
-	TAP_LOG(INFO, "%s: %p: RX configured queues number: %u",
-		dev->device->name, (void *)dev, dev->data->nb_rx_queues);
+	TAP_LOG(INFO, "%s: %s: RX configured queues number: %u",
+		dev->device->name, pmd->name, dev->data->nb_rx_queues);
 
 	return 0;
 }
-- 
2.20.1

Re: [dpdk-dev] [PATCH 3/6] net/tap: check interface name in kvargs

[Luse, Paul E]

Thanks Jim!!

-from my iPhone 

> On Jan 11, 2019, at 11:37 AM, Wiles, Keith <keith.wiles@intel.com> wrote:
> 
> 
> 
>> On Jan 11, 2019, at 12:06 PM, Stephen Hemminger <stephen@networkplumber.org> wrote:
>> 
>> If interface name is passed to remote or iface then check
>> the length and for invalid characters. This avoids problems where
>> name gets truncated or rejected by kernel.
>> 
>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>> ---
>> drivers/net/tap/rte_eth_tap.c | 37 +++++++++++++++++++++++++++++++----
>> 1 file changed, 33 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
>> index d7f77d664502..6a388eed0dd4 100644
>> --- a/drivers/net/tap/rte_eth_tap.c
>> +++ b/drivers/net/tap/rte_eth_tap.c
>> @@ -37,6 +37,7 @@
>> #include <linux/if_tun.h>
>> #include <linux/if_ether.h>
>> #include <fcntl.h>
>> +#include <ctype.h>
>> 
>> #include <tap_rss.h>
>> #include <rte_eth_tap.h>
>> @@ -1884,6 +1885,23 @@ eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
>>    return -EINVAL;
>> }
>> 
>> +/* make sure name is a possible Linux network device name */
>> +static bool is_valid_iface(const char *name)
> 
> I am sorry, but the function name must be on the next line as per the style. I know you do not like it, but that is what the style states.
>> +{
>> +    if (*name == '\0')
>> +        return false;
>> +
>> +    if (strnlen(name, IFNAMSIZ) == IFNAMSIZ)
>> +        return false;
>> +
>> +    while (*name) {
>> +        if (*name == '/' || *name == ':' || isspace(*name))
>> +            return false;
>> +        name++;
>> +    }
>> +    return true;
>> +}
>> +
>> static int
>> set_interface_name(const char *key __rte_unused,
>>           const char *value,
>> @@ -1891,12 +1909,17 @@ set_interface_name(const char *key __rte_unused,
>> {
>>    char *name = (char *)extra_args;
>> 
>> -    if (value)
>> +    if (value) {
>> +        if (!is_valid_iface(value)) {
>> +            TAP_LOG(ERR, "TAP invalid remote interface name (%s)",
>> +                value);
>> +            return -1;
>> +        }
>>        strlcpy(name, value, RTE_ETH_NAME_MAX_LEN);
>> -    else
>> +    } else {
>>        snprintf(name, RTE_ETH_NAME_MAX_LEN, "%s%d",
>>             DEFAULT_TAP_NAME, tun_unit - 1);
>> -
>> +    }
> 
> This is also a one line else and the style states to remove the {}.
>>    return 0;
>> }
>> 
>> @@ -1907,8 +1930,14 @@ set_remote_iface(const char *key __rte_unused,
>> {
>>    char *name = (char *)extra_args;
>> 
>> -    if (value)
>> +    if (value) {
>> +        if (!is_valid_iface(value)) {
>> +            TAP_LOG(ERR, "TAP invalid remote interface name (%s)",
>> +                value);
>> +            return -1;
>> +        }
>>        strlcpy(name, value, RTE_ETH_NAME_MAX_LEN);
>> +    }
>> 
>>    return 0;
>> }
>> -- 
>> 2.20.1
>> 
> 
> I hate having to be the style police, but until we use something to validate the DPDK coding style (as I posted the uncrustify config last month) we have to keep the style the same or change the DPDK coding standard.
> 
> Regards,
> Keith
> 

Re: [dpdk-dev] [PATCH v2 7/7] net/tap: don't print pointer in info message

[Ferruh Yigit]

On 1/11/2019 8:35 PM, Stephen Hemminger wrote:
> Printing pointer in log is uninformative (unless in a debugger),
> instead print the assigned kernel device name which correlates
> well with what TAP is doing.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

Re: [dpdk-dev] [PATCH v2 0/6] net/tap: fixes and cleanups

[Ferruh Yigit]

On 1/11/2019 8:35 PM, Stephen Hemminger wrote:
> The tap device (used by vdev_netvsc on Azure) has a bug that
> prevents it working with primary/secondary process model because
> the device name generation assumed a single process.  The fix for
> this is to have the kernel assign the device name (patch #5).
> 
> While investigating this, found a number of other small issues
> that should be cleaned up as well.
> 
> v2
>   - encorporate Keith's style feedback and ack
>   - use strlcpy instead of snprintf when copying "dtap%d"
>     since it makes intent clearer
>   - add another message cleanup
> 
> Stephen Hemminger (7):
>   net/tap: use strlcpy for interface name
>   net/tap: allow full length names
>   net/tap: check interface name in kvargs
>   net/tap: lower the priority of log messages
>   net/tap: let kernel choose tun device name
>   net/tap: get rid of global tuntap_name
>   net/tap: don't print pointer in info message

Series applied to dpdk-next-net/master, thanks.

Re: [dpdk-dev] [PATCH v2 7/7] net/tap: don't print pointer in info message

[Wiles, Keith]

> On Jan 11, 2019, at 2:35 PM, Stephen Hemminger <stephen@networkplumber.org> wrote:
> 
> Printing pointer in log is uninformative (unless in a debugger),
> instead print the assigned kernel device name which correlates
> well with what TAP is doing.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
> drivers/net/tap/rte_eth_tap.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index 705c90dadee5..586c8a952df9 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -853,6 +853,8 @@ 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 > RTE_PMD_TAP_MAX_QUEUES) {
> 		TAP_LOG(ERR,
> 			"%s: number of rx queues %d exceeds max num of queues %d",
> @@ -870,11 +872,11 @@ tap_dev_configure(struct rte_eth_dev *dev)
> 		return -1;
> 	}
> 
> -	TAP_LOG(INFO, "%s: %p: TX configured queues number: %u",
> -		dev->device->name, (void *)dev, dev->data->nb_tx_queues);
> +	TAP_LOG(INFO, "%s: %s: TX configured queues number: %u",
> +		dev->device->name, pmd->name, dev->data->nb_tx_queues);
> 
> -	TAP_LOG(INFO, "%s: %p: RX configured queues number: %u",
> -		dev->device->name, (void *)dev, dev->data->nb_rx_queues);
> +	TAP_LOG(INFO, "%s: %s: RX configured queues number: %u",
> +		dev->device->name, pmd->name, dev->data->nb_rx_queues);

Acked-by: Keith Wiles <keith.wiles@intel.com>
> 
> 	return 0;
> }
> -- 
> 2.20.1
> 

Regards,
Keith

Re: [dpdk-dev] [PATCH 3/6] net/tap: check interface name in kvargs

[Thomas Monjalon]

11/01/2019 20:50, Wiles, Keith:
> 
> > On Jan 11, 2019, at 1:49 PM, Stephen Hemminger <stephen@networkplumber.org> wrote:
> > 
> > On Fri, 11 Jan 2019 19:37:00 +0000
> > "Wiles, Keith" <keith.wiles@intel.com> wrote:
> > 
> >>> +/* make sure name is a possible Linux network device name */
> >>> +static bool is_valid_iface(const char *name)  
> >> 
> >> I am sorry, but the function name must be on the next line as per the style. I know you do not like it, but that is what the style states.
> > 
> > I am surprised because most of the DPDK follows kernel style.
> > And in kernel style one line is preferred if line is not too long.
> 
> Last time I looked at the DPDK coding style it is not the case. We do not really follow Linux coding style only started with it and made changes :-(

Not really. The original code style is borrowed from FreeBSD:
	https://www.freebsd.org/cgi/man.cgi?query=style&sektion=9