diff mbox series

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

Message ID afa206858a88910691bdb917d0956cea3f32f667.1600770261.git.sd@queasysnail.net (mailing list archive)
State Rejected, archived
Delegated to: Simon Wunderlich
Headers
Series net: iflink and link-netnsid fixes |

Commit Message

Sabrina Dubroca Oct. 1, 2020, 7:59 a.m. UTC 
  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(-)

Comments

Sven Eckelmann May 14, 2022, 10:21 a.m. UTC | #1 
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/
diff mbox series

Patch

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