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  :
> 
>   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.

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

[Vincent Bernat]

 ❦ 30 mai 2016 17:19 CEST, Nicolas Dichtel  :

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 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:  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:  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:  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:  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 
> ---
Please specify the version in the title (something like '[PATCH net v3]' here,
see --subject-prefix).

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

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  :
> 
>>> @@ -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 12:12 CEST, Vincent Bernat  :

> 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)

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

[Vincent Bernat]

 ❦ 30 mai 2016 11:23 CEST, Nicolas Dichtel  :

>> @@ -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)

[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:  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:  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:  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:  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 
---
 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

[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:  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:  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:  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:  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 
---
 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.

[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:  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:  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:  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:  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 
---
 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