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

[PATCH net 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.

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                            | 63 ++++++++++++----
 net/core/dev.h                            |  3 +
 tools/testing/selftests/net/Makefile      |  1 +
 tools/testing/selftests/net/netns-name.sh | 91 +++++++++++++++++++++++
 4 files changed, 142 insertions(+), 16 deletions(-)
 create mode 100755 tools/testing/selftests/net/netns-name.sh

-- 
2.41.0

[PATCH net 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>
---
CC: daniel@iogearbox.net
CC: opurdila@ixiacom.com
---
 net/core/dev.c | 42 ++++++++++++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 14 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 5aaf5753d4e4..b08031957ffe 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1157,22 +1157,32 @@ int dev_alloc_name(struct net_device *dev, const char *name)
 }
 EXPORT_SYMBOL(dev_alloc_name);
 
+static int dev_prep_valid_name(struct net *net, struct net_device *dev,
+			       const char *want_name, char *out_name)
+{
+	int ret;
+
+	BUG_ON(!net);
+
+	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_get_valid_name(struct net *net, struct net_device *dev,
 			      const char *name)
 {
-	BUG_ON(!net);
-
-	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;
+	return dev_prep_valid_name(net, dev, name, dev->name);
 }
 
 /**
@@ -11038,6 +11048,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 +11075,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 +11146,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 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")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: gnault@redhat.com
CC: liuhangbin@gmail.com
CC: lucien.xin@gmail.com
CC: jiri@resnulli.us
---
 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 b08031957ffe..f4fa2692cf6d 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)
@@ -11047,6 +11048,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;
@@ -11079,6 +11081,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..d093be175bd0 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, name_node)				\
+	list_for_each_entry((name_node), &(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 3/5] net: avoid UAF on deleted altname

[Jakub Kicinski]

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

Because the name nodes don't hold a reference on the netdevice
either, take the heavier approach of inserting synchronization
points. Subsequent patch will remove the one added on device
deletion path.

Fixes: ff92741270bf ("net: introduce name_node struct to be used in hashlist")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: jiri@resnulli.us
---
 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 f4fa2692cf6d..7d5107cd5792 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;
@@ -10937,6 +10938,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. */
@@ -10964,6 +10966,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 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")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: jiri@resnulli.us
---
 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 7d5107cd5792..25a8321ecdbf 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);
@@ -10938,7 +10947,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. */
@@ -10966,9 +10974,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 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>
---
 tools/testing/selftests/net/Makefile      |  1 +
 tools/testing/selftests/net/netns-name.sh | 91 +++++++++++++++++++++++
 2 files changed, 92 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..59e4a498aab4
--- /dev/null
+++ b/tools/testing/selftests/net/netns-name.sh
@@ -0,0 +1,91 @@
+#!/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() {
+    if [ ! -z "$1" ]; then
+	echo "ERROR: $1"
+    else
+	echo "ERROR: unexpected return code"
+    fi
+    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 1/5] net: fix ifname in netlink ntf during netns move

[Jakub Kicinski]

On Mon, 16 Oct 2023 13:16:53 -0700 Jakub Kicinski wrote:
> +static int dev_prep_valid_name(struct net *net, struct net_device *dev,
> +			       const char *want_name, char *out_name)

> +	if (strchr(want_name, '%')) {
> +		ret = __dev_alloc_name(net, want_name, out_name);
> +		return ret < 0 ? ret : 0;

> -	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;
> +	return dev_prep_valid_name(net, dev, name, dev->name);

Humpf, this is not right. IDK what magic seeing something on the ML
has but I looked at this 3 times, and the moment I see it on the list
I immediately realize that the dev_alloc_name_ns() -> __dev_alloc_name()
conversion here is not really exact. We need to go thru a temp buffer
like dev_alloc_name_ns() does, because for whatever reason
__dev_alloc_name_ns() uses its input argument as a scratch buffer.
So if we pass dev->name directly and it fails the name will be
scrambled.
-- 
pw-bot: cr

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

[Jiri Pirko]

Mon, Oct 16, 2023 at 10:16:54PM CEST, kuba@kernel.org wrote:
>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")
>Signed-off-by: Jakub Kicinski <kuba@kernel.org>
>---
>CC: gnault@redhat.com
>CC: liuhangbin@gmail.com
>CC: lucien.xin@gmail.com
>CC: jiri@resnulli.us
>---
> 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 b08031957ffe..f4fa2692cf6d 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) {

Well, cleaner would be to do this in a separate patch and the fix itself
too.

One way or another, code looks fine.

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

Thanks!


> 				if (!sscanf(name_node->name, name, &i))
> 					continue;
> 				if (i < 0 || i >= max_netdevices)
>@@ -11047,6 +11048,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;
>@@ -11079,6 +11081,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..d093be175bd0 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, name_node)				\
>+	list_for_each_entry((name_node), &(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
>

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

[Jiri Pirko]

Mon, Oct 16, 2023 at 10:16:55PM CEST, kuba@kernel.org wrote:
>Altnames are accessed under RCU (__dev_get_by_name())

dev_get_by_name_rcu()


>but freed by kfree() with no synchronization point.
>
>Because the name nodes don't hold a reference on the netdevice
>either, take the heavier approach of inserting synchronization

What about to use kfree_rcu() in netdev_name_node_free()
and treat node_name->dev as a rcu pointer instead?

struct net_device *dev_get_by_name_rcu(struct net *net, const char *name)
{
        struct netdev_name_node *node_name;

        node_name = netdev_name_node_lookup_rcu(net, name);
        return node_name ? rcu_deferecence(node_name->dev) : NULL;
}

This would avoid synchronize_rcu() in netdev_name_node_alt_destroy()

Btw, the next patch is smooth with this.


>points. Subsequent patch will remove the one added on device
>deletion path.
>
>Fixes: ff92741270bf ("net: introduce name_node struct to be used in hashlist")
>Signed-off-by: Jakub Kicinski <kuba@kernel.org>
>---
>CC: jiri@resnulli.us
>---
> 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 f4fa2692cf6d..7d5107cd5792 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;
>@@ -10937,6 +10938,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. */
>@@ -10964,6 +10966,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
>

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

[Jiri Pirko]

Mon, Oct 16, 2023 at 10:16:56PM CEST, kuba@kernel.org wrote:
>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")
>Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Nice, thanks!

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

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

[Przemek Kitszel]

On 10/16/23 22:16, Jakub Kicinski wrote:
> Add selftest for fixes around naming netdevs and namespaces.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>   tools/testing/selftests/net/Makefile      |  1 +
>   tools/testing/selftests/net/netns-name.sh | 91 +++++++++++++++++++++++
>   2 files changed, 92 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..59e4a498aab4
> --- /dev/null
> +++ b/tools/testing/selftests/net/netns-name.sh
> @@ -0,0 +1,91 @@
> +#!/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() {
> +    if [ ! -z "$1" ]; then

s/! -z/-n/

> +	echo "ERROR: $1"
> +    else
> +	echo "ERROR: unexpected return code"

I see that in some cases unexpected rc is 0, but it's worth printing.

At the expense of requiring reader to know about default values syntax,
whole if could become:
   echo "ERROR: ${1-unexpected return code} (rc=$_)"

I didn't do my homework (of checking expectations of selftests
"framework"), but perhaps errors/diag messages should go to stderr? >&2

> +    fi
> +    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 || \

\ after ||, |, && is redundant, not sure if it improves readability or not

> +    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

I like this patch (and the rest of the series), especially for the fact
how easy it is to test (compared to internals of HW drivers ;) )

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

[Jakub Kicinski]

On Tue, 17 Oct 2023 09:51:02 +0200 Jiri Pirko wrote:
> >but freed by kfree() with no synchronization point.
> >
> >Because the name nodes don't hold a reference on the netdevice
> >either, take the heavier approach of inserting synchronization  
> 
> What about to use kfree_rcu() in netdev_name_node_free()
> and treat node_name->dev as a rcu pointer instead?
> 
> struct net_device *dev_get_by_name_rcu(struct net *net, const char *name)
> {
>         struct netdev_name_node *node_name;
> 
>         node_name = netdev_name_node_lookup_rcu(net, name);
>         return node_name ? rcu_deferecence(node_name->dev) : NULL;
> }
> 
> This would avoid synchronize_rcu() in netdev_name_node_alt_destroy()
> 
> Btw, the next patch is smooth with this.

As I said in the commit message, I prefer the explicit sync.
Re-inserting the device and taking refs already necessitate it.

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

[Jakub Kicinski]

On Tue, 17 Oct 2023 13:25:10 +0200 Przemek Kitszel wrote:
> On 10/16/23 22:16, Jakub Kicinski wrote:
> > +fail() {
> > +    if [ ! -z "$1" ]; then  
> 
> s/! -z/-n/

I find ! -z easier to read.
! -z == "not zero", -n == "not??"

> > +	echo "ERROR: $1"
> > +    else
> > +	echo "ERROR: unexpected return code"  
> 
> I see that in some cases unexpected rc is 0, but it's worth printing.
> 
> At the expense of requiring reader to know about default values syntax,
> whole if could become:
>    echo "ERROR: ${1-unexpected return code} (rc=$_)"

SG!

> I didn't do my homework (of checking expectations of selftests
> "framework"), but perhaps errors/diag messages should go to stderr? >&2

Hm, I've never done that but won't hurt!

> > +    fi
> > +    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 || \  
> 
> \ after ||, |, && is redundant, not sure if it improves readability or not

Roger.

> I like this patch (and the rest of the series), especially for the fact
> how easy it is to test (compared to internals of HW drivers ;) )

Or notification fixes without something more flexible like YNL :(

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

[Jiri Pirko]

Tue, Oct 17, 2023 at 04:52:59PM CEST, kuba@kernel.org wrote:
>On Tue, 17 Oct 2023 09:51:02 +0200 Jiri Pirko wrote:
>> >but freed by kfree() with no synchronization point.
>> >
>> >Because the name nodes don't hold a reference on the netdevice
>> >either, take the heavier approach of inserting synchronization  
>> 
>> What about to use kfree_rcu() in netdev_name_node_free()
>> and treat node_name->dev as a rcu pointer instead?
>> 
>> struct net_device *dev_get_by_name_rcu(struct net *net, const char *name)
>> {
>>         struct netdev_name_node *node_name;
>> 
>>         node_name = netdev_name_node_lookup_rcu(net, name);
>>         return node_name ? rcu_deferecence(node_name->dev) : NULL;
>> }
>> 
>> This would avoid synchronize_rcu() in netdev_name_node_alt_destroy()
>> 
>> Btw, the next patch is smooth with this.
>
>As I said in the commit message, I prefer the explicit sync.
>Re-inserting the device and taking refs already necessitate it.

You don't need any ref, just rcu_dereference() the netdev pointer.

Synchronize_rcu() should be avoided if possible.

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

[Jiri Pirko]

Tue, Oct 17, 2023 at 06:10:44PM CEST, jiri@resnulli.us wrote:
>Tue, Oct 17, 2023 at 04:52:59PM CEST, kuba@kernel.org wrote:
>>On Tue, 17 Oct 2023 09:51:02 +0200 Jiri Pirko wrote:
>>> >but freed by kfree() with no synchronization point.
>>> >
>>> >Because the name nodes don't hold a reference on the netdevice
>>> >either, take the heavier approach of inserting synchronization  
>>> 
>>> What about to use kfree_rcu() in netdev_name_node_free()
>>> and treat node_name->dev as a rcu pointer instead?
>>> 
>>> struct net_device *dev_get_by_name_rcu(struct net *net, const char *name)
>>> {
>>>         struct netdev_name_node *node_name;
>>> 
>>>         node_name = netdev_name_node_lookup_rcu(net, name);
>>>         return node_name ? rcu_deferecence(node_name->dev) : NULL;
>>> }
>>> 
>>> This would avoid synchronize_rcu() in netdev_name_node_alt_destroy()
>>> 
>>> Btw, the next patch is smooth with this.
>>
>>As I said in the commit message, I prefer the explicit sync.
>>Re-inserting the device and taking refs already necessitate it.
>
>You don't need any ref, just rcu_dereference() the netdev pointer.

Oh wait, you are right. Sorry for the fuzz.

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

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

[Jakub Kicinski]

On Tue, 17 Oct 2023 18:35:31 +0200 Jiri Pirko wrote:
> >>As I said in the commit message, I prefer the explicit sync.
> >>Re-inserting the device and taking refs already necessitate it.  
> >
> >You don't need any ref, just rcu_dereference() the netdev pointer.  
> 
> Oh wait, you are right. Sorry for the fuzz.
> 
> Reviewed-by: Jiri Pirko <jiri@nvidia.com>

Thanks! I'll improve the commit message for v2.