Re: [PATCH] do_open(): Fix O_DIRECTORY | O_CREAT behavior

[Linus Torvalds]

On Tue, Mar 21, 2023 at 1:16 PM Christian Brauner <brauner@xxxxxxxxxx> wrote:
>
> But yes, that is a valid complaint so - without having tested - sm like:

I'd actually go a bit further, and really spell all the bits out explicitly.

I mean, I was *literally* involved in that original O_TMPFILE_MASK thing:

   https://lore.kernel.org/all/CA+55aFxA3qoM5wpMUya7gEA8SZyJep7kMBRjrPOsOm_OudD8aQ@xxxxxxxxxxxxxx/

with the whole O_DIRECOTY games to make O_TMPFILE safer, but despite
that I didn't remember this at all, and my suggested "maybe something
like this" patch was broken for the O_TMPFILE case.

So while we do have all this documented in our history (both git
commit logs and lore.kernel.org), I actually think it would be lovely
to just make build_open_flags() be very explicit about all the exact
O_xyz flags, and really write out the logic fully.

For example, even your clarified version that gets rid of the
"O_TMPFILE_MASK" thing still eends up doing

        if (flags & __O_TMPFILE) {
                if ((flags & O_TMPFILE) != O_TMPFILE)
                        return -EINVAL;

and so when you look at that code, you don't actually realize that
O_TMPFILE _cotnains_ that __O_TMPFILE bit, and what the above really
means is "also check O_DIRECTORY".

So considering how I couldn't remember this mess myself, despite
having been involved with it personally (a decade ago..), I really do
think that maybe this shoudl be open-coded with a comment, and the
above code should instead be

        if (flags & __O_TMPFILE) {
                if (!(flags & O_DIRECTORY))
                        return -EINVAL;

together with an explicit comment about how O_TMPFILE is the
*combination* of __O_TMPFILE and O_DIRECTORY, along with a short
explanation as to why.

Now, I agree that that test for O_DIRECTORY then _looks_ odd, but the
thing is, it then makes the reality of this all much more explicit.

In contrast, doing that

                if ((flags & O_TMPFILE) != O_TMPFILE)

may *look* more natural in that context, but if you actually start
thinking about it, that check makes no sense unless you then look up
what O_TMPFILE is, and the history behind it.

So I'd rather have code that looks a bit odd, but that explains itself
and is explicit about what it does, than code that _tries_ to look
natural but actually hides the reason for what it is doing.

And then next time somebody looks at that O_DIRECTORY | O_CREAT
combination, suddenly the __O_TMPFILE interaction is there, and very
explicit.

Hmm?

I don't feel *hugely* strongly about this, so in the end I'll bow to
your decision, but considering that my initial patch looked sane but
was buggy because I had forgotten about O_TMPFILE, I really think we
should make this more explicit at a source level..

               Linus

            Linus