linux-kernel - Re: [GIT PULL] pidfd updates

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-=wjpBq2D97ih_AA0D7+KJ8ihT6WW_cn1BQc43wVgUioH2w@mail.gmail.com>
Date:   Tue, 25 Apr 2023 09:28:54 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Christian Brauner <brauner@...nel.org>
Cc:     Al Viro <viro@...iv.linux.org.uk>, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [GIT PULL] pidfd updates

On Tue, Apr 25, 2023 at 5:34 AM Christian Brauner <brauner@...nel.org> wrote:
>
> Hell, you could even extend that proposal below to wrap the
> put_user()...
>
> struct fd_file {
>         struct file *file;
>         int fd;
>         int __user *fd_user;
> };

So I don't like this extended version, but your proposal patch below
looks good to me.

Why? Simply because the "two-word struct" is actually a good way to
return two values. But a three-word one would be passed on the stack.

Both gcc and clang return small structs (where "small" is literally
just two words) in registers, and it's part of most (all?) ABIs and
we've relied on that before.

It's kind of the same thing as returning a "long long" in two
registers, except it works with structs too.

We've relied on that for ages, with things like 'struct pte' often
being that kind of two-word struct (going back a *loong* time to the
bad old days of 32-bit x86 and PAE).

And in the vfs layer we actually also do it for 'struct fd', which is
a very similar construct.

So yes, doing something like this:

> +struct fd_file {
> +       struct file *file;
> +       int fd;
> +};

would make me happy, and still return just "one" value, it's just that
the value now is both of those things we need.

And then your helpers:

> +static inline void fd_publish(struct fd_file *fdf)
> +static inline void fd_discard(struct fd_file *fdf)

solves my other issue, except I'd literally make it pass those structs
by value, exactly like fdget/fdput does.

Now, since they are inline functions, the code generation doesn't
really change (compilers are smart enough to not actually generate any
pointer stuff), but I prefer to make things like that expliict, and
have source code that matches the code generation.

(Which is also why I do *not* endorse passing bigger structs by value,
because then the compiler will just pass it as a "pointer to a copy"
instead, again violating the whole concept of "source matches what
happens in reality")

I think the above helper could be improved further with Al's
suggestion to make 'fd_publish()' return an error code, and allow the
file pointer (and maybe even the fd index) to be an error pointer (and
error number), so that you could often unify the error/success paths.

IOW, I like this, and I think it's superior to my stupid original suggestion.

                  Linus