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

[Linus Torvalds <torvalds@linux-foundation.org>]

On Tue, Mar 21, 2023 at 9:17 AM Christian Brauner <brauner@kernel.org> wrote:
>
>  #define WILL_CREATE(flags)     (flags & (O_CREAT | __O_TMPFILE))
> +#define INVALID_CREATE(flags) \
> +       ((flags & (O_DIRECTORY | O_CREAT)) == (O_DIRECTORY | O_CREAT))
>  #define O_PATH_FLAGS           (O_DIRECTORY | O_NOFOLLOW | O_PATH | O_CLOEXEC)
>
>  inline struct open_how build_open_how(int flags, umode_t mode)
> @@ -1207,6 +1209,10 @@ inline int build_open_flags(const struct open_how *how, struct open_flags *op)
>                 if (!(acc_mode & MAY_WRITE))
>                         return -EINVAL;
>         }
> +
> +       if (INVALID_CREATE(flags))
> +               return -EINVAL;
> +
>         if (flags & O_PATH) {
>                 /* O_PATH only permits certain other flags to be set. */
>                 if (flags & ~O_PATH_FLAGS)

So the patch looks simple enough, but

 (a) I'm not entirely sure I like the extra indirection through
another #define. This impenetrable thicket of different macros makes
it a bit hard to see what is going on. I'm not blaming you for it, it
predates this patch, but..

 (b) this seems to make that O_TMPFILE_MASK macro pointless.

I think (b) kind of re-inforces the point of (a) here.

The only reason for O_TMPFILE_MASK is literally that old historical
"make sure old kernels return errors when they don't support
O_TEMPFILE", and thus the magic re-use of old bit patterns.

But now that we do that "return error if both O_DIRECTORY and O_CREAT
are set", the O_TMPFILE_MASK check is basically dead, because it ends
up checking for that same bit pattern except also __O_TMPFILE.

And that is *not* obvious from the code, exactly because of that
thicket of different macros.

In fact, since that whole

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

is done inside an "if (flags & __O_TMPFILE)", the compiler might as
well reduce it *exactly* down to that exact same test as
INVALID_CREATE() now is.

So I really get the feeling that the macros actually hide what is
going on, and are the exact opposite of being helpful. Case in point:
with your patch, you now have the exact same test twice in a row,
except it *looks* like two different tests and one of them is
conditional on __O_TMPFILE.

For all I know, the compiler may actually notice the redundancy and
remove one of them, but we shouldn't write bad code with the
expectation that "the compiler will fix it up". Particularly when it
just makes it harder for people to understand too.

                     Linus