Re: Regression in 4.1 with veth and IFLA_LINK

Here is a proposed patch to fix this by providing a symlink to the
peer instead. This may not totally replace the use of iflink since the
peer won't be available through netlink anymore but maybe this is good
enough for the intended use.

While the documentation doesn't say exactly what kind of relationship
iflink should represent, until a45253, only lower devices were
advertised this way. While veth cannot have a lower device, using iflink
to advertise the peer may create infinite loops in programs using iflink
to discover device topology.

Instead of advertising the peer link with iflink, a symbolic link "peer"
is added to each peer.

Signed-off-by: Vincent Bernat <redacted>
---
 drivers/net/veth.c | 40 +++++++++++++++++++++++++---------------
 1 file changed, 25 insertions(+), 15 deletions(-)

diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index c8186ffda1a3..47f165bc3107 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c

@@ -105,6 +105,17 @@ static const struct ethtool_ops veth_ethtool_ops = {
 	.get_ethtool_stats	= veth_get_ethtool_stats,
 };
 
+static int veth_peer_sysfs_add(struct net_device *dev,
+			      struct net_device *peer_dev)
+{
+	return sysfs_create_link(&(dev->dev.kobj), &(peer_dev->dev.kobj),
+				 "peer");
+}
+static void veth_peer_sysfs_del(struct net_device *dev)
+{
+	sysfs_remove_link(&(dev->dev.kobj), "peer");
+}
+
 static netdev_tx_t veth_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct veth_priv *priv = netdev_priv(dev);

@@ -263,20 +274,6 @@ static void veth_poll_controller(struct net_device *dev)
 }
 #endif	/* CONFIG_NET_POLL_CONTROLLER */
 
-static int veth_get_iflink(const struct net_device *dev)
-{
-	struct veth_priv *priv = netdev_priv(dev);
-	struct net_device *peer;
-	int iflink;
-
-	rcu_read_lock();
-	peer = rcu_dereference(priv->peer);
-	iflink = peer ? peer->ifindex : 0;
-	rcu_read_unlock();
-
-	return iflink;
-}
-
 static const struct net_device_ops veth_netdev_ops = {
 	.ndo_init            = veth_dev_init,
 	.ndo_open            = veth_open,

@@ -289,7 +286,6 @@ static const struct net_device_ops veth_netdev_ops = {
 #ifdef CONFIG_NET_POLL_CONTROLLER
 	.ndo_poll_controller	= veth_poll_controller,
 #endif
-	.ndo_get_iflink		= veth_get_iflink,
 };
 
 #define VETH_FEATURES (NETIF_F_SG | NETIF_F_FRAGLIST | NETIF_F_ALL_TSO |    \

@@ -436,6 +432,13 @@ static int veth_newlink(struct net *src_net, struct net_device *dev,
 
 	netif_carrier_off(dev);
 
+	err = veth_peer_sysfs_add(dev, peer);
+	if (err < 0)
+		goto err_configure_dev;
+	err = veth_peer_sysfs_add(peer, dev);
+	if (err < 0)
+		goto err_configure_dev;
+
 	/*
 	 * tie the deviced together
 	 */

@@ -447,9 +450,13 @@ static int veth_newlink(struct net *src_net, struct net_device *dev,
 	rcu_assign_pointer(priv->peer, dev);
 	return 0;
 
+err_configure_dev:
+	veth_peer_sysfs_del(dev);
+	veth_peer_sysfs_del(peer);
 err_register_dev:
 	/* nothing to do */
 err_configure_peer:
+	veth_peer_sysfs_del(dev);
 	unregister_netdevice(peer);
 	return err;
 

@@ -466,6 +473,9 @@ static void veth_dellink(struct net_device *dev, struct list_head *head)
 	priv = netdev_priv(dev);
 	peer = rtnl_dereference(priv->peer);
 
+	veth_peer_sysfs_del(dev);
+	if (peer) veth_peer_sysfs_del(dev);
+
 	/* Note : dellink() is called from default_device_exit_batch(),
 	 * before a rcu_synchronize() point. The devices are guaranteed
 	 * not being freed before one RCU grace period.

-- 
2.5.0

On Wed, 19 Aug 2015 08:44:08 +0200, Vincent Bernat wrote:

While the documentation doesn't say exactly what kind of relationship
iflink should represent, until a45253, only lower devices were
advertised this way. While veth cannot have a lower device, using iflink
to advertise the peer may create infinite loops in programs using iflink
to discover device topology.

Instead of advertising the peer link with iflink, a symbolic link "peer"
is added to each peer.

By removing veth_get_iflink, you're also stopping IFLA_LINK being
advertised in netlink messages, which consequently makes impossible to
reliably match veth peers across name spaces again. This would be a
huge step backwards.

 Jiri

-- 
Jiri Benc

 ❦ 19 août 2015 13:00 +0200, Jiri Benc [off-list ref] :

quoted

While the documentation doesn't say exactly what kind of relationship
iflink should represent, until a45253, only lower devices were
advertised this way. While veth cannot have a lower device, using iflink
to advertise the peer may create infinite loops in programs using iflink
to discover device topology.

Instead of advertising the peer link with iflink, a symbolic link "peer"
is added to each peer.

By removing veth_get_iflink, you're also stopping IFLA_LINK being
advertised in netlink messages, which consequently makes impossible to
reliably match veth peers across name spaces again. This would be a
huge step backwards.

That's the main goal of this patch: advertising the peer link as
IFLA_LINK attribute triggers an infinite loop in userland software when
they follow iflink to discover network devices topology. iflink has
always been the index of a lower device. If a sysfs symbolic link is not
good enough, I can propose a new IFLA_PEER attribute instead.
-- 
How apt the poor are to be proud.
		-- William Shakespeare, "Twelfth-Night"

On Wed, 19 Aug 2015 14:13:00 +0200, Vincent Bernat wrote:

That's the main goal of this patch: advertising the peer link as
IFLA_LINK attribute triggers an infinite loop in userland software when
they follow iflink to discover network devices topology. iflink has
always been the index of a lower device. If a sysfs symbolic link is not
good enough, I can propose a new IFLA_PEER attribute instead.

This would cause regression and break applications for those of us who
started relying on the netnsid feature to match interfaces across net
name spaces.

This is tough. If you're going to do such thing, you would at least
need to also introduce IFLA_PEER_NETNSID.

 Jiri

-- 
Jiri Benc

 ❦ 19 août 2015 14:38 +0200, Jiri Benc [off-list ref] :

quoted

That's the main goal of this patch: advertising the peer link as
IFLA_LINK attribute triggers an infinite loop in userland software when
they follow iflink to discover network devices topology. iflink has
always been the index of a lower device. If a sysfs symbolic link is not
good enough, I can propose a new IFLA_PEER attribute instead.

This would cause regression and break applications for those of us who
started relying on the netnsid feature to match interfaces across net
name spaces.

Yes. Unfortunately.

This is tough. If you're going to do such thing, you would at least
need to also introduce IFLA_PEER_NETNSID.

Yes I can.

In my opinion, the change of semantics of IFLA_LINK is a break of
API. However, I can live with it since it's easy to workaround it. It
just seemed easier to start the discussion with a patch.
-- 
Parenthesise to avoid ambiguity.
            - The Elements of Programming Style (Kernighan & Plauger)

Le 19/08/2015 14:48, Vincent Bernat a écrit :

  ❦ 19 août 2015 14:38 +0200, Jiri Benc [off-list ref] :

quoted

quoted

That's the main goal of this patch: advertising the peer link as
IFLA_LINK attribute triggers an infinite loop in userland software when
they follow iflink to discover network devices topology. iflink has
always been the index of a lower device. If a sysfs symbolic link is not
good enough, I can propose a new IFLA_PEER attribute instead.

This would cause regression and break applications for those of us who
started relying on the netnsid feature to match interfaces across net
name spaces.

Yes. Unfortunately.

quoted

This is tough. If you're going to do such thing, you would at least
need to also introduce IFLA_PEER_NETNSID.

Probably better to introduce veth netlink attribute then, something like
IFLA_VETH_PEER and keeps IFLA_LINK_NETNSID.

Yes I can.

In my opinion, the change of semantics of IFLA_LINK is a break of
API. However, I can live with it since it's easy to workaround it. It
just seemed easier to start the discussion with a patch.

I also don't know what is the best way to handle this. veth advertises
its peer via IFLA_LINK since 4.1, so it's too late to change it for this
release.

On Wed, 19 Aug 2015 18:33:14 +0200, Nicolas Dichtel wrote:

Probably better to introduce veth netlink attribute then, something like
IFLA_VETH_PEER and keeps IFLA_LINK_NETNSID.

I'd prefer IFLA_PEER. More generic attribute will be helpful should we
introduce an interface similar to veth in the future.

Also, I'd not combine IFLA_LINK_NETNSID with IFLA_PEER. There might
very well be an interface in the future that will need both IFLA_LINK and
IFLA_PEER and this would just create a confusion. It may be unlikely
but the attributes are cheap and it doesn't make sense to design uAPI
in a way that might bring problems in the future.

I also don't know what is the best way to handle this. veth advertises
its peer via IFLA_LINK since 4.1, so it's too late to change it for this
release.

Apparently we need to pick our poison. Either way, we break something.

 Jiri

-- 
Jiri Benc

Le 20/08/2015 13:53, Jiri Benc a écrit :

On Wed, 19 Aug 2015 18:33:14 +0200, Nicolas Dichtel wrote:

quoted

Probably better to introduce veth netlink attribute then, something like
IFLA_VETH_PEER and keeps IFLA_LINK_NETNSID.

I'd prefer IFLA_PEER. More generic attribute will be helpful should we
introduce an interface similar to veth in the future.s

Ok.

Also, I'd not combine IFLA_LINK_NETNSID with IFLA_PEER. There might
very well be an interface in the future that will need both IFLA_LINK and
IFLA_PEER and this would just create a confusion. It may be unlikely
but the attributes are cheap and it doesn't make sense to design uAPI
in a way that might bring problems in the future.

Ok, but then this IFLA_PEER can include the ifindex and the nsid. No need
to have two new attributes.

quoted

I also don't know what is the best way to handle this. veth advertises
its peer via IFLA_LINK since 4.1, so it's too late to change it for this
release.

Apparently we need to pick our poison. Either way, we break something.

Sure. I would prefer to have the same mechanism in all version, but I can
live with the other solution.

David, any thoughts about this?

From: Nicolas Dichtel <redacted>
Date: Thu, 20 Aug 2015 16:31:11 +0200

Le 20/08/2015 13:53, Jiri Benc a écrit :

quoted

On Wed, 19 Aug 2015 18:33:14 +0200, Nicolas Dichtel wrote:

quoted

Probably better to introduce veth netlink attribute then, something
like
IFLA_VETH_PEER and keeps IFLA_LINK_NETNSID.

I'd prefer IFLA_PEER. More generic attribute will be helpful should we
introduce an interface similar to veth in the future.s

Ok.

quoted

Also, I'd not combine IFLA_LINK_NETNSID with IFLA_PEER. There might
very well be an interface in the future that will need both IFLA_LINK
and
IFLA_PEER and this would just create a confusion. It may be unlikely
but the attributes are cheap and it doesn't make sense to design uAPI
in a way that might bring problems in the future.

Ok, but then this IFLA_PEER can include the ifindex and the nsid. No
need
to have two new attributes.

quoted

quoted

I also don't know what is the best way to handle this. veth advertises
its peer via IFLA_LINK since 4.1, so it's too late to change it for
this
release.

Apparently we need to pick our poison. Either way, we break something.

Sure. I would prefer to have the same mechanism in all version, but I
can
live with the other solution.

David, any thoughts about this?

You can't break the 4.1 semantics, it's in a released kernel and people
_ARE_ using it.

 ❦ 20 août 2015 14:07 -0700, David Miller [off-list ref] :

quoted

quoted

quoted

I also don't know what is the best way to handle this. veth advertises
its peer via IFLA_LINK since 4.1, so it's too late to change it for
this
release.

Apparently we need to pick our poison. Either way, we break something.

Sure. I would prefer to have the same mechanism in all version, but I
can live with the other solution.

David, any thoughts about this?

You can't break the 4.1 semantics, it's in a released kernel and people
_ARE_ using it.

I had a look at what other kind of daemons may exploit the pre-4.1
semantics (of not having an infinite loop when following iflink) and
failed to find any other users than "lldpd". Other LLDP daemons (lldpad,
ladvd, openlldpd) have other ways to find the lower interface. I would
also have thought that NetSNMP would use it to implement ifStackTable
but it doesn't in fact implement this table.
-- 
It were not best that we should all think alike; it is difference of opinion
that makes horse-races.
		-- Mark Twain, "Pudd'nhead Wilson's Calendar"