From bd07063dd11f6fda903802a5868960be3690d327 Mon Sep 17 00:00:00 2001 From: Jakub Kicinski Date: Mon, 23 Oct 2023 08:23:41 -0700 Subject: [PATCH 1/6] net: don't use input buffer of __dev_alloc_name() as a scratch space Callers of __dev_alloc_name() want to pass dev->name as the output buffer. Make __dev_alloc_name() not clobber that buffer on failure, and remove the workarounds in callers. dev_alloc_name_ns() is now completely unnecessary. The extra strscpy() added here will be gone by the end of the patch series. Reviewed-by: Jiri Pirko Link: https://lore.kernel.org/r/20231023152346.3639749-2-kuba@kernel.org Signed-off-by: Jakub Kicinski --- net/core/dev.c | 33 ++++++++------------------------- 1 file changed, 8 insertions(+), 25 deletions(-) diff --git a/net/core/dev.c b/net/core/dev.c index 1025dc79bc49e..874c7daa81f54 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -1057,7 +1057,7 @@ EXPORT_SYMBOL(dev_valid_name); * __dev_alloc_name - allocate a name for a device * @net: network namespace to allocate the device name in * @name: name format string - * @buf: scratch buffer and result name string + * @res: result name string * * Passed a format string - eg "lt%d" it will try and find a suitable * id. It scans list of devices to build up a free map, then chooses @@ -1068,13 +1068,14 @@ EXPORT_SYMBOL(dev_valid_name); * Returns the number of the unit assigned or a negative errno code. */ -static int __dev_alloc_name(struct net *net, const char *name, char *buf) +static int __dev_alloc_name(struct net *net, const char *name, char *res) { int i = 0; const char *p; const int max_netdevices = 8*PAGE_SIZE; unsigned long *inuse; struct net_device *d; + char buf[IFNAMSIZ]; if (!dev_valid_name(name)) return -EINVAL; @@ -1124,8 +1125,10 @@ static int __dev_alloc_name(struct net *net, const char *name, char *buf) } snprintf(buf, IFNAMSIZ, name, i); - if (!netdev_name_in_use(net, buf)) + if (!netdev_name_in_use(net, buf)) { + strscpy(res, buf, IFNAMSIZ); return i; + } /* It is possible to run out of possible slots * when the name is long and there isn't enough space left @@ -1154,20 +1157,6 @@ static int dev_prep_valid_name(struct net *net, struct net_device *dev, return 0; } -static int dev_alloc_name_ns(struct net *net, - struct net_device *dev, - const char *name) -{ - char buf[IFNAMSIZ]; - int ret; - - BUG_ON(!net); - ret = __dev_alloc_name(net, name, buf); - if (ret >= 0) - strscpy(dev->name, buf, IFNAMSIZ); - return ret; -} - /** * dev_alloc_name - allocate a name for a device * @dev: device @@ -1184,20 +1173,14 @@ static int dev_alloc_name_ns(struct net *net, int dev_alloc_name(struct net_device *dev, const char *name) { - return dev_alloc_name_ns(dev_net(dev), dev, name); + return __dev_alloc_name(dev_net(dev), name, dev->name); } EXPORT_SYMBOL(dev_alloc_name); static int dev_get_valid_name(struct net *net, struct net_device *dev, const char *name) { - char buf[IFNAMSIZ]; - int ret; - - ret = dev_prep_valid_name(net, dev, name, buf); - if (ret >= 0) - strscpy(dev->name, buf, IFNAMSIZ); - return ret; + return dev_prep_valid_name(net, dev, name, dev->name); } /** From 556c755a4d8143007c745b6ad894611a04173b53 Mon Sep 17 00:00:00 2001 From: Jakub Kicinski Date: Mon, 23 Oct 2023 08:23:42 -0700 Subject: [PATCH 2/6] net: make dev_alloc_name() call dev_prep_valid_name() __dev_alloc_name() handles both the sprintf and non-sprintf target names. This complicates the code. dev_prep_valid_name() already handles the non-sprintf case, before calling __dev_alloc_name(), make the only other caller also go thru dev_prep_valid_name(). This way we can drop the non-sprintf handling in __dev_alloc_name() in one of the next changes. commit 55a5ec9b7710 ("Revert "net: core: dev_get_valid_name is now the same as dev_alloc_name_ns"") and commit 029b6d140550 ("Revert "net: core: maybe return -EEXIST in __dev_alloc_name"") tell us that we can't start returning -EEXIST from dev_alloc_name() on name duplicates. Bite the bullet and pass the expected errno to dev_prep_valid_name(). dev_prep_valid_name() must now propagate out the allocated id for printf names. Reviewed-by: Jiri Pirko Link: https://lore.kernel.org/r/20231023152346.3639749-3-kuba@kernel.org Signed-off-by: Jakub Kicinski --- net/core/dev.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/net/core/dev.c b/net/core/dev.c index 874c7daa81f54..004e9f26b1600 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -1137,19 +1137,18 @@ static int __dev_alloc_name(struct net *net, const char *name, char *res) return -ENFILE; } +/* Returns negative errno or allocated unit id (see __dev_alloc_name()) */ static int dev_prep_valid_name(struct net *net, struct net_device *dev, - const char *want_name, char *out_name) + const char *want_name, char *out_name, + int dup_errno) { - int ret; - if (!dev_valid_name(want_name)) return -EINVAL; if (strchr(want_name, '%')) { - ret = __dev_alloc_name(net, want_name, out_name); - return ret < 0 ? ret : 0; + return __dev_alloc_name(net, want_name, out_name); } else if (netdev_name_in_use(net, want_name)) { - return -EEXIST; + return -dup_errno; } else if (out_name != want_name) { strscpy(out_name, want_name, IFNAMSIZ); } @@ -1173,14 +1172,17 @@ static int dev_prep_valid_name(struct net *net, struct net_device *dev, int dev_alloc_name(struct net_device *dev, const char *name) { - return __dev_alloc_name(dev_net(dev), name, dev->name); + return dev_prep_valid_name(dev_net(dev), dev, name, dev->name, ENFILE); } EXPORT_SYMBOL(dev_alloc_name); static int dev_get_valid_name(struct net *net, struct net_device *dev, const char *name) { - return dev_prep_valid_name(net, dev, name, dev->name); + int ret; + + ret = dev_prep_valid_name(net, dev, name, dev->name, EEXIST); + return ret < 0 ? ret : 0; } /** @@ -11118,7 +11120,7 @@ int __dev_change_net_namespace(struct net_device *dev, struct net *net, /* We get here if we can't use the current device name */ if (!pat) goto out; - err = dev_prep_valid_name(net, dev, pat, new_name); + err = dev_prep_valid_name(net, dev, pat, new_name, EEXIST); if (err < 0) goto out; } From 9a810468126c846299d867f73dd7053064c29be1 Mon Sep 17 00:00:00 2001 From: Jakub Kicinski Date: Mon, 23 Oct 2023 08:23:43 -0700 Subject: [PATCH 3/6] net: reduce indentation of __dev_alloc_name() All callers of __dev_valid_name() go thru dev_prep_valid_name() which handles the non-printf case. Focus __dev_alloc_name() on the sprintf case, remove the indentation level. Minor functional change of returning -EINVAL if % is not found, which should now never happen. Reviewed-by: Jiri Pirko Link: https://lore.kernel.org/r/20231023152346.3639749-4-kuba@kernel.org Signed-off-by: Jakub Kicinski --- net/core/dev.c | 56 +++++++++++++++++++++++--------------------------- 1 file changed, 26 insertions(+), 30 deletions(-) diff --git a/net/core/dev.c b/net/core/dev.c index 004e9f26b1600..bbfb02b4a228c 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -1080,50 +1080,46 @@ static int __dev_alloc_name(struct net *net, const char *name, char *res) if (!dev_valid_name(name)) return -EINVAL; + /* Verify the string as this thing may have come from the user. + * There must be one "%d" and no other "%" characters. + */ p = strchr(name, '%'); - if (p) { - /* - * Verify the string as this thing may have come from - * the user. There must be either one "%d" and no other "%" - * characters. - */ - if (p[1] != 'd' || strchr(p + 2, '%')) - return -EINVAL; - - /* Use one page as a bit array of possible slots */ - inuse = bitmap_zalloc(max_netdevices, GFP_ATOMIC); - if (!inuse) - return -ENOMEM; + if (!p || p[1] != 'd' || strchr(p + 2, '%')) + return -EINVAL; - for_each_netdev(net, d) { - struct netdev_name_node *name_node; + /* Use one page as a bit array of possible slots */ + inuse = bitmap_zalloc(max_netdevices, GFP_ATOMIC); + if (!inuse) + return -ENOMEM; - netdev_for_each_altname(d, name_node) { - if (!sscanf(name_node->name, name, &i)) - continue; - if (i < 0 || i >= max_netdevices) - continue; + for_each_netdev(net, d) { + struct netdev_name_node *name_node; - /* avoid cases where sscanf is not exact inverse of printf */ - snprintf(buf, IFNAMSIZ, name, i); - if (!strncmp(buf, name_node->name, IFNAMSIZ)) - __set_bit(i, inuse); - } - if (!sscanf(d->name, name, &i)) + netdev_for_each_altname(d, name_node) { + if (!sscanf(name_node->name, name, &i)) continue; if (i < 0 || i >= max_netdevices) continue; - /* avoid cases where sscanf is not exact inverse of printf */ + /* avoid cases where sscanf is not exact inverse of printf */ snprintf(buf, IFNAMSIZ, name, i); - if (!strncmp(buf, d->name, IFNAMSIZ)) + if (!strncmp(buf, name_node->name, IFNAMSIZ)) __set_bit(i, inuse); } + if (!sscanf(d->name, name, &i)) + continue; + if (i < 0 || i >= max_netdevices) + continue; - i = find_first_zero_bit(inuse, max_netdevices); - bitmap_free(inuse); + /* avoid cases where sscanf is not exact inverse of printf */ + snprintf(buf, IFNAMSIZ, name, i); + if (!strncmp(buf, d->name, IFNAMSIZ)) + __set_bit(i, inuse); } + i = find_first_zero_bit(inuse, max_netdevices); + bitmap_free(inuse); + snprintf(buf, IFNAMSIZ, name, i); if (!netdev_name_in_use(net, buf)) { strscpy(res, buf, IFNAMSIZ); From 7ad17b04dc7bdcdd1f85e460c38da55b0afa2422 Mon Sep 17 00:00:00 2001 From: Jakub Kicinski Date: Mon, 23 Oct 2023 08:23:44 -0700 Subject: [PATCH 4/6] net: trust the bitmap in __dev_alloc_name() Prior to restructuring __dev_alloc_name() handled both printf and non-printf names. In a clever attempt at code reuse it always prints the name into a buffer and checks if it's a duplicate. Trust the bitmap, and return an error if its full. This shrinks the possible ID space by one from 32K to 32K - 1, as previously the max value would have been tried as a valid ID. It seems very unlikely that anyone would care as we heard no requests to increase the max beyond 32k. Reviewed-by: Jiri Pirko Link: https://lore.kernel.org/r/20231023152346.3639749-5-kuba@kernel.org Signed-off-by: Jakub Kicinski --- net/core/dev.c | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/net/core/dev.c b/net/core/dev.c index bbfb02b4a228c..d2698b4bbad4f 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -1119,18 +1119,11 @@ static int __dev_alloc_name(struct net *net, const char *name, char *res) i = find_first_zero_bit(inuse, max_netdevices); bitmap_free(inuse); + if (i == max_netdevices) + return -ENFILE; - snprintf(buf, IFNAMSIZ, name, i); - if (!netdev_name_in_use(net, buf)) { - strscpy(res, buf, IFNAMSIZ); - return i; - } - - /* It is possible to run out of possible slots - * when the name is long and there isn't enough space left - * for the digits, or if all bits are used. - */ - return -ENFILE; + snprintf(res, IFNAMSIZ, name, i); + return i; } /* Returns negative errno or allocated unit id (see __dev_alloc_name()) */ From 70e1b14c1bcbbb0854311ff8bed6cf4db75d5f05 Mon Sep 17 00:00:00 2001 From: Jakub Kicinski Date: Mon, 23 Oct 2023 08:23:45 -0700 Subject: [PATCH 5/6] net: remove dev_valid_name() check from __dev_alloc_name() __dev_alloc_name() is only called by dev_prep_valid_name(), which already checks that name is valid. Reviewed-by: Jiri Pirko Link: https://lore.kernel.org/r/20231023152346.3639749-6-kuba@kernel.org Signed-off-by: Jakub Kicinski --- net/core/dev.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/net/core/dev.c b/net/core/dev.c index d2698b4bbad4f..0830f29672214 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -1077,9 +1077,6 @@ static int __dev_alloc_name(struct net *net, const char *name, char *res) struct net_device *d; char buf[IFNAMSIZ]; - if (!dev_valid_name(name)) - return -EINVAL; - /* Verify the string as this thing may have come from the user. * There must be one "%d" and no other "%" characters. */ From ce4cfa2318afcd74cc41992e306a28fa04e5d484 Mon Sep 17 00:00:00 2001 From: Jakub Kicinski Date: Mon, 23 Oct 2023 08:23:46 -0700 Subject: [PATCH 6/6] net: remove else after return in dev_prep_valid_name() Remove unnecessary else clauses after return. I copied this if / else construct from somewhere, it makes the code harder to read. Reviewed-by: Jiri Pirko Link: https://lore.kernel.org/r/20231023152346.3639749-7-kuba@kernel.org Signed-off-by: Jakub Kicinski --- net/core/dev.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/net/core/dev.c b/net/core/dev.c index 0830f29672214..a37a932a3e145 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -1131,14 +1131,13 @@ static int dev_prep_valid_name(struct net *net, struct net_device *dev, if (!dev_valid_name(want_name)) return -EINVAL; - if (strchr(want_name, '%')) { + if (strchr(want_name, '%')) return __dev_alloc_name(net, want_name, out_name); - } else if (netdev_name_in_use(net, want_name)) { + + if (netdev_name_in_use(net, want_name)) return -dup_errno; - } else if (out_name != want_name) { + if (out_name != want_name) strscpy(out_name, want_name, IFNAMSIZ); - } - return 0; }