[PATCH net-next v2] ip6_gre: fix changelink conflict checks for L2 tunnels

[PATCH net-next v2] ip6_gre: fix changelink conflict checks for L2 tunnels

[Xia Yan]

ip6gre_changelink_common() calls ip6gre_tunnel_locate(), which only
looks for ARPHRD_IP6GRE devices. The same changelink helper also handles
ip6gretap and ip6erspan devices.

This lets an L2 tunnel change its parameters to match another L2 tunnel,
since the duplicate check looks for an L3 device instead. Incoming
packets can then be delivered to the wrong interface. An L3 tunnel with
matching parameters can also make a valid L2 update fail with EEXIST.

Use ip6gre_tunnel_find() with dev->type, as the newlink paths do. Skip
collect_md entries while walking the bucket: they can coexist with
ordinary tunnels with the same lookup parameters, and their uniqueness
is checked through separate per-net pointers. Keep searching past them
so that a real duplicate later in the bucket is still rejected.

Found while comparing the duplicate checks in newlink and changelink.
Confirmed with ip link commands. For example, with an IPv6 underlay
named under already configured:

  ip link add tap1 type ip6gretap local fd26:106::1 \
      remote fd26:106::2 key 100 dev under
  ip link add tap2 type ip6gretap local fd26:106::1 \
      remote fd26:106::2 key 200 dev under
  ip link set tap1 up
  ip link set tap2 up
  ip link set tap2 type ip6gretap key 100

Before the fix, the last command succeeds and received frames move from
tap1 to tap2. After the fix, it fails with EEXIST and tap2 is unchanged.
The collect_md case was raised during v1 review and confirmed with
namespace tests. The reproducers use existing interfaces without kernel
changes or fault injection.

Tested under UML with standalone namespace tests for ip6gretap and
ip6erspan, including duplicate updates, L2/L3 coexistence, IPv4/IPv6
traffic and normal updates. Additional tests cover both ordinary and
metadata creation orders, all four L2 kind combinations, a real duplicate
behind metadata, and L3 ioctl changes. They reproduce the collect_md
failure with v1 and pass with this change.

Full x86_64 W=1 builds with allmodconfig- and allyesconfig-derived
configurations also pass, with unchanged warning sets.

Signed-off-by: Xia Yan <yanxia@cmss.chinamobile.com>
---
Changes in v2:
- Target net-next and drop the Fixes and stable tags, as requested by
  Ido Schimmel.
- Skip collect_md entries during lookup to avoid rejecting valid updates
  and to find ordinary duplicates behind metadata entries.
- Extend standalone UML validation to cover ordinary/metadata ordering,
  all four L2 kind combinations, and L3 lookup paths.

v1: https://lore.kernel.org/netdev/20261028115710.27806-1-yanxia@cmss.chinamobile.com/

Runtime and build validation used net-next 7b6ab4dbc156.
Kernel selftests were not run.

The allmodconfig- and allyesconfig-derived builds had WERROR, KASAN and
KCSAN off. The allyesconfig-derived build also had KUNIT_ALL_TESTS,
MCTP_TEST and MCTP_TRANSPORT_USBLIB_TEST off. The full default-to-derived
configuration diffs and the unchanged before/after warning sets are
retained in the validation archive.

 net/ipv6/ip6_gre.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
index b5d6ed8..07f7220 100644
--- a/net/ipv6/ip6_gre.c
+++ b/net/ipv6/ip6_gre.c
@@ -306,6 +306,9 @@ static struct ip6_tnl *ip6gre_tunnel_find(struct net *net,
 	head = __ip6gre_bucket(ign, parms);
 
 	hlist_for_each_entry(t, head, hash_node) {
+		if (t->parms.collect_md)
+			continue;
+
 		if (ipv6_addr_equal(local, &t->parms.laddr) &&
 		    ipv6_addr_equal(remote, &t->parms.raddr) &&
 		    key == t->parms.i_key &&
@@ -2055,7 +2058,7 @@ ip6gre_changelink_common(struct net_device *dev, struct nlattr *tb[],
 
 	ip6gre_netlink_parms(data, p_p);
 
-	t = ip6gre_tunnel_locate(net, p_p, 0);
+	t = ip6gre_tunnel_find(net, p_p, dev->type);
 
 	if (t) {
 		if (t->dev != dev)

base-commit: 7b6ab4dbc156e2f96419715130b3d5738a306c93
-- 
2.39.0.windows.2

Re: [PATCH net-next v2] ip6_gre: fix changelink conflict checks for L2 tunnels

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
ip6_gre: fix changelink conflict checks for L2 tunnels

This commit changes ip6gre_changelink_common() to use ip6gre_tunnel_find()
with dev->type instead of ip6gre_tunnel_locate(). That way, L2 tunnels are
checked against tunnels of their own type. It also makes
ip6gre_tunnel_find() skip collect_md entries while walking the bucket.

> diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
> index b5d6ed8f0c369..07f7220df938f 100644
> --- a/net/ipv6/ip6_gre.c
> +++ b/net/ipv6/ip6_gre.c
> @@ -306,6 +306,9 @@ static struct ip6_tnl *ip6gre_tunnel_find(struct net *net,
>  	head = __ip6gre_bucket(ign, parms);
>  
>  	hlist_for_each_entry(t, head, hash_node) {
> +		if (t->parms.collect_md)
> +			continue;
> +

[Severity: Medium]
Could skipping collect_md entries here reopen the misdelivery this commit
is meant to close, in the case where the other tunnel is collect_md?

ip6gre_tunnel_link() still links collect_md tunnels into the hash buckets
with hlist_add_head_rcu(). The receive walk in ip6gre_tunnel_lookup()
never checks collect_md:

	head = &ign->tunnels_wc[h1];
	hlist_for_each_entry_rcu(t, head, hash_node) {
		if (t->parms.i_key != key ||
		    !(t->dev->flags & IFF_UP))
			continue;

		if (ip6gre_tunnel_match(t, dev_type, link, &cand_score, &cand))
			return cand;
	}

When two entries score the same, the first one in the bucket wins. That
is normally the one linked most recently.

For example:

  ip link add md0 type ip6gretap external
  ip link add tap1 type ip6gretap

Before this patch, the second command failed with EEXIST because
ip6gre_tunnel_find(..., ARPHRD_ETHER) matched md0. Changing an existing
ordinary ip6gretap to zero params with changelink was rejected too,
because locate matched ip6gre0.

With this patch both succeed. tap1 ends up at the head of tunnels_wc[0]
and takes the keyless traffic md0 used to get, without the metadata dst.

The same looks possible for ip6erspan. It also looks possible for L3
ip6gre collect_md tunnels with explicit params, through
SIOCADDTUNNEL/SIOCCHGTUNNEL and ip6gre_tunnel_locate().

The commit message says:

  Skip collect_md entries while walking the bucket: they can coexist with
  ordinary tunnels with the same lookup parameters

That holds for the per-net collect_md_tun pointers. Does it also hold for
the receive demux in ip6gre_tunnel_lookup()?

Before this patch, that coexistence could only happen if the ordinary
tunnel was created first.

[Severity: Medium]
The subject and message describe a change to changelink only:

  ip6_gre: fix changelink conflict checks for L2 tunnels
  ...
  Use ip6gre_tunnel_find() with dev->type, as the newlink paths do.

The skip is in the shared helper, though. Doesn't it also change
ip6gre_newlink(), ip6erspan_newlink(), and every legacy ioctl that goes
through ip6gre_tunnel_locate()?

These are some differences users can see:

- newlink of an ordinary tunnel whose laddr/raddr/i_key/link/type match an
  existing collect_md tunnel used to fail with -EEXIST. It now succeeds.

- SIOCADDTUNNEL with params that match a collect_md ip6gre device used to
  return -ENOBUFS through this code in ip6gre_tunnel_locate():

	t = ip6gre_tunnel_find(net, parms, ARPHRD_IP6GRE);
	if (t && create)
		return NULL;

  It now registers a new ip6gre%d device.

- SIOCDELTUNNEL on the fallback device with params that match a
  collect_md device used to delete that device. It now returns -ENOENT,
  or -EPERM for zero params, because the lookup finds ip6gre0 itself.

- SIOCGETTUNNEL on the fallback device used to return the collect_md
  tunnel's parms. It now returns the fallback device's own parms.

This also differs from ip_tunnel_find() in net/ipv4/ip_tunnel.c, which
does not skip collect_md entries.

If only changelink should ignore collect_md entries, could the skip go at
that call site, or behind a parameter? If not, could the commit message
describe the newlink and ioctl changes?

[ ... ]

> @@ -2055,7 +2058,7 @@ ip6gre_changelink_common(struct net_device *dev, struct nlattr *tb[],
>  
>  	ip6gre_netlink_parms(data, p_p);
>  
> -	t = ip6gre_tunnel_locate(net, p_p, 0);
> +	t = ip6gre_tunnel_find(net, p_p, dev->type);
>  
>  	if (t) {
>  		if (t->dev != dev)

[Severity: High]
Can this return -EEXIST when a collect_md device changes its own
parameters?

ip6gre_tunnel_find() now skips every collect_md entry. A collect_md
device therefore no longer matches its own hash entry, and the walk moves
on to the next entries.

In every netns, the fallback ip6gre0 sits in tunnels_wc[0] with all-zero
parms and dev->type set to ARPHRD_IP6GRE:

net/ipv6/ip6_gre.c:ip6gre_init_net() {
    ...
	t = netdev_priv(ign->fb_tunnel_dev);
	hlist_add_head_rcu(&t->hash_node, &ign->tunnels_wc[0]);
    ...
}

ip6gre_newlink() never calls ip6gre_tunnel_find() for collect_md devices:

	if (nt->parms.collect_md) {
		if (rtnl_dereference(ign->collect_md_tun))
			err = -EEXIST;
	} else {
		if (ip6gre_tunnel_find(net, &nt->parms, dev->type))
			err = -EEXIST;
	}

So "ip link add md0 type ip6gre external" creates md0 with zero params
and links it into tunnels_wc[0] ahead of ip6gre0. A later
"ip link set md0 type ip6gre external ..." then goes:

ip6gre_changelink()
  ip6gre_changelink_common()
    ip6gre_tunnel_find(net, p_p, ARPHRD_IP6GRE)
      skips md0, matches ip6gre0
    t->dev != dev -> ERR_PTR(-EEXIST)

Before this patch, ip6gre_tunnel_locate() found md0 first, so
t->dev == dev and the update succeeded.

SIOCCHGTUNNEL on md0 looks affected the same way. In
ip6gre_tunnel_siocdevprivate(), ip6gre_tunnel_locate(net, &p1, 0) now
returns the fallback tunnel, and the t->dev != dev branch sets -EEXIST.

The L2 kinds can hit this too, when a collect_md ip6gretap or ip6erspan
device shares lookup params with an ordinary L2 tunnel. newlink allows
that.

Would it work to skip the ordinary duplicate search when
nt->parms.collect_md is set, as newlink does? Another option is to accept
a match on the device's own entry.
-- 
pw-bot: cr