* Re: [PATCH net-next v2] ip6_gre: fix changelink conflict checks for L2 tunnels
2026-11-04 15:10 [PATCH net-next v2] ip6_gre: fix changelink conflict checks for L2 tunnels Xia Yan
@ 2026-11-10 20:58 ` Jakub Kicinski
0 siblings, 0 replies; 2+ messages in thread
From: Jakub Kicinski @ 2026-11-10 20:58 UTC (permalink / raw)
To: yanxia
Cc: Jakub Kicinski, netdev, dsahern, idosch, davem, edumazet, pabeni,
horms, linux-kernel, kuniyu, xeb, yana06070, netdev-bot+sashiko
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
^ permalink raw reply [flat|nested] 2+ messages in thread