Re: [PATCH net 2/2] ip6_gre: Use cached t->net in ip6erspan_changelink().

[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: Use cached t->net in ip6erspan_changelink().

This commit fixes an issue where ip6erspan_changelink() uses the device
creation netns instead of the per-netns ip6gre hash. This ensures the tunnel
is inserted into the correct hash, avoiding a stale entry and subsequent
use-after-free during netns destruction.

> diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
> index 63fc8556b475..c5978e0bdfe3 100644
> --- a/net/ipv6/ip6_gre.c
> +++ b/net/ipv6/ip6_gre.c
> @@ -2262,7 +2262,8 @@ static int ip6erspan_changelink(struct net_device *dev, struct nlattr *tb[],
>  				struct nlattr *data[],
>  				struct netlink_ext_ack *extack)
>  {
> -	struct ip6gre_net *ign = net_generic(dev_net(dev), ip6gre_net_id);
> +	struct ip6_tnl *nt = netdev_priv(dev);
> +	struct ip6gre_net *ign = net_generic(nt->net, ip6gre_net_id);
>  	struct __ip6_tnl_parm p;
>  	struct ip6_tnl *t;

While reviewing this area, I noticed a regression further down in
ip6erspan_changelink() regarding the metadata tunnel unlinking.

If an existing ERSPAN tunnel with metadata collection enabled is modified via
a netlink changelink request to disable metadata collection, does the unlink
sequence clear the correct pointer?

Looking at the rest of ip6erspan_changelink():

    ip6gre_tunnel_unlink_md(ign, t);
    ip6gre_tunnel_unlink(ign, t);
    ip6erspan_tnl_change(t, &p, !tb[IFLA_MTU]);
    ip6erspan_tunnel_link_md(ign, t);

Because ip6gre_tunnel_unlink_md() is called instead of
ip6erspan_tunnel_unlink_md(), it evaluates the old collect_md state but
clears ign->collect_md_tun instead of ign->collect_md_tun_erspan.

This seems to leave ign->collect_md_tun_erspan pointing to the tunnel.

Since ip6erspan_tnl_change() updates the parameters to disable collect_md,
the subsequent call to ip6erspan_tunnel_link_md() will skip updating the
pointer.

When the tunnel is eventually deleted, ip6erspan_tunnel_unlink_md() would
be bypassed entirely because collect_md is now false.

Could this leave ign->collect_md_tun_erspan as a dangling pointer,
causing a use-after-free when an incoming ERSPAN packet triggers
ip6gre_tunnel_lookup() and dereferences it?