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

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

[William Dauchy]

when creating a new ipip interface with no local/remote configuration,
the lookup is done with TUNNEL_NO_KEY flag, making it impossible to
match the new interface (only possible match being fallback or metada
case interface); e.g: `ip link add tunl1 type ipip dev eth0`

If we consider `key` being zero in ipip case, we might consider ok to
go through this last loop, and make it possible to match such interface.
In fact this is what is done when we create a gre interface without key
and local/remote.

context being on my side, I'm creating an extra ipip interface attached
to the physical one, and moving it to a dedicated namespace.

Fixes: c54419321455 ("GRE: Refactor GRE tunneling code.")
Signed-off-by: William Dauchy <w.dauchy@criteo.com>
---
 net/ipv4/ip_tunnel.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
index 74e1d964a615..f6578bcadbed 100644
--- a/net/ipv4/ip_tunnel.c
+++ b/net/ipv4/ip_tunnel.c
@@ -142,9 +142,6 @@ 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 ||
-- 
2.25.1

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

[William Dauchy]

On Wed, Mar 25, 2020 at 4:03 PM William Dauchy <w.dauchy@criteo.com> wrote:
> -       if (flags & TUNNEL_NO_KEY)
> -               goto skip_key_lookup;

I later realised I would also need to remove the `skip_key_lookup`
below, but waiting for feedback to see whether this is a good
approach.

Thanks,
-- 
William

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

[kbuild test robot]

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

Hi William,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on net/master]
[also build test WARNING on linus/master v5.6-rc7 next-20200325]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/William-Dauchy/net-ip_tunnel-fix-interface-lookup-with-no-key/20200326-101601
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git 2910594fd38d1cb3c32fbf235e6c6228c780ab87
config: x86_64-defconfig (attached as .config)
compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project 4b428e8f18c7006f69b3d4ef0fdf091d998d0941)
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> net/ipv4/ip_tunnel.c:158:1: warning: unused label 'skip_key_lookup' [-Wunused-label]
   skip_key_lookup:
   ^~~~~~~~~~~~~~~~
   1 warning generated.

vim +/skip_key_lookup +158 net/ipv4/ip_tunnel.c

c5441932145563 Pravin B Shelar 2013-03-25   73  
c5441932145563 Pravin B Shelar 2013-03-25   74     Tunnel hash table:
c5441932145563 Pravin B Shelar 2013-03-25   75     We require exact key match i.e. if a key is present in packet
c5441932145563 Pravin B Shelar 2013-03-25   76     it will match only tunnel with the same key; if it is not present,
c5441932145563 Pravin B Shelar 2013-03-25   77     it will match only keyless tunnel.
c5441932145563 Pravin B Shelar 2013-03-25   78  
c5441932145563 Pravin B Shelar 2013-03-25   79     All keysless packets, if not matched configured keyless tunnels
c5441932145563 Pravin B Shelar 2013-03-25   80     will match fallback tunnel.
c5441932145563 Pravin B Shelar 2013-03-25   81     Given src, dst and key, find appropriate for input tunnel.
c5441932145563 Pravin B Shelar 2013-03-25   82  */
c5441932145563 Pravin B Shelar 2013-03-25   83  struct ip_tunnel *ip_tunnel_lookup(struct ip_tunnel_net *itn,
c5441932145563 Pravin B Shelar 2013-03-25   84  				   int link, __be16 flags,
c5441932145563 Pravin B Shelar 2013-03-25   85  				   __be32 remote, __be32 local,
c5441932145563 Pravin B Shelar 2013-03-25   86  				   __be32 key)
c5441932145563 Pravin B Shelar 2013-03-25   87  {
c5441932145563 Pravin B Shelar 2013-03-25   88  	unsigned int hash;
c5441932145563 Pravin B Shelar 2013-03-25   89  	struct ip_tunnel *t, *cand = NULL;
c5441932145563 Pravin B Shelar 2013-03-25   90  	struct hlist_head *head;
c5441932145563 Pravin B Shelar 2013-03-25   91  
967680e02724af Duan Jiong      2014-01-19   92  	hash = ip_tunnel_hash(key, remote);
c5441932145563 Pravin B Shelar 2013-03-25   93  	head = &itn->tunnels[hash];
c5441932145563 Pravin B Shelar 2013-03-25   94  
c5441932145563 Pravin B Shelar 2013-03-25   95  	hlist_for_each_entry_rcu(t, head, hash_node) {
c5441932145563 Pravin B Shelar 2013-03-25   96  		if (local != t->parms.iph.saddr ||
c5441932145563 Pravin B Shelar 2013-03-25   97  		    remote != t->parms.iph.daddr ||
c5441932145563 Pravin B Shelar 2013-03-25   98  		    !(t->dev->flags & IFF_UP))
c5441932145563 Pravin B Shelar 2013-03-25   99  			continue;
c5441932145563 Pravin B Shelar 2013-03-25  100  
c5441932145563 Pravin B Shelar 2013-03-25  101  		if (!ip_tunnel_key_match(&t->parms, flags, key))
c5441932145563 Pravin B Shelar 2013-03-25  102  			continue;
c5441932145563 Pravin B Shelar 2013-03-25  103  
c5441932145563 Pravin B Shelar 2013-03-25  104  		if (t->parms.link == link)
c5441932145563 Pravin B Shelar 2013-03-25  105  			return t;
c5441932145563 Pravin B Shelar 2013-03-25  106  		else
c5441932145563 Pravin B Shelar 2013-03-25  107  			cand = t;
c5441932145563 Pravin B Shelar 2013-03-25  108  	}
c5441932145563 Pravin B Shelar 2013-03-25  109  
c5441932145563 Pravin B Shelar 2013-03-25  110  	hlist_for_each_entry_rcu(t, head, hash_node) {
c5441932145563 Pravin B Shelar 2013-03-25  111  		if (remote != t->parms.iph.daddr ||
e0056593b61253 Dmitry Popov    2014-07-05  112  		    t->parms.iph.saddr != 0 ||
c5441932145563 Pravin B Shelar 2013-03-25  113  		    !(t->dev->flags & IFF_UP))
c5441932145563 Pravin B Shelar 2013-03-25  114  			continue;
c5441932145563 Pravin B Shelar 2013-03-25  115  
c5441932145563 Pravin B Shelar 2013-03-25  116  		if (!ip_tunnel_key_match(&t->parms, flags, key))
c5441932145563 Pravin B Shelar 2013-03-25  117  			continue;
c5441932145563 Pravin B Shelar 2013-03-25  118  
c5441932145563 Pravin B Shelar 2013-03-25  119  		if (t->parms.link == link)
c5441932145563 Pravin B Shelar 2013-03-25  120  			return t;
c5441932145563 Pravin B Shelar 2013-03-25  121  		else if (!cand)
c5441932145563 Pravin B Shelar 2013-03-25  122  			cand = t;
c5441932145563 Pravin B Shelar 2013-03-25  123  	}
c5441932145563 Pravin B Shelar 2013-03-25  124  
967680e02724af Duan Jiong      2014-01-19  125  	hash = ip_tunnel_hash(key, 0);
c5441932145563 Pravin B Shelar 2013-03-25  126  	head = &itn->tunnels[hash];
c5441932145563 Pravin B Shelar 2013-03-25  127  
c5441932145563 Pravin B Shelar 2013-03-25  128  	hlist_for_each_entry_rcu(t, head, hash_node) {
e0056593b61253 Dmitry Popov    2014-07-05  129  		if ((local != t->parms.iph.saddr || t->parms.iph.daddr != 0) &&
e0056593b61253 Dmitry Popov    2014-07-05  130  		    (local != t->parms.iph.daddr || !ipv4_is_multicast(local)))
e0056593b61253 Dmitry Popov    2014-07-05  131  			continue;
e0056593b61253 Dmitry Popov    2014-07-05  132  
e0056593b61253 Dmitry Popov    2014-07-05  133  		if (!(t->dev->flags & IFF_UP))
c5441932145563 Pravin B Shelar 2013-03-25  134  			continue;
c5441932145563 Pravin B Shelar 2013-03-25  135  
c5441932145563 Pravin B Shelar 2013-03-25  136  		if (!ip_tunnel_key_match(&t->parms, flags, key))
c5441932145563 Pravin B Shelar 2013-03-25  137  			continue;
c5441932145563 Pravin B Shelar 2013-03-25  138  
c5441932145563 Pravin B Shelar 2013-03-25  139  		if (t->parms.link == link)
c5441932145563 Pravin B Shelar 2013-03-25  140  			return t;
c5441932145563 Pravin B Shelar 2013-03-25  141  		else if (!cand)
c5441932145563 Pravin B Shelar 2013-03-25  142  			cand = t;
c5441932145563 Pravin B Shelar 2013-03-25  143  	}
c5441932145563 Pravin B Shelar 2013-03-25  144  
c5441932145563 Pravin B Shelar 2013-03-25  145  	hlist_for_each_entry_rcu(t, head, hash_node) {
c5441932145563 Pravin B Shelar 2013-03-25  146  		if (t->parms.i_key != key ||
e0056593b61253 Dmitry Popov    2014-07-05  147  		    t->parms.iph.saddr != 0 ||
e0056593b61253 Dmitry Popov    2014-07-05  148  		    t->parms.iph.daddr != 0 ||
c5441932145563 Pravin B Shelar 2013-03-25  149  		    !(t->dev->flags & IFF_UP))
c5441932145563 Pravin B Shelar 2013-03-25  150  			continue;
c5441932145563 Pravin B Shelar 2013-03-25  151  
c5441932145563 Pravin B Shelar 2013-03-25  152  		if (t->parms.link == link)
c5441932145563 Pravin B Shelar 2013-03-25  153  			return t;
c5441932145563 Pravin B Shelar 2013-03-25  154  		else if (!cand)
c5441932145563 Pravin B Shelar 2013-03-25  155  			cand = t;
c5441932145563 Pravin B Shelar 2013-03-25  156  	}
c5441932145563 Pravin B Shelar 2013-03-25  157  
c5441932145563 Pravin B Shelar 2013-03-25 @158  skip_key_lookup:
c5441932145563 Pravin B Shelar 2013-03-25  159  	if (cand)
c5441932145563 Pravin B Shelar 2013-03-25  160  		return cand;
c5441932145563 Pravin B Shelar 2013-03-25  161  
2e15ea390e6f44 Pravin B Shelar 2015-08-07  162  	t = rcu_dereference(itn->collect_md_tun);
833a8b405465e9 Haishuang Yan   2017-09-12  163  	if (t && t->dev->flags & IFF_UP)
2e15ea390e6f44 Pravin B Shelar 2015-08-07  164  		return t;
2e15ea390e6f44 Pravin B Shelar 2015-08-07  165  
c5441932145563 Pravin B Shelar 2013-03-25  166  	if (itn->fb_tunnel_dev && itn->fb_tunnel_dev->flags & IFF_UP)
c5441932145563 Pravin B Shelar 2013-03-25  167  		return netdev_priv(itn->fb_tunnel_dev);
c5441932145563 Pravin B Shelar 2013-03-25  168  
c5441932145563 Pravin B Shelar 2013-03-25  169  	return NULL;
c5441932145563 Pravin B Shelar 2013-03-25  170  }
c5441932145563 Pravin B Shelar 2013-03-25  171  EXPORT_SYMBOL_GPL(ip_tunnel_lookup);
c5441932145563 Pravin B Shelar 2013-03-25  172  

:::::: The code at line 158 was first introduced by commit
:::::: c54419321455631079c7d6e60bc732dd0c5914c5 GRE: Refactor GRE tunneling code.

:::::: TO: Pravin B Shelar <pshelar@nicira.com>
:::::: CC: David S. Miller <davem@davemloft.net>

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 29216 bytes --]

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

[Nicolas Dichtel]

Le 25/03/2020 à 16:03, William Dauchy a écrit :
> when creating a new ipip interface with no local/remote configuration,
> the lookup is done with TUNNEL_NO_KEY flag, making it impossible to
> match the new interface (only possible match being fallback or metada
> case interface); e.g: `ip link add tunl1 type ipip dev eth0`
> 
> If we consider `key` being zero in ipip case, we might consider ok to
> go through this last loop, and make it possible to match such interface.
> In fact this is what is done when we create a gre interface without key
> and local/remote.
> 
> context being on my side, I'm creating an extra ipip interface attached
> to the physical one, and moving it to a dedicated namespace.
> 
> Fixes: c54419321455 ("GRE: Refactor GRE tunneling code.")
> Signed-off-by: William Dauchy <w.dauchy@criteo.com>
> ---
>  net/ipv4/ip_tunnel.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
> index 74e1d964a615..f6578bcadbed 100644
> --- a/net/ipv4/ip_tunnel.c
> +++ b/net/ipv4/ip_tunnel.c
> @@ -142,9 +142,6 @@ struct ip_tunnel *ip_tunnel_lookup(struct ip_tunnel_net *itn,
>  			cand = t;
>  	}
>  
> -	if (flags & TUNNEL_NO_KEY)
> -		goto skip_key_lookup;
> -
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.


Regards,
Nicolas

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

[William Dauchy]

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

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

[Nicolas Dichtel]

Le 26/03/2020 à 19:56, William Dauchy a écrit :
> 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;
> 
Maybe less code duplication? Something like:

diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
index 74e1d964a615..cd4b84310d92 100644
--- a/net/ipv4/ip_tunnel.c
+++ b/net/ipv4/ip_tunnel.c
@@ -142,11 +142,8 @@ 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 ||
+		if ((!(flags & TUNNEL_NO_KEY) && t->parms.i_key != key) ||
 		    t->parms.iph.saddr != 0 ||
 		    t->parms.iph.daddr != 0 ||
 		    !(t->dev->flags & IFF_UP))
@@ -158,7 +155,6 @@ struct ip_tunnel *ip_tunnel_lookup(struct ip_tunnel_net *itn,
 			cand = t;
 	}

-skip_key_lookup:
 	if (cand)
 		return cand;

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

[William Dauchy]

when creating a new ipip interface with no local/remote configuration,
the lookup is done with TUNNEL_NO_KEY flag, making it impossible to
match the new interface (only possible match being fallback or metada
case interface); e.g: `ip link add tunl1 type ipip dev eth0`

To fix this case, adding a flag check before the key comparison so we
permit to match an interface with no local/remote config; it also avoids
breaking possible userland tools relying on TUNNEL_NO_KEY flag and
uninitialised key.

context being on my side, I'm creating an extra ipip interface attached
to the physical one, and moving it to a dedicated namespace.

Fixes: c54419321455 ("GRE: Refactor GRE tunneling code.")
Signed-off-by: William Dauchy <w.dauchy@criteo.com>
---
 net/ipv4/ip_tunnel.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
index 74e1d964a615..b30485615426 100644
--- a/net/ipv4/ip_tunnel.c
+++ b/net/ipv4/ip_tunnel.c
@@ -142,11 +142,8 @@ 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 ||
+		if ((flags & TUNNEL_KEY && t->parms.i_key != key) ||
 		    t->parms.iph.saddr != 0 ||
 		    t->parms.iph.daddr != 0 ||
 		    !(t->dev->flags & IFF_UP))
@@ -158,7 +155,6 @@ struct ip_tunnel *ip_tunnel_lookup(struct ip_tunnel_net *itn,
 			cand = t;
 	}
 
-skip_key_lookup:
 	if (cand)
 		return cand;
 
-- 
2.25.1

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

[Nicolas Dichtel]

Le 27/03/2020 à 15:54, William Dauchy a écrit :
> when creating a new ipip interface with no local/remote configuration,
> the lookup is done with TUNNEL_NO_KEY flag, making it impossible to
> match the new interface (only possible match being fallback or metada
> case interface); e.g: `ip link add tunl1 type ipip dev eth0`
> 
> To fix this case, adding a flag check before the key comparison so we
> permit to match an interface with no local/remote config; it also avoids
> breaking possible userland tools relying on TUNNEL_NO_KEY flag and
> uninitialised key.
> 
> context being on my side, I'm creating an extra ipip interface attached
> to the physical one, and moving it to a dedicated namespace.
> 
> Fixes: c54419321455 ("GRE: Refactor GRE tunneling code.")
> Signed-off-by: William Dauchy <w.dauchy@criteo.com>
> ---
>  net/ipv4/ip_tunnel.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
> index 74e1d964a615..b30485615426 100644
> --- a/net/ipv4/ip_tunnel.c
> +++ b/net/ipv4/ip_tunnel.c
> @@ -142,11 +142,8 @@ 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 ||
> +		if ((flags & TUNNEL_KEY && t->parms.i_key != key) ||
I saw this flag, and frankly, having a flag named TUNNEL_KEY and another named
TUNNEL_NO_KEY is an horrible API design.

But I prefer to be conservative and avoid (ugly) surprise, thus I think it's
better to keep TUNNEL_NO_KEY.


Regards,
Nicolas

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

[William Dauchy]

when creating a new ipip interface with no local/remote configuration,
the lookup is done with TUNNEL_NO_KEY flag, making it impossible to
match the new interface (only possible match being fallback or metada
case interface); e.g: `ip link add tunl1 type ipip dev eth0`

To fix this case, adding a flag check before the key comparison so we
permit to match an interface with no local/remote config; it also avoids
breaking possible userland tools relying on TUNNEL_NO_KEY flag and
uninitialised key.

context being on my side, I'm creating an extra ipip interface attached
to the physical one, and moving it to a dedicated namespace.

Fixes: c54419321455 ("GRE: Refactor GRE tunneling code.")
Signed-off-by: William Dauchy <w.dauchy@criteo.com>
---
 net/ipv4/ip_tunnel.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
index 74e1d964a615..cd4b84310d92 100644
--- a/net/ipv4/ip_tunnel.c
+++ b/net/ipv4/ip_tunnel.c
@@ -142,11 +142,8 @@ 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 ||
+		if ((!(flags & TUNNEL_NO_KEY) && t->parms.i_key != key) ||
 		    t->parms.iph.saddr != 0 ||
 		    t->parms.iph.daddr != 0 ||
 		    !(t->dev->flags & IFF_UP))
@@ -158,7 +155,6 @@ struct ip_tunnel *ip_tunnel_lookup(struct ip_tunnel_net *itn,
 			cand = t;
 	}
 
-skip_key_lookup:
 	if (cand)
 		return cand;
 
-- 
2.25.1

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

[Nicolas Dichtel]

Le 27/03/2020 à 19:56, William Dauchy a écrit :
> when creating a new ipip interface with no local/remote configuration,
> the lookup is done with TUNNEL_NO_KEY flag, making it impossible to
> match the new interface (only possible match being fallback or metada
> case interface); e.g: `ip link add tunl1 type ipip dev eth0`
> 
> To fix this case, adding a flag check before the key comparison so we
> permit to match an interface with no local/remote config; it also avoids
> breaking possible userland tools relying on TUNNEL_NO_KEY flag and
> uninitialised key.
> 
> context being on my side, I'm creating an extra ipip interface attached
> to the physical one, and moving it to a dedicated namespace.
> 
> Fixes: c54419321455 ("GRE: Refactor GRE tunneling code.")
> Signed-off-by: William Dauchy <w.dauchy@criteo.com>
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>

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

[David Miller]

From: William Dauchy <w.dauchy@criteo.com>
Date: Fri, 27 Mar 2020 19:56:39 +0100

> when creating a new ipip interface with no local/remote configuration,
> the lookup is done with TUNNEL_NO_KEY flag, making it impossible to
> match the new interface (only possible match being fallback or metada
> case interface); e.g: `ip link add tunl1 type ipip dev eth0`
> 
> To fix this case, adding a flag check before the key comparison so we
> permit to match an interface with no local/remote config; it also avoids
> breaking possible userland tools relying on TUNNEL_NO_KEY flag and
> uninitialised key.
> 
> context being on my side, I'm creating an extra ipip interface attached
> to the physical one, and moving it to a dedicated namespace.
> 
> Fixes: c54419321455 ("GRE: Refactor GRE tunneling code.")
> Signed-off-by: William Dauchy <w.dauchy@criteo.com>

Applied and queued up for -stable, thanks.