Re: [PATCH net v2] rtnetlink: always put ILFA_LINK for links with a link-netnsid

[Stephen Hemminger <stephen@networkplumber.org>]

On Mon, 13 May 2019 17:18:28 +0200
Nicolas Dichtel <nicolas.dichtel@6wind.com> wrote:

> Le 13/05/2019 à 17:07, David Ahern a écrit :
> > On 5/13/19 7:47 AM, Sabrina Dubroca wrote:  
> >> Currently, nla_put_iflink() doesn't put the IFLA_LINK attribute when
> >> iflink == ifindex.
> >>
> >> In some cases, a device can be created in a different netns with the
> >> same ifindex as its parent. That device will not dump its IFLA_LINK
> >> attribute, which can confuse some userspace software that expects it.
> >> For example, if the last ifindex created in init_net and foo are both
> >> 8, these commands will trigger the issue:
> >>
> >>     ip link add parent type dummy                   # ifindex 9
> >>     ip link add link parent netns foo type macvlan  # ifindex 9 in ns foo
> >>
> >> So, in case a device puts the IFLA_LINK_NETNSID attribute in a dump,
> >> always put the IFLA_LINK attribute as well.
> >>
> >> Thanks to Dan Winship for analyzing the original OpenShift bug down to
> >> the missing netlink attribute.
> >>
> >> Analyzed-by: Dan Winship <danw@redhat.com>
> >> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> >> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
> >> Acked-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> >> ---
> >> v2: change Fixes tag, it's been here forever as Nicolas said, and add his Ack
> >>
> >>  net/core/rtnetlink.c | 16 ++++++++++------
> >>  1 file changed, 10 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> >> index 2bd12afb9297..adcc045952c2 100644
> >> --- a/net/core/rtnetlink.c
> >> +++ b/net/core/rtnetlink.c
> >> @@ -1496,14 +1496,15 @@ static int put_master_ifindex(struct sk_buff *skb, struct net_device *dev)
> >>  	return ret;
> >>  }
> >>  
> >> -static int nla_put_iflink(struct sk_buff *skb, const struct net_device *dev)
> >> +static int nla_put_iflink(struct sk_buff *skb, const struct net_device *dev,
> >> +			  bool force)
> >>  {
> >>  	int ifindex = dev_get_iflink(dev);
> >>  
> >> -	if (dev->ifindex == ifindex)
> >> -		return 0;
> >> +	if (force || dev->ifindex != ifindex)
> >> +		return nla_put_u32(skb, IFLA_LINK, ifindex);
> >>  
> >> -	return nla_put_u32(skb, IFLA_LINK, ifindex);
> >> +	return 0;
> >>  }
> >>  
> >>  static noinline_for_stack int nla_put_ifalias(struct sk_buff *skb,  
> > 
> > why not always adding the attribute?
> >   
> Adding this attribute may change the output of 'ip link'.
> See this patch for example:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=95ec655bc465

+1

Printing eno1@eno1 is going to make scripts and users upset.