[PATCH net] net/sched: cls_api: reclaim an empty proto on the error path

[PATCH net] net/sched: cls_api: reclaim an empty proto on the error path

[Jamal Hadi Salim]

Two racing tc filter add requests on the same chain/prio of an
unlocked classifier both run change() on the shared proto and both
can fail: the winner's tcf_chain_tp_delete_empty() attempt gives up
because the loser's handle is still in the idr, and the loser's
error path drops only its own reference without a second reclamation
attempt. The empty proto stays linked in the chain, holding the
chain reference, a block reference and the classifier module
reference until the chain or block is torn down.

Reclaim the proto on the error path of any failed request that holds
a proto reference. The reclamation is emptiness-gated:
tcf_chain_tp_delete_empty() unlinks the proto only when
delete_empty() admits it is empty, so a live shared proto is never
unlinked. A proto the request created is reclaimed unconditionally -
it is the only owner, so marking it for deletion is safe even without
a delete_empty callback. Classifiers without one (the check marks the
proto unconditionally) are rtnl-serialized, so the raced window this
guard closes cannot arise for them.

This is a follow-up to commit d4e359b3608a ("net/sched: cls_api: fix
teardown of an adopted proto on insert-race loss"), which stopped the
loser of the insert race from unlinking the winner's live proto but
left the empty-proto residual in place.

Conditions to recreate:
- CONFIG_NET_CLS_FLOWER=y; veth pair
- tc qdisc add dev veth0 ingress
- two concurrent `tc filter add dev veth0 ingress protocol ip pref 1
  flower skip_sw ... action drop` (both fail in fl_hw_replace_filter
  after publishing their handle in the idr); repeat in a loop
- an empty flower tp stays linked after both requests fail; visible
  as a bare `filter protocol ip pref 1 flower chain 0` header in
  `tc filter show` with no filter entries
- CAP_NET_ADMIN (namespace-local via unshare -Urn suffices)

Fixes: 8b64678e0af8 ("net: sched: refactor tp insert/delete for concurrent execution")
Reported-by: Sashiko (nipa) <sashiko-bot@kernel.org>
Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260805134049.927864-1-victor@mojatatu.com
Tested-by: hybris <hybris@mojatatu.ai>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/sched/cls_api.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index c47d2ee13641..a9f54988561f 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -2463,7 +2463,20 @@ static int tc_new_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
 	}
 
 errout:
-	if (err && tp_state == TP_CREATED)
+	if (err && !IS_ERR_OR_NULL(tp) &&
+	    (tp_state == TP_CREATED || tp->ops->delete_empty))
+		/*
+		 * The request is dropping its reference to tp. If it was
+		 * the last user (the idr is empty now), reclaim the proto.
+		 * A tp this request created is reclaimed unconditionally:
+		 * it is the only owner, so marking it for deletion is
+		 * safe. Otherwise only classifiers with a delete_empty
+		 * callback are reclaimed -- the callback admits an empty
+		 * proto only, so a live shared proto is never unlinked.
+		 * Classifiers without one (deleting is set
+		 * unconditionally) are rtnl-serialized, so the raced
+		 * window this guard closes cannot arise for them.
+		 */
 		tcf_chain_tp_delete_empty(chain, tp, rtnl_held, NULL);
 errout_tp:
 	if (chain) {
-- 
2.43.0

Re: [PATCH net] net/sched: cls_api: reclaim an empty proto on the error path

[Simon Horman]

On Thu, Sep 24, 2026 at 04:32:49AM -0400, Jamal Hadi Salim wrote:
> Two racing tc filter add requests on the same chain/prio of an
> unlocked classifier both run change() on the shared proto and both
> can fail: the winner's tcf_chain_tp_delete_empty() attempt gives up
> because the loser's handle is still in the idr, and the loser's
> error path drops only its own reference without a second reclamation
> attempt. The empty proto stays linked in the chain, holding the
> chain reference, a block reference and the classifier module
> reference until the chain or block is torn down.
> 
> Reclaim the proto on the error path of any failed request that holds
> a proto reference. The reclamation is emptiness-gated:
> tcf_chain_tp_delete_empty() unlinks the proto only when
> delete_empty() admits it is empty, so a live shared proto is never
> unlinked. A proto the request created is reclaimed unconditionally -
> it is the only owner, so marking it for deletion is safe even without
> a delete_empty callback. Classifiers without one (the check marks the
> proto unconditionally) are rtnl-serialized, so the raced window this
> guard closes cannot arise for them.
> 
> This is a follow-up to commit d4e359b3608a ("net/sched: cls_api: fix
> teardown of an adopted proto on insert-race loss"), which stopped the
> loser of the insert race from unlinking the winner's live proto but
> left the empty-proto residual in place.
> 
> Conditions to recreate:
> - CONFIG_NET_CLS_FLOWER=y; veth pair
> - tc qdisc add dev veth0 ingress
> - two concurrent `tc filter add dev veth0 ingress protocol ip pref 1
>   flower skip_sw ... action drop` (both fail in fl_hw_replace_filter
>   after publishing their handle in the idr); repeat in a loop
> - an empty flower tp stays linked after both requests fail; visible
>   as a bare `filter protocol ip pref 1 flower chain 0` header in
>   `tc filter show` with no filter entries
> - CAP_NET_ADMIN (namespace-local via unshare -Urn suffices)
> 
> Fixes: 8b64678e0af8 ("net: sched: refactor tp insert/delete for concurrent execution")
> Reported-by: Sashiko (nipa) <sashiko-bot@kernel.org>
> Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260805134049.927864-1-victor@mojatatu.com
> Tested-by: hybris <hybris@mojatatu.ai>
> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net] net/sched: cls_api: reclaim an empty proto on the error path

[Jakub Kicinski]

On Thu, 24 Sep 2026 04:32:49 -0400 Jamal Hadi Salim wrote:
> Tested-by: hybris <hybris@mojatatu.ai>

Please stop adding these tags, they tell us absolutely nothing.

Re: [PATCH net] net/sched: cls_api: reclaim an empty proto on the error path

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Thu, 24 Sep 2026 04:32:49 -0400 you wrote:
> Two racing tc filter add requests on the same chain/prio of an
> unlocked classifier both run change() on the shared proto and both
> can fail: the winner's tcf_chain_tp_delete_empty() attempt gives up
> because the loser's handle is still in the idr, and the loser's
> error path drops only its own reference without a second reclamation
> attempt. The empty proto stays linked in the chain, holding the
> chain reference, a block reference and the classifier module
> reference until the chain or block is torn down.
> 
> [...]

Here is the summary with links:
  - [net] net/sched: cls_api: reclaim an empty proto on the error path
    https://git.kernel.org/netdev/net/c/0bb8eab29b55

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net] net/sched: cls_api: reclaim an empty proto on the error path

[Jamal Hadi Salim]

On Fri, Sep 25, 2026 at 8:18 PM 'Jakub Kicinski' via Hyper-Yielding
Back-end Review & Insight System <hybris@mojatatu.com> wrote:
>
> On Thu, 24 Sep 2026 04:32:49 -0400 Jamal Hadi Salim wrote:
> > Tested-by: hybris <hybris@mojatatu.ai>
>
> Please stop adding these tags, they tell us absolutely nothing.

This says that our _automated_ bot tested and it adds the tag before
we send the patch.
It does more than tdc for a specific patch set (e.g., PoCs for code
paths not reachable via tdc) and it does find issues sometimes before
we send out.
If a human had done the same, wouldn't the protocol be for us to add this tag?

cheers,
jamal

Re: [PATCH net] net/sched: cls_api: reclaim an empty proto on the error path

[Jakub Kicinski]

On Sat, 26 Sep 2026 05:49:57 -0400 Jamal Hadi Salim wrote:
> On Fri, Sep 25, 2026 at 8:18 PM 'Jakub Kicinski' via Hyper-Yielding
> Back-end Review & Insight System <hybris@mojatatu.com> wrote:
> >
> > On Thu, 24 Sep 2026 04:32:49 -0400 Jamal Hadi Salim wrote:  
> > > Tested-by: hybris <hybris@mojatatu.ai>  
> >
> > Please stop adding these tags, they tell us absolutely nothing.  
> 
> This says that our _automated_ bot tested and it adds the tag before
> we send the patch.
> It does more than tdc for a specific patch set (e.g., PoCs for code
> paths not reachable via tdc) and it does find issues sometimes before
> we send out.

My main point is what I said - it tells us nothing upstream.
It's like a gerrit change-id tag, internal to mojatatu.

> If a human had done the same, wouldn't the protocol be for us to add this tag?

LLM != human.

Re: [PATCH net] net/sched: cls_api: reclaim an empty proto on the error path

[Jamal Hadi Salim]

On Mon, Sep 28, 2026 at 8:47 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Sat, 26 Sep 2026 05:49:57 -0400 Jamal Hadi Salim wrote:
> > On Fri, Sep 25, 2026 at 8:18 PM 'Jakub Kicinski' via Hyper-Yielding
> > Back-end Review & Insight System <hybris@mojatatu.com> wrote:
> > >
> > > On Thu, 24 Sep 2026 04:32:49 -0400 Jamal Hadi Salim wrote:
> > > > Tested-by: hybris <hybris@mojatatu.ai>
> > >
> > > Please stop adding these tags, they tell us absolutely nothing.
> >
> > This says that our _automated_ bot tested and it adds the tag before
> > we send the patch.
> > It does more than tdc for a specific patch set (e.g., PoCs for code
> > paths not reachable via tdc) and it does find issues sometimes before
> > we send out.
>
> My main point is what I said - it tells us nothing upstream.
> It's like a gerrit change-id tag, internal to mojatatu.
>

Fair enough. The idea was to provide confidence that what we post is
going through some serious testing.
A couple of times in this flood something slipped through that missed
a subtle test (which resulted in a Vx) which this bot doesnt miss.
So implicitly i suppose it means if we dont say "tested-by" it means
we hae tested it?

> > If a human had done the same, wouldn't the protocol be for us to add this tag?
>
> LLM != human.

Well.. this specific bot is mostly muscle (scripts) than brain. But
let's see how long that " LLM != human" inequality lasts ;->

cheers,
jamal