Re: [PATCH net] net/sched: cls_api: fix tp_created race losing existing tcf_proto

On Wed, Aug 5, 2026 at 5:30 AM Aohan Mei <[email protected]> wrote:
>
> tc_new_tfilter() attaches a filter to a chain. When no
> tcf_proto (tp) exists for the given (protocol, prio), it creates one
> with tcf_proto_create(), marks tp_created so the error path can clean
> it up, and inserts it with tcf_chain_tp_insert_unique().
>
> However, tcf_proto_create() can sleep, opening a race window in which a
> concurrent thread may insert a tp with the same (protocol, prio). In
> that case tcf_chain_tp_insert_unique() destroys tp_new and returns the
> *existing* tp, but tp_created is never reset. If the request then fails
> (e.g. kind mismatch), the errout path calls
> tcf_chain_tp_delete_empty() on a tp this thread never created. For
> classifiers without a delete_empty callback (all but cls_flower),
> tcf_proto_check_delete() removes the tp unconditionally: a live tp and
> all its filters are silently lost while the owner's change() still
> reports success. The race is reachable because cls_flower on
> ingress/clsact qdiscs runs without rtnl_lock and can interleave with
> rtnl_lock-holding classifiers such as u32.
>
> Fix this by making tcf_chain_tp_insert_unique() report through a new
> "inserted" out-parameter whether tp_new was actually inserted, and
> gate the errout delete_empty call on it instead of tp_created.
> tp_created itself must stay set on this path: the chain reference was
> consumed by the destroyed tp_new, and errout_tp relies on tp_created
> to decide whether to tcf_chain_put(), so resetting it would underflow
> the chain refcount.
>
> Fixes: 8b64678e0af8 ("net: sched: refactor tp insert/delete for concurrent execution")
> Cc: [email protected]
> Reported-by: TencentOS Corvus AI <[email protected]>
> Signed-off-by: Aohan Mei <[email protected]>

This is the same issue Sashiko found. A patch is here:

https://lore.kernel.org/netdev/[email protected]/

Also, please add assisted-by: tags going forward if this was ai generated.

cheers,
jamal

> ---
>  net/sched/cls_api.c | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index fee4524ad..9ceb2b538 100644
> --- a/net/sched/cls_api.c
> +++ b/net/sched/cls_api.c
> @@ -1937,7 +1937,8 @@ static struct tcf_proto *tcf_chain_tp_find(struct tcf_chain *chain,
>  static struct tcf_proto *tcf_chain_tp_insert_unique(struct tcf_chain *chain,
>                                                     struct tcf_proto *tp_new,
>                                                     u32 protocol, u32 prio,
> -                                                   bool rtnl_held)
> +                                                   bool rtnl_held,
> +                                                   bool *inserted)
>  {
>         struct tcf_chain_info chain_info;
>         struct tcf_proto *tp;
> @@ -1948,6 +1949,7 @@ static struct tcf_proto *tcf_chain_tp_insert_unique(struct tcf_chain *chain,
>         if (tcf_proto_exists_destroying(chain, tp_new)) {
>                 mutex_unlock(&chain->filter_chain_lock);
>                 tcf_proto_destroy(tp_new, rtnl_held, false, NULL);
> +               *inserted = false;
>                 return ERR_PTR(-EAGAIN);
>         }
>
> @@ -1964,6 +1966,11 @@ static struct tcf_proto *tcf_chain_tp_insert_unique(struct tcf_chain *chain,
>                 tp_new = ERR_PTR(err);
>         }
>
> +       /* Tell the caller whether tp_new was actually inserted, or an
> +        * already existing tp is being returned instead.
> +        */
> +       *inserted = !tp && !err;
> +
>         return tp_new;
>  }
>
> @@ -2254,11 +2261,13 @@ static int tc_new_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
>         void *fh;
>         int err;
>         int tp_created;
> +       bool tp_inserted;
>         bool rtnl_held = false;
>         u32 flags;
>
>  replay:
>         tp_created = 0;
> +       tp_inserted = false;
>
>         err = nlmsg_parse_deprecated(n, sizeof(*t), tca, TCA_MAX,
>                                      rtm_tca_policy, extack);
> @@ -2382,7 +2391,7 @@ static int tc_new_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
>
>                 tp_created = 1;
>                 tp = tcf_chain_tp_insert_unique(chain, tp_new, protocol, prio,
> -                                               rtnl_held);
> +                                               rtnl_held, &tp_inserted);
>                 if (IS_ERR(tp)) {
>                         err = PTR_ERR(tp);
>                         goto errout_tp;
> @@ -2440,7 +2449,13 @@ static int tc_new_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
>         }
>
>  errout:
> -       if (err && tp_created)
> +       /* Only delete a tp that we actually inserted ourselves. When
> +        * tcf_chain_tp_insert_unique() raced with a concurrent insertion
> +        * it returns the existing tp; tp_created must stay set then (the
> +        * chain reference was consumed by the destroyed tp_new), but the
> +        * existing tp must not be deleted.
> +        */
> +       if (err && tp_inserted)
>                 tcf_chain_tp_delete_empty(chain, tp, rtnl_held, NULL);
>  errout_tp:
>         if (chain) {
> --
> 2.50.1 (Apple Git-155)
>