netdev - Re: [PATCH][CFT][experimental] net/socket.c: use straight fdget/fdput (resend)

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 for Android: free password hash cracker in your pocket

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

Date: Sat, 25 May 2024 22:07:39 -0700
From: Linus Torvalds <torvalds@...ux-foundation.org>
To: Al Viro <viro@...iv.linux.org.uk>
Cc: netdev@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH][CFT][experimental] net/socket.c: use straight fdget/fdput (resend)

On Sat, 25 May 2024 at 20:45, Al Viro <viro@...iv.linux.org.uk> wrote:
>
> Checking the theory that the important part in sockfd_lookup_light() is
> avoiding needless file refcount operations, not the marginal reduction
> of the register pressure from not keeping a struct file pointer in the
> caller.

Yeah, the register pressure thing is not likely an issue. That said, I
think we can fix it.

> If that's true, we should get the same benefits from straight fdget()/fdput().

Agreed.

That said, your patch is just horrendous.

> +static inline bool fd_empty(struct fd f)
> +{
> +       // better code with gcc that way
> +       return unlikely(!(f.flags | (unsigned long)f.file));
> +}

Ok, this is disgusting. I went all "WTF?"

And then looked at why you would say that testing two different fields
in a 'struct fd' would possibly be better than just checking one.

And I see why that's the case - it basically short-circuits the
(inlined) __to_fd() logic, and the compiler can undo it and look at
the original single-word value.

But it's still disgusting. If there is anything else in between, the
compiler wouldn't then notice any more.

What we *really* should do is have 'struct fd' just *be* that
single-word value, never expand it to two words at all, and instead of
doing "fd.file" we'd do "fd_file(fd)" and "fd_flags(fd)".

Maybe it's not too late to do that?

This is *particularly* true for the socket code that doesn't even want
the 'struct file *' at all outside of that "check that it's a socket,
then turn it into a socket pointer". So _particularly_ in that
context, having a "fd_file()" helper to do the (trivial) unpacking
would work very well.

But even without changing 'struct fd', maybe we could just have
"__fdget()" and friends not return a "unsigned long", but a "struct
rawfd".

Which would is a struct with an unsigned long.

There's actually a possible future standard C++ extension for what we
want to do ("C++ tagged pointers") and while it might make it into C
eventually, we'd have to do it manully with ugly inline helpers (LLVM
does it manually in C++ with a PointerIntPair class).

IOW, we could do something like the attached. I think it's actually
almost a cleanup, and now your socket things can use "struct rawfd"
and that fd_empty() becomes

   static inline bool rawfd_empty(struct rawfd raw)
   { return !raw.word; }

instead. Still somewhat disgusting, but now it's a "C doesn't have
tagged pointers, so we do this by hand" _understandable_ disgusting.

Hmm? The attached patch compiles. It looks "ObviouslyCorrect(tm)". But
it might be "TotallyBroken(tm)". You get the idea.

               Linus

Download attachment "patch.diff" of type "application/x-patch" (3455 bytes)