Date: Tue, 25 Jul 2023 13:04:40 -0400
From: Jamal Hadi Salim <jhs@...atatu.com>
To: Linus Torvalds <torvalds@...uxfoundation.org>
Cc: davem@...emloft.net, kuba@...nel.org, edumazet@...gle.com, 
	pabeni@...hat.com, jiri@...nulli.us, xiyou.wangcong@...il.com, 
	netdev@...r.kernel.org, mgcho.minic@...il.com, security@...nel.org
Subject: Re: [PATCH net 1/1] net: sched: cls_u32: Fix match key mis-addressing

On Mon, Jul 24, 2023 at 1:49 PM Linus Torvalds
<torvalds@...uxfoundation.org> wrote:
>
> On Mon, 24 Jul 2023 at 08:19, Jamal Hadi Salim <jhs@...atatu.com> wrote:
> >
> > +               /* The tableid from handle and tableid from htid must match */
> >                 if (TC_U32_HTID(handle) && TC_U32_HTID(handle ^ htid)) {
>
> Well, I guess the comment at least talks about matching.
>
> I still do think that most people aren't going to read
>
>     "Oh, TC_U32_HTID(handle ^ htid) being non-zero means that they
> they differ in the hash table bits".
>
> because the whole trick depends on bit op details, and depends on the
> fact that TC_U32_HTID() is purely a bit masking operation.
>
> But whatever.

I will improve the text leading to this to state that the goal of
building the new handle is to come up with an "address" or "path" for
the table entry. Then the table id mismatch is understood from that
context as part of the rules for building the table entry "address".

> I would also like to see some explanation of this pattern
>
>                 handle = htid | TC_U32_NODE(handle);
>
> and why that "binary or" makes sense. Are the node bits in 'htid'
> guaranteed to be zero?

Per existing user space tools, yes - they are guaranteed to be zero
(per my gitchelogy of both kernel +  iproute2 since inception this has
been the behavior);

> Because if 'htid' can have node bits set, what's the logical reason
> for or'ing things together?
>

Hrm. I am not sure if this is what you are getting to: but you caught
a different bug there.
While none of existing user space tooling(I know of) sends htid with
nodeid set (i.e it is always zero) syzkaller or someone else's poc
could send a non-zero nodeid in the htid. It is not catastrophic but
user expectation will be skewed - both the handle nodeid + htid node
id would be used to derive the nodeid. It will fail from the idr
allocation if the nodeid is in use.
It feels like a second patch though -  rejecting htid with a specified
nodeid (that way expected user space behavior is maintained).

> And why is that variable called 'htid', when clearly it also has the
> bucket ID, and the comment even says we have a valid bucket id?
>

The name could be changed. I am not sure what a good name is but the
semantics are: it carries a minimum of hash table id and at most hash
table id + bucket id. This sounds lame: ht_maybe_plus_bktid ? or make
them two separate variables htid and bktid.

> So I do still find this code to be explicitly written to be confusing.
> It's literally using variable names *designed* to not make sense, and
> have other meanings.
>
> Hmm?
>
> I'm not hating the patch, but when I look at that code I just go "this
> is confusing". And I don't think it's because I'm confused. I think
> it's the code.

I dont think it was intended that way - or maybe i have been taking it
for granted given on/off staring at the code for different reasons
over the years.
If it was to be written today we should certainly have made the 3 ids
independent and really should have allowed only one (instead of two
ways) to specify the different object ids from user space. It is at
least 2 decades old, in those days maybe saving a bit of space was a
good thing.

cheers,
jamal
>                Linus

Powered by blists - more mailing lists