[net-next PATCH] net: rtnetlink: Support alias interfaces with RTM_GETLINK

[net-next PATCH] net: rtnetlink: Support alias interfaces with RTM_GETLINK

[Phil Sutter]

Align interface name handling regarding alias interfaces in
rtnl_getlink() with dev_ioctl() treating SIOCGIFINDEX ioctl calls. The
latter function strips any colon suffix before doing the interface
lookup, do the same for RTM_GETLINK requests.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 net/core/rtnetlink.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index a51cab95ba64c..aaee4df0ff00c 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -3312,8 +3312,10 @@ static int rtnl_getlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 			return PTR_ERR(tgt_net);
 	}
 
-	if (tb[IFLA_IFNAME])
+	if (tb[IFLA_IFNAME]) {
 		nla_strlcpy(ifname, tb[IFLA_IFNAME], IFNAMSIZ);
+		*strchrnul(ifname, ':') = '\0';
+	}
 
 	if (tb[IFLA_EXT_MASK])
 		ext_filter_mask = nla_get_u32(tb[IFLA_EXT_MASK]);
-- 
2.20.1

Re: [net-next PATCH] net: rtnetlink: Support alias interfaces with RTM_GETLINK

[Phil Sutter]

On Thu, Feb 07, 2019 at 11:24:38AM +0100, Phil Sutter wrote:
> Align interface name handling regarding alias interfaces in
> rtnl_getlink() with dev_ioctl() treating SIOCGIFINDEX ioctl calls. The
> latter function strips any colon suffix before doing the interface
> lookup, do the same for RTM_GETLINK requests.

After a second thought, I'll self-NACK this one: Given that netlink API
is completely unrelated to ioctl one, there is no inherent need to do
things the same way. Looking at RTM_NEWLINK handler, it seems possible
to create interface names containing a colon via netlink. Of course
those interfaces are not accessible via ioctl() then, but so are
secondary interface addresses which don't have a properly chosen label.

Sorry for the noise, Phil

Re: [net-next PATCH] net: rtnetlink: Support alias interfaces with RTM_GETLINK

[Michal Kubecek]

On Thu, Feb 07, 2019 at 01:27:28PM +0100, Phil Sutter wrote:
> On Thu, Feb 07, 2019 at 11:24:38AM +0100, Phil Sutter wrote:
> > Align interface name handling regarding alias interfaces in
> > rtnl_getlink() with dev_ioctl() treating SIOCGIFINDEX ioctl calls. The
> > latter function strips any colon suffix before doing the interface
> > lookup, do the same for RTM_GETLINK requests.
> 
> After a second thought, I'll self-NACK this one: Given that netlink API
> is completely unrelated to ioctl one, there is no inherent need to do
> things the same way. Looking at RTM_NEWLINK handler, it seems possible
> to create interface names containing a colon via netlink.

Not since commit a4176a939186 ("net: reject creation of netdev names
with colons") which disallowed using such names in general.

But I still don't think it would be a good idea. It's bad enough that
(as I just learned to my surprise) "ip link del dummy1:0" deletes dummy1
without any complaint because ip uses SIOCGIFINDEX ioctl for ifindex
lookup.

Michal Kubecek

Re: [net-next PATCH] net: rtnetlink: Support alias interfaces with RTM_GETLINK

[Phil Sutter]

Hi,

On Thu, Feb 07, 2019 at 01:39:39PM +0100, Michal Kubecek wrote:
> On Thu, Feb 07, 2019 at 01:27:28PM +0100, Phil Sutter wrote:
> > On Thu, Feb 07, 2019 at 11:24:38AM +0100, Phil Sutter wrote:
> > > Align interface name handling regarding alias interfaces in
> > > rtnl_getlink() with dev_ioctl() treating SIOCGIFINDEX ioctl calls. The
> > > latter function strips any colon suffix before doing the interface
> > > lookup, do the same for RTM_GETLINK requests.
> > 
> > After a second thought, I'll self-NACK this one: Given that netlink API
> > is completely unrelated to ioctl one, there is no inherent need to do
> > things the same way. Looking at RTM_NEWLINK handler, it seems possible
> > to create interface names containing a colon via netlink.
> 
> Not since commit a4176a939186 ("net: reject creation of netdev names
> with colons") which disallowed using such names in general.

In my typical afterthought I tried 'ip link add d0:1 type dummy' and was
surprised to get EINVAL from kernel side. Just discovered that line in
dev_valid_name(), too.

> But I still don't think it would be a good idea. It's bad enough that
> (as I just learned to my surprise) "ip link del dummy1:0" deletes dummy1
> without any complaint because ip uses SIOCGIFINDEX ioctl for ifindex
> lookup.

I'm struggling a bit with all this. The original problem is iproute2
commit 50b9950dd9011 ("link dump filter") which changed 'ip link show'
to not use if_indextoname() when given just an interface name. So lookup
happens by name (via RTM_GETLINK) and consequently 'ip link show eth0:1'
doesn't give link stats of eth0 anymore.

Given that iproute2 is supposed to be backwards compatible, the only
valid option I see is to make sure netlink API calls like the above
behave identical to the ioctl ones they replace. Which means allowing
for 'ip link show eth0:42' even if there's no address with that label
assigned to eth0 as well as your example above.

Cheers, Phil

Re: [net-next PATCH] net: rtnetlink: Support alias interfaces with RTM_GETLINK

[Michal Kubecek]

On Thu, Feb 07, 2019 at 02:02:15PM +0100, Phil Sutter wrote:
> On Thu, Feb 07, 2019 at 01:39:39PM +0100, Michal Kubecek wrote:
> 
> > But I still don't think it would be a good idea. It's bad enough that
> > (as I just learned to my surprise) "ip link del dummy1:0" deletes dummy1
> > without any complaint because ip uses SIOCGIFINDEX ioctl for ifindex
> > lookup.
> 
> I'm struggling a bit with all this. The original problem is iproute2
> commit 50b9950dd9011 ("link dump filter") which changed 'ip link show'
> to not use if_indextoname() when given just an interface name. So lookup
> happens by name (via RTM_GETLINK) and consequently 'ip link show eth0:1'
> doesn't give link stats of eth0 anymore.

I would rather consider it a bug that it ever did. It's quite harmless
with "show" but with "set" or "delete", the effect can be quite
disastrous.

We want to preserve backward compatibility in general but iproute2
commit 50b9950dd901 is 4.5 years old and nobody seems to have complained
about the change in behaviour until now.

> Given that iproute2 is supposed to be backwards compatible, the only
> valid option I see is to make sure netlink API calls like the above
> behave identical to the ioctl ones they replace. Which means allowing
> for 'ip link show eth0:42' even if there's no address with that label
> assigned to eth0 as well as your example above.

One reason why I don't like this idea is that iproute2 is not the only
user of rtnetlink interface. There is wicked and glibc for sure, most
likely also NetworkManager (don't remember) and systemd-networkd (didn't
check) and certainly many others I never heard of. Changing the logic
in kernel rtnetlink implementation would affect all of them.

If we want to restore the old behaviour of ip (which I'm not convinced
of), it would make more sense to me to strip the :* suffix in iproute2.

Michal Kubecek

Re: [net-next PATCH] net: rtnetlink: Support alias interfaces with RTM_GETLINK

[Phil Sutter]

On Thu, Feb 07, 2019 at 02:21:56PM +0100, Michal Kubecek wrote:
> On Thu, Feb 07, 2019 at 02:02:15PM +0100, Phil Sutter wrote:
> > On Thu, Feb 07, 2019 at 01:39:39PM +0100, Michal Kubecek wrote:
> > 
> > > But I still don't think it would be a good idea. It's bad enough that
> > > (as I just learned to my surprise) "ip link del dummy1:0" deletes dummy1
> > > without any complaint because ip uses SIOCGIFINDEX ioctl for ifindex
> > > lookup.
> > 
> > I'm struggling a bit with all this. The original problem is iproute2
> > commit 50b9950dd9011 ("link dump filter") which changed 'ip link show'
> > to not use if_indextoname() when given just an interface name. So lookup
> > happens by name (via RTM_GETLINK) and consequently 'ip link show eth0:1'
> > doesn't give link stats of eth0 anymore.
> 
> I would rather consider it a bug that it ever did. It's quite harmless
> with "show" but with "set" or "delete", the effect can be quite
> disastrous.

Yes, you're right. And unless maintainers care about this compatibility,
I guess these monsters will slowly disappear as things progress towards
netlink.

> We want to preserve backward compatibility in general but iproute2
> commit 50b9950dd901 is 4.5 years old and nobody seems to have complained
> about the change in behaviour until now.

Well, enterprise distributions deliberately try to prevent those changes
from hitting their customers. And (perfect match), these users are the
least tolerating ones. :)

> > Given that iproute2 is supposed to be backwards compatible, the only
> > valid option I see is to make sure netlink API calls like the above
> > behave identical to the ioctl ones they replace. Which means allowing
> > for 'ip link show eth0:42' even if there's no address with that label
> > assigned to eth0 as well as your example above.
> 
> One reason why I don't like this idea is that iproute2 is not the only
> user of rtnetlink interface. There is wicked and glibc for sure, most
> likely also NetworkManager (don't remember) and systemd-networkd (didn't
> check) and certainly many others I never heard of. Changing the logic
> in kernel rtnetlink implementation would affect all of them.
> 
> If we want to restore the old behaviour of ip (which I'm not convinced
> of), it would make more sense to me to strip the :* suffix in iproute2.

I just sent a patch, let's see what userspace has to say about it.

Thanks, Phil