[PATCH] veth: delay peer link configuration after interfaces are tied

[PATCH] veth: delay peer link configuration after interfaces are tied

[Vincent Bernat]

When the peer link is created, its "iflink" information is not
advertised through netlink. If a user is maintaining a cache from all
updates, it will miss this information:

    2: veth0@NONE: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN group default
        link/ether ae:0e:08:af:fb:a0 brd ff:ff:ff:ff:ff:ff
    3: veth1@veth0: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN group default
        link/ether 3a:31:f1:36:2e:e5 brd ff:ff:ff:ff:ff:ff

To avoid this situation, the peer link is only configured after both
interfaces are tied together:

    3: veth0@veth1: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN group default
        link/ether ee:0d:80:46:36:fe brd ff:ff:ff:ff:ff:ff
    4: veth1@veth0: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN group default
        link/ether ba:25:bc:7a:0d:c8 brd ff:ff:ff:ff:ff:ff

Signed-off-by: Vincent Bernat <vincent@bernat.im>
---
 drivers/net/veth.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index f37a6e61d4ad..9726c4dbf659 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -432,10 +432,6 @@ static int veth_newlink(struct net *src_net, struct net_device *dev,
 
 	netif_carrier_off(peer);
 
-	err = rtnl_configure_link(peer, ifmp);
-	if (err < 0)
-		goto err_configure_peer;
-
 	/*
 	 * register dev last
 	 *
@@ -466,6 +462,10 @@ static int veth_newlink(struct net *src_net, struct net_device *dev,
 
 	priv = netdev_priv(peer);
 	rcu_assign_pointer(priv->peer, dev);
+
+	err = rtnl_configure_link(peer, ifmp);
+	if (err < 0)
+		goto err_configure_peer;
 	return 0;
 
 err_register_dev:
-- 
2.8.1

Re: [PATCH] veth: delay peer link configuration after interfaces are tied

[Nicolas Dichtel]

Le 29/05/2016 13:17, Vincent Bernat a écrit :
[snip]
> diff --git a/drivers/net/veth.c b/drivers/net/veth.c
> index f37a6e61d4ad..9726c4dbf659 100644
> --- a/drivers/net/veth.c
> +++ b/drivers/net/veth.c
> @@ -432,10 +432,6 @@ static int veth_newlink(struct net *src_net, struct net_device *dev,
>  
>  	netif_carrier_off(peer);
>  
> -	err = rtnl_configure_link(peer, ifmp);
> -	if (err < 0)
> -		goto err_configure_peer;
> -
>  	/*
>  	 * register dev last
>  	 *
> @@ -466,6 +462,10 @@ static int veth_newlink(struct net *src_net, struct net_device *dev,
>  
>  	priv = netdev_priv(peer);
>  	rcu_assign_pointer(priv->peer, dev);
> +
> +	err = rtnl_configure_link(peer, ifmp);
> +	if (err < 0)
> +		goto err_configure_peer;
You should fix the error path. 'unregister_netdevice(dev)' is missing.

Re: [PATCH] veth: delay peer link configuration after interfaces are tied

[Vincent Bernat]

 ❦ 30 mai 2016 11:23 CEST, Nicolas Dichtel <nicolas.dichtel@6wind.com> :

>> @@ -466,6 +462,10 @@ static int veth_newlink(struct net *src_net, struct net_device *dev,
>>  
>>  	priv = netdev_priv(peer);
>>  	rcu_assign_pointer(priv->peer, dev);
>> +
>> +	err = rtnl_configure_link(peer, ifmp);
>> +	if (err < 0)
>> +		goto err_configure_peer;

> You should fix the error path. 'unregister_netdevice(dev)' is missing.

I am sending another patch to fix that. I am quite unsure if I do the
right thing here.
-- 
Don't stop with your first draft.
            - The Elements of Programming Style (Kernighan & Plauger)

Re: [PATCH] veth: delay peer link configuration after interfaces are tied

[Nicolas Dichtel]

Le 30/05/2016 12:11, Vincent Bernat a écrit :
>  ❦ 30 mai 2016 11:23 CEST, Nicolas Dichtel <nicolas.dichtel@6wind.com> :
> 
>>> @@ -466,6 +462,10 @@ static int veth_newlink(struct net *src_net, struct net_device *dev,
>>>  
>>>  	priv = netdev_priv(peer);
>>>  	rcu_assign_pointer(priv->peer, dev);
>>> +
>>> +	err = rtnl_configure_link(peer, ifmp);
>>> +	if (err < 0)
>>> +		goto err_configure_peer;
> 
>> You should fix the error path. 'unregister_netdevice(dev)' is missing.
> 
> I am sending another patch to fix that. I am quite unsure if I do the
> right thing here.
> 
A less intrusive fix is to call 'rtmsg_ifinfo(RTM_NEWLINK, peer, ~0U,
GFP_KERNEL);' a the end of veth_newlink().

Re: [PATCH] veth: delay peer link configuration after interfaces are tied

[Vincent Bernat]

 ❦ 30 mai 2016 17:19 CEST, Nicolas Dichtel <nicolas.dichtel@6wind.com> :

>>>>  	priv = netdev_priv(peer);
>>>>  	rcu_assign_pointer(priv->peer, dev);
>>>> +
>>>> +	err = rtnl_configure_link(peer, ifmp);
>>>> +	if (err < 0)
>>>> +		goto err_configure_peer;
>> 
>>> You should fix the error path. 'unregister_netdevice(dev)' is missing.
>> 
>> I am sending another patch to fix that. I am quite unsure if I do the
>> right thing here.
>> 
> A less intrusive fix is to call 'rtmsg_ifinfo(RTM_NEWLINK, peer, ~0U,
> GFP_KERNEL);' a the end of veth_newlink().

I did that at first. Maybe this would make more sense to do
that. Otherwise, the first message contains an iflink value that we
cannot resolve with just the received netlink messages (since the
information is in the next netlink message). "ip monitor" seems to be
able to get the info, but I suppose it does an additional
lookup.
-- 
Make sure every module hides something.
            - The Elements of Programming Style (Kernighan & Plauger)

Re: [PATCH] veth: delay peer link configuration after interfaces are tied

[Nicolas Dichtel]

Le 30/05/2016 17:26, Vincent Bernat a écrit :
>  ❦ 30 mai 2016 17:19 CEST, Nicolas Dichtel <nicolas.dichtel@6wind.com> :
> 
>>>>>  	priv = netdev_priv(peer);
>>>>>  	rcu_assign_pointer(priv->peer, dev);
>>>>> +
>>>>> +	err = rtnl_configure_link(peer, ifmp);
>>>>> +	if (err < 0)
>>>>> +		goto err_configure_peer;
>>>
>>>> You should fix the error path. 'unregister_netdevice(dev)' is missing.
>>>
>>> I am sending another patch to fix that. I am quite unsure if I do the
>>> right thing here.
>>>
>> A less intrusive fix is to call 'rtmsg_ifinfo(RTM_NEWLINK, peer, ~0U,
>> GFP_KERNEL);' a the end of veth_newlink().
> 
> I did that at first. Maybe this would make more sense to do
> that. Otherwise, the first message contains an iflink value that we
> cannot resolve with just the received netlink messages (since the
> information is in the next netlink message). "ip monitor" seems to be
> able to get the info, but I suppose it does an additional
> lookup.
> 
Yes, it's a chicken and egg problem ;-)
I think that the first message with an iflink set to '0' is not a problem if a
second one update this value. Daemons that listen to those rtnl messages must
always update their caches. When the peer is put in another netns, its ifindex
may change again.

[net v3] veth: advertise peer link once both links are tied together

[Vincent Bernat]

When the peer link is created, its "iflink" information is not
advertised through Netlink. Once created, the local device is advertised
with this information but if a user is maintaining a cache from all
updates, it will still miss the iflink for the peer link:

    2: veth0@NONE: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN group default
        link/ether ae:0e:08:af:fb:a0 brd ff:ff:ff:ff:ff:ff
    3: veth1@veth0: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN group default
        link/ether 3a:31:f1:36:2e:e5 brd ff:ff:ff:ff:ff:ff

With this patch, we advertise again the peer link to let any user pick
the appropriate iflink information:

    3: veth0@NONE: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN group default
        link/ether fa:ba:12:26:99:00 brd ff:ff:ff:ff:ff:ff
    3: veth0@veth1: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN group default
        link/ether fa:ba:12:26:99:00 brd ff:ff:ff:ff:ff:ff
    4: veth1@veth0: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN group default
        link/ether ea:e4:e2:26:3c:87 brd ff:ff:ff:ff:ff:ff

Signed-off-by: Vincent Bernat <vincent@bernat.im>
---
v3:
 - send an additional netlink messages once the peer link is tied to
   avoid any chicken/egg problem

v2:
 - ensure the device is unregistered in case of link configuration failure

 drivers/net/veth.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index f37a6e61d4ad..2376d17b8f53 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -466,6 +466,8 @@ static int veth_newlink(struct net *src_net, struct net_device *dev,
 
 	priv = netdev_priv(peer);
 	rcu_assign_pointer(priv->peer, dev);
+
+	rtmsg_ifinfo(RTM_NEWLINK, peer, IFF_SLAVE, GFP_KERNEL);
 	return 0;
 
 err_register_dev:
-- 
2.8.1

Re: [net v3] veth: advertise peer link once both links are tied together

[Vincent Bernat]

 ❦ 30 mai 2016 17:58 CEST, Vincent Bernat <vincent@bernat.im> :

> +
> +	rtmsg_ifinfo(RTM_NEWLINK, peer, IFF_SLAVE, GFP_KERNEL);

Maybe ~0U would be better than hijacking IFF_SLAVE?
-- 
Anyone who has had a bull by the tail knows five or six more things
than someone who hasn't.
		-- Mark Twain

Re: [net v3] veth: advertise peer link once both links are tied together

[Nicolas Dichtel]

Le 30/05/2016 18:01, Vincent Bernat a écrit :
>  ❦ 30 mai 2016 17:58 CEST, Vincent Bernat <vincent@bernat.im> :
> 
>> +
>> +	rtmsg_ifinfo(RTM_NEWLINK, peer, IFF_SLAVE, GFP_KERNEL);
> 
> Maybe ~0U would be better than hijacking IFF_SLAVE?
IFF_SLAVE is wrong. It's a flag here, that will be put in the ifi_change field
not an attribute number.

Re: [net v3] veth: advertise peer link once both links are tied together

[Vincent Bernat]

 ❦ 30 mai 2016 18:27 CEST, Nicolas Dichtel <nicolas.dichtel@6wind.com> :

>>> +
>>> +	rtmsg_ifinfo(RTM_NEWLINK, peer, IFF_SLAVE, GFP_KERNEL);
>> 
>> Maybe ~0U would be better than hijacking IFF_SLAVE?
> IFF_SLAVE is wrong. It's a flag here, that will be put in the ifi_change field
> not an attribute number.

There are some use of IFF_SLAVE (in bonding/bond_main.c). But, I'll
update the patch nonetheless.
-- 
Use free-form input when possible.
            - The Elements of Programming Style (Kernighan & Plauger)

Re: [net v3] veth: advertise peer link once both links are tied together

[Nicolas Dichtel]

Le 31/05/2016 08:29, Vincent Bernat a écrit :
>  ❦ 30 mai 2016 18:27 CEST, Nicolas Dichtel <nicolas.dichtel@6wind.com> :
> 
>>>> +
>>>> +	rtmsg_ifinfo(RTM_NEWLINK, peer, IFF_SLAVE, GFP_KERNEL);
>>>
>>> Maybe ~0U would be better than hijacking IFF_SLAVE?
>> IFF_SLAVE is wrong. It's a flag here, that will be put in the ifi_change field
>> not an attribute number.
> 
> There are some use of IFF_SLAVE (in bonding/bond_main.c). But, I'll
> update the patch nonetheless.
Sorry, I read it too quickly, IFF_SLAVE is a flag, not an attribute.
But this field indicates to the userland which flags has changed and this flag
does not change here ;-)

Re: [net v3] veth: advertise peer link once both links are tied together

[Lance Richardson]

----- Original Message -----
> From: "Nicolas Dichtel" <nicolas.dichtel@6wind.com>
> To: "Vincent Bernat" <vincent@bernat.im>
> Cc: "David S. Miller" <davem@davemloft.net>, "Vijay Pandurangan" <vijayp@vijayp.ca>, "Paolo Abeni"
> <pabeni@redhat.com>, netdev@vger.kernel.org
> Sent: Tuesday, May 31, 2016 5:17:20 AM
> Subject: Re: [net v3] veth: advertise peer link once both links are tied together
> 
> Le 31/05/2016 08:29, Vincent Bernat a écrit :
> >  ❦ 30 mai 2016 18:27 CEST, Nicolas Dichtel <nicolas.dichtel@6wind.com> :
> > 
> >>>> +
> >>>> +	rtmsg_ifinfo(RTM_NEWLINK, peer, IFF_SLAVE, GFP_KERNEL);
> >>>
> >>> Maybe ~0U would be better than hijacking IFF_SLAVE?
> >> IFF_SLAVE is wrong. It's a flag here, that will be put in the ifi_change
> >> field
> >> not an attribute number.
> > 
> > There are some use of IFF_SLAVE (in bonding/bond_main.c). But, I'll
> > update the patch nonetheless.
> Sorry, I read it too quickly, IFF_SLAVE is a flag, not an attribute.
> But this field indicates to the userland which flags has changed and this
> flag
> does not change here ;-)
> 

I've been pondering how to fix this very problem off-and-on for a few months
now, without having arrived at any solution that was particularly satisfying.

The main constraints I've been trying to meet are:
   - User-space should be informed of veth pairing for both peers.
   - RTM_NEWLINK messages should not refer to interfaces that haven't
     been announced to user-space via previous RTM_NEWLINK messages.
   - The first (and only the first) RTM_NEWLINK message for a given
     interface should have ifi_changes set to ~0U, subsequent RTM_NEWLINK
     messages should have ifi_changes set to reflect the flags that
     have changed.

This is the closest I've come to a satisfactory solution, it does meet
the above constraints but still seems a little unnatural to me:

diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index f37a6e6..9151686 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -466,8 +466,16 @@ static int veth_newlink(struct net *src_net, struct net_device *dev,
 
        priv = netdev_priv(peer);
        rcu_assign_pointer(priv->peer, dev);
+
+       err = rtnl_configure_link(dev, NULL);
+       if (err < 0)
+               goto err_configure_dev;
+
+       rtmsg_ifinfo(RTM_NEWLINK, peer, 0, GFP_KERNEL);
        return 0;
 
+err_configure_dev:
+       /* nothing to do */
 err_register_dev:
        /* nothing to do */
 err_configure_peer:
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index d69c464..28ee417 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -2165,6 +2165,7 @@ static int rtnl_dellink(struct sk_buff *skb, struct nlmsghdr *nlh)
 int rtnl_configure_link(struct net_device *dev, const struct ifinfomsg *ifm)
 {
        unsigned int old_flags;
+       unsigned int gchanges;
        int err;
 
        old_flags = dev->flags;
@@ -2174,9 +2175,13 @@ int rtnl_configure_link(struct net_device *dev, const struct ifinfomsg *ifm)
                        return err;
        }
 
-       dev->rtnl_link_state = RTNL_LINK_INITIALIZED;
+       if (dev->rtnl_link_state == RTNL_LINK_INITIALIZING) {
+               dev->rtnl_link_state = RTNL_LINK_INITIALIZED;
+               gchanges = ~0U;
+       } else
+               gchanges = dev->flags ^ old_flags;
 
-       __dev_notify_flags(dev, old_flags, ~0U);
+       __dev_notify_flags(dev, old_flags, gchanges);
        return 0;
 }
 EXPORT_SYMBOL(rtnl_configure_link);

Sample output:

# ip link add dev vm1 type veth peer name vm2
5: vm2@NONE: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN 
    link/ether 36:a7:b4:dc:27:3b brd ff:ff:ff:ff:ff:ff
6: vm1@vm2: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN 
    link/ether 72:78:30:f6:e6:3a brd ff:ff:ff:ff:ff:ff
5: vm2@vm1: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN 
    link/ether 36:a7:b4:dc:27:3b brd ff:ff:ff:ff:ff:ff

Re: [net v3] veth: advertise peer link once both links are tied together

[Nicolas Dichtel]

Le 08/06/2016 22:30, Lance Richardson a écrit :
[snip]
> I've been pondering how to fix this very problem off-and-on for a few months
> now, without having arrived at any solution that was particularly satisfying.
> 
> The main constraints I've been trying to meet are:
>    - User-space should be informed of veth pairing for both peers.
>    - RTM_NEWLINK messages should not refer to interfaces that haven't
>      been announced to user-space via previous RTM_NEWLINK messages.
>    - The first (and only the first) RTM_NEWLINK message for a given
>      interface should have ifi_changes set to ~0U, subsequent RTM_NEWLINK
>      messages should have ifi_changes set to reflect the flags that
>      have changed.
> 
> This is the closest I've come to a satisfactory solution, it does meet
> the above constraints but still seems a little unnatural to me:
The patch looks good to me. Can you submit it formally?

Re: [net v3] veth: advertise peer link once both links are tied together

[Lance Richardson]

----- Original Message -----
> From: "Nicolas Dichtel" <nicolas.dichtel@6wind.com>
> To: "Lance Richardson" <lrichard@redhat.com>, "Vincent Bernat" <vincent@bernat.im>
> Cc: "David S. Miller" <davem@davemloft.net>, "Vijay Pandurangan" <vijayp@vijayp.ca>, "Paolo Abeni"
> <pabeni@redhat.com>, netdev@vger.kernel.org
> Sent: Friday, June 10, 2016 9:15:01 AM
> Subject: Re: [net v3] veth: advertise peer link once both links are tied together
> 
> Le 08/06/2016 22:30, Lance Richardson a écrit :
> [snip]
> > I've been pondering how to fix this very problem off-and-on for a few
> > months
> > now, without having arrived at any solution that was particularly
> > satisfying.
> > 
> > The main constraints I've been trying to meet are:
> >    - User-space should be informed of veth pairing for both peers.
> >    - RTM_NEWLINK messages should not refer to interfaces that haven't
> >      been announced to user-space via previous RTM_NEWLINK messages.
> >    - The first (and only the first) RTM_NEWLINK message for a given
> >      interface should have ifi_changes set to ~0U, subsequent RTM_NEWLINK
> >      messages should have ifi_changes set to reflect the flags that
> >      have changed.
> > 
> > This is the closest I've come to a satisfactory solution, it does meet
> > the above constraints but still seems a little unnatural to me:
> The patch looks good to me. Can you submit it formally?
> 

Will post in a bit, thanks!

  Lance

[net v4] veth: advertise peer link once both links are tied together

[Vincent Bernat]

When the peer link is created, its "iflink" information is not
advertised through Netlink. Once created, the local device is advertised
with this information but if a user is maintaining a cache from all
updates, it will still miss the iflink for the peer link:

    2: veth0@NONE: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN group default
        link/ether ae:0e:08:af:fb:a0 brd ff:ff:ff:ff:ff:ff
    3: veth1@veth0: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN group default
        link/ether 3a:31:f1:36:2e:e5 brd ff:ff:ff:ff:ff:ff

With this patch, we advertise again the peer link to let any user pick
the appropriate iflink information:

    3: veth0@NONE: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN group default
        link/ether fa:ba:12:26:99:00 brd ff:ff:ff:ff:ff:ff
    3: veth0@veth1: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN group default
        link/ether fa:ba:12:26:99:00 brd ff:ff:ff:ff:ff:ff
    4: veth1@veth0: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN group default
        link/ether ea:e4:e2:26:3c:87 brd ff:ff:ff:ff:ff:ff

Signed-off-by: Vincent Bernat <vincent@bernat.im>
---
v4:
 - use ~0U instead of IFF_SLAVE for ifi_change

v3:
 - send an additional netlink messages once the peer link is tied to
   avoid any chicken/egg problem

v2:
 - ensure the device is unregistered in case of link configuration failure

drivers/net/veth.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index f37a6e61d4ad..aaa1b023b9f2 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -466,6 +466,8 @@ static int veth_newlink(struct net *src_net, struct net_device *dev,
 
 	priv = netdev_priv(peer);
 	rcu_assign_pointer(priv->peer, dev);
+
+	rtmsg_ifinfo(RTM_NEWLINK, peer, ~0U, GFP_KERNEL);
 	return 0;
 
 err_register_dev:
-- 
2.8.1

Re: [net v4] veth: advertise peer link once both links are tied together

[Vincent Bernat]

 ❦ 31 mai 2016 08:30 CEST, Vincent Bernat <vincent@bernat.im> :

> diff --git a/drivers/net/veth.c b/drivers/net/veth.c
> index f37a6e61d4ad..aaa1b023b9f2 100644
> --- a/drivers/net/veth.c
> +++ b/drivers/net/veth.c
> @@ -466,6 +466,8 @@ static int veth_newlink(struct net *src_net, struct net_device *dev,
>  
>  	priv = netdev_priv(peer);
>  	rcu_assign_pointer(priv->peer, dev);
> +
> +	rtmsg_ifinfo(RTM_NEWLINK, peer, ~0U, GFP_KERNEL);
>  	return 0;

Well, this is wrong here too because the NEWLINK message for dev was not
issued either (link state is not initialized), so we are back to square
one.

     3: veth0@NONE: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN group default
         link/ether fa:ba:12:26:99:00 brd ff:ff:ff:ff:ff:ff
     3: veth0@veth1: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN group default
         link/ether fa:ba:12:26:99:00 brd ff:ff:ff:ff:ff:ff
     4: veth1@veth0: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN group default
         link/ether ea:e4:e2:26:3c:87 brd ff:ff:ff:ff:ff:ff

The netlink message for veth1 is sent by rtnl_newlink() after invocation
of veth_newlink(). So, I don't see any easy way to fix that. Maybe not
worth it.
-- 
Let the data structure the program.
            - The Elements of Programming Style (Kernighan & Plauger)

[PATCH] veth: delay peer link configuration after interfaces are tied

[Vincent Bernat]

When the peer link is created, its "iflink" information is not
advertised through netlink. If a user is maintaining a cache from all
updates, it will miss this information:

    2: veth0@NONE: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN group default
        link/ether ae:0e:08:af:fb:a0 brd ff:ff:ff:ff:ff:ff
    3: veth1@veth0: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN group default
        link/ether 3a:31:f1:36:2e:e5 brd ff:ff:ff:ff:ff:ff

To avoid this situation, the peer link is only configured after both
interfaces are tied together:

    3: veth0@veth1: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN group default
        link/ether ee:0d:80:46:36:fe brd ff:ff:ff:ff:ff:ff
    4: veth1@veth0: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN group default
        link/ether ba:25:bc:7a:0d:c8 brd ff:ff:ff:ff:ff:ff

Signed-off-by: Vincent Bernat <vincent@bernat.im>
---
 drivers/net/veth.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index f37a6e61d4ad..9726c4dbf659 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -432,10 +432,6 @@ static int veth_newlink(struct net *src_net, struct net_device *dev,
 
 	netif_carrier_off(peer);
 
-	err = rtnl_configure_link(peer, ifmp);
-	if (err < 0)
-		goto err_configure_peer;
-
 	/*
 	 * register dev last
 	 *
@@ -466,6 +462,10 @@ static int veth_newlink(struct net *src_net, struct net_device *dev,
 
 	priv = netdev_priv(peer);
 	rcu_assign_pointer(priv->peer, dev);
+
+	err = rtnl_configure_link(peer, ifmp);
+	if (err < 0)
+		goto err_configure_peer;
 	return 0;
 
 err_register_dev:
-- 
2.8.1

Re: [PATCH] veth: delay peer link configuration after interfaces are tied

[Vincent Bernat]

 ❦ 30 mai 2016 12:12 CEST, Vincent Bernat <vincent@bernat.im> :

> When the peer link is created, its "iflink" information is not
[...]

And that's the wrong patch... Please, ignore this one.
-- 
Don't stop with your first draft.
            - The Elements of Programming Style (Kernighan & Plauger)

[PATCH] veth: delay peer link configuration after interfaces are tied

[Vincent Bernat]

When the peer link is created, its "iflink" information is not
advertised through netlink. If a user is maintaining a cache from all
updates, it will miss this information:

    2: veth0@NONE: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN group default
        link/ether ae:0e:08:af:fb:a0 brd ff:ff:ff:ff:ff:ff
    3: veth1@veth0: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN group default
        link/ether 3a:31:f1:36:2e:e5 brd ff:ff:ff:ff:ff:ff

To avoid this situation, the peer link is only configured after both
interfaces are tied together:

    3: veth0@veth1: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN group default
        link/ether ee:0d:80:46:36:fe brd ff:ff:ff:ff:ff:ff
    4: veth1@veth0: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN group default
        link/ether ba:25:bc:7a:0d:c8 brd ff:ff:ff:ff:ff:ff

Signed-off-by: Vincent Bernat <vincent@bernat.im>
---
 drivers/net/veth.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index f37a6e61d4ad..7580f6765948 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -432,10 +432,6 @@ static int veth_newlink(struct net *src_net, struct net_device *dev,
 
 	netif_carrier_off(peer);
 
-	err = rtnl_configure_link(peer, ifmp);
-	if (err < 0)
-		goto err_configure_peer;
-
 	/*
 	 * register dev last
 	 *
@@ -466,8 +462,17 @@ static int veth_newlink(struct net *src_net, struct net_device *dev,
 
 	priv = netdev_priv(peer);
 	rcu_assign_pointer(priv->peer, dev);
+
+	err = rtnl_configure_link(peer, ifmp);
+	if (err < 0)
+		goto err_configure_link;
 	return 0;
 
+err_configure_link:
+	RCU_INIT_POINTER(priv->peer, NULL);
+	priv = netdev_priv(dev);
+	RCU_INIT_POINTER(priv->peer, NULL);
+	unregister_netdevice(dev);
 err_register_dev:
 	/* nothing to do */
 err_configure_peer:
-- 
2.8.1

Re: [PATCH] veth: delay peer link configuration after interfaces are tied

[Nicolas Dichtel]

Le 30/05/2016 12:13, Vincent Bernat a écrit :
> When the peer link is created, its "iflink" information is not
> advertised through netlink. If a user is maintaining a cache from all
> updates, it will miss this information:
> 
>     2: veth0@NONE: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN group default
>         link/ether ae:0e:08:af:fb:a0 brd ff:ff:ff:ff:ff:ff
>     3: veth1@veth0: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN group default
>         link/ether 3a:31:f1:36:2e:e5 brd ff:ff:ff:ff:ff:ff
> 
> To avoid this situation, the peer link is only configured after both
> interfaces are tied together:
> 
>     3: veth0@veth1: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN group default
>         link/ether ee:0d:80:46:36:fe brd ff:ff:ff:ff:ff:ff
>     4: veth1@veth0: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN group default
>         link/ether ba:25:bc:7a:0d:c8 brd ff:ff:ff:ff:ff:ff
> 
> Signed-off-by: Vincent Bernat <vincent@bernat.im>
> ---
Please specify the version in the title (something like '[PATCH net v3]' here,
see --subject-prefix).

Also add the changelog after the '---' (see --annotate).