[PATCH net-next v2 0/6] net: deduplicate netdev name allocation

[PATCH net-next v2 0/6] net: deduplicate netdev name allocation

[Jakub Kicinski]

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.

v2: extend commit message on patch 4
v1: https://lore.kernel.org/all/20231020011856.3244410-1-kuba@kernel.org/

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

[PATCH net-next v2 1/6] net: don't use input buffer of __dev_alloc_name() as a scratch space

[Jakub Kicinski]

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 <jiri@nvidia.com>
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

[PATCH net-next v2 2/6] net: make dev_alloc_name() call dev_prep_valid_name()

[Jakub Kicinski]

__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 <jiri@nvidia.com>
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

[PATCH net-next v2 3/6] net: reduce indentation of __dev_alloc_name()

[Jakub Kicinski]

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 <jiri@nvidia.com>
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

[PATCH net-next v2 4/6] net: trust the bitmap in __dev_alloc_name()

[Jakub Kicinski]

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.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
v2:
 - mention the loss of 1 entry in the commit message
---
 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

Re: [PATCH net-next v2 4/6] net: trust the bitmap in __dev_alloc_name()

[Jiri Pirko]

Mon, Oct 23, 2023 at 05:23:44PM 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.
>
>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.
>
>Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Jiri Pirko <jiri@nvidia.com>

[PATCH net-next v2 5/6] net: remove dev_valid_name() check from __dev_alloc_name()

[Jakub Kicinski]

__dev_alloc_name() is only called by dev_prep_valid_name(),
which already checks that name is valid.

Reviewed-by: Jiri Pirko <jiri@nvidia.com>
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

[PATCH net-next v2 6/6] net: remove else after return in dev_prep_valid_name()

[Jakub Kicinski]

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 <jiri@nvidia.com>
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

Re: [PATCH net-next v2 0/6] net: deduplicate netdev name allocation

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Mon, 23 Oct 2023 08:23:40 -0700 you wrote:
> 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.
> 
> [...]

Here is the summary with links:
  - [net-next,v2,1/6] net: don't use input buffer of __dev_alloc_name() as a scratch space
    https://git.kernel.org/netdev/net-next/c/bd07063dd11f
  - [net-next,v2,2/6] net: make dev_alloc_name() call dev_prep_valid_name()
    https://git.kernel.org/netdev/net-next/c/556c755a4d81
  - [net-next,v2,3/6] net: reduce indentation of __dev_alloc_name()
    https://git.kernel.org/netdev/net-next/c/9a810468126c
  - [net-next,v2,4/6] net: trust the bitmap in __dev_alloc_name()
    https://git.kernel.org/netdev/net-next/c/7ad17b04dc7b
  - [net-next,v2,5/6] net: remove dev_valid_name() check from __dev_alloc_name()
    https://git.kernel.org/netdev/net-next/c/70e1b14c1bcb
  - [net-next,v2,6/6] net: remove else after return in dev_prep_valid_name()
    https://git.kernel.org/netdev/net-next/c/ce4cfa2318af

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html