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

On Wed, Aug 5, 2026 at 11:28 PM Aohan Mei <[email protected]> wrote:
>
>
>
> 在 2026/8/6 01:52, Jamal Hadi Salim 写道:
> > 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)
> >>
>
> Thanks for the pointer. We tested Victor's patch locally and confirm it
> is an equivalent fix for the same issue, happy to defer to his version.
>

Thanks. Please respond to that patch and add a tested-by tag

> BTW, "TencentOS Corvus AI" found this bug; and the patch was mannualy

Also add a reported-by tag to the patch response

> written and validated, so no `assisted-by` tag here. Noted for future
> submissions.

It does look like there was some human touch to it (other than the
verbose comment) - and is a reasonable patch except you missed one
spot.
I am wondering how you tested it. We had to craft printks to see the issue.

I was kind of suprised how quickly you found the issue. Victor had
something already based on what Sashiko said but i said to wait until
the first patch made it in.
Does Corvus AI watch what Sashiko comments on?

cheers,
jamal
> Thanks,
> Aohan Mei