Re: [PATCH net] net, ip_tunnel: fix interface lookup with no key

[William Dauchy <w.dauchy@criteo.com>]

Hello Nicolas,

Thanks for your review.

On Thu, Mar 26, 2020 at 07:01:20PM +0100, Nicolas Dichtel wrote:
> Hmm, removing this test may break some existing scenario. This flag is part of
> the UAPI (for gre). Suppose that a tool configures a gre tunnel, leaves the key
> uninitialized and set this flag. After this patch, the lookup may return
> something else.

Indeed I was not sure about possible other impacts, as it seemed to not
break iproute2 tooling, but it's true it could break other tools.
But if we consider to remove the key test in that case, would it be
acceptable to have something like:

diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
index 74e1d964a615..8bdb9856d4c4 100644
--- a/net/ipv4/ip_tunnel.c
+++ b/net/ipv4/ip_tunnel.c
@@ -142,23 +142,32 @@ struct ip_tunnel *ip_tunnel_lookup(struct ip_tunnel_net *itn,
    cand = t;
  }

- if (flags & TUNNEL_NO_KEY)
-  goto skip_key_lookup;
-
- hlist_for_each_entry_rcu(t, head, hash_node) {
-  if (t->parms.i_key != key ||
-      t->parms.iph.saddr != 0 ||
-      t->parms.iph.daddr != 0 ||
-      !(t->dev->flags & IFF_UP))
-   continue;
+ if (flags & TUNNEL_NO_KEY) {
+  hlist_for_each_entry_rcu(t, head, hash_node) {
+   if (t->parms.iph.saddr != 0 ||
+       t->parms.iph.daddr != 0 ||
+       !(t->dev->flags & IFF_UP))
+    continue;

   if (t->parms.link == link)
    return t;
   else if (!cand)
    cand = t;
- }
+ } else {
+
+  hlist_for_each_entry_rcu(t, head, hash_node) {
+   if (t->parms.i_key != key ||
+       t->parms.iph.saddr != 0 ||
+       t->parms.iph.daddr != 0 ||
+       !(t->dev->flags & IFF_UP))
+    continue;
+
+   if (t->parms.link == link)
+    return t;
+   else if (!cand)
+    cand = t;
+  }

-skip_key_lookup:
  if (cand)
   return cand;



-- 
William