[PATCH net v2 0/5] net: fix bugs in device netns-move and rename

[PATCH net v2 0/5] net: fix bugs in device netns-move and rename

[Jakub Kicinski]

Daniel reported issues with the uevents generated during netdev
namespace move, if the netdev is getting renamed at the same time.

While the issue that he actually cares about is not fixed here,
there is a bunch of seemingly obvious other bugs in this code.
Fix the purely networking bugs while the discussion around
the uevent fix is still ongoing.

v2:
 - fix the bug in patch 1
 - improvements in patch 5
v1: https://lore.kernel.org/all/20231016201657.1754763-1-kuba@kernel.org/

Link: https://lore.kernel.org/all/20231010121003.x3yi6fihecewjy4e@House.clients.dxld.at/

Jakub Kicinski (5):
  net: fix ifname in netlink ntf during netns move
  net: check for altname conflicts when changing netdev's netns
  net: avoid UAF on deleted altname
  net: move altnames together with the netdevice
  selftests: net: add very basic test for netdev names and namespaces

 net/core/dev.c                            | 65 +++++++++++++----
 net/core/dev.h                            |  3 +
 tools/testing/selftests/net/Makefile      |  1 +
 tools/testing/selftests/net/netns-name.sh | 87 +++++++++++++++++++++++
 4 files changed, 141 insertions(+), 15 deletions(-)
 create mode 100755 tools/testing/selftests/net/netns-name.sh

-- 
2.41.0

[PATCH net v2 1/5] net: fix ifname in netlink ntf during netns move

[Jakub Kicinski]

dev_get_valid_name() overwrites the netdev's name on success.
This makes it hard to use in prepare-commit-like fashion,
where we do validation first, and "commit" to the change
later.

Factor out a helper which lets us save the new name to a buffer.
Use it to fix the problem of notification on netns move having
incorrect name:

 5: eth0: <BROADCAST,NOARP> mtu 1500 qdisc noop state DOWN group default
     link/ether be:4d:58:f9:d5:40 brd ff:ff:ff:ff:ff:ff
 6: eth1: <BROADCAST,NOARP> mtu 1500 qdisc noop state DOWN group default
     link/ether 1e:4a:34:36:e3:cd brd ff:ff:ff:ff:ff:ff

 [ ~]# ip link set dev eth0 netns 1 name eth1

ip monitor inside netns:
 Deleted inet eth0
 Deleted inet6 eth0
 Deleted 5: eth1: <BROADCAST,NOARP> mtu 1500 qdisc noop state DOWN group default
     link/ether be:4d:58:f9:d5:40 brd ff:ff:ff:ff:ff:ff new-netnsid 0 new-ifindex 7

Name is reported as eth1 in old netns for ifindex 5, already renamed.

Fixes: d90310243fd7 ("net: device name allocation cleanups")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
v2:
 - use a temp buffer in dev_get_valid_name() to avoid
   clobering dev->name on error
 - move dev_prep_valid_name() up a bit, this will help later
   cleanups in net-next

CC: daniel@iogearbox.net
CC: opurdila@ixiacom.com
---
 net/core/dev.c | 44 +++++++++++++++++++++++++++++++-------------
 1 file changed, 31 insertions(+), 13 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 5aaf5753d4e4..f109ad34d660 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1123,6 +1123,26 @@ static int __dev_alloc_name(struct net *net, const char *name, char *buf)
 	return -ENFILE;
 }
 
+static int dev_prep_valid_name(struct net *net, struct net_device *dev,
+			       const char *want_name, char *out_name)
+{
+	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;
+	} else if (netdev_name_in_use(net, want_name)) {
+		return -EEXIST;
+	} else if (out_name != want_name) {
+		strscpy(out_name, want_name, IFNAMSIZ);
+	}
+
+	return 0;
+}
+
 static int dev_alloc_name_ns(struct net *net,
 			     struct net_device *dev,
 			     const char *name)
@@ -1160,19 +1180,13 @@ EXPORT_SYMBOL(dev_alloc_name);
 static int dev_get_valid_name(struct net *net, struct net_device *dev,
 			      const char *name)
 {
-	BUG_ON(!net);
+	char buf[IFNAMSIZ];
+	int ret;
 
-	if (!dev_valid_name(name))
-		return -EINVAL;
-
-	if (strchr(name, '%'))
-		return dev_alloc_name_ns(net, dev, name);
-	else if (netdev_name_in_use(net, name))
-		return -EEXIST;
-	else if (dev->name != name)
-		strscpy(dev->name, name, IFNAMSIZ);
-
-	return 0;
+	ret = dev_prep_valid_name(net, dev, name, buf);
+	if (ret >= 0)
+		strscpy(dev->name, buf, IFNAMSIZ);
+	return ret;
 }
 
 /**
@@ -11038,6 +11052,7 @@ int __dev_change_net_namespace(struct net_device *dev, struct net *net,
 			       const char *pat, int new_ifindex)
 {
 	struct net *net_old = dev_net(dev);
+	char new_name[IFNAMSIZ] = {};
 	int err, new_nsid;
 
 	ASSERT_RTNL();
@@ -11064,7 +11079,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_get_valid_name(net, dev, pat);
+		err = dev_prep_valid_name(net, dev, pat, new_name);
 		if (err < 0)
 			goto out;
 	}
@@ -11135,6 +11150,9 @@ int __dev_change_net_namespace(struct net_device *dev, struct net *net,
 	kobject_uevent(&dev->dev.kobj, KOBJ_ADD);
 	netdev_adjacent_add_links(dev);
 
+	if (new_name[0]) /* Rename the netdev to prepared name */
+		strscpy(dev->name, new_name, IFNAMSIZ);
+
 	/* Fixup kobjects */
 	err = device_rename(&dev->dev, dev->name);
 	WARN_ON(err);
-- 
2.41.0

[PATCH net v2 2/5] net: check for altname conflicts when changing netdev's netns

[Jakub Kicinski]

It's currently possible to create an altname conflicting
with an altname or real name of another device by creating
it in another netns and moving it over:

 [ ~]$ ip link add dev eth0 type dummy

 [ ~]$ ip netns add test
 [ ~]$ ip -netns test link add dev ethX netns test type dummy
 [ ~]$ ip -netns test link property add dev ethX altname eth0
 [ ~]$ ip -netns test link set dev ethX netns 1

 [ ~]$ ip link
 ...
 3: eth0: <BROADCAST,NOARP> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
     link/ether 02:40:88:62:ec:b8 brd ff:ff:ff:ff:ff:ff
 ...
 5: ethX: <BROADCAST,NOARP> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
     link/ether 26:b7:28:78:38:0f brd ff:ff:ff:ff:ff:ff
     altname eth0

Create a macro for walking the altnames, this hopefully makes
it clearer that the list we walk contains only altnames.
Which is otherwise not entirely intuitive.

Fixes: 36fbf1e52bd3 ("net: rtnetlink: add linkprop commands to add and delete alternative ifnames")
Reviewed-by: Jiri Pirko <jiri@nvidia.com>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: gnault@redhat.com
CC: liuhangbin@gmail.com
CC: lucien.xin@gmail.com
---
 net/core/dev.c | 9 ++++++++-
 net/core/dev.h | 3 +++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index f109ad34d660..ae557193b77c 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1086,7 +1086,8 @@ static int __dev_alloc_name(struct net *net, const char *name, char *buf)
 
 		for_each_netdev(net, d) {
 			struct netdev_name_node *name_node;
-			list_for_each_entry(name_node, &d->name_node->list, list) {
+
+			netdev_for_each_altname(d, name_node) {
 				if (!sscanf(name_node->name, name, &i))
 					continue;
 				if (i < 0 || i >= max_netdevices)
@@ -11051,6 +11052,7 @@ EXPORT_SYMBOL(unregister_netdev);
 int __dev_change_net_namespace(struct net_device *dev, struct net *net,
 			       const char *pat, int new_ifindex)
 {
+	struct netdev_name_node *name_node;
 	struct net *net_old = dev_net(dev);
 	char new_name[IFNAMSIZ] = {};
 	int err, new_nsid;
@@ -11083,6 +11085,11 @@ int __dev_change_net_namespace(struct net_device *dev, struct net *net,
 		if (err < 0)
 			goto out;
 	}
+	/* Check that none of the altnames conflicts. */
+	err = -EEXIST;
+	netdev_for_each_altname(dev, name_node)
+		if (netdev_name_in_use(net, name_node->name))
+			goto out;
 
 	/* Check that new_ifindex isn't used yet. */
 	if (new_ifindex) {
diff --git a/net/core/dev.h b/net/core/dev.h
index e075e198092c..fa2e9c5c4122 100644
--- a/net/core/dev.h
+++ b/net/core/dev.h
@@ -62,6 +62,9 @@ struct netdev_name_node {
 int netdev_get_name(struct net *net, char *name, int ifindex);
 int dev_change_name(struct net_device *dev, const char *newname);
 
+#define netdev_for_each_altname(dev, namenode)				\
+	list_for_each_entry((namenode), &(dev)->name_node->list, list)
+
 int netdev_name_node_alt_create(struct net_device *dev, const char *name);
 int netdev_name_node_alt_destroy(struct net_device *dev, const char *name);
 
-- 
2.41.0

[PATCH net v2 3/5] net: avoid UAF on deleted altname

[Jakub Kicinski]

Altnames are accessed under RCU (dev_get_by_name_rcu())
but freed by kfree() with no synchronization point.

Each node has one or two allocations (node and a variable-size
name, sometimes the name is netdev->name). Adding rcu_heads
here is a bit tedious. Besides most code which unlists the names
already has rcu barriers - so take the simpler approach of adding
synchronize_rcu(). Note that the one on the unregistration path
(which matters more) is removed by the next fix.

Fixes: ff92741270bf ("net: introduce name_node struct to be used in hashlist")
Reviewed-by: Jiri Pirko <jiri@nvidia.com>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
v2: update the commit msg a bit
---
 net/core/dev.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index ae557193b77c..559705aeefe4 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -345,7 +345,6 @@ int netdev_name_node_alt_create(struct net_device *dev, const char *name)
 static void __netdev_name_node_alt_destroy(struct netdev_name_node *name_node)
 {
 	list_del(&name_node->list);
-	netdev_name_node_del(name_node);
 	kfree(name_node->name);
 	netdev_name_node_free(name_node);
 }
@@ -364,6 +363,8 @@ int netdev_name_node_alt_destroy(struct net_device *dev, const char *name)
 	if (name_node == dev->name_node || name_node->dev != dev)
 		return -EINVAL;
 
+	netdev_name_node_del(name_node);
+	synchronize_rcu();
 	__netdev_name_node_alt_destroy(name_node);
 
 	return 0;
@@ -10941,6 +10942,7 @@ void unregister_netdevice_many_notify(struct list_head *head,
 	synchronize_net();
 
 	list_for_each_entry(dev, head, unreg_list) {
+		struct netdev_name_node *name_node;
 		struct sk_buff *skb = NULL;
 
 		/* Shutdown queueing discipline. */
@@ -10968,6 +10970,9 @@ void unregister_netdevice_many_notify(struct list_head *head,
 		dev_uc_flush(dev);
 		dev_mc_flush(dev);
 
+		netdev_for_each_altname(dev, name_node)
+			netdev_name_node_del(name_node);
+		synchronize_rcu();
 		netdev_name_node_alt_flush(dev);
 		netdev_name_node_free(dev->name_node);
 
-- 
2.41.0

[PATCH net v2 4/5] net: move altnames together with the netdevice

[Jakub Kicinski]

The altname nodes are currently not moved to the new netns
when netdevice itself moves:

  [ ~]# ip netns add test
  [ ~]# ip -netns test link add name eth0 type dummy
  [ ~]# ip -netns test link property add dev eth0 altname some-name
  [ ~]# ip -netns test link show dev some-name
  2: eth0: <BROADCAST,NOARP> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
      link/ether 1e:67:ed:19:3d:24 brd ff:ff:ff:ff:ff:ff
      altname some-name
  [ ~]# ip -netns test link set dev eth0 netns 1
  [ ~]# ip link
  ...
  3: eth0: <BROADCAST,NOARP> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
      link/ether 02:40:88:62:ec:b8 brd ff:ff:ff:ff:ff:ff
      altname some-name
  [ ~]# ip li show dev some-name
  Device "some-name" does not exist.

Remove them from the hash table when device is unlisted
and add back when listed again.

Fixes: 36fbf1e52bd3 ("net: rtnetlink: add linkprop commands to add and delete alternative ifnames")
Reviewed-by: Jiri Pirko <jiri@nvidia.com>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/core/dev.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 559705aeefe4..9f3f8930c691 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -381,6 +381,7 @@ static void netdev_name_node_alt_flush(struct net_device *dev)
 /* Device list insertion */
 static void list_netdevice(struct net_device *dev)
 {
+	struct netdev_name_node *name_node;
 	struct net *net = dev_net(dev);
 
 	ASSERT_RTNL();
@@ -391,6 +392,10 @@ static void list_netdevice(struct net_device *dev)
 	hlist_add_head_rcu(&dev->index_hlist,
 			   dev_index_hash(net, dev->ifindex));
 	write_unlock(&dev_base_lock);
+
+	netdev_for_each_altname(dev, name_node)
+		netdev_name_node_add(net, name_node);
+
 	/* We reserved the ifindex, this can't fail */
 	WARN_ON(xa_store(&net->dev_by_index, dev->ifindex, dev, GFP_KERNEL));
 
@@ -402,12 +407,16 @@ static void list_netdevice(struct net_device *dev)
  */
 static void unlist_netdevice(struct net_device *dev, bool lock)
 {
+	struct netdev_name_node *name_node;
 	struct net *net = dev_net(dev);
 
 	ASSERT_RTNL();
 
 	xa_erase(&net->dev_by_index, dev->ifindex);
 
+	netdev_for_each_altname(dev, name_node)
+		netdev_name_node_del(name_node);
+
 	/* Unlink dev from the device chain */
 	if (lock)
 		write_lock(&dev_base_lock);
@@ -10942,7 +10951,6 @@ void unregister_netdevice_many_notify(struct list_head *head,
 	synchronize_net();
 
 	list_for_each_entry(dev, head, unreg_list) {
-		struct netdev_name_node *name_node;
 		struct sk_buff *skb = NULL;
 
 		/* Shutdown queueing discipline. */
@@ -10970,9 +10978,6 @@ void unregister_netdevice_many_notify(struct list_head *head,
 		dev_uc_flush(dev);
 		dev_mc_flush(dev);
 
-		netdev_for_each_altname(dev, name_node)
-			netdev_name_node_del(name_node);
-		synchronize_rcu();
 		netdev_name_node_alt_flush(dev);
 		netdev_name_node_free(dev->name_node);
 
-- 
2.41.0

[PATCH net v2 5/5] selftests: net: add very basic test for netdev names and namespaces

[Jakub Kicinski]

Add selftest for fixes around naming netdevs and namespaces.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
v2:
 - drop the \ from line ends
 - use Przemek's magic for the error message
 - redirect errors to stderr
---
 tools/testing/selftests/net/Makefile      |  1 +
 tools/testing/selftests/net/netns-name.sh | 87 +++++++++++++++++++++++
 2 files changed, 88 insertions(+)
 create mode 100755 tools/testing/selftests/net/netns-name.sh

diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile
index 8b017070960d..4a2881d43989 100644
--- a/tools/testing/selftests/net/Makefile
+++ b/tools/testing/selftests/net/Makefile
@@ -34,6 +34,7 @@ TEST_PROGS += gro.sh
 TEST_PROGS += gre_gso.sh
 TEST_PROGS += cmsg_so_mark.sh
 TEST_PROGS += cmsg_time.sh cmsg_ipv6.sh
+TEST_PROGS += netns-name.sh
 TEST_PROGS += srv6_end_dt46_l3vpn_test.sh
 TEST_PROGS += srv6_end_dt4_l3vpn_test.sh
 TEST_PROGS += srv6_end_dt6_l3vpn_test.sh
diff --git a/tools/testing/selftests/net/netns-name.sh b/tools/testing/selftests/net/netns-name.sh
new file mode 100755
index 000000000000..7d3d3fc99461
--- /dev/null
+++ b/tools/testing/selftests/net/netns-name.sh
@@ -0,0 +1,87 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+
+set -o pipefail
+
+NS=netns-name-test
+DEV=dummy-dev0
+DEV2=dummy-dev1
+ALT_NAME=some-alt-name
+
+RET_CODE=0
+
+cleanup() {
+    ip netns del $NS
+}
+
+trap cleanup EXIT
+
+fail() {
+    echo "ERROR: ${1:-unexpected return code} (ret: $_)" >&2
+    RET_CODE=1
+}
+
+ip netns add $NS
+
+#
+# Test basic move without a rename
+#
+ip -netns $NS link add name $DEV type dummy || fail
+ip -netns $NS link set dev $DEV netns 1 ||
+    fail "Can't perform a netns move"
+ip link show dev $DEV >> /dev/null || fail "Device not found after move"
+ip link del $DEV || fail
+
+#
+# Test move with a conflict
+#
+ip link add name $DEV type dummy
+ip -netns $NS link add name $DEV type dummy || fail
+ip -netns $NS link set dev $DEV netns 1 2> /dev/null &&
+    fail "Performed a netns move with a name conflict"
+ip link show dev $DEV >> /dev/null || fail "Device not found after move"
+ip -netns $NS link del $DEV || fail
+ip link del $DEV || fail
+
+#
+# Test move with a conflict and rename
+#
+ip link add name $DEV type dummy
+ip -netns $NS link add name $DEV type dummy || fail
+ip -netns $NS link set dev $DEV netns 1 name $DEV2 ||
+    fail "Can't perform a netns move with rename"
+ip link del $DEV2 || fail
+ip link del $DEV || fail
+
+#
+# Test dup alt-name with netns move
+#
+ip link add name $DEV type dummy || fail
+ip link property add dev $DEV altname $ALT_NAME || fail
+ip -netns $NS link add name $DEV2 type dummy || fail
+ip -netns $NS link property add dev $DEV2 altname $ALT_NAME || fail
+
+ip -netns $NS link set dev $DEV2 netns 1 2> /dev/null &&
+    fail "Moved with alt-name dup"
+
+ip link del $DEV || fail
+ip -netns $NS link del $DEV2 || fail
+
+#
+# Test creating alt-name in one net-ns and using in another
+#
+ip -netns $NS link add name $DEV type dummy || fail
+ip -netns $NS link property add dev $DEV altname $ALT_NAME || fail
+ip -netns $NS link set dev $DEV netns 1 || fail
+ip link show dev $ALT_NAME >> /dev/null || fail "Can't find alt-name after move"
+ip  -netns $NS link show dev $ALT_NAME 2> /dev/null &&
+    fail "Can still find alt-name after move"
+ip link del $DEV || fail
+
+echo -ne "$(basename $0) \t\t\t\t"
+if [ $RET_CODE -eq 0 ]; then
+    echo "[  OK  ]"
+else
+    echo "[ FAIL ]"
+fi
+exit $RET_CODE
-- 
2.41.0

Re: [PATCH net v2 1/5] net: fix ifname in netlink ntf during netns move

[Jiri Pirko]

Wed, Oct 18, 2023 at 03:38:13AM CEST, kuba@kernel.org wrote:
>dev_get_valid_name() overwrites the netdev's name on success.
>This makes it hard to use in prepare-commit-like fashion,
>where we do validation first, and "commit" to the change
>later.
>
>Factor out a helper which lets us save the new name to a buffer.
>Use it to fix the problem of notification on netns move having
>incorrect name:
>
> 5: eth0: <BROADCAST,NOARP> mtu 1500 qdisc noop state DOWN group default
>     link/ether be:4d:58:f9:d5:40 brd ff:ff:ff:ff:ff:ff
> 6: eth1: <BROADCAST,NOARP> mtu 1500 qdisc noop state DOWN group default
>     link/ether 1e:4a:34:36:e3:cd brd ff:ff:ff:ff:ff:ff
>
> [ ~]# ip link set dev eth0 netns 1 name eth1
>
>ip monitor inside netns:
> Deleted inet eth0
> Deleted inet6 eth0
> Deleted 5: eth1: <BROADCAST,NOARP> mtu 1500 qdisc noop state DOWN group default
>     link/ether be:4d:58:f9:d5:40 brd ff:ff:ff:ff:ff:ff new-netnsid 0 new-ifindex 7
>
>Name is reported as eth1 in old netns for ifindex 5, already renamed.
>
>Fixes: d90310243fd7 ("net: device name allocation cleanups")
>Signed-off-by: Jakub Kicinski <kuba@kernel.org>
>---
>v2:
> - use a temp buffer in dev_get_valid_name() to avoid
>   clobering dev->name on error
> - move dev_prep_valid_name() up a bit, this will help later
>   cleanups in net-next
>
>CC: daniel@iogearbox.net
>CC: opurdila@ixiacom.com
>---
> net/core/dev.c | 44 +++++++++++++++++++++++++++++++-------------
> 1 file changed, 31 insertions(+), 13 deletions(-)
>
>diff --git a/net/core/dev.c b/net/core/dev.c
>index 5aaf5753d4e4..f109ad34d660 100644
>--- a/net/core/dev.c
>+++ b/net/core/dev.c
>@@ -1123,6 +1123,26 @@ static int __dev_alloc_name(struct net *net, const char *name, char *buf)
> 	return -ENFILE;
> }
> 
>+static int dev_prep_valid_name(struct net *net, struct net_device *dev,
>+			       const char *want_name, char *out_name)
>+{
>+	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;
>+	} else if (netdev_name_in_use(net, want_name)) {
>+		return -EEXIST;
>+	} else if (out_name != want_name) {

How this can happen?
You call dev_prep_valid_name() twice:
	ret = dev_prep_valid_name(net, dev, name, buf);
	err = dev_prep_valid_name(net, dev, pat, new_name);

Both buf and new_name are on stack tmp variables.


>+		strscpy(out_name, want_name, IFNAMSIZ);

You don't need the strscpy here, callers do that.


>+	}
>+
>+	return 0;
>+}
>+
> static int dev_alloc_name_ns(struct net *net,
> 			     struct net_device *dev,
> 			     const char *name)
>@@ -1160,19 +1180,13 @@ EXPORT_SYMBOL(dev_alloc_name);
> static int dev_get_valid_name(struct net *net, struct net_device *dev,
> 			      const char *name)
> {
>-	BUG_ON(!net);
>+	char buf[IFNAMSIZ];
>+	int ret;
> 
>-	if (!dev_valid_name(name))
>-		return -EINVAL;
>-
>-	if (strchr(name, '%'))
>-		return dev_alloc_name_ns(net, dev, name);
>-	else if (netdev_name_in_use(net, name))
>-		return -EEXIST;
>-	else if (dev->name != name)
>-		strscpy(dev->name, name, IFNAMSIZ);
>-
>-	return 0;
>+	ret = dev_prep_valid_name(net, dev, name, buf);
>+	if (ret >= 0)

How ret could be bigger than 0?


>+		strscpy(dev->name, buf, IFNAMSIZ);
>+	return ret;
> }
> 
> /**
>@@ -11038,6 +11052,7 @@ int __dev_change_net_namespace(struct net_device *dev, struct net *net,
> 			       const char *pat, int new_ifindex)
> {
> 	struct net *net_old = dev_net(dev);
>+	char new_name[IFNAMSIZ] = {};
> 	int err, new_nsid;
> 
> 	ASSERT_RTNL();
>@@ -11064,7 +11079,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_get_valid_name(net, dev, pat);
>+		err = dev_prep_valid_name(net, dev, pat, new_name);
> 		if (err < 0)
> 			goto out;
> 	}
>@@ -11135,6 +11150,9 @@ int __dev_change_net_namespace(struct net_device *dev, struct net *net,
> 	kobject_uevent(&dev->dev.kobj, KOBJ_ADD);
> 	netdev_adjacent_add_links(dev);
> 
>+	if (new_name[0]) /* Rename the netdev to prepared name */

strlen() would probably read a bit nicer?


>+		strscpy(dev->name, new_name, IFNAMSIZ);
>+
> 	/* Fixup kobjects */
> 	err = device_rename(&dev->dev, dev->name);
> 	WARN_ON(err);
>-- 
>2.41.0
>

Re: [PATCH net v2 1/5] net: fix ifname in netlink ntf during netns move

[Jakub Kicinski]

On Wed, 18 Oct 2023 09:12:58 +0200 Jiri Pirko wrote:
> >+static int dev_prep_valid_name(struct net *net, struct net_device *dev,
> >+			       const char *want_name, char *out_name)
> >+{
> >+	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;
> >+	} else if (netdev_name_in_use(net, want_name)) {
> >+		return -EEXIST;
> >+	} else if (out_name != want_name) {  
> 
> How this can happen?
> You call dev_prep_valid_name() twice:
> 	ret = dev_prep_valid_name(net, dev, name, buf);
> 	err = dev_prep_valid_name(net, dev, pat, new_name);
> 
> Both buf and new_name are on stack tmp variables.

I'm moving this code 1-to-1. I have patches queued up to clean all 
this up in net-next. Please let me know if you see any bugs.

Re: [PATCH net v2 5/5] selftests: net: add very basic test for netdev names and namespaces

[Przemek Kitszel]

On 10/18/23 03:38, Jakub Kicinski wrote:
> Add selftest for fixes around naming netdevs and namespaces.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> v2:
>   - drop the \ from line ends
>   - use Przemek's magic for the error message
>   - redirect errors to stderr
> ---
>   tools/testing/selftests/net/Makefile      |  1 +
>   tools/testing/selftests/net/netns-name.sh | 87 +++++++++++++++++++++++
>   2 files changed, 88 insertions(+)
>   create mode 100755 tools/testing/selftests/net/netns-name.sh

Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>

Re: [PATCH net v2 1/5] net: fix ifname in netlink ntf during netns move

[Jiri Pirko]

Wed, Oct 18, 2023 at 05:13:41PM CEST, kuba@kernel.org wrote:
>On Wed, 18 Oct 2023 09:12:58 +0200 Jiri Pirko wrote:
>> >+static int dev_prep_valid_name(struct net *net, struct net_device *dev,
>> >+			       const char *want_name, char *out_name)
>> >+{
>> >+	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;
>> >+	} else if (netdev_name_in_use(net, want_name)) {
>> >+		return -EEXIST;
>> >+	} else if (out_name != want_name) {  
>> 
>> How this can happen?
>> You call dev_prep_valid_name() twice:
>> 	ret = dev_prep_valid_name(net, dev, name, buf);
>> 	err = dev_prep_valid_name(net, dev, pat, new_name);
>> 
>> Both buf and new_name are on stack tmp variables.
>
>I'm moving this code 1-to-1. I have patches queued up to clean all 
>this up in net-next. Please let me know if you see any bugs.

Okay, fair enough. It just looks odd, but I get it for "net".

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

Re: [PATCH net v2 0/5] net: fix bugs in device netns-move and rename

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Tue, 17 Oct 2023 18:38:12 -0700 you wrote:
> Daniel reported issues with the uevents generated during netdev
> namespace move, if the netdev is getting renamed at the same time.
> 
> While the issue that he actually cares about is not fixed here,
> there is a bunch of seemingly obvious other bugs in this code.
> Fix the purely networking bugs while the discussion around
> the uevent fix is still ongoing.
> 
> [...]

Here is the summary with links:
  - [net,v2,1/5] net: fix ifname in netlink ntf during netns move
    https://git.kernel.org/netdev/net/c/311cca40661f
  - [net,v2,2/5] net: check for altname conflicts when changing netdev's netns
    https://git.kernel.org/netdev/net/c/7663d522099e
  - [net,v2,3/5] net: avoid UAF on deleted altname
    https://git.kernel.org/netdev/net/c/1a83f4a7c156
  - [net,v2,4/5] net: move altnames together with the netdevice
    https://git.kernel.org/netdev/net/c/8e15aee62161
  - [net,v2,5/5] selftests: net: add very basic test for netdev names and namespaces
    https://git.kernel.org/netdev/net/c/3920431d98a9

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