* [net-next-2.6 PATCH] net: device name allocation cleanups @ 2009-11-17 16:49 Octavian Purdila 2009-11-17 17:01 ` Stephen Hemminger 0 siblings, 1 reply; 7+ messages in thread From: Octavian Purdila @ 2009-11-17 16:49 UTC (permalink / raw) To: netdev 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [net-next-2.6 PATCH] net: device name allocation cleanups 2009-11-17 16:49 [net-next-2.6 PATCH] net: device name allocation cleanups Octavian Purdila @ 2009-11-17 17:01 ` Stephen Hemminger 2009-11-17 20:33 ` Octavian Purdila 0 siblings, 1 reply; 7+ messages in thread From: Stephen Hemminger @ 2009-11-17 17:01 UTC (permalink / raw) To: Octavian Purdila; +Cc: netdev 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. -- ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [net-next-2.6 PATCH] net: device name allocation cleanups 2009-11-17 17:01 ` Stephen Hemminger @ 2009-11-17 20:33 ` Octavian Purdila 2009-11-18 0:42 ` Stephen Hemminger 2009-11-18 0:46 ` Stephen Hemminger 0 siblings, 2 replies; 7+ messages in thread From: Octavian Purdila @ 2009-11-17 20:33 UTC (permalink / raw) To: Stephen Hemminger; +Cc: netdev 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [net-next-2.6 PATCH] net: device name allocation cleanups 2009-11-17 20:33 ` Octavian Purdila @ 2009-11-18 0:42 ` Stephen Hemminger 2009-11-18 0:46 ` Stephen Hemminger 1 sibling, 0 replies; 7+ messages in thread From: Stephen Hemminger @ 2009-11-18 0:42 UTC (permalink / raw) To: Octavian Purdila; +Cc: netdev 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'? -- ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [net-next-2.6 PATCH] net: device name allocation cleanups 2009-11-17 20:33 ` Octavian Purdila 2009-11-18 0:42 ` Stephen Hemminger @ 2009-11-18 0:46 ` Stephen Hemminger 2009-11-18 12:36 ` Octavian Purdila 1 sibling, 1 reply; 7+ messages in thread From: Stephen Hemminger @ 2009-11-18 0:46 UTC (permalink / raw) To: Octavian Purdila; +Cc: netdev 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; ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [net-next-2.6 PATCH] net: device name allocation cleanups 2009-11-18 0:46 ` Stephen Hemminger @ 2009-11-18 12:36 ` Octavian Purdila 2009-11-18 13:07 ` David Miller 0 siblings, 1 reply; 7+ messages in thread From: Octavian Purdila @ 2009-11-18 12:36 UTC (permalink / raw) To: Stephen Hemminger; +Cc: netdev 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [net-next-2.6 PATCH] net: device name allocation cleanups 2009-11-18 12:36 ` Octavian Purdila @ 2009-11-18 13:07 ` David Miller 0 siblings, 0 replies; 7+ messages in thread From: David Miller @ 2009-11-18 13:07 UTC (permalink / raw) To: opurdila; +Cc: shemminger, netdev 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2009-11-18 13:07 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-11-17 16:49 [net-next-2.6 PATCH] net: device name allocation cleanups Octavian Purdila 2009-11-17 17:01 ` Stephen Hemminger 2009-11-17 20:33 ` Octavian Purdila 2009-11-18 0:42 ` Stephen Hemminger 2009-11-18 0:46 ` Stephen Hemminger 2009-11-18 12:36 ` Octavian Purdila 2009-11-18 13:07 ` David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).