Re: [PATCH net] net/sched: act_api: collapse duplicated slots in DELACTION

[Jamal Hadi Salim]

On Sun, Oct 18, 2026 at 12:58 PM Hang Nan <2122295973@xxxxxx> wrote:
>
> A batched RTM_DELACTION takes one reference per slot in
> tca_action_gd() and drops them one by one in tcf_action_delete().
> Every slot also calls tcf_idr_delete_index() with the index it saved
> before the put, and that helper drops the IDR reference as soon as
> the count reaches zero.
>
> Nothing stops userspace from naming the same action in several
> slots.  With N slots holding the same action the request takes N
> references, performs N puts, and calls tcf_idr_delete_index() for
> every slot whose put did not reach zero; that helper drops the IDR
> reference again each time.  From N == 3 on, one drop lands on an
> action that has already been freed:
>


Nacked-by: Jamal Hadi Salim <jhs@xxxxxxxxxxxx>

See: https://lore.kernel.org/netdev/QDISC-K1TH.v1.r3.20261012134653-1@xxxxxxxxxxxx/

cheers,
jamal
>     refs = 1 (IDR) + N
>
>     slot 0: put -> N,     delete_index -> N - 1
>     ...
>     slot k: put -> 1,     delete_index -> 0  (idr_remove + free)
>     slot k+1: put on the freed action        (refcount underflow)
>
> With two slots the second put is the one that reaches zero, so
> __tcf_action_put() returns 1 and that slot skips the
> tcf_idr_delete_index() call; the accounting stays balanced, which is
> why the bug needs three slots.
>
> A three-slot RTM_DELACTION for an unbound gact action (index 0x1002)
> on 7.3.0-rc7-00245-g32c52ec87d62 reports
>
>     refcount_t: underflow; use-after-free.
>     WARNING: lib/refcount.c:28 at refcount_warn_saturate+0x59/0x90
>     Call Trace:
>      __tcf_action_put+0x7e/0x80
>      tca_action_gd+0x439/0x640
>      tc_ctl_action+0x10a/0x170
>      rtnetlink_rcv_msg+0x14c/0x3f0
>
> and returns -ENOENT, because the IDR entry is already gone.
>
> Collapse a repeated action while the slots are being resolved: the
> batch keeps the first reference and releases the extra ones, so the
> IDR reference is dropped exactly once and the delete succeeds.
> Comparing the resolved pointers is equivalent to comparing
> {kind, index} and also covers actions that live in a per-action
> IDR.
>
> Fixes: 97a3f84f2c84 ("net_sched: remove unnecessary ops->delete()")
> Signed-off-by: Hang Nan <2122295973@xxxxxx>
>
> ---
> This report was produced with the help of AI tooling, so per
> Documentation/process/security-bugs.rst it is being reported publicly; the
> reproducer is not attached and is available on request.
>
> The two kernels were built from 32c52ec87d62 with the same config
> (CONFIG_NET_ACT_GACT=y) and run the same initramfs.  A batch of two distinct
> actions is deleted on both.  Two duplicate slots also delete cleanly on both.
> With three duplicate slots the unpatched kernel prints the warning above and
> returns -ENOENT, while the patched kernel collapses the batch and deletes the
> action (no warning, rc = 0).  Each kernel was run twice with the same result.
>
>  net/sched/act_api.c | 29 +++++++++++++++++++++++++++--
>  1 file changed, 27 insertions(+), 2 deletions(-)
>
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index 9eb1dec..dc51ccf 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -2021,11 +2021,28 @@ static int tcf_del_notify(struct net *net, struct nlmsghdr *n,
>                             n->nlmsg_flags & NLM_F_ECHO);
>  }
>
> +/* A batched request may name the same action in more than one slot.  Each
> + * slot takes its own reference, but tcf_action_delete() drops the IDR
> + * reference once per slot, so a duplicated action must be collapsed before
> + * the deletion starts.
> + */
> +static bool tcf_action_batch_dup(struct tc_action *actions[], int n,
> +                                const struct tc_action *act)
> +{
> +       int i;
> +
> +       for (i = 0; i < n; i++)
> +               if (actions[i] == act)
> +                       return true;
> +
> +       return false;
> +}
> +
>  static int
>  tca_action_gd(struct net *net, struct nlattr *nla, struct nlmsghdr *n,
>               u32 portid, int event, struct netlink_ext_ack *extack)
>  {
> -       int i, ret;
> +       int i, ret, nact = 0;
>         struct nlattr *tb[TCA_ACT_MAX_PRIO + 1];
>         struct tc_action *act;
>         size_t attr_size = 0;
> @@ -2050,8 +2067,16 @@ tca_action_gd(struct net *net, struct nlattr *nla, struct nlmsghdr *n,
>                         ret = PTR_ERR(act);
>                         goto err;
>                 }
> +               if (event == RTM_DELACTION &&
> +                   tcf_action_batch_dup(actions, nact, act)) {
> +                       /* The IDR reference is dropped once, by the first
> +                        * slot that names this action.
> +                        */
> +                       tcf_action_put(act);
> +                       continue;
> +               }
>                 attr_size += tcf_action_fill_size(act);
> -               actions[i - 1] = act;
> +               actions[nact++] = act;
>         }
>
>         attr_size = tcf_action_full_attrs_size(attr_size);
>