Re: [PATCH net v3 1/6] amt: fix use-after-free when a Membership Update re-arms a freed tunnel

[Taehee Yoo]

On Tue, Oct 13, 2026 at 11:01 PM <netdev-bot+sashiko@xxxxxxxxxx> wrote:
>
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider.
>
> Critical: 0 · High: 2 (1) · Medium: 0 · Low: 1
>
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
>
> - [High] The fix stops a concurrent Membership Update from re-arming
>   gc_wq, but that Update can still arm other delayed work that points at…
> - [High] amt_dev_stop() replaced the RCU-safe list_del_rcu() loop with
>   list_splice_init(&amt->tunnel_list, &doomed), which moves the RCU-…
> - [Low] At this commit, a tunnel that only ever received a Request lives
>   three times as long (3*GMI instead of GMI), and the commit message…
>
> Pre-existing issues:
> - [High] This problem wasn't introduced by this patch, but tunnel
>   teardown does not wait for a group_timer callback that is already…
>
> --
>
> amt: fix use-after-free when a Membership Update re-arms a freed tunnel
>
> amt_update_handler() no longer re-arms tunnel->gc_wq. It now stamps a new
> tunnel->last_seen field, and amt_tunnel_expire() either re-arms itself or
> frees the tunnel under amt->lock. The patch also adds a tunnel->dead flag,
> and amt_dev_stop() now marks tunnels dead and splices the tunnel list out
> under amt->lock before freeing them.
>
> > diff --git a/drivers/net/amt.c b/drivers/net/amt.c
> > index b53f8ec556615..b284fa7b7307e 100644
> > --- a/drivers/net/amt.c
> > +++ b/drivers/net/amt.c
>
> [ ... ]
>
> > @@ -2494,9 +2515,12 @@ static bool amt_update_handler(struct amt_dev *amt, struct sk_buff *skb)
> >               if (tunnel->ip4 == saddr) {
> >                       if ((nonce == tunnel->nonce &&
> >                            response_mac == tunnel->mac)) {
> > -                             mod_delayed_work(amt_wq, &tunnel->gc_wq,
> > -                                              msecs_to_jiffies(amt_gmi(amt))
> > -                                                               * 3);
> > +                             /* Refresh liveness only.  gc_wq is re-armed by
> > +                              * amt_tunnel_expire() itself from last_seen, never
> > +                              * from here, so this cannot re-queue the work of a
> > +                              * tunnel that is being freed.
> > +                              */
> > +                             WRITE_ONCE(tunnel->last_seen, jiffies);
> >                               goto report;
>
> [Severity: High]
> The Update path no longer re-arms gc_wq. Can it still arm other delayed
> work that points at a tunnel being freed?
>
> amt_update_handler() still holds only rcu_read_lock(), and it never checks
> the new tunnel->dead flag. After the goto report it does this:
>
> drivers/net/amt.c:amt_update_handler() {
>     ...
>                 spin_lock_bh(&tunnel->lock);
>                 amt_igmp_report_handler(amt, skb, tunnel);
>                 spin_unlock_bh(&tunnel->lock);
>     ...
> }
>
> The MLD branch does the same.
>
> Suppose amt_tunnel_expire() reads last_seen before this WRITE_ONCE() is
> visible. It then goes ahead with the teardown:
>
> CPU1 (amt_tunnel_expire)            CPU2 (amt_update_handler)
>                                     finds tunnel in RCU walk
> deadline = READ_ONCE(last_seen)...
> tunnel->dead = true;
> list_del_rcu(&tunnel->list);
> amt_clear_groups(tunnel);
> kfree_rcu(tunnel, rcu);
>                                     WRITE_ONCE(tunnel->last_seen, jiffies);
>                                     spin_lock_bh(&tunnel->lock);
>                                     amt_igmp_report_handler()
>                                       amt_igmpv2_report_handler()
>                                         amt_add_group()
>                                           gnode->tunnel_list = tunnel;
>                                           hlist_add_head_rcu(...)
>                                         mod_delayed_work(group_timer)
>                                         dev_hold(amt->dev)
>
> Nothing deletes that gnode afterwards. Would the gnode, its sources and
> the netdev reference all leak?
>
> The tunnel is freed once the grace period ends. When group_timer fires,
> amt_group_work() loads gnode->tunnel_list and calls
> spin_lock_bh(&tunnel->lock) on the freed tunnel. amt_del_group() also
> writes gnode->tunnel_list->nr_groups.
>
> The commit message describes this same class of bug: an Update holding a
> cached tunnel pointer arms delayed work that outlives the tunnel.
>
> amt_dev_stop() seems to have the same window. An Update that found the
> tunnel before the list_splice_init() can add groups after
> amt_clear_groups() has run. This path looks unchanged through the end of
> the series.
>
> Could amt_update_handler() check tunnel->dead under tunnel->lock and bail
> out before calling the report handlers? Both freers set dead before
> amt_clear_groups() takes tunnel->lock, so that check should close the
> window.
>

I agree with Sashiko's suggestion.

Please make tunnel->dead protected by tunnel->lock as well, and
have amt_update_handler() check it under tunnel->lock before calling
the report handlers.

> [ ... ]
>
> > @@ -2732,6 +2756,7 @@ static bool amt_request_handler(struct amt_dev *amt, struct sk_buff *skb)
> >       for (i = 0; i < amt->hash_buckets; i++)
> >               INIT_HLIST_HEAD(&tunnel->groups[i]);
> >
> > +     tunnel->last_seen = jiffies;
> >       INIT_DELAYED_WORK(&tunnel->gc_wq, amt_tunnel_expire);
> >
> >       list_add_tail_rcu(&tunnel->list, &amt->tunnel_list);
>
> [Severity: Low]
> Does this extend the lifetime of a Request-only tunnel from GMI to 3*GMI?
>
> amt_request_handler() still arms the first expiry at one GMI:
>
>         mod_delayed_work(amt_wq, &tunnel->gc_wq,
>                          msecs_to_jiffies(amt_gmi(amt)));
>
> When that work fires, amt_tunnel_expire() computes:
>
>         deadline = READ_ONCE(tunnel->last_seen) +
>                    msecs_to_jiffies(amt_gmi(amt)) * 3;
>
> The deadline has not passed yet, so it re-arms for roughly another 2*GMI.
> Before this patch, a tunnel that only received a Request was freed after
> GMI. Unauthenticated Requests can now hold max_tunnels slots three times
> longer, and the commit message does not mention the change.
>
> The new comment in struct amt_tunnel_list also describes last_seen as the
> "Last time a valid Membership Update refreshed this tunnel". At this
> commit the Request path writes it as well.
>
> A later commit in the series, "amt: do not create tunnel state for
> unauthenticated Requests", seems to fix this. That commit stops the
> Request path from allocating tunnels, which also makes the struct comment
> accurate.
>
> [ ... ]
>
> > @@ -3053,6 +3078,7 @@ static int amt_dev_stop(struct net_device *dev)
> >       struct amt_tunnel_list *tunnel, *tmp;
> >       struct sk_buff *skb;
> >       struct sock *sk;
> > +     LIST_HEAD(doomed);
> >       int i;
> >
> >       disable_delayed_work_sync(&amt->req_wq);
> > @@ -3079,9 +3105,19 @@ static int amt_dev_stop(struct net_device *dev)
> >       amt->req_cnt = 0;
> >       WRITE_ONCE(amt->remote_ip, 0);
> >
> > -     list_for_each_entry_safe(tunnel, tmp, &amt->tunnel_list, list) {
> > -             list_del_rcu(&tunnel->list);
> > -             amt->nr_tunnels--;
>
> [ ... ]
>
> > +     spin_lock_bh(&amt->lock);
> > +     list_for_each_entry(tunnel, &amt->tunnel_list, list)
> > +             tunnel->dead = true;
> > +     list_splice_init(&amt->tunnel_list, &doomed);
> > +     amt->nr_tunnels = 0;
> > +     spin_unlock_bh(&amt->lock);
>
> [Severity: High]
> Is list_splice_init() safe here while RX readers may still be walking
> amt->tunnel_list with list_for_each_entry_rcu()?
>
> The old loop used list_del_rcu(). After this splice, the last tunnel's
> ->next points at the on-stack doomed head. amt->tunnel_list is
> reinitialised to empty, with no grace period in between.
>
> The only grace period in amt_dev_stop() is the synchronize_net() before
> udp_tunnel_sock_release(). udp_tunnel_sock_release() clears sk_user_data
> with rcu_assign_sk_user_data(sk, NULL), then shuts down and releases the
> socket without waiting for readers.
>
> So an amt_rcv() call that already loaded amt from sk_user_data could
> still be inside amt_request_handler() or amt_update_handler():

I think this is correct.
amt_dev_stop() needs to wait for in-flight amt_rcv() calls to finish
before tearing down the tunnel list.

>
>         list_for_each_entry_rcu(tunnel, &amt->tunnel_list, list) {
>                 if (tunnel->ip4 == saddr) {
>
> A reader that has already passed the last tunnel would never reach the
> terminator, &amt->tunnel_list. Would it then treat &doomed as a struct
> amt_tunnel_list and read ip4 from stack memory past the list_head? It
> would then follow doomed.next back to the first tunnel and loop in softirq
> under rcu_read_lock().
>
> That would stall RCU, which also holds up the pending kfree_rcu() calls.
> Once amt_dev_stop() returns and its stack frame is reused, the reader
> follows garbage pointers. If the stack bytes happen to match saddr, the
> send: path in amt_request_handler() writes tunnel->nonce through the bogus
> pointer.
>
> There is a second problem. A reader that reaches the allocation in
> amt_request_handler() after the splice (amt_tunnel_get_or_create() later
> in the series) can list_add_tail_rcu() a new tunnel onto the emptied
> amt->tunnel_list. Nothing in this stop path frees that tunnel.
>
> Would list_splice_init_rcu(), which waits for a grace period, or keeping
> list_del_rcu() work better here? This code also looks unchanged at the end
> of the series.
>
> > +
> > +     list_for_each_entry_safe(tunnel, tmp, &doomed, list) {
> >               cancel_delayed_work_sync(&tunnel->gc_wq);
> >               amt_clear_groups(tunnel);
> >               kfree_rcu(tunnel, rcu);
>
> [Severity: High]
> This is a pre-existing issue, not one introduced by this patch. Does this
> teardown wait for a gnode->group_timer callback that is already running?
>
> amt_clear_groups() calls amt_del_group() for each group under
> tunnel->lock. amt_del_group() only does a non-synchronous cancel:
>
> drivers/net/amt.c:amt_del_group() {
>         if (cancel_delayed_work(&gnode->group_timer))
>                 dev_put(amt->dev);
>         hlist_del_rcu(&gnode->node);
>         gnode->tunnel_list->nr_groups--;
>         ...
>         kfree_rcu(gnode, rcu);
> }
>
> If amt_group_work() is already running, cancel_delayed_work() returns
> false. amt_group_work() loads the tunnel pointer outside any RCU read
> section:
>
>         struct amt_tunnel_list *tunnel = gnode->tunnel_list;
>         ...
>         spin_lock_bh(&tunnel->lock);
>
> If the callback is preempted before spin_lock_bh(), could the grace period
> finish so that it takes tunnel->lock inside a freed tunnel?
>
> If the memory has not been freed yet, the callback takes tunnel->lock once
> amt_clear_groups() drops it. For an EXCLUDE mode group the source lists
> are empty by then, so delete_group stays true and amt_del_group() runs a
> second time.
>
> Would that call hlist_del_rcu() again on a node whose pprev is already
> LIST_POISON2? It would also decrement nr_groups twice and call
> kfree_rcu(gnode) twice.
>
> amt_tunnel_expire() has the same issue, since it also calls
> amt_clear_groups() before kfree_rcu(tunnel). Only gc_wq is cancelled
> synchronously here; the per-group work is never waited for.
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261012125943.558629-1-omar%40blockcast.net