Re: [PATCH net v2 2/4] net/sched: cls_route: Reject handle aliasing

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] net/sched/cls_route.c:route4_set_parms() — the duplicate-
  handle scan rewritten by this patch (line 463: `if (fp->handle ==…

--

net/sched: cls_route: Reject handle aliasing

route4_set_parms() scanned the destination chain for the filter's old
handle instead of the handle it is about to be linked under, so a change
that moves a filter into a chain already holding nhandle missed the
collision and created two filters with the same handle.  The patch makes
the scan compare against nhandle and adds an extack message.

Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260829205422.854785-1-victor%40mojatatu.com

> diff --git a/net/sched/cls_route.c b/net/sched/cls_route.c
> index 17b0ebb766626..9710b77d379c4 100644
> --- a/net/sched/cls_route.c
> +++ b/net/sched/cls_route.c
> @@ -460,8 +460,12 @@ static int route4_set_parms(struct net *net, struct tcf_proto *tp,
>  		for (fp = rtnl_dereference(b->ht[h2]);
>  		     fp;
>  		     fp = rtnl_dereference(fp->next))
> -			if (fp->handle == f->handle)
> +			if (fp->handle == nhandle) {
> +				NL_SET_ERR_MSG_FMT(extack,
> +						   "Handle %x is already in use",
> +						   nhandle);
>  				return -EEXIST;
> +			}

[Severity: Medium]

Should this scan skip the filter that is being replaced?  When
route4_change() is called with an existing filter, fold is still linked in
its bucket chain while route4_set_parms() runs, and f was seeded from it:

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

		f->tp = fold->tp;
		f->bkt = fold->bkt;
		new = false;
	}

	err = route4_set_parms(net, tp, base, f, handle, head, tb,
			       tca[TCA_RATE], new, flags, extack);
	...
}

fold is unlinked from b->ht[] only after route4_set_parms() returns
successfully, so the new scan can walk into fold itself.

Two things seem to follow from that.  First, for an in-place change that
keeps the same to/from, nhandle equals fold->handle and the scan matches
fold, returning -EEXIST for the very filter the user asked to modify.  The
old code compared fp->handle == f->handle, which is also fold->handle, so
the self-match itself is pre-existing and not introduced here, but the new
extack now reports "Handle %x is already in use" for the object being
modified, which reads as a foreign collision.

Second, does this newly allow a filter to be silently renumbered?  The
0x7F00 order bits are only folded in for the create path:

net/sched/cls_route.c:route4_set_parms() {
	...
	if (handle && new) {
		nhandle |= handle & 0x7F00;
	...
}

So for something like

  tc filter change ... pref 100 handle 0x10101 route from 1 to 1 classid 1:9

nhandle comes out as 0x10001 while fold->handle is 0x10101.  to_hash() uses
only id & 0xFF plus the 0x8000 bit, and from_hash(nhandle >> 16) is
unchanged, so both handles land in the same b->ht[] chain and fold really is
visible to the scan.  With the old comparison against f->handle the scan
matched fold and the change was refused; with the comparison against
nhandle it no longer matches, the change is accepted, and

	f->handle = nhandle;

renumbers the filter from 0x10101 to 0x10001.  After that, del or replace by
handle 0x10101 no longer addresses the intended filter.  Was that change of
behaviour intended here?

There is no memory-safety consequence either way: the -EEXIST return happens
before refcount_inc(&b->filters_ref) and before the f->handle / f->bkt
assignments.

For what it is worth, both halves appear to be addressed later in the same
series by "net/sched: cls_route: Fix in-place replace" (ba2f28f8), which
turns the test into

	if (fp != fold && fp->handle == nhandle) {

and carries the order bits over with

	if (handle && (!fold || nhandle == (handle & ~0x7F00)))
		nhandle |= handle & 0x7F00;

so the concern is limited to the intermediate tree state at this commit.
Would it make sense to fold the fp != fold exclusion and the 0x7F00
carry-over into this patch so no bisect point renumbers filters?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260907192133.2639067-1-victor%40mojatatu.com