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

[Paolo Abeni <pabeni@redhat.com>]

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.