netdev - Re: [PATCH net 1/1] net: sched: cls_u32: Fix match key mis-addressing

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHk-=wiH27m7UeddwD8JPUxjxXHMs=kv8x1WrLAho=dZ8CUhyg@mail.gmail.com>
Date: Mon, 24 Jul 2023 10:49:33 -0700
From: Linus Torvalds <torvalds@...uxfoundation.org>
To: Jamal Hadi Salim <jhs@...atatu.com>
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, 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 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?

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

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?

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.

               Linus