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

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

[Hang Nan]

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:

    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@qq.com>

---
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);

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@qq.com> 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@mojatatu.com>

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

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@qq.com>
>
> ---
> 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);
>