[net-next-2.6 PATCH] net: device name allocation cleanups

[net-next-2.6 PATCH] net: device name allocation cleanups

[Octavian Purdila]

Signed-off-by: Octavian Purdila <opurdila@ixiacom.com>
---
 net/core/dev.c |   58 +++++++++++++------------------------------------------
 1 files changed, 14 insertions(+), 44 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 4b24d79..6226960 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -862,6 +862,9 @@ static int __dev_alloc_name(struct net *net, const char *name, char *buf)
 	unsigned long *inuse;
 	struct net_device *d;
 
+	if (!dev_valid_name(name))
+		return -EINVAL;
+
 	p = strnchr(name, IFNAMSIZ-1, '%');
 	if (p) {
 		/*
@@ -901,7 +904,9 @@ static int __dev_alloc_name(struct net *net, const char *name, char *buf)
 	 * when the name is long and there isn't enough space left
 	 * for the digits, or if all bits are used.
 	 */
-	return -ENFILE;
+	if (p)
+		return -ENFILE;
+	return -EEXIST;
 }
 
 /**
@@ -956,22 +961,14 @@ int dev_change_name(struct net_device *dev, const char *newname)
 	if (dev->flags & IFF_UP)
 		return -EBUSY;
 
-	if (!dev_valid_name(newname))
-		return -EINVAL;
-
 	if (strncmp(newname, dev->name, IFNAMSIZ) == 0)
 		return 0;
 
 	memcpy(oldname, dev->name, IFNAMSIZ);
 
-	if (strchr(newname, '%')) {
-		err = dev_alloc_name(dev, newname);
-		if (err < 0)
-			return err;
-	} else if (__dev_get_by_name(net, newname))
-		return -EEXIST;
-	else
-		strlcpy(dev->name, newname, IFNAMSIZ);
+	err = dev_alloc_name(dev, newname);
+	if (err < 0)
+		return err;
 
 rollback:
 	/* For now only devices in the initial network namespace
@@ -4864,8 +4861,6 @@ EXPORT_SYMBOL(netdev_fix_features);
 
 int register_netdevice(struct net_device *dev)
 {
-	struct hlist_head *head;
-	struct hlist_node *p;
 	int ret;
 	struct net *net = dev_net(dev);
 
@@ -4894,26 +4889,10 @@ int register_netdevice(struct net_device *dev)
 		}
 	}
 
-	if (!dev_valid_name(dev->name)) {
-		ret = -EINVAL;
-		goto err_uninit;
-	}
-
 	dev->ifindex = dev_new_index(net);
 	if (dev->iflink == -1)
 		dev->iflink = dev->ifindex;
 
-	/* Check for existence of name */
-	head = dev_name_hash(net, dev->name);
-	hlist_for_each(p, head) {
-		struct net_device *d
-			= hlist_entry(p, struct net_device, name_hlist);
-		if (!strncmp(d->name, dev->name, IFNAMSIZ)) {
-			ret = -EEXIST;
-			goto err_uninit;
-		}
-	}
-
 	/* Fix illegal checksum combinations */
 	if ((dev->features & NETIF_F_HW_CSUM) &&
 	    (dev->features & (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))) {
@@ -5038,11 +5017,9 @@ int register_netdev(struct net_device *dev)
 	 * If the name is a format string the caller wants us to do a
 	 * name allocation.
 	 */
-	if (strchr(dev->name, '%')) {
-		err = dev_alloc_name(dev, dev->name);
-		if (err < 0)
-			goto out;
-	}
+	err = dev_alloc_name(dev, dev->name);
+	if (err < 0)
+		goto out;
 
 	err = register_netdevice(dev);
 out:
@@ -5461,16 +5438,9 @@ int dev_change_net_namespace(struct net_device *dev, struct net *net, const char
 		/* We get here if we can't use the current device name */
 		if (!pat)
 			goto out;
-		if (!dev_valid_name(pat))
-			goto out;
-		if (strchr(pat, '%')) {
-			if (__dev_alloc_name(net, pat, buf) < 0)
-				goto out;
-			destname = buf;
-		} else
-			destname = pat;
-		if (__dev_get_by_name(net, destname))
+		if (__dev_alloc_name(net, pat, buf) < 0)
 			goto out;
+		destname = buf;
 	}
 
 	/*
-- 
1.5.6.5

Re: [net-next-2.6 PATCH] net: device name allocation cleanups

[Stephen Hemminger]

On Tue, 17 Nov 2009 18:49:03 +0200
Octavian Purdila <opurdila@ixiacom.com> wrote:

> Signed-off-by: Octavian Purdila <opurdila@ixiacom.com>
> ---

You have merged the dev_alloc_name into all the registration paths.
Before it was possible for device to call register_netdevice with a name
with a % in it. Only if it did dev_alloc_name or register_netdev would
the %d be interpreted.

So this patch causes a change which may be visible in user space.

-- 

Re: [net-next-2.6 PATCH] net: device name allocation cleanups

[Octavian Purdila]

On Tuesday 17 November 2009 19:01:57 you wrote:
 
> You have merged the dev_alloc_name into all the registration paths.
> Before it was possible for device to call register_netdevice with a name
> with a % in it. Only if it did dev_alloc_name or register_netdev would
> the %d be interpreted.
> 
> So this patch causes a change which may be visible in user space.

Oops, didn't saw that coming :) How about this one?

(tested all code paths except the dev_change_net_namespace)

[net-next-2.6 PATCH] net: device name allocation cleanups

Signed-off-by: Octavian Purdila <opurdila@ixiacom.com>
---
 net/core/dev.c |   73 +++++++++++++++++++++----------------------------------
 1 files changed, 28 insertions(+), 45 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 4b24d79..eec13de 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -893,7 +893,8 @@ static int __dev_alloc_name(struct net *net, const char *name, char *buf)
 		free_page((unsigned long) inuse);
 	}
 
-	snprintf(buf, IFNAMSIZ, name, i);
+	if (buf != name)
+		snprintf(buf, IFNAMSIZ, name, i);
 	if (!__dev_get_by_name(net, buf))
 		return i;
 
@@ -933,6 +934,25 @@ int dev_alloc_name(struct net_device *dev, const char *name)
 }
 EXPORT_SYMBOL(dev_alloc_name);
 
+static int dev_get_valid_name(struct net *net, const char *name, char *buf,
+			      int fmt)
+{
+	if (!dev_valid_name(name))
+		return -EINVAL;
+
+	if (fmt) {
+		if (strchr(name, '%'))
+			return __dev_alloc_name(net, name, buf);
+	}
+
+	if (__dev_get_by_name(net, name))
+		return -EEXIST;
+	else
+		if (buf != name)
+			strlcpy(buf, name, IFNAMSIZ);
+
+	return 0;
+}
 
 /**
  *	dev_change_name - change name of a device
@@ -956,22 +976,14 @@ int dev_change_name(struct net_device *dev, const char *newname)
 	if (dev->flags & IFF_UP)
 		return -EBUSY;
 
-	if (!dev_valid_name(newname))
-		return -EINVAL;
-
 	if (strncmp(newname, dev->name, IFNAMSIZ) == 0)
 		return 0;
 
 	memcpy(oldname, dev->name, IFNAMSIZ);
 
-	if (strchr(newname, '%')) {
-		err = dev_alloc_name(dev, newname);
-		if (err < 0)
-			return err;
-	} else if (__dev_get_by_name(net, newname))
-		return -EEXIST;
-	else
-		strlcpy(dev->name, newname, IFNAMSIZ);
+	err = dev_get_valid_name(net, newname, dev->name, 1);
+	if (err < 0)
+		return err;
 
 rollback:
 	/* For now only devices in the initial network namespace
@@ -4864,8 +4876,6 @@ EXPORT_SYMBOL(netdev_fix_features);
 
 int register_netdevice(struct net_device *dev)
 {
-	struct hlist_head *head;
-	struct hlist_node *p;
 	int ret;
 	struct net *net = dev_net(dev);
 
@@ -4894,26 +4904,14 @@ int register_netdevice(struct net_device *dev)
 		}
 	}
 
-	if (!dev_valid_name(dev->name)) {
-		ret = -EINVAL;
+	ret = dev_get_valid_name(net, dev->name, dev->name, 0);
+	if (ret)
 		goto err_uninit;
-	}
 
 	dev->ifindex = dev_new_index(net);
 	if (dev->iflink == -1)
 		dev->iflink = dev->ifindex;
 
-	/* Check for existence of name */
-	head = dev_name_hash(net, dev->name);
-	hlist_for_each(p, head) {
-		struct net_device *d
-			= hlist_entry(p, struct net_device, name_hlist);
-		if (!strncmp(d->name, dev->name, IFNAMSIZ)) {
-			ret = -EEXIST;
-			goto err_uninit;
-		}
-	}
-
 	/* Fix illegal checksum combinations */
 	if ((dev->features & NETIF_F_HW_CSUM) &&
 	    (dev->features & (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))) {
@@ -5422,8 +5420,6 @@ EXPORT_SYMBOL(unregister_netdev);
 
 int dev_change_net_namespace(struct net_device *dev, struct net *net, const char *pat)
 {
-	char buf[IFNAMSIZ];
-	const char *destname;
 	int err;
 
 	ASSERT_RTNL();
@@ -5456,20 +5452,11 @@ int dev_change_net_namespace(struct net_device *dev, struct net *net, const char
 	 * we can use it in the destination network namespace.
 	 */
 	err = -EEXIST;
-	destname = dev->name;
-	if (__dev_get_by_name(net, destname)) {
+	if (__dev_get_by_name(net, dev->name)) {
 		/* We get here if we can't use the current device name */
 		if (!pat)
 			goto out;
-		if (!dev_valid_name(pat))
-			goto out;
-		if (strchr(pat, '%')) {
-			if (__dev_alloc_name(net, pat, buf) < 0)
-				goto out;
-			destname = buf;
-		} else
-			destname = pat;
-		if (__dev_get_by_name(net, destname))
+		if (dev_get_valid_name(net, pat, dev->name, 1))
 			goto out;
 	}
 
@@ -5505,10 +5492,6 @@ int dev_change_net_namespace(struct net_device *dev, struct net *net, const char
 	/* Actually switch the network namespace */
 	dev_net_set(dev, net);
 
-	/* Assign the new device name */
-	if (destname != dev->name)
-		strcpy(dev->name, destname);
-
 	/* If there is an ifindex conflict assign a new one */
 	if (__dev_get_by_index(net, dev->ifindex)) {
 		int iflink = (dev->iflink == dev->ifindex);
-- 
1.5.6.5

Re: [net-next-2.6 PATCH] net: device name allocation cleanups

[Stephen Hemminger]

On Tue, 17 Nov 2009 22:33:53 +0200
Octavian Purdila <opurdila@ixiacom.com> wrote:

> +static int dev_get_valid_name(struct net *net, const char *name, char *buf,
> +			      int fmt)
                           
you are using fmt as a boolean so declare it 'bool'?

-- 

Re: [net-next-2.6 PATCH] net: device name allocation cleanups

[Stephen Hemminger]

On Tue, 17 Nov 2009 22:33:53 +0200
Octavian Purdila <opurdila@ixiacom.com> wrote:

> +	if (fmt) {
> +		if (strchr(name, '%'))
> +			return __dev_alloc_name(net, name, buf);
> +	}
> +
> +	if (__dev_get_by_name(net, name))


Flatten this logic out.

        if (fmt && strchr(name, '%'))
		return __dev_alloc_name(net, name, buf);
	else if (__dev_get_by_name(net, name))
		return -EEXIST;
	else if (buf != name) 
		strlcpy(buf, name, IFNAMSIZ);

	return 0;

Re: [net-next-2.6 PATCH] net: device name allocation cleanups

[Octavian Purdila]

On Wednesday 18 November 2009 02:46:33 you wrote:
> 
> Flatten this logic out.
> 
>         if (fmt && strchr(name, '%'))
> 		return __dev_alloc_name(net, name, buf);
> 	else if (__dev_get_by_name(net, name))
> 		return -EEXIST;
> 	else if (buf != name)
> 		strlcpy(buf, name, IFNAMSIZ);
> 
> 	return 0;
> 

Thanks for your help ! Here is a new version which addresses the spotted issues:

[net-next-2.6 PATCH] net: device name allocation cleanups

Signed-off-by: Octavian Purdila <opurdila@ixiacom.com>
---
 net/core/dev.c |   69 +++++++++++++++++++------------------------------------
 1 files changed, 24 insertions(+), 45 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 4b24d79..f684ff8 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -893,7 +893,8 @@ static int __dev_alloc_name(struct net *net, const char *name, char *buf)
 		free_page((unsigned long) inuse);
 	}
 
-	snprintf(buf, IFNAMSIZ, name, i);
+	if (buf != name)
+		snprintf(buf, IFNAMSIZ, name, i);
 	if (!__dev_get_by_name(net, buf))
 		return i;
 
@@ -933,6 +934,21 @@ int dev_alloc_name(struct net_device *dev, const char *name)
 }
 EXPORT_SYMBOL(dev_alloc_name);
 
+static int dev_get_valid_name(struct net *net, const char *name, char *buf,
+			      bool fmt)
+{
+	if (!dev_valid_name(name))
+		return -EINVAL;
+
+	if (fmt && strchr(name, '%'))
+		return __dev_alloc_name(net, name, buf);
+	else if (__dev_get_by_name(net, name))
+		return -EEXIST;
+	else if (buf != name)
+		strlcpy(buf, name, IFNAMSIZ);
+
+	return 0;
+}
 
 /**
  *	dev_change_name - change name of a device
@@ -956,22 +972,14 @@ int dev_change_name(struct net_device *dev, const char *newname)
 	if (dev->flags & IFF_UP)
 		return -EBUSY;
 
-	if (!dev_valid_name(newname))
-		return -EINVAL;
-
 	if (strncmp(newname, dev->name, IFNAMSIZ) == 0)
 		return 0;
 
 	memcpy(oldname, dev->name, IFNAMSIZ);
 
-	if (strchr(newname, '%')) {
-		err = dev_alloc_name(dev, newname);
-		if (err < 0)
-			return err;
-	} else if (__dev_get_by_name(net, newname))
-		return -EEXIST;
-	else
-		strlcpy(dev->name, newname, IFNAMSIZ);
+	err = dev_get_valid_name(net, newname, dev->name, 1);
+	if (err < 0)
+		return err;
 
 rollback:
 	/* For now only devices in the initial network namespace
@@ -4864,8 +4872,6 @@ EXPORT_SYMBOL(netdev_fix_features);
 
 int register_netdevice(struct net_device *dev)
 {
-	struct hlist_head *head;
-	struct hlist_node *p;
 	int ret;
 	struct net *net = dev_net(dev);
 
@@ -4894,26 +4900,14 @@ int register_netdevice(struct net_device *dev)
 		}
 	}
 
-	if (!dev_valid_name(dev->name)) {
-		ret = -EINVAL;
+	ret = dev_get_valid_name(net, dev->name, dev->name, 0);
+	if (ret)
 		goto err_uninit;
-	}
 
 	dev->ifindex = dev_new_index(net);
 	if (dev->iflink == -1)
 		dev->iflink = dev->ifindex;
 
-	/* Check for existence of name */
-	head = dev_name_hash(net, dev->name);
-	hlist_for_each(p, head) {
-		struct net_device *d
-			= hlist_entry(p, struct net_device, name_hlist);
-		if (!strncmp(d->name, dev->name, IFNAMSIZ)) {
-			ret = -EEXIST;
-			goto err_uninit;
-		}
-	}
-
 	/* Fix illegal checksum combinations */
 	if ((dev->features & NETIF_F_HW_CSUM) &&
 	    (dev->features & (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))) {
@@ -5422,8 +5416,6 @@ EXPORT_SYMBOL(unregister_netdev);
 
 int dev_change_net_namespace(struct net_device *dev, struct net *net, const char *pat)
 {
-	char buf[IFNAMSIZ];
-	const char *destname;
 	int err;
 
 	ASSERT_RTNL();
@@ -5456,20 +5448,11 @@ int dev_change_net_namespace(struct net_device *dev, struct net *net, const char
 	 * we can use it in the destination network namespace.
 	 */
 	err = -EEXIST;
-	destname = dev->name;
-	if (__dev_get_by_name(net, destname)) {
+	if (__dev_get_by_name(net, dev->name)) {
 		/* We get here if we can't use the current device name */
 		if (!pat)
 			goto out;
-		if (!dev_valid_name(pat))
-			goto out;
-		if (strchr(pat, '%')) {
-			if (__dev_alloc_name(net, pat, buf) < 0)
-				goto out;
-			destname = buf;
-		} else
-			destname = pat;
-		if (__dev_get_by_name(net, destname))
+		if (dev_get_valid_name(net, pat, dev->name, 1))
 			goto out;
 	}
 
@@ -5505,10 +5488,6 @@ int dev_change_net_namespace(struct net_device *dev, struct net *net, const char
 	/* Actually switch the network namespace */
 	dev_net_set(dev, net);
 
-	/* Assign the new device name */
-	if (destname != dev->name)
-		strcpy(dev->name, destname);
-
 	/* If there is an ifindex conflict assign a new one */
 	if (__dev_get_by_index(net, dev->ifindex)) {
 		int iflink = (dev->iflink == dev->ifindex);
-- 
1.5.6.5

Re: [net-next-2.6 PATCH] net: device name allocation cleanups

[David Miller]

From: Octavian Purdila <opurdila@ixiacom.com>
Date: Wed, 18 Nov 2009 14:36:59 +0200

> Thanks for your help ! Here is a new version which addresses the spotted issues:
> 
> [net-next-2.6 PATCH] net: device name allocation cleanups
> 
> Signed-off-by: Octavian Purdila <opurdila@ixiacom.com>

Applied, thanks.