[PATCH net 00/12] net: iflink and link-netnsid fixes

[PATCH net 00/12] net: iflink and link-netnsid fixes

[Sabrina Dubroca]

In a lot of places, we use this kind of comparison to detect if a
device has a lower link:

  dev->ifindex != dev_get_iflink(dev)

This seems to be a leftover of the pre-netns days, when the ifindex
was unique over the whole system. Nowadays, with network namespaces,
it's very easy to create a device with the same ifindex as its lower
link:

    ip netns add main
    ip netns add peer
    ip -net main link add dummy0 type dummy
    ip -net main link add link dummy0 macvlan0 netns peer type macvlan
    ip -net main link show type dummy
        9: dummy0: <BROADCAST,NOARP> mtu 1500 qdisc noop ...
    ip -net peer link show type macvlan
        9: macvlan0@if9: <BROADCAST,MULTICAST> mtu 1500 qdisc noop ...

To detect if a device has a lower link, we can simply check the
existence of the dev->netdev_ops->ndo_get_iflink operation, instead of
checking its return value. In particular, I attempted to fix one of
these checks in commit feadc4b6cf42 ("rtnetlink: always put IFLA_LINK
for links with a link-netnsid"), but this patch isn't correct, since
tunnel devices can export IFLA_LINK_NETNSID without IFLA_LINK. That
patch needs to be reverted.

This series will fix all those bogus comparisons, and export missing
IFLA_LINK_NETNSID attributes in bridge and ipv6 dumps.

ipvlan and geneve are also missing the get_link_net operation, so
userspace can't know when those device are cross-netns. There are a
couple of other device types that have an ndo_get_iflink op but no
get_link_net (virt_wifi, ipoib), and should probably also have a
get_link_net.

Sabrina Dubroca (12):
  ipvlan: add get_link_net
  geneve: add get_link_net
  Revert "rtnetlink: always put IFLA_LINK for links with a link-netnsid"
  rtnetlink: always put IFLA_LINK for links with ndo_get_iflink
  bridge: always put IFLA_LINK for ports with ndo_get_iflink
  bridge: advertise IFLA_LINK_NETNSID when dumping bridge ports
  ipv6: always put IFLA_LINK for devices with ndo_get_iflink
  ipv6: advertise IFLA_LINK_NETNSID when dumping ipv6 addresses
  net: link_watch: fix operstate when the link has the same index as the
    device
  net: link_watch: fix detection of urgent events
  batman-adv: fix iflink detection in batadv_is_on_batman_iface
  batman-adv: fix detection of lower link in batadv_get_real_netdevice

 drivers/net/can/vxcan.c          |  2 +-
 drivers/net/geneve.c             |  8 ++++++++
 drivers/net/ipvlan/ipvlan_main.c |  9 +++++++++
 drivers/net/veth.c               |  2 +-
 include/net/rtnetlink.h          |  4 ++++
 net/batman-adv/hard-interface.c  |  4 ++--
 net/bridge/br_netlink.c          |  4 +++-
 net/core/link_watch.c            |  4 ++--
 net/core/rtnetlink.c             | 25 ++++++++++++-------------
 net/ipv6/addrconf.c              | 11 ++++++++++-
 10 files changed, 52 insertions(+), 21 deletions(-)

-- 
2.28.0

[PATCH net 11/12] batman-adv: fix iflink detection in batadv_is_on_batman_iface

[Sabrina Dubroca]

BATMAN compares ifindex with dev_get_iflink to detect devices that
don't have a parent, but that's wrong, since a device can have the
same index as its parent if it's created in a different network
namespace:

    ip netns add main
    ip netns add peer
    ip -net main link add dummy0 type dummy
    # keep ifindex in sync between the namespaces
    ip -net peer link add eatidx type dummy

    ip netns exec main batctl if add dummy0
    # macsec0 and bat0 have the same ifindex
    ip -net main link add link bat0 netns peer type macsec
    ip netns exec peer batctl if add macsec0

That last command would fail if we didn't keep the ifindex in sync
between the two namespaces, and should also fail when the macsec0
device has the same ifindex as its link. Let's use the presence of a
ndo_get_iflink operation, rather than the value it returns, to detect
a device without a link.

Fixes: b7eddd0b3950 ("batman-adv: prevent using any virtual device created on batman-adv as hard-interface")
Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
 net/batman-adv/hard-interface.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/batman-adv/hard-interface.c b/net/batman-adv/hard-interface.c
index fa06b51c0144..0d87c5d56844 100644
--- a/net/batman-adv/hard-interface.c
+++ b/net/batman-adv/hard-interface.c
@@ -159,7 +159,7 @@ static bool batadv_is_on_batman_iface(const struct net_device *net_dev)
 
 	/* no more parents..stop recursion */
 	if (dev_get_iflink(net_dev) == 0 ||
-	    dev_get_iflink(net_dev) == net_dev->ifindex)
+	    !(net_dev->netdev_ops && net_dev->netdev_ops->ndo_get_iflink))
 		return false;
 
 	parent_net = batadv_getlink_net(net_dev, net);
-- 
2.28.0

[PATCH net 12/12] batman-adv: fix detection of lower link in batadv_get_real_netdevice

[Sabrina Dubroca]

Currently, batadv_get_real_netdevice can return different results in
this situation:

    ip netns add main
    ip netns add peer
    ip -net main link add dummy1 type dummy
    ip -net main link add link dummy1 netns peer type macsec # same ifindex as dummy1
    ip -net main link add link dummy1 netns peer type macsec port 2

Let's use the presence of a ndo_get_iflink operation, rather than the
value it returns, to detect a device without a link.

Fixes: 5ed4a460a1d3 ("batman-adv: additional checks for virtual interfaces on top of WiFi")
Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
 net/batman-adv/hard-interface.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/batman-adv/hard-interface.c b/net/batman-adv/hard-interface.c
index 0d87c5d56844..8f7d2dd37321 100644
--- a/net/batman-adv/hard-interface.c
+++ b/net/batman-adv/hard-interface.c
@@ -223,7 +223,7 @@ static struct net_device *batadv_get_real_netdevice(struct net_device *netdev)
 	if (!netdev)
 		return NULL;
 
-	if (netdev->ifindex == dev_get_iflink(netdev)) {
+	if (!(netdev->netdev_ops && netdev->netdev_ops->ndo_get_iflink)) {
 		dev_hold(netdev);
 		return netdev;
 	}
-- 
2.28.0

Re: [PATCH net 00/12] net: iflink and link-netnsid fixes

[Stephen Hemminger]

On Thu,  1 Oct 2020 09:59:24 +0200
Sabrina Dubroca <sd@queasysnail.net> wrote:

> In a lot of places, we use this kind of comparison to detect if a
> device has a lower link:
> 
>   dev->ifindex != dev_get_iflink(dev)


Since this is a common operation, it would be good to add a new
helper function in netdevice.h

In your patch set, you are copying the same code snippet which
seems to indicate that it should be a helper.

Something like:

static inline bool netdev_has_link(const struct net_device *dev)
{
	const struct net_device_ops *ops = dev->netdev_ops;

	return ops && ops->ndo_get_iflink;
}

Re: [PATCH net 00/12] net: iflink and link-netnsid fixes

[Sabrina Dubroca]

2020-10-01, 14:25:38 -0700, Stephen Hemminger wrote:
> On Thu,  1 Oct 2020 09:59:24 +0200
> Sabrina Dubroca <sd@queasysnail.net> wrote:
> 
> > In a lot of places, we use this kind of comparison to detect if a
> > device has a lower link:
> > 
> >   dev->ifindex != dev_get_iflink(dev)
> 
> 
> Since this is a common operation, it would be good to add a new
> helper function in netdevice.h
> 
> In your patch set, you are copying the same code snippet which
> seems to indicate that it should be a helper.
> 
> Something like:
> 
> static inline bool netdev_has_link(const struct net_device *dev)
> {
> 	const struct net_device_ops *ops = dev->netdev_ops;
> 
> 	return ops && ops->ndo_get_iflink;
> }

Good idea, I'll add that in v2.

-- 
Sabrina

Re: [PATCH net 11/12] batman-adv: fix iflink detection in batadv_is_on_batman_iface

[Sven Eckelmann]

[-- Attachment #1: Type: text/plain, Size: 1500 bytes --]

On Thursday, 1 October 2020 09:59:35 CEST Sabrina Dubroca wrote:
> device has the same ifindex as its link. Let's use the presence of a
> ndo_get_iflink operation, rather than the value it returns, to detect
> a device without a link.

There wasn't any activity in this patchset since a while, it doesn't apply
anymore and the assumptions made here doesn't seem to be reflect the current
situation in the kernel. See commit 6c1f41afc1db ("batman-adv: Don't expect
inter-netns unique iflink indices"):

> But only checking for dev->netdev_ops->ndo_get_iflink is also not an option
> because ipoib_get_iflink implements it even when it sometimes returns an
> iflink != ifindex and sometimes iflink == ifindex. The caller must
> therefore make sure itself to check both netns and iflink + ifindex for
> equality. Only when they are equal, a "physical" interface was detected
> which should stop the traversal. On the other hand, vxcan_get_iflink can
> also return 0 in case there was currently no valid peer. In this case, it
> is still necessary to stop.

It would would be nice when the situation would be better but the proposed 
patches don't solve it. So I will mark the two patches as "Rejected" (from 
"Changes requested") in batadv's patchwork. It is not meant as sign of
disapproval of someone working in this area to improve the situation - I just
don't want to wait for the v2 [1] anymore.

Kind regards,
	Sven

[1] https://lore.kernel.org/all/20201002090703.GD3565727@bistromath.localdomain/

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]