[PATCH net] net/sched: cls_route: free emptied bucket on filter move

[PATCH net] net/sched: cls_route: free emptied bucket on filter move

[Victor Nogueira]

route4_change can move an existing filter to a different top-level
bucket: route4_set_parms recomputes the handle from TCA_ROUTE4_TO/
FROM/IIF, and the handle-mismatch check is gated on the 'new' flag, so
for an existing filter the new handle may differ from the old one and
land in a different bucket. When this happens, the filter is unlinked
from the old bucket, but the bucket itself is never freed once it goes
empty. The stale empty bucket remains in head->table[], causing
route4_delete to report *last=false even after the last live filter is
gone. That pins the empty tcf_proto and causes a leak.

Fix this by refcounting the filters linked to a bucket and freeing the
bucket when the count drops to zero. The existing scan in route4_delete
goes away with it.

The count is updated at all sites that link or unlink a filter during add,
change and delete, and the bucket is dropped from head->table[] as soon as
it reaches zero.

Conditions to recreate the bug:
  CONFIG_NET_CLS_ROUTE4=y, CONFIG_NET_SCH_INGRESS=y, CONFIG_NET_CLS_ACT=y.

  tc qdisc replace dev lo clsact
  tc filter add dev lo ingress protocol ip pref 100 route from 1 to 1
  tc filter change dev lo ingress protocol ip pref 100 handle 0x10001 \
    route from 1 to 2
  tc filter del dev lo ingress protocol ip pref 100 handle 0x10002 \
    route from 1 to 2
  tc filter show dev lo ingress | grep -c 'pref 100 route chain 0 '

Fixes: 1e052be69d04 ("net_sched: destroy proto tp when all filters are gone")
Reported-by: Vega <vega@nebusec.ai>
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Victor Nogueira <victor@mojatatu.com>
---
 net/sched/cls_route.c | 45 +++++++++++++++++++++----------------------
 1 file changed, 22 insertions(+), 23 deletions(-)

diff --git a/net/sched/cls_route.c b/net/sched/cls_route.c
index 0d1324c90583..17b0ebb76662 100644
--- a/net/sched/cls_route.c
+++ b/net/sched/cls_route.c
@@ -11,6 +11,7 @@
 #include <linux/kernel.h>
 #include <linux/string.h>
 #include <linux/errno.h>
+#include <linux/refcount.h>
 #include <linux/skbuff.h>
 #include <net/dst.h>
 #include <net/route.h>
@@ -41,6 +42,7 @@ struct route4_head {
 struct route4_bucket {
 	/* 16 FROM buckets + 16 IIF buckets + 1 wildcard bucket */
 	struct route4_filter __rcu	*ht[16 + 16 + 1];
+	refcount_t			filters_ref;
 	struct rcu_head			rcu;
 };
 
@@ -336,7 +338,7 @@ static int route4_delete(struct tcf_proto *tp, void *arg, bool *last,
 	struct route4_filter *nf;
 	struct route4_bucket *b;
 	unsigned int h = 0;
-	int i, h1;
+	int h1;
 
 	if (!head || !f)
 		return -EINVAL;
@@ -362,23 +364,14 @@ static int route4_delete(struct tcf_proto *tp, void *arg, bool *last,
 			tcf_exts_get_net(&f->exts);
 			tcf_queue_work(&f->rwork, route4_delete_filter_work);
 
-			/* Strip RTNL protected tree */
-			for (i = 0; i <= 32; i++) {
-				struct route4_filter *rt;
-
-				rt = rtnl_dereference(b->ht[i]);
-				if (rt)
-					goto out;
+			if (refcount_dec_and_test(&b->filters_ref)) {
+				RCU_INIT_POINTER(head->table[to_hash(h)], NULL);
+				kfree_rcu(b, rcu);
 			}
-
-			/* OK, session has no flows */
-			RCU_INIT_POINTER(head->table[to_hash(h)], NULL);
-			kfree_rcu(b, rcu);
 			break;
 		}
 	}
 
-out:
 	*last = true;
 	for (h1 = 0; h1 <= 256; h1++) {
 		if (rcu_access_pointer(head->table[h1])) {
@@ -459,6 +452,7 @@ static int route4_set_parms(struct net *net, struct tcf_proto *tp,
 		if (b == NULL)
 			return -ENOBUFS;
 
+		refcount_set(&b->filters_ref, 1);
 		rcu_assign_pointer(head->table[h1], b);
 	} else {
 		unsigned int h2 = from_hash(nhandle >> 16);
@@ -468,6 +462,8 @@ static int route4_set_parms(struct net *net, struct tcf_proto *tp,
 		     fp = rtnl_dereference(fp->next))
 			if (fp->handle == f->handle)
 				return -EEXIST;
+
+		refcount_inc(&b->filters_ref);
 	}
 
 	if (tb[TCA_ROUTE4_TO])
@@ -500,7 +496,7 @@ static int route4_change(struct net *net, struct sk_buff *in_skb,
 	struct route4_filter *fold, *f1, *pfp, *f = NULL;
 	struct route4_bucket *b;
 	struct nlattr *tb[TCA_ROUTE4_MAX + 1];
-	unsigned int h, th;
+	unsigned int h;
 	int err;
 	bool new = true;
 
@@ -560,17 +556,20 @@ static int route4_change(struct net *net, struct sk_buff *in_skb,
 	rcu_assign_pointer(*fp, f);
 
 	if (fold) {
-		th = to_hash(fold->handle);
+		b = fold->bkt;
 		h = from_hash(fold->handle >> 16);
-		b = rtnl_dereference(head->table[th]);
-		if (b) {
-			fp = &b->ht[h];
-			for (pfp = rtnl_dereference(*fp); pfp;
-			     fp = &pfp->next, pfp = rtnl_dereference(*fp)) {
-				if (pfp == fold) {
-					rcu_assign_pointer(*fp, fold->next);
-					break;
+		fp = &b->ht[h];
+		for (pfp = rtnl_dereference(*fp); pfp;
+		     fp = &pfp->next, pfp = rtnl_dereference(*fp)) {
+			if (pfp == fold) {
+				rcu_assign_pointer(*fp, fold->next);
+				if (refcount_dec_and_test(&b->filters_ref)) {
+					unsigned int th = to_hash(fold->handle);
+
+					RCU_INIT_POINTER(head->table[th], NULL);
+					kfree_rcu(b, rcu);
 				}
+				break;
 			}
 		}
 	}
-- 
2.55.0

Re: [PATCH net] net/sched: cls_route: free emptied bucket on filter move

[Paolo Abeni]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
net/sched: cls_route: free emptied bucket on filter move

This adds a refcount of the filters linked into a route4_bucket so the
bucket can be dropped from head->table[] and freed as soon as the last
filter leaves it, replacing the open coded scan in route4_delete().  It
also reworks the fold unlink path in route4_change() to use fold->bkt
directly instead of looking the bucket up by to_hash(fold->handle).

> diff --git a/net/sched/cls_route.c b/net/sched/cls_route.c
> index 0d1324c90583..17b0ebb76662 100644
> --- a/net/sched/cls_route.c
> +++ b/net/sched/cls_route.c

[ ... ]

> @@ -459,6 +452,7 @@ static int route4_set_parms(struct net *net, struct tcf_proto *tp,
>  		if (b == NULL)
>  			return -ENOBUFS;
>  
> +		refcount_set(&b->filters_ref, 1);
>  		rcu_assign_pointer(head->table[h1], b);
>  	} else {
>  		unsigned int h2 = from_hash(nhandle >> 16);
> @@ -468,6 +462,8 @@ static int route4_set_parms(struct net *net, struct tcf_proto *tp,
>  		     fp = rtnl_dereference(fp->next))
>  			if (fp->handle == f->handle)
>  				return -EEXIST;
> +
> +		refcount_inc(&b->filters_ref);
>  	}

[Severity: Medium]
Should this duplicate scan compare against nhandle rather than f->handle?

The destination bucket and chain are selected from nhandle:

net/sched/cls_route.c:route4_set_parms() {
	...
	h1 = to_hash(nhandle);
	b = rtnl_dereference(head->table[h1]);
	...
		unsigned int h2 = from_hash(nhandle >> 16);

		for (fp = rtnl_dereference(b->ht[h2]);
		     fp;
		     fp = rtnl_dereference(fp->next))
			if (fp->handle == f->handle)
				return -EEXIST;
	...
	f->handle = nhandle;
}

but for a change route4_change() has copied the old handle into f before
calling here:

net/sched/cls_route.c:route4_change() {
	...
	if (fold) {
		f->id = fold->id;
		f->iif = fold->iif;
		f->handle = fold->handle;
	...
}

and f->handle is only set to nhandle further down, after this loop.

Taking the reproducer from the changelog one step further on the patched
tree:

  tc filter add ... route from 1 to 1     -> handle 0x10001, bucket 1
  tc filter add ... route from 1 to 2     -> handle 0x10002, bucket 2
  tc filter change ... handle 0x10001 route from 1 to 2

nhandle becomes 0x10002 and new is false, so the mismatch check under
"if (handle && new)" is skipped.  Bucket 2 exists, so the scan above walks
ht[1], finds the filter with handle 0x10002, compares it to f->handle
(still 0x10001), does not match, and the new refcount_inc() is taken.

Does that leave two filters carrying handle 0x10002 in the same chain?
route4_get() returns the first match only:

net/sched/cls_route.c:route4_get() {
	...
		for (f = rtnl_dereference(b->ht[h2]);
		     f;
		     f = rtnl_dereference(f->next))
			if (f->handle == handle)
				return f;
	...
}

so get, change, delete and dump would resolve the handle by list order.

The mirror case looks affected too: an in-place change that keeps the same
to/from computes nhandle == fold->handle, the scan then finds fold itself
in the chain and returns -EEXIST, rejecting the change.

The comparison itself predates this patch, but this patch is what turns the
renumbering/bucket-move path into a supported, leak-free operation, and it
adds code immediately after the faulty comparison.  Would
"fp->handle == nhandle && fp != fold", or rejecting the handle mismatch for
existing filters as well, belong in the same change?

The refcount accounting itself looks exact in the duplicate case, since each
linked filter contributed one reference, so no premature free follows from
this.

Also, the changelog says the handle-mismatch check being gated on the 'new'
flag is what allows the move, but it does not mention that the filter's
handle visible to userspace changes as a result.  Could that be spelled out?

> @@ -560,17 +556,20 @@ static int route4_change(struct net *net, struct sk_buff *in_skb,
>  	rcu_assign_pointer(*fp, f);
>  
>  	if (fold) {
> -		th = to_hash(fold->handle);
> +		b = fold->bkt;
>  		h = from_hash(fold->handle >> 16);
> -		b = rtnl_dereference(head->table[th]);
> -		if (b) {
> -			fp = &b->ht[h];
> -			for (pfp = rtnl_dereference(*fp); pfp;
> -			     fp = &pfp->next, pfp = rtnl_dereference(*fp)) {
> -				if (pfp == fold) {
> -					rcu_assign_pointer(*fp, fold->next);
> -					break;
> +		fp = &b->ht[h];
> +		for (pfp = rtnl_dereference(*fp); pfp;
> +		     fp = &pfp->next, pfp = rtnl_dereference(*fp)) {
> +			if (pfp == fold) {
> +				rcu_assign_pointer(*fp, fold->next);
> +				if (refcount_dec_and_test(&b->filters_ref)) {
> +					unsigned int th = to_hash(fold->handle);
> +
> +					RCU_INIT_POINTER(head->table[th], NULL);
> +					kfree_rcu(b, rcu);
>  				}
> +				break;
>  			}
>  		}
>  	}

[Severity: Low]
Could a tdc case be added for the cross-bucket move this fixes?

tools/testing/selftests/tc-testing/tc-tests/filters/route.json has exactly
one test touching the replace path, "b042" ("Try to delete class referenced
by route after a replace"):

  $TC filter add dev $DEV1 parent 10: prio 1 route from 10 classid 10:1 action ok
  $TC filter replace dev $DEV1 parent 10: prio 1 route from 5 classid 10:1 action drop

With TCA_ROUTE4_TO absent, route4_set_parms() builds nhandle as
0x8000 | id << 16, so the handles are 0xA8000 and 0x58000, and to_hash() is
256 for both.  Both filters land in the same top-level bucket, and the
replaced handle 0x58000 does not exist, so *arg is NULL and the fold path
above is never entered.

That leaves both new free sites untested: this refcount_dec_and_test() plus
kfree_rcu(b, rcu) in route4_change(), and the equivalent one in
route4_delete().  Neither a return of the leak nor a premature bucket free
introduced by the new refcounting would be caught.

The reproducer in the changelog (add "from 1 to 1", change to "from 1 to 2",
delete 0x10002, then count the remaining filters) maps directly onto a tdc
case in route.json.
-- 
This is an AI-generated review.

Re: [PATCH net] net/sched: cls_route: free emptied bucket on filter move

[Victor Nogueira]

On Tue, Sep 1, 2026 at 6:51 AM Paolo Abeni <pabeni@redhat.com> wrote:
>
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://netdev-ai.bots.linux.dev/sashiko/
> [...]
> > diff --git a/net/sched/cls_route.c b/net/sched/cls_route.c
> > index 0d1324c90583..17b0ebb76662 100644
> > --- a/net/sched/cls_route.c
> > +++ b/net/sched/cls_route.c
>
> [ ... ]
>
> > @@ -459,6 +452,7 @@ static int route4_set_parms(struct net *net, struct tcf_proto *tp,
> >               if (b == NULL)
> >                       return -ENOBUFS;
> >
> > +             refcount_set(&b->filters_ref, 1);
> >               rcu_assign_pointer(head->table[h1], b);
> >       } else {
> >               unsigned int h2 = from_hash(nhandle >> 16);
> > @@ -468,6 +462,8 @@ static int route4_set_parms(struct net *net, struct tcf_proto *tp,
> >                    fp = rtnl_dereference(fp->next))
> >                       if (fp->handle == f->handle)
> >                               return -EEXIST;
> > +
> > +             refcount_inc(&b->filters_ref);
> >       }
>
> [Severity: Medium]
> Should this duplicate scan compare against nhandle rather than f->handle?
> The destination bucket and chain are selected from nhandle:
>
> net/sched/cls_route.c:route4_set_parms() {
>         ...
>         h1 = to_hash(nhandle);
>         b = rtnl_dereference(head->table[h1]);
>         ...
>                 unsigned int h2 = from_hash(nhandle >> 16);
>
>                 for (fp = rtnl_dereference(b->ht[h2]);
>                      fp;
>                      fp = rtnl_dereference(fp->next))
>                         if (fp->handle == f->handle)
>                                 return -EEXIST;
>         ...
>         f->handle = nhandle;
> }
>
> but for a change route4_change() has copied the old handle into f before
> calling here:
>
> net/sched/cls_route.c:route4_change() {
>         ...
>         if (fold) {
>                 f->id = fold->id;
>                 f->iif = fold->iif;
>                 f->handle = fold->handle;
>         ...
> }
>
> and f->handle is only set to nhandle further down, after this loop.
>
> Taking the reproducer from the changelog one step further on the patched
> tree:
>
>   tc filter add ... route from 1 to 1     -> handle 0x10001, bucket 1
>   tc filter add ... route from 1 to 2     -> handle 0x10002, bucket 2
>   tc filter change ... handle 0x10001 route from 1 to 2
>
> nhandle becomes 0x10002 and new is false, so the mismatch check under
> "if (handle && new)" is skipped.  Bucket 2 exists, so the scan above walks
> ht[1], finds the filter with handle 0x10002, compares it to f->handle
> (still 0x10001), does not match, and the new refcount_inc() is taken.
>
> Does that leave two filters carrying handle 0x10002 in the same chain?
> [...]
> The comparison itself predates this patch, but this patch is what turns the
> renumbering/bucket-move path into a supported, leak-free operation, and it
> adds code immediately after the faulty comparison.  Would
> "fp->handle == nhandle && fp != fold", or rejecting the handle mismatch for
> existing filters as well, belong in the same change?

This is a pre-existing issue.
It is a bug, but I think it's more orthogonal than Clashiko suggests.
I see 2 separate issues:

- The leak (which this patch fixes)
- Duplicate handles

I don't think this patch makes the example Clashiko described
"supported", it just fixes the leak but leaves the duplicate handles
issue unaddressed.
I believe fixing both in the same patch would make it look confusing.

> [Severity: Low]
> Could a tdc case be added for the cross-bucket move this fixes?

That makes sense.
I can send a v2 with the tdc test and then send another patchset
(including a tdc test) later to fix the duplicate handles issue.
Would that be ok?

cheers,
Victor

Re: [PATCH net] net/sched: cls_route: free emptied bucket on filter move

[Jakub Kicinski]

On Tue, 1 Sep 2026 18:50:00 -0300 Victor Nogueira wrote:
> > Does that leave two filters carrying handle 0x10002 in the same chain?
> > [...]
> > The comparison itself predates this patch, but this patch is what turns the
> > renumbering/bucket-move path into a supported, leak-free operation, and it
> > adds code immediately after the faulty comparison.  Would
> > "fp->handle == nhandle && fp != fold", or rejecting the handle mismatch for
> > existing filters as well, belong in the same change?  
> 
> This is a pre-existing issue.
> It is a bug, but I think it's more orthogonal than Clashiko suggests.
> I see 2 separate issues:
> 
> - The leak (which this patch fixes)
> - Duplicate handles
> 
> I don't think this patch makes the example Clashiko described
> "supported", it just fixes the leak but leaves the duplicate handles
> issue unaddressed.
> I believe fixing both in the same patch would make it look confusing.

Let's fix it in the same series.