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

[Aohan Mei <ljp1205831794@gmail.com>]

在 2026/8/6 01:52, Jamal Hadi Salim 写道:
> On Wed, Aug 5, 2026 at 5:30 AM Aohan Mei <ljp1205831794@gmail.com> 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: stable@vger.kernel.org
>> Reported-by: TencentOS Corvus AI <corvus@tencent.com>
>> Signed-off-by: Aohan Mei <henrymei@tencent.com>
> 
> This is the same issue Sashiko found. A patch is here:
> 
> https://lore.kernel.org/netdev/20260805134049.927864-1-victor@mojatatu.com/
> 
> 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.

BTW, "TencentOS Corvus AI" found this bug; and the patch was mannualy 
written and validated, so no `assisted-by` tag here. Noted for future 
submissions.

Thanks,
Aohan Mei