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

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

[Sabrina Dubroca]

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,
@@ -1520,6 +1521,8 @@ static int rtnl_fill_link_netnsid(struct sk_buff *skb,
 				  const struct net_device *dev,
 				  struct net *src_net)
 {
+	bool put_iflink = false;
+
 	if (dev->rtnl_link_ops && dev->rtnl_link_ops->get_link_net) {
 		struct net *link_net = dev->rtnl_link_ops->get_link_net(dev);
 
@@ -1528,10 +1531,12 @@ static int rtnl_fill_link_netnsid(struct sk_buff *skb,
 
 			if (nla_put_s32(skb, IFLA_LINK_NETNSID, id))
 				return -EMSGSIZE;
+
+			put_iflink = true;
 		}
 	}
 
-	return 0;
+	return nla_put_iflink(skb, dev, put_iflink);
 }
 
 static int rtnl_fill_link_af(struct sk_buff *skb,
@@ -1617,7 +1622,6 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb,
 #ifdef CONFIG_RPS
 	    nla_put_u32(skb, IFLA_NUM_RX_QUEUES, dev->num_rx_queues) ||
 #endif
-	    nla_put_iflink(skb, dev) ||
 	    put_master_ifindex(skb, dev) ||
 	    nla_put_u8(skb, IFLA_CARRIER, netif_carrier_ok(dev)) ||
 	    (dev->qdisc &&
-- 
2.21.0

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

[Nicolas Dichtel]

Le 13/05/2019 à 15:47, Sabrina Dubroca a écrit :
> 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")
I would say:
Fixes: 5e6700b3bf98 ("sit: add support of x-netns")

Because before this patch, there was no device with an iflink that can be put in
another netns.


Regards,
Nicolas

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

[Sabrina Dubroca]

2019-05-13, 16:50:51 +0200, Nicolas Dichtel wrote:
> Le 13/05/2019 à 15:47, Sabrina Dubroca a écrit :
> > 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")
> I would say:
> Fixes: 5e6700b3bf98 ("sit: add support of x-netns")
> 
> Because before this patch, there was no device with an iflink that can be put in
> another netns.

That tells us how far back we might want to backport this fix, but not
which commit introduced the bug. I think Fixes should refer to the
introduction of the faulty code, not to what patch made it visible (if
we can find both).

Thanks,

-- 
Sabrina

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

[Nicolas Dichtel]

Le 13/05/2019 à 17:08, Sabrina Dubroca a écrit :
> 2019-05-13, 16:50:51 +0200, Nicolas Dichtel wrote:
>> Le 13/05/2019 à 15:47, Sabrina Dubroca a écrit :
>>> 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")
>> I would say:
>> Fixes: 5e6700b3bf98 ("sit: add support of x-netns")
>>
>> Because before this patch, there was no device with an iflink that can be put in
>> another netns.
> 
> That tells us how far back we might want to backport this fix, but not
> which commit introduced the bug. I think Fixes should refer to the
> introduction of the faulty code, not to what patch made it visible (if
> we can find both).
No sure to follow you. The problem you describe cannot happen before commit
5e6700b3bf98, so there cannot be a "faulty" patch before that commit.
Anyway, both commits are very old, so it doesn't matter.


Thank you,
Nicolas

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

[Sabrina Dubroca]

2019-05-13, 17:13:36 +0200, Nicolas Dichtel wrote:
> Le 13/05/2019 à 17:08, Sabrina Dubroca a écrit :
> > 2019-05-13, 16:50:51 +0200, Nicolas Dichtel wrote:
> >> Le 13/05/2019 à 15:47, Sabrina Dubroca a écrit :
> >>> 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")
> >> I would say:
> >> Fixes: 5e6700b3bf98 ("sit: add support of x-netns")
> >>
> >> Because before this patch, there was no device with an iflink that can be put in
> >> another netns.
> > 
> > That tells us how far back we might want to backport this fix, but not
> > which commit introduced the bug. I think Fixes should refer to the
> > introduction of the faulty code, not to what patch made it visible (if
> > we can find both).
> No sure to follow you. The problem you describe cannot happen before commit
> 5e6700b3bf98, so there cannot be a "faulty" patch before that commit.

What about macvlan devices?

From commit b863ceb7ddce ("[NET]: Add macvlan driver"):

static int macvlan_init(struct net_device *dev)
{
...
        dev->iflink             = lowerdev->ifindex;
...
}

vlan devices also had an iflink assigned since commit ddd7bf9fe4e5.

What am I missing?

-- 
Sabrina

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

[Nicolas Dichtel]

Le 13/05/2019 à 23:46, Sabrina Dubroca a écrit :
> 2019-05-13, 17:13:36 +0200, Nicolas Dichtel wrote:
>> Le 13/05/2019 à 17:08, Sabrina Dubroca a écrit :
>>> 2019-05-13, 16:50:51 +0200, Nicolas Dichtel wrote:
>>>> Le 13/05/2019 à 15:47, Sabrina Dubroca a écrit :
>>>>> 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")
>>>> I would say:
>>>> Fixes: 5e6700b3bf98 ("sit: add support of x-netns")
>>>>
>>>> Because before this patch, there was no device with an iflink that can be put in
>>>> another netns.
>>>
>>> That tells us how far back we might want to backport this fix, but not
>>> which commit introduced the bug. I think Fixes should refer to the
>>> introduction of the faulty code, not to what patch made it visible (if
>>> we can find both).
>> No sure to follow you. The problem you describe cannot happen before commit
>> 5e6700b3bf98, so there cannot be a "faulty" patch before that commit.
> 
> What about macvlan devices?
> 
> From commit b863ceb7ddce ("[NET]: Add macvlan driver"):
> 
> static int macvlan_init(struct net_device *dev)
> {
> ...
>         dev->iflink             = lowerdev->ifindex;
> ...
> }
> 
> vlan devices also had an iflink assigned since commit ddd7bf9fe4e5.
> 
> What am I missing?
You miss the fact that netns have been introduced after both commits.

What about this one?
Fixes: d8a5ec672768 ("[NET]: netlink support for moving devices between network
namespaces.")



Regards,
Nicolas

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

[Sabrina Dubroca]

2019-05-14, 09:32:32 +0200, Nicolas Dichtel wrote:
> Le 13/05/2019 à 23:46, Sabrina Dubroca a écrit :
> > 2019-05-13, 17:13:36 +0200, Nicolas Dichtel wrote:
> >> Le 13/05/2019 à 17:08, Sabrina Dubroca a écrit :
> >>> 2019-05-13, 16:50:51 +0200, Nicolas Dichtel wrote:
> >>>> Le 13/05/2019 à 15:47, Sabrina Dubroca a écrit :
> >>>>> 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")
> >>>> I would say:
> >>>> Fixes: 5e6700b3bf98 ("sit: add support of x-netns")
> >>>>
> >>>> Because before this patch, there was no device with an iflink that can be put in
> >>>> another netns.
> >>>
> >>> That tells us how far back we might want to backport this fix, but not
> >>> which commit introduced the bug. I think Fixes should refer to the
> >>> introduction of the faulty code, not to what patch made it visible (if
> >>> we can find both).
> >> No sure to follow you. The problem you describe cannot happen before commit
> >> 5e6700b3bf98, so there cannot be a "faulty" patch before that commit.
> > 
> > What about macvlan devices?
> > 
> > From commit b863ceb7ddce ("[NET]: Add macvlan driver"):
> > 
> > static int macvlan_init(struct net_device *dev)
> > {
> > ...
> >         dev->iflink             = lowerdev->ifindex;
> > ...
> > }
> > 
> > vlan devices also had an iflink assigned since commit ddd7bf9fe4e5.
> > 
> > What am I missing?
> You miss the fact that netns have been introduced after both commits.

Ah, right.

> What about this one?
> Fixes: d8a5ec672768 ("[NET]: netlink support for moving devices between network
> namespaces.")

Nice. Now I think the bug can't really trigger unless one of these
commits are present:

aa79e66eee5d ("net: Make ifindex generation per-net namespace")
9c7dafbfab15 ("net: Allow to create links with given ifindex")

I'll use those two as Fixes tags for v3, unless you want something
different.

Thanks,

-- 
Sabrina

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

[Nicolas Dichtel]

Le 14/05/2019 à 10:01, Sabrina Dubroca a écrit :
> 2019-05-14, 09:32:32 +0200, Nicolas Dichtel wrote:
[snip]
>> What about this one?
>> Fixes: d8a5ec672768 ("[NET]: netlink support for moving devices between network
>> namespaces.")
> 
> Nice. Now I think the bug can't really trigger unless one of these
> commits are present:
> 
> aa79e66eee5d ("net: Make ifindex generation per-net namespace")
> 9c7dafbfab15 ("net: Allow to create links with given ifindex")
> 
I don't think so.

Please have a look to commit ce286d327341 ("[NET]: Implement network device
movement between namespaces").
In dev_change_net_namespace(), there is the following code:

       /* If there is an ifindex conflict assign a new one */
       if (__dev_get_by_index(net, dev->ifindex)) {
               int iflink = (dev->iflink == dev->ifindex);
               dev->ifindex = dev_new_index(net);
               if (iflink)
                       dev->iflink = dev->ifindex;
       }

This code may change the ifindex of an interface when this interface moves to
another netns. This may happen even before the commits you propose, because the
global ifindex counter can wrap around.


Regards,
Nicolas

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

[Sabrina Dubroca]

2019-05-14, 12:05:16 +0200, Nicolas Dichtel wrote:
> Le 14/05/2019 à 10:01, Sabrina Dubroca a écrit :
> > 2019-05-14, 09:32:32 +0200, Nicolas Dichtel wrote:
> [snip]
> >> What about this one?
> >> Fixes: d8a5ec672768 ("[NET]: netlink support for moving devices between network
> >> namespaces.")
> > 
> > Nice. Now I think the bug can't really trigger unless one of these
> > commits are present:
> > 
> > aa79e66eee5d ("net: Make ifindex generation per-net namespace")
> > 9c7dafbfab15 ("net: Allow to create links with given ifindex")
> > 
> I don't think so.
> 
> Please have a look to commit ce286d327341 ("[NET]: Implement network device
> movement between namespaces").
> In dev_change_net_namespace(), there is the following code:
> 
>        /* If there is an ifindex conflict assign a new one */
>        if (__dev_get_by_index(net, dev->ifindex)) {
>                int iflink = (dev->iflink == dev->ifindex);
>                dev->ifindex = dev_new_index(net);
>                if (iflink)
>                        dev->iflink = dev->ifindex;
>        }
> 
> This code may change the ifindex of an interface when this interface moves to
> another netns. This may happen even before the commits you propose, because the
> global ifindex counter can wrap around.

Yes, that's possible although quite unlikely. I'll go with d8a5ec672768.

-- 
Sabrina

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

[Nicolas Dichtel]

Le 14/05/2019 à 12:24, Sabrina Dubroca a écrit :
[snip]
> Yes, that's possible although quite unlikely. I'll go with d8a5ec672768.
> 
Agreed.


Thank you,
Nicolas

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

[David Miller]

From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Date: Mon, 13 May 2019 16:50:51 +0200

> Le 13/05/2019 à 15:47, Sabrina Dubroca a écrit :
>> 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")
> I would say:
> Fixes: 5e6700b3bf98 ("sit: add support of x-netns")
> 
> Because before this patch, there was no device with an iflink that can be put in
> another netns.

I kind of agree.

What's important for people is knowing at what point they need to backport a
fix in order to actually fix a real problem.

Sabrina, please adjust the Fixes tag, thank you.

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

[David Ahern]

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?

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

[Nicolas Dichtel]

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


Nicolas

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

[Stephen Hemminger]

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.

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

[David Ahern]

On 5/13/19 9:18 AM, Nicolas Dichtel wrote:
> 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
> 
> 

I figured that would be the response, but wanted to make sure.

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

[Edward Cree]

IFLA_LINK is typoed in subject line, you might want to fix that if respinning.

-Ed