Skip to content

Commit

Permalink
Merge branch 'net-fix-bugs-in-device-netns-move-and-rename'
Browse files Browse the repository at this point in the history
Jakub Kicinski says:

====================
net: fix bugs in device netns-move and rename

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/r/20231018013817.2391509-1-kuba@kernel.org
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
  • Loading branch information
Paolo Abeni committed Oct 19, 2023
2 parents a602ee3 + 3920431 commit f7d86df
Show file tree
Hide file tree
Showing 4 changed files with 141 additions and 15 deletions.
65 changes: 50 additions & 15 deletions net/core/dev.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand All @@ -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;
Expand All @@ -380,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();
Expand All @@ -390,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));

Expand All @@ -401,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);
Expand Down Expand Up @@ -1086,7 +1096,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)
Expand Down Expand Up @@ -1123,6 +1134,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)
Expand Down Expand Up @@ -1160,19 +1191,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);

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);
char buf[IFNAMSIZ];
int ret;

return 0;
ret = dev_prep_valid_name(net, dev, name, buf);
if (ret >= 0)
strscpy(dev->name, buf, IFNAMSIZ);
return ret;
}

/**
Expand Down Expand Up @@ -11037,7 +11062,9 @@ 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;

ASSERT_RTNL();
Expand All @@ -11064,10 +11091,15 @@ 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;
}
/* 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) {
Expand Down Expand Up @@ -11135,6 +11167,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);
Expand Down
3 changes: 3 additions & 0 deletions net/core/dev.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
1 change: 1 addition & 0 deletions tools/testing/selftests/net/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
87 changes: 87 additions & 0 deletions tools/testing/selftests/net/netns-name.sh
Original file line number Diff line number Diff line change
@@ -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

0 comments on commit f7d86df

Please sign in to comment.