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

[Paolo Abeni <pabeni@redhat.com>]

On 9/9/26 12:22 PM, netdev-bot+sashiko@kernel.org wrote:
> 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" 
FTR: due to the above point, this whole wall of text is pure noise IMHO.
Still there are a few comments referring pre-existing issues that could
deserve some follow-up, please have a look at the full report.

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

/P