[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZS+FehME4fC4b7w4@nanopsycho>
Date: Wed, 18 Oct 2023 09:12:58 +0200
From: Jiri Pirko <jiri@...nulli.us>
To: Jakub Kicinski <kuba@...nel.org>
Cc: davem@...emloft.net, netdev@...r.kernel.org, edumazet@...gle.com,
pabeni@...hat.com, przemyslaw.kitszel@...el.com,
daniel@...earbox.net, opurdila@...acom.com
Subject: Re: [PATCH net v2 1/5] net: fix ifname in netlink ntf during netns
move
Wed, Oct 18, 2023 at 03:38:13AM CEST, kuba@...nel.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@...nel.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@...earbox.net
>CC: opurdila@...acom.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
>
Powered by blists - more mailing lists