There is no need to call dev_get_iflink multiple times for the same net_device in batadv_is_on_batman_iface. And since some of the .ndo_get_iflink callbacks are dynamic (for example via RCUs like in vxcan_get_iflink), it could easily happen that the returned values are not stable. The pre-checks before __dev_get_by_index are then of course bogus.
Fixes: 3d48811b27f5 ("batman-adv: prevent using any virtual device created on batman-adv as hard-interface") Signed-off-by: Sven Eckelmann sven@narfation.org --- net/batman-adv/hard-interface.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/net/batman-adv/hard-interface.c b/net/batman-adv/hard-interface.c index 59d19097..774a26c5 100644 --- a/net/batman-adv/hard-interface.c +++ b/net/batman-adv/hard-interface.c @@ -149,22 +149,23 @@ static bool batadv_is_on_batman_iface(const struct net_device *net_dev) struct net *net = dev_net(net_dev); struct net_device *parent_dev; struct net *parent_net; + int iflink; bool ret;
/* check if this is a batman-adv mesh interface */ if (batadv_softif_is_valid(net_dev)) return true;
+ iflink = dev_get_iflink(net_dev); + /* no more parents..stop recursion */ - if (dev_get_iflink(net_dev) == 0 || - dev_get_iflink(net_dev) == net_dev->ifindex) + if (iflink == 0 || iflink == net_dev->ifindex) return false;
parent_net = batadv_getlink_net(net_dev, net);
/* recurse over the parent device */ - parent_dev = __dev_get_by_index((struct net *)parent_net, - dev_get_iflink(net_dev)); + parent_dev = __dev_get_by_index((struct net *)parent_net, iflink); /* if we got a NULL parent_dev there is something broken.. */ if (!parent_dev) { pr_err("Cannot find parent device\n");
There is no need to call dev_get_iflink multiple times for the same net_device in batadv_get_real_netdevice. And since some of the ndo_get_iflink callbacks are dynamic (for example via RCUs like in vxcan_get_iflink), it could easily happen that the returned values are not stable. The pre-checks before __dev_get_by_index are then of course bogus.
Fixes: 2b45bb6c3aad ("batman-adv: additional checks for virtual interfaces on top of WiFi") Signed-off-by: Sven Eckelmann sven@narfation.org --- net/batman-adv/hard-interface.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/net/batman-adv/hard-interface.c b/net/batman-adv/hard-interface.c index 774a26c5..8ed128bb 100644 --- a/net/batman-adv/hard-interface.c +++ b/net/batman-adv/hard-interface.c @@ -215,14 +215,16 @@ static struct net_device *batadv_get_real_netdevice(struct net_device *netdev) struct net_device *real_netdev = NULL; struct net *real_net; struct net *net; - int ifindex; + int iflink;
ASSERT_RTNL();
if (!netdev) return NULL;
- if (netdev->ifindex == dev_get_iflink(netdev)) { + iflink = dev_get_iflink(netdev); + + if (netdev->ifindex == iflink) { dev_hold(netdev); return netdev; } @@ -232,9 +234,8 @@ static struct net_device *batadv_get_real_netdevice(struct net_device *netdev) goto out;
net = dev_net(hard_iface->soft_iface); - ifindex = dev_get_iflink(netdev); real_net = batadv_getlink_net(netdev, net); - real_netdev = dev_get_by_index(real_net, ifindex); + real_netdev = dev_get_by_index(real_net, iflink);
out: batadv_hardif_put(hard_iface);
The ifindex doesn't have to be unique for multiple network namespaces on the same machine.
$ ip netns add test1 $ ip -net test1 link add dummy1 type dummy $ ip netns add test2 $ ip -net test2 link add dummy2 type dummy
$ ip -net test1 link show dev dummy1 6: dummy1: <BROADCAST,NOARP> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000 link/ether 96:81:55:1e:dd:85 brd ff:ff:ff:ff:ff:ff $ ip -net test2 link show dev dummy2 6: dummy2: <BROADCAST,NOARP> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000 link/ether 5a:3c:af:35:07:c3 brd ff:ff:ff:ff:ff:ff
But the batman-adv code to walk through the various layers of virtual interfaces uses this assumption because dev_get_iflink handles it internally and doesn't return the actual netns of the iflink. And dev_get_iflink only documents the situation where ifindex == iflink for physical devices.
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, an "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.
Fixes: 3d48811b27f5 ("batman-adv: prevent using any virtual device created on batman-adv as hard-interface") Fixes: 2b45bb6c3aad ("batman-adv: additional checks for virtual interfaces on top of WiFi") Reported-by: Sabrina Dubroca sd@queasysnail.net Signed-off-by: Sven Eckelmann sven@narfation.org --- net/batman-adv/hard-interface.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-)
diff --git a/net/batman-adv/hard-interface.c b/net/batman-adv/hard-interface.c index 8ed128bb..cdb6e542 100644 --- a/net/batman-adv/hard-interface.c +++ b/net/batman-adv/hard-interface.c @@ -157,13 +157,15 @@ static bool batadv_is_on_batman_iface(const struct net_device *net_dev) return true;
iflink = dev_get_iflink(net_dev); - - /* no more parents..stop recursion */ - if (iflink == 0 || iflink == net_dev->ifindex) + if (iflink == 0) return false;
parent_net = batadv_getlink_net(net_dev, net);
+ /* iflink to itself, most likely physical device */ + if (net == parent_net && iflink == net_dev->ifindex) + return false; + /* recurse over the parent device */ parent_dev = __dev_get_by_index((struct net *)parent_net, iflink); /* if we got a NULL parent_dev there is something broken.. */ @@ -223,8 +225,7 @@ static struct net_device *batadv_get_real_netdevice(struct net_device *netdev) return NULL;
iflink = dev_get_iflink(netdev); - - if (netdev->ifindex == iflink) { + if (iflink == 0) { dev_hold(netdev); return netdev; } @@ -235,6 +236,14 @@ static struct net_device *batadv_get_real_netdevice(struct net_device *netdev)
net = dev_net(hard_iface->soft_iface); real_net = batadv_getlink_net(netdev, net); + + /* iflink to itself, most likely physical device */ + if (net == real_net && netdev->ifindex == iflink) { + real_netdev = netdev; + dev_hold(real_netdev); + goto out; + } + real_netdev = dev_get_by_index(real_net, iflink);
out:
The error message "Cannot find parent device" was shown for users of macvtap (on batadv devices) whenever the macvtap was moved to a different netns. This happens because macvtap doesn't provide
The situation for which this message is printed is actually not an error but just a warning that the optional sanity check was skipped. So demote the message from error to warning and adjust the text to better explain what happened.
Reported-by: Leonardo Mörlein freifunk@irrelefant.net Signed-off-by: Sven Eckelmann sven@narfation.org --- net/batman-adv/hard-interface.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/batman-adv/hard-interface.c b/net/batman-adv/hard-interface.c index cdb6e542..83fb51b6 100644 --- a/net/batman-adv/hard-interface.c +++ b/net/batman-adv/hard-interface.c @@ -168,9 +168,9 @@ static bool batadv_is_on_batman_iface(const struct net_device *net_dev)
/* recurse over the parent device */ parent_dev = __dev_get_by_index((struct net *)parent_net, iflink); - /* if we got a NULL parent_dev there is something broken.. */ if (!parent_dev) { - pr_err("Cannot find parent device\n"); + pr_warn("Cannot find parent device. Skipping batadv-on-batadv check for %s\n", + net_dev->name); return false; }
b.a.t.m.a.n@lists.open-mesh.org
-
Sven Eckelmann