[PATCHi next] veth: advertise peer link relationship for both devices

Subsystems: networking drivers, networking [general], the rest

4 messages, 2 authors, 2016-06-12 · open the first message on its own page

[PATCHi next] veth: advertise peer link relationship for both devices

From: Lance Richardson <hidden>
Date: 2016-06-10 16:32:24

Currently, when creating a veth pair, notfications to user
space only include link peer for one end of the veth pair:
   # ip monitor link &
   # ip link add dev vm1 type veth peer name vm2
   30: vm2@NONE: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN
       link/ether be:e3:b7:0e:14:52 brd ff:ff:ff:ff:ff:ff
   31: vm1@vm2: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN
       link/ether da:e6:a6:c5:42:54 brd ff:ff:ff:ff:ff:ff

With this change, netlink notifications are sent with complete
information for both interfaces of the veth pair:

   # 3: vm2@NONE: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN
       link/ether e2:94:54:8a:ac:f5 brd ff:ff:ff:ff:ff:ff
   4: vm1@vm2: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN
       link/ether b2:05:70:e0:fc:35 brd ff:ff:ff:ff:ff:ff
   3: vm2@vm1: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN
       link/ether e2:94:54:8a:ac:f5 brd ff:ff:ff:ff:ff:ff

Signed-off-by: Lance Richardson <redacted>
---
 drivers/net/veth.c   | 10 +++++++++-
 net/core/rtnetlink.c | 10 ++++++++--
 2 files changed, 17 insertions(+), 3 deletions(-)

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

@@ -458,7 +458,7 @@ static int veth_newlink(struct net *src_net, struct net_device *dev,
 	netif_carrier_off(dev);
 
 	/*
-	 * tie the deviced together
+	 * tie the devices together
 	 */
 
 	priv = netdev_priv(dev);

@@ -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..e0956bb 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,14 @@ 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);

-- 
2.5.5

Re: [PATCHi next] veth: advertise peer link relationship for both devices

From: David Miller <davem@davemloft.net>
Date: 2016-06-11 22:43:41

From: Lance Richardson <redacted>
Date: Fri, 10 Jun 2016 12:32:19 -0400

Currently, when creating a veth pair, notfications to user
space only include link peer for one end of the veth pair:
   # ip monitor link &
   # ip link add dev vm1 type veth peer name vm2
   30: vm2@NONE: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN
       link/ether be:e3:b7:0e:14:52 brd ff:ff:ff:ff:ff:ff
   31: vm1@vm2: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN
       link/ether da:e6:a6:c5:42:54 brd ff:ff:ff:ff:ff:ff

With this change, netlink notifications are sent with complete
information for both interfaces of the veth pair:

   # 3: vm2@NONE: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN
       link/ether e2:94:54:8a:ac:f5 brd ff:ff:ff:ff:ff:ff
   4: vm1@vm2: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN
       link/ether b2:05:70:e0:fc:35 brd ff:ff:ff:ff:ff:ff
   3: vm2@vm1: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN
       link/ether e2:94:54:8a:ac:f5 brd ff:ff:ff:ff:ff:ff

Signed-off-by: Lance Richardson <redacted>

I don't know about this.

First of all, those notifications you get above tell you everything you
need to know in order to figure out what both ends of the veth pair are.

In fact, I would say that the vm1@vm2 notification #31 above is the _only_
one you absolutely need.

quoted hunk

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

If you're registering the peer here explicitly, this means a link configure
somewhere else is now superfluous.

I really don't like this change at all, both from a necessity perspective as
well as from it's implementation.

Re: [PATCHi next] veth: advertise peer link relationship for both devices

From: Lance Richardson <hidden>
Date: 2016-06-12 02:04:18

----- Original Message -----

From: "David Miller" <davem@davemloft.net>
To: lrichard@redhat.com
Cc: netdev@vger.kernel.org, "nicolas dichtel" <redacted>
Sent: Saturday, June 11, 2016 6:43:40 PM
Subject: Re: [PATCHi next] veth: advertise peer link relationship for both devices

From: Lance Richardson <redacted>
Date: Fri, 10 Jun 2016 12:32:19 -0400

quoted

Currently, when creating a veth pair, notfications to user
space only include link peer for one end of the veth pair:
   # ip monitor link &
   # ip link add dev vm1 type veth peer name vm2
   30: vm2@NONE: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN
       link/ether be:e3:b7:0e:14:52 brd ff:ff:ff:ff:ff:ff
   31: vm1@vm2: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN
       link/ether da:e6:a6:c5:42:54 brd ff:ff:ff:ff:ff:ff

With this change, netlink notifications are sent with complete
information for both interfaces of the veth pair:

   # 3: vm2@NONE: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN
       link/ether e2:94:54:8a:ac:f5 brd ff:ff:ff:ff:ff:ff
   4: vm1@vm2: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN
       link/ether b2:05:70:e0:fc:35 brd ff:ff:ff:ff:ff:ff
   3: vm2@vm1: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN
       link/ether e2:94:54:8a:ac:f5 brd ff:ff:ff:ff:ff:ff

Signed-off-by: Lance Richardson <redacted>

I don't know about this.

First of all, those notifications you get above tell you everything you
need to know in order to figure out what both ends of the veth pair are.

In fact, I would say that the vm1@vm2 notification #31 above is the _only_
one you absolutely need.

quoted

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

If you're registering the peer here explicitly, this means a link configure
somewhere else is now superfluous.

I really don't like this change at all, both from a necessity perspective as
well as from it's implementation.

I'll confess to not being super-happy with it myself, which is why I've
been sitting on this patch for some time now. A hard NAK will help justify
a "will not fix" to the reporter of this issue.

Thanks,

  Lance

Re: [PATCHi next] veth: advertise peer link relationship for both devices

From: David Miller <davem@davemloft.net>
Date: 2016-06-12 02:30:21

From: Lance Richardson <redacted>
Date: Sat, 11 Jun 2016 22:04:13 -0400 (EDT)

I'll confess to not being super-happy with it myself, which is why I've
been sitting on this patch for some time now. A hard NAK will help justify
a "will not fix" to the reporter of this issue.

Since userspace can learn everything is needs to using the existing
notifications, there is no reason for this change, NACK.

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help