[PATCH net] net/sched: cls_api: fix teardown of an adopted proto on insert-race loss

[PATCH net] net/sched: cls_api: fix teardown of an adopted proto on insert-race loss

[Victor Nogueira]

In tc_new_tfilter() the create branch sets tp_created = 1 before calling
tcf_chain_tp_insert_unique(). When the caller loses the race (another
request inserted a proto at the same chain/prio first), insert_unique()
destroys the caller's own tp_new and returns the winner's proto with an
extra reference. tp_created was never cleared, so the loser's errout
path treated the winner's live proto as its own and called
tcf_chain_tp_delete_empty() on it, silently unlinking an active
classifier that the winning request already advertised via
RTM_NEWTFILTER.

Track the outcome of the insert step in a single tri-state variable so
each errout path reacts correctly:

- TP_NOT_CREATED: no proto created; pursue the old path.
- TP_CREATED: proto inserted successfully; same code path as before.
- TP_NOT_OWNED: New - lost the insert race; tp is another request's proto
  (chain ref already released by tp_new's destroy)

Both errout reactions are single expressions derived from the state.

This fix is motivated by the Sashiko's automated review of Patch
(net/sched: cls_api: Always acquire rtnl_lock when destroying locked
classifiers) [1][2]. The review identified the silent-unlink behaviour of
an adopted proto's teardown when a request loses the
tcf_chain_tp_insert_unique() race.

[1] https://sashiko.dev/#/patchset/20260801125632.360365-1-jhs%40mojatatu.com
[2] https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260801125632.360365-1-jhs%40mojatatu.com

Fixes: 8b64678e0af8 ("net: sched: refactor tp insert/delete for concurrent execution")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/patchset/20260801125632.360365-1-jhs%40mojatatu.com
Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260801125632.360365-1-jhs%40mojatatu.com
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Victor Nogueira <victor@mojatatu.com>
---
 net/sched/cls_api.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 4e6a2812a4f3..3271963c945d 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -2248,6 +2248,12 @@ static bool is_ingress_or_clsact(struct tcf_block *block, struct Qdisc *q)
 	return tcf_block_shared(block) || (q && !!(q->flags & TCQ_F_INGRESS));
 }
 
+enum tcf_tp_insert_state {
+	TP_NOT_CREATED = 0, /* did not create and insert a new tp */
+	TP_CREATED, /* created and inserted a new tp */
+	TP_NOT_OWNED, /* created a proto but failed to insert */
+};
+
 static int tc_new_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
 			  struct netlink_ext_ack *extack)
 {
@@ -2268,12 +2274,12 @@ static int tc_new_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
 	unsigned long cl;
 	void *fh;
 	int err;
-	int tp_created;
+	enum tcf_tp_insert_state tp_state;
 	bool rtnl_held = false;
 	u32 flags;
 
 replay:
-	tp_created = 0;
+	tp_state = TP_NOT_CREATED;
 
 	err = nlmsg_parse_deprecated(n, sizeof(*t), tca, TCA_MAX,
 				     rtm_tca_policy, extack);
@@ -2395,13 +2401,15 @@ static int tc_new_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
 			goto errout_tp;
 		}
 
-		tp_created = 1;
+		tp_state = TP_CREATED;
 		tp = tcf_chain_tp_insert_unique(chain, tp_new, protocol, prio,
 						rtnl_held);
 		if (IS_ERR(tp)) {
 			err = PTR_ERR(tp);
 			goto errout_tp;
 		}
+		if (tp != tp_new)
+			tp_state = TP_NOT_OWNED;
 	} else {
 		mutex_unlock(&chain->filter_chain_lock);
 	}
@@ -2455,13 +2463,13 @@ static int tc_new_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
 	}
 
 errout:
-	if (err && tp_created)
+	if (err && tp_state == TP_CREATED)
 		tcf_chain_tp_delete_empty(chain, tp, rtnl_held, NULL);
 errout_tp:
 	if (chain) {
 		if (tp && !IS_ERR(tp))
 			tcf_proto_put(tp, rtnl_held, NULL);
-		if (!tp_created)
+		if (tp_state == TP_NOT_CREATED)
 			tcf_chain_put(chain);
 	}
 	tcf_block_release(q, block, rtnl_held);
-- 
2.55.0

Re: [PATCH net] net/sched: cls_api: fix teardown of an adopted proto on insert-race loss

[Aohan Mei]

在 2026/8/5 21:40, Victor Nogueira 写道:
> In tc_new_tfilter() the create branch sets tp_created = 1 before calling
> tcf_chain_tp_insert_unique(). When the caller loses the race (another
> request inserted a proto at the same chain/prio first), insert_unique()
> destroys the caller's own tp_new and returns the winner's proto with an
> extra reference. tp_created was never cleared, so the loser's errout
> path treated the winner's live proto as its own and called
> tcf_chain_tp_delete_empty() on it, silently unlinking an active
> classifier that the winning request already advertised via
> RTM_NEWTFILTER.
> 
> Track the outcome of the insert step in a single tri-state variable so
> each errout path reacts correctly:
> 
> - TP_NOT_CREATED: no proto created; pursue the old path.
> - TP_CREATED: proto inserted successfully; same code path as before.
> - TP_NOT_OWNED: New - lost the insert race; tp is another request's proto
>    (chain ref already released by tp_new's destroy)
> 
> Both errout reactions are single expressions derived from the state.
> 
> This fix is motivated by the Sashiko's automated review of Patch
> (net/sched: cls_api: Always acquire rtnl_lock when destroying locked
> classifiers) [1][2]. The review identified the silent-unlink behaviour of
> an adopted proto's teardown when a request loses the
> tcf_chain_tp_insert_unique() race.
> 
> [1] https://sashiko.dev/#/patchset/20260801125632.360365-1-jhs%40mojatatu.com
> [2] https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260801125632.360365-1-jhs%40mojatatu.com
> 
> Fixes: 8b64678e0af8 ("net: sched: refactor tp insert/delete for concurrent execution")
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Closes: https://sashiko.dev/#/patchset/20260801125632.360365-1-jhs%40mojatatu.com
> Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260801125632.360365-1-jhs%40mojatatu.com
> Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
> Signed-off-by: Victor Nogueira <victor@mojatatu.com>
> ---
>   net/sched/cls_api.c | 18 +++++++++++++-----
>   1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index 4e6a2812a4f3..3271963c945d 100644
> --- a/net/sched/cls_api.c
> +++ b/net/sched/cls_api.c
> @@ -2248,6 +2248,12 @@ static bool is_ingress_or_clsact(struct tcf_block *block, struct Qdisc *q)
>   	return tcf_block_shared(block) || (q && !!(q->flags & TCQ_F_INGRESS));
>   }
>   
> +enum tcf_tp_insert_state {
> +	TP_NOT_CREATED = 0, /* did not create and insert a new tp */
> +	TP_CREATED, /* created and inserted a new tp */
> +	TP_NOT_OWNED, /* created a proto but failed to insert */
> +};
> +
>   static int tc_new_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
>   			  struct netlink_ext_ack *extack)
>   {
> @@ -2268,12 +2274,12 @@ static int tc_new_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
>   	unsigned long cl;
>   	void *fh;
>   	int err;
> -	int tp_created;
> +	enum tcf_tp_insert_state tp_state;
>   	bool rtnl_held = false;
>   	u32 flags;
>   
>   replay:
> -	tp_created = 0;
> +	tp_state = TP_NOT_CREATED;
>   
>   	err = nlmsg_parse_deprecated(n, sizeof(*t), tca, TCA_MAX,
>   				     rtm_tca_policy, extack);
> @@ -2395,13 +2401,15 @@ static int tc_new_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
>   			goto errout_tp;
>   		}
>   
> -		tp_created = 1;
> +		tp_state = TP_CREATED;
>   		tp = tcf_chain_tp_insert_unique(chain, tp_new, protocol, prio,
>   						rtnl_held);
>   		if (IS_ERR(tp)) {
>   			err = PTR_ERR(tp);
>   			goto errout_tp;
>   		}
> +		if (tp != tp_new)
> +			tp_state = TP_NOT_OWNED;
>   	} else {
>   		mutex_unlock(&chain->filter_chain_lock);
>   	}
> @@ -2455,13 +2463,13 @@ static int tc_new_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
>   	}
>   
>   errout:
> -	if (err && tp_created)
> +	if (err && tp_state == TP_CREATED)
>   		tcf_chain_tp_delete_empty(chain, tp, rtnl_held, NULL);
>   errout_tp:
>   	if (chain) {
>   		if (tp && !IS_ERR(tp))
>   			tcf_proto_put(tp, rtnl_held, NULL);
> -		if (!tp_created)
> +		if (tp_state == TP_NOT_CREATED)
>   			tcf_chain_put(chain);
>   	}
>   	tcf_block_release(q, block, rtnl_held);

The fix seems good:

Reported-by: TencentOS Corvus AI <corvus@tencent.com>
Tested-by: Aohan Mei <henrymei@tencent.com>