Re: [PATCH net-next 05/13] amt: match the Membership Update tunnel by outer family

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message says "an IPv6 relay never accepts a Membership
  Update", as if this were a live, user-visible failure.

Pre-existing issues:
- [High] amt_update_handler() can re-arm the delayed work of a tunnel
  that amt_tunnel_expire() is already freeing, leading to use-after-free…

--

amt: match the Membership Update tunnel by outer family

This patch changes amt_update_handler() to read the outer source address
with amt_outer_saddr() instead of ip_hdr(skb)->saddr. It also matches the
tunnel with amt_addr_equal(), as amt_request_handler() does, so the lookup
works for both outer address families.

amt_update_handler() finds the gateway's tunnel by comparing
tunnel->addr.ip4 with ip_hdr(skb)->saddr, which on an IPv6 outer header
reads bytes 4-7 of the source address as an IPv4 address, so an IPv6
relay never accepts a Membership Update.

[Severity: Low]
Can an IPv6 relay exist at this point in the series? amt_v6() is:

static bool amt_v6(const struct amt_dev *amt)
{
	return IS_ENABLED(CONFIG_IPV6) && !ipv6_addr_any(&amt->local_ipv6);
}

Nothing assigns amt->local_ipv6 at this commit. The first assignment is
in amt_newlink(), added later by "amt: add netlink attributes for an IPv6
outer transport".

Until then, amt_outer_saddr() always takes the IPv4 path. That makes
amt_addr_equal() in amt_update_handler() give the same result as the old
tunnel->addr.ip4 == saddr comparison.

This isn't a bug, but could the commit message say that nothing changes
until the netlink attribute is added? The first patch of the series does
this.

[ ... ]

[Severity: High]
This is a pre-existing issue and this patch did not introduce it. Can
this mod_delayed_work() re-arm the gc_wq of a tunnel that
amt_tunnel_expire() is already freeing?

amt_update_handler() is called from amt_rcv() under rcu_read_lock_bh().
It does not hold amt->lock or tunnel->lock, and it does not check whether
the tunnel is being torn down.

amt_tunnel_expire() runs from that same gc_wq:

drivers/net/amt.c:amt_tunnel_expire() {
	spin_lock_bh(&amt->lock);
	rcu_read_lock();
	list_del_rcu(&tunnel->list);
	amt->nr_tunnels--;
	amt_clear_groups(tunnel);
	rcu_read_unlock();
	spin_unlock_bh(&amt->lock);
	kfree_rcu(tunnel, rcu);
}

Consider this ordering for a tunnel T:

CPU1 (amt_wq)
amt_tunnel_expire(T) starts, T->gc_wq is no longer pending

CPU2 (amt_rcv, rcu_read_lock_bh held)
amt_update_handler()
    list_for_each_entry_rcu() finds T before list_del_rcu()
    mod_delayed_work(amt_wq, &T->gc_wq, ...) arms T->gc_wq.timer

CPU1
    list_del_rcu(&T->list);
    kfree_rcu(T, rcu);

After CPU2 leaves its RCU section, the grace period ends and T is freed.
T->gc_wq.timer is still queued at that point.

When the timer fires, amt_tunnel_expire() runs on the freed tunnel. It
calls list_del_rcu() a second time, decrements nr_tunnels again, and
calls kfree_rcu() a second time.

The report: path has a similar problem. It takes tunnel->lock and calls
amt_igmp_report_handler() or amt_mld_report_handler(), which can add
group nodes to T after amt_clear_groups() has already run. Wouldn't those
nodes be left with live timers that still reference T after it is freed?

A remote gateway can trigger this. It only needs a valid nonce and
response MAC, and the timing is predictable because gc_wq fires 3 * GMI
after the last Update.

IPv4 relays could already hit this. Once the series is complete, this
patch makes the same path reachable for IPv6 relays. The code is still
the same at the end of the series.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261009122426.551178-1-omar%40blockcast.net