* [PATCH net-next 0/6] net: deduplicate netdev name allocation
@ 2023-10-20 1:18 Jakub Kicinski
2023-10-20 1:18 ` [PATCH net-next 1/6] net: don't use input buffer of __dev_alloc_name() as a scratch space Jakub Kicinski
` (5 more replies)
0 siblings, 6 replies; 17+ messages in thread
From: Jakub Kicinski @ 2023-10-20 1:18 UTC (permalink / raw)
To: davem; +Cc: netdev, edumazet, pabeni, johannes.berg, mpe, j, jiri
After recent fixes we have even more duplicated code in netdev name
allocation helpers. There are two complications in this code.
First, __dev_alloc_name() clobbers its output arg even if allocation
fails, forcing callers to do extra copies. Second as our experience in
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"")
taught us, user space is very sensitive to the exact error codes.
Align the callers of __dev_alloc_name(), and remove some of its
complexity.
Jakub Kicinski (6):
net: don't use input buffer of __dev_alloc_name() as a scratch space
net: make dev_alloc_name() call dev_prep_valid_name()
net: reduce indentation of __dev_alloc_name()
net: trust the bitmap in __dev_alloc_name()
net: remove dev_valid_name() check from __dev_alloc_name()
net: remove else after return in dev_prep_valid_name()
net/core/dev.c | 120 +++++++++++++++++++------------------------------
1 file changed, 45 insertions(+), 75 deletions(-)
--
2.41.0
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH net-next 1/6] net: don't use input buffer of __dev_alloc_name() as a scratch space
2023-10-20 1:18 [PATCH net-next 0/6] net: deduplicate netdev name allocation Jakub Kicinski
@ 2023-10-20 1:18 ` Jakub Kicinski
2023-10-20 10:15 ` Jiri Pirko
2023-10-20 1:18 ` [PATCH net-next 2/6] net: make dev_alloc_name() call dev_prep_valid_name() Jakub Kicinski
` (4 subsequent siblings)
5 siblings, 1 reply; 17+ messages in thread
From: Jakub Kicinski @ 2023-10-20 1:18 UTC (permalink / raw)
To: davem; +Cc: netdev, edumazet, pabeni, johannes.berg, mpe, j, jiri
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.
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
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 1025dc79bc49..874c7daa81f5 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);
}
/**
--
2.41.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH net-next 2/6] net: make dev_alloc_name() call dev_prep_valid_name()
2023-10-20 1:18 [PATCH net-next 0/6] net: deduplicate netdev name allocation Jakub Kicinski
2023-10-20 1:18 ` [PATCH net-next 1/6] net: don't use input buffer of __dev_alloc_name() as a scratch space Jakub Kicinski
@ 2023-10-20 1:18 ` Jakub Kicinski
2023-10-20 10:24 ` Jiri Pirko
2023-10-20 1:18 ` [PATCH net-next 3/6] net: reduce indentation of __dev_alloc_name() Jakub Kicinski
` (3 subsequent siblings)
5 siblings, 1 reply; 17+ messages in thread
From: Jakub Kicinski @ 2023-10-20 1:18 UTC (permalink / raw)
To: davem; +Cc: netdev, edumazet, pabeni, johannes.berg, mpe, j, jiri
__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.
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
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 874c7daa81f5..004e9f26b160 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;
}
--
2.41.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH net-next 3/6] net: reduce indentation of __dev_alloc_name()
2023-10-20 1:18 [PATCH net-next 0/6] net: deduplicate netdev name allocation Jakub Kicinski
2023-10-20 1:18 ` [PATCH net-next 1/6] net: don't use input buffer of __dev_alloc_name() as a scratch space Jakub Kicinski
2023-10-20 1:18 ` [PATCH net-next 2/6] net: make dev_alloc_name() call dev_prep_valid_name() Jakub Kicinski
@ 2023-10-20 1:18 ` Jakub Kicinski
2023-10-20 10:26 ` Jiri Pirko
2023-10-20 1:18 ` [PATCH net-next 4/6] net: trust the bitmap in __dev_alloc_name() Jakub Kicinski
` (2 subsequent siblings)
5 siblings, 1 reply; 17+ messages in thread
From: Jakub Kicinski @ 2023-10-20 1:18 UTC (permalink / raw)
To: davem; +Cc: netdev, edumazet, pabeni, johannes.berg, mpe, j, jiri
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.
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
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 004e9f26b160..bbfb02b4a228 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;
+ if (!p || 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;
+ /* Use one page as a bit array of possible slots */
+ inuse = bitmap_zalloc(max_netdevices, GFP_ATOMIC);
+ if (!inuse)
+ return -ENOMEM;
- for_each_netdev(net, d) {
- struct netdev_name_node *name_node;
+ for_each_netdev(net, d) {
+ struct netdev_name_node *name_node;
- 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 */
- 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);
--
2.41.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH net-next 4/6] net: trust the bitmap in __dev_alloc_name()
2023-10-20 1:18 [PATCH net-next 0/6] net: deduplicate netdev name allocation Jakub Kicinski
` (2 preceding siblings ...)
2023-10-20 1:18 ` [PATCH net-next 3/6] net: reduce indentation of __dev_alloc_name() Jakub Kicinski
@ 2023-10-20 1:18 ` Jakub Kicinski
2023-10-20 10:38 ` Jiri Pirko
2023-10-20 1:18 ` [PATCH net-next 5/6] net: remove dev_valid_name() check from __dev_alloc_name() Jakub Kicinski
2023-10-20 1:18 ` [PATCH net-next 6/6] net: remove else after return in dev_prep_valid_name() Jakub Kicinski
5 siblings, 1 reply; 17+ messages in thread
From: Jakub Kicinski @ 2023-10-20 1:18 UTC (permalink / raw)
To: davem; +Cc: netdev, edumazet, pabeni, johannes.berg, mpe, j, jiri
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.
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
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 bbfb02b4a228..d2698b4bbad4 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()) */
--
2.41.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH net-next 5/6] net: remove dev_valid_name() check from __dev_alloc_name()
2023-10-20 1:18 [PATCH net-next 0/6] net: deduplicate netdev name allocation Jakub Kicinski
` (3 preceding siblings ...)
2023-10-20 1:18 ` [PATCH net-next 4/6] net: trust the bitmap in __dev_alloc_name() Jakub Kicinski
@ 2023-10-20 1:18 ` Jakub Kicinski
2023-10-20 10:39 ` Jiri Pirko
2023-10-20 1:18 ` [PATCH net-next 6/6] net: remove else after return in dev_prep_valid_name() Jakub Kicinski
5 siblings, 1 reply; 17+ messages in thread
From: Jakub Kicinski @ 2023-10-20 1:18 UTC (permalink / raw)
To: davem; +Cc: netdev, edumazet, pabeni, johannes.berg, mpe, j, jiri
__dev_alloc_name() is only called by dev_prep_valid_name(),
which already checks that name is valid.
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
net/core/dev.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/net/core/dev.c b/net/core/dev.c
index d2698b4bbad4..0830f2967221 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.
*/
--
2.41.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH net-next 6/6] net: remove else after return in dev_prep_valid_name()
2023-10-20 1:18 [PATCH net-next 0/6] net: deduplicate netdev name allocation Jakub Kicinski
` (4 preceding siblings ...)
2023-10-20 1:18 ` [PATCH net-next 5/6] net: remove dev_valid_name() check from __dev_alloc_name() Jakub Kicinski
@ 2023-10-20 1:18 ` Jakub Kicinski
2023-10-20 10:45 ` Jiri Pirko
5 siblings, 1 reply; 17+ messages in thread
From: Jakub Kicinski @ 2023-10-20 1:18 UTC (permalink / raw)
To: davem; +Cc: netdev, edumazet, pabeni, johannes.berg, mpe, j, jiri
Remove unnecessary else clauses after return.
I copied this if / else construct from somewhere,
it makes the code harder to read.
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
net/core/dev.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/net/core/dev.c b/net/core/dev.c
index 0830f2967221..a37a932a3e14 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)) {
- return -dup_errno;
- } else if (out_name != want_name) {
- strscpy(out_name, want_name, IFNAMSIZ);
- }
+ if (netdev_name_in_use(net, want_name))
+ return -dup_errno;
+ if (out_name != want_name)
+ strscpy(out_name, want_name, IFNAMSIZ);
return 0;
}
--
2.41.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH net-next 1/6] net: don't use input buffer of __dev_alloc_name() as a scratch space
2023-10-20 1:18 ` [PATCH net-next 1/6] net: don't use input buffer of __dev_alloc_name() as a scratch space Jakub Kicinski
@ 2023-10-20 10:15 ` Jiri Pirko
0 siblings, 0 replies; 17+ messages in thread
From: Jiri Pirko @ 2023-10-20 10:15 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: davem, netdev, edumazet, pabeni, johannes.berg, mpe, j
Fri, Oct 20, 2023 at 03:18:51AM CEST, kuba@kernel.org wrote:
>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.
>
>Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Reviewed-by: Jiri Pirko <jiri@nvidia.com>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH net-next 2/6] net: make dev_alloc_name() call dev_prep_valid_name()
2023-10-20 1:18 ` [PATCH net-next 2/6] net: make dev_alloc_name() call dev_prep_valid_name() Jakub Kicinski
@ 2023-10-20 10:24 ` Jiri Pirko
2023-10-20 19:01 ` Jakub Kicinski
0 siblings, 1 reply; 17+ messages in thread
From: Jiri Pirko @ 2023-10-20 10:24 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: davem, netdev, edumazet, pabeni, johannes.berg, mpe, j
Fri, Oct 20, 2023 at 03:18:52AM CEST, kuba@kernel.org wrote:
>__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.
>
>Signed-off-by: Jakub Kicinski <kuba@kernel.org>
>---
> 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 874c7daa81f5..004e9f26b160 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;
Why can't you just return dev_prep_valid_name() ?
No caller seems to care about 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;
> }
>--
>2.41.0
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH net-next 3/6] net: reduce indentation of __dev_alloc_name()
2023-10-20 1:18 ` [PATCH net-next 3/6] net: reduce indentation of __dev_alloc_name() Jakub Kicinski
@ 2023-10-20 10:26 ` Jiri Pirko
0 siblings, 0 replies; 17+ messages in thread
From: Jiri Pirko @ 2023-10-20 10:26 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: davem, netdev, edumazet, pabeni, johannes.berg, mpe, j
Fri, Oct 20, 2023 at 03:18:53AM CEST, kuba@kernel.org wrote:
>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.
>
>Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Reviewed-by: Jiri Pirko <jiri@nvidia.com>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH net-next 4/6] net: trust the bitmap in __dev_alloc_name()
2023-10-20 1:18 ` [PATCH net-next 4/6] net: trust the bitmap in __dev_alloc_name() Jakub Kicinski
@ 2023-10-20 10:38 ` Jiri Pirko
2023-10-20 19:04 ` Jakub Kicinski
0 siblings, 1 reply; 17+ messages in thread
From: Jiri Pirko @ 2023-10-20 10:38 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: davem, netdev, edumazet, pabeni, johannes.berg, mpe, j
Fri, Oct 20, 2023 at 03:18:54AM CEST, kuba@kernel.org wrote:
>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.
>
>Signed-off-by: Jakub Kicinski <kuba@kernel.org>
>---
> 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 bbfb02b4a228..d2698b4bbad4 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;
Hmm, aren't you changeing functionality here? I mean, prior to this
patch, the i of value "max_netdevices" was happily used, wan't it?
In theory it may break things allowing n-1 netdevices of a name instead
of n.
>
>- 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()) */
>--
>2.41.0
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH net-next 5/6] net: remove dev_valid_name() check from __dev_alloc_name()
2023-10-20 1:18 ` [PATCH net-next 5/6] net: remove dev_valid_name() check from __dev_alloc_name() Jakub Kicinski
@ 2023-10-20 10:39 ` Jiri Pirko
0 siblings, 0 replies; 17+ messages in thread
From: Jiri Pirko @ 2023-10-20 10:39 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: davem, netdev, edumazet, pabeni, johannes.berg, mpe, j
Fri, Oct 20, 2023 at 03:18:55AM CEST, kuba@kernel.org wrote:
>__dev_alloc_name() is only called by dev_prep_valid_name(),
>which already checks that name is valid.
>
>Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Reviewed-by: Jiri Pirko <jiri@nvidia.com>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH net-next 6/6] net: remove else after return in dev_prep_valid_name()
2023-10-20 1:18 ` [PATCH net-next 6/6] net: remove else after return in dev_prep_valid_name() Jakub Kicinski
@ 2023-10-20 10:45 ` Jiri Pirko
0 siblings, 0 replies; 17+ messages in thread
From: Jiri Pirko @ 2023-10-20 10:45 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: davem, netdev, edumazet, pabeni, johannes.berg, mpe, j
Fri, Oct 20, 2023 at 03:18:56AM CEST, kuba@kernel.org wrote:
>Remove unnecessary else clauses after return.
>I copied this if / else construct from somewhere,
>it makes the code harder to read.
>
>Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Reviewed-by: Jiri Pirko <jiri@nvidia.com>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH net-next 2/6] net: make dev_alloc_name() call dev_prep_valid_name()
2023-10-20 10:24 ` Jiri Pirko
@ 2023-10-20 19:01 ` Jakub Kicinski
2023-10-21 7:01 ` Jiri Pirko
0 siblings, 1 reply; 17+ messages in thread
From: Jakub Kicinski @ 2023-10-20 19:01 UTC (permalink / raw)
To: Jiri Pirko; +Cc: davem, netdev, edumazet, pabeni, johannes.berg, mpe, j
On Fri, 20 Oct 2023 12:24:57 +0200 Jiri Pirko wrote:
> > 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;
>
> Why can't you just return dev_prep_valid_name() ?
>
> No caller seems to care about ret > 0
AFACT dev_change_name() has some weird code that ends up return
the value all the way to the ioctl and user space. Note that it
has both err and ret variables :S
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH net-next 4/6] net: trust the bitmap in __dev_alloc_name()
2023-10-20 10:38 ` Jiri Pirko
@ 2023-10-20 19:04 ` Jakub Kicinski
2023-10-21 7:00 ` Jiri Pirko
0 siblings, 1 reply; 17+ messages in thread
From: Jakub Kicinski @ 2023-10-20 19:04 UTC (permalink / raw)
To: Jiri Pirko; +Cc: davem, netdev, edumazet, pabeni, johannes.berg, mpe, j
On Fri, 20 Oct 2023 12:38:31 +0200 Jiri Pirko wrote:
> >+ if (i == max_netdevices)
> >+ return -ENFILE;
>
> Hmm, aren't you changeing functionality here? I mean, prior to this
> patch, the i of value "max_netdevices" was happily used, wan't it?
> In theory it may break things allowing n-1 netdevices of a name instead
> of n.
Good point, I should add that to the commit message.
But we don't care, right? Nobody is asking to increase
the limit, feel like chances that someone will care
about 32k vs 32k - 1 devices are extremely low.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH net-next 4/6] net: trust the bitmap in __dev_alloc_name()
2023-10-20 19:04 ` Jakub Kicinski
@ 2023-10-21 7:00 ` Jiri Pirko
0 siblings, 0 replies; 17+ messages in thread
From: Jiri Pirko @ 2023-10-21 7:00 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: davem, netdev, edumazet, pabeni, johannes.berg, mpe, j
Fri, Oct 20, 2023 at 09:04:36PM CEST, kuba@kernel.org wrote:
>On Fri, 20 Oct 2023 12:38:31 +0200 Jiri Pirko wrote:
>> >+ if (i == max_netdevices)
>> >+ return -ENFILE;
>>
>> Hmm, aren't you changeing functionality here? I mean, prior to this
>> patch, the i of value "max_netdevices" was happily used, wan't it?
>> In theory it may break things allowing n-1 netdevices of a name instead
>> of n.
>
>Good point, I should add that to the commit message.
>But we don't care, right? Nobody is asking to increase
>the limit, feel like chances that someone will care
>about 32k vs 32k - 1 devices are extremely low.
Yes, I think that would be fine. Rare conditions.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH net-next 2/6] net: make dev_alloc_name() call dev_prep_valid_name()
2023-10-20 19:01 ` Jakub Kicinski
@ 2023-10-21 7:01 ` Jiri Pirko
0 siblings, 0 replies; 17+ messages in thread
From: Jiri Pirko @ 2023-10-21 7:01 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: davem, netdev, edumazet, pabeni, johannes.berg, mpe, j
Fri, Oct 20, 2023 at 09:01:49PM CEST, kuba@kernel.org wrote:
>On Fri, 20 Oct 2023 12:24:57 +0200 Jiri Pirko wrote:
>> > 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;
>>
>> Why can't you just return dev_prep_valid_name() ?
>>
>> No caller seems to care about ret > 0
>
>AFACT dev_change_name() has some weird code that ends up return
>the value all the way to the ioctl and user space. Note that it
>has both err and ret variables :S
Ah, blah. Guess we are stick to this crap :/
Reviewed-by: Jiri Pirko <jiri@nvidia.com>
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2023-10-21 7:02 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-10-20 1:18 [PATCH net-next 0/6] net: deduplicate netdev name allocation Jakub Kicinski
2023-10-20 1:18 ` [PATCH net-next 1/6] net: don't use input buffer of __dev_alloc_name() as a scratch space Jakub Kicinski
2023-10-20 10:15 ` Jiri Pirko
2023-10-20 1:18 ` [PATCH net-next 2/6] net: make dev_alloc_name() call dev_prep_valid_name() Jakub Kicinski
2023-10-20 10:24 ` Jiri Pirko
2023-10-20 19:01 ` Jakub Kicinski
2023-10-21 7:01 ` Jiri Pirko
2023-10-20 1:18 ` [PATCH net-next 3/6] net: reduce indentation of __dev_alloc_name() Jakub Kicinski
2023-10-20 10:26 ` Jiri Pirko
2023-10-20 1:18 ` [PATCH net-next 4/6] net: trust the bitmap in __dev_alloc_name() Jakub Kicinski
2023-10-20 10:38 ` Jiri Pirko
2023-10-20 19:04 ` Jakub Kicinski
2023-10-21 7:00 ` Jiri Pirko
2023-10-20 1:18 ` [PATCH net-next 5/6] net: remove dev_valid_name() check from __dev_alloc_name() Jakub Kicinski
2023-10-20 10:39 ` Jiri Pirko
2023-10-20 1:18 ` [PATCH net-next 6/6] net: remove else after return in dev_prep_valid_name() Jakub Kicinski
2023-10-20 10:45 ` Jiri Pirko
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.