• Andrey Ignatov's avatar
    rtnetlink: Return correct error on changing device netns · 96a6b93b
    Andrey Ignatov authored
    Currently when device is moved between network namespaces using
    RTM_NEWLINK message type and one of netns attributes (FLA_NET_NS_PID,
    IFLA_NET_NS_FD, IFLA_TARGET_NETNSID) but w/o specifying IFLA_IFNAME, and
    target namespace already has device with same name, userspace will get
    EINVAL what is confusing and makes debugging harder.
    
    Fix it so that userspace gets more appropriate EEXIST instead what makes
    debugging much easier.
    
    Before:
    
      # ./ifname.sh
      + ip netns add ns0
      + ip netns exec ns0 ip link add l0 type dummy
      + ip netns exec ns0 ip link show l0
      8: l0: <BROADCAST,NOARP> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
          link/ether 66:90:b5:d5:78:69 brd ff:ff:ff:ff:ff:ff
      + ip link add l0 type dummy
      + ip link show l0
      10: l0: <BROADCAST,NOARP> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
          link/ether 6e:c6:1f:15:20:8d brd ff:ff:ff:ff:ff:ff
      + ip link set l0 netns ns0
      RTNETLINK answers: Invalid argument
    
    After:
    
      # ./ifname.sh
      + ip netns add ns0
      + ip netns exec ns0 ip link add l0 type dummy
      + ip netns exec ns0 ip link show l0
      8: l0: <BROADCAST,NOARP> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
          link/ether 1e:4a:72:e3:e3:8f brd ff:ff:ff:ff:ff:ff
      + ip link add l0 type dummy
      + ip link show l0
      10: l0: <BROADCAST,NOARP> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
          link/ether f2:fc:fe:2b:7d:a6 brd ff:ff:ff:ff:ff:ff
      + ip link set l0 netns ns0
      RTNETLINK answers: File exists
    
    The problem is that do_setlink() passes its `char *ifname` argument,
    that it gets from a caller, to __dev_change_net_namespace() as is (as
    `const char *pat`), but semantics of ifname and pat can be different.
    
    For example, __rtnl_newlink() does this:
    
    net/core/rtnetlink.c
        3270	char ifname[IFNAMSIZ];
         ...
        3286	if (tb[IFLA_IFNAME])
        3287		nla_strscpy(ifname, tb[IFLA_IFNAME], IFNAMSIZ);
        3288	else
        3289		ifname[0] = '\0';
         ...
        3364	if (dev) {
         ...
        3394		return do_setlink(skb, dev, ifm, extack, tb, ifname, status);
        3395	}
    
    , i.e. do_setlink() gets ifname pointer that is always valid no matter
    if user specified IFLA_IFNAME or not and then do_setlink() passes this
    ifname pointer as is to __dev_change_net_namespace() as pat argument.
    
    But the pat (pattern) in __dev_change_net_namespace() is used as:
    
    net/core/dev.c
       11198	err = -EEXIST;
       11199	if (__dev_get_by_name(net, dev->name)) {
       11200		/* We get here if we can't use the current device name */
       11201		if (!pat)
       11202			goto out;
       11203		err = dev_get_valid_name(net, dev, pat);
       11204		if (err < 0)
       11205			goto out;
       11206	}
    
    As the result the `goto out` path on line 11202 is neven taken and
    instead of returning EEXIST defined on line 11198,
    __dev_change_net_namespace() returns an error from dev_get_valid_name()
    and this, in turn, will be EINVAL for ifname[0] = '\0' set earlier.
    
    Fixes: d8a5ec67 ("[NET]: netlink support for moving devices between network namespaces.")
    Signed-off-by: default avatarAndrey Ignatov <rdna@fb.com>
    Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
    96a6b93b
rtnetlink.c 137 KB