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

[Lance Richardson <lrichard@redhat.com>]

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