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

[Christian Brauner <brauner@kernel.org>]

On Tue, Mar 21, 2023 at 10:35:47AM -0700, Linus Torvalds wrote:
> 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.

So I tried to justify this in the commit message but it might've gotten lost in
there. My thinking had been:

    With this patch, we categorically reject O_DIRECTORY | O_CREAT and
    return EINVAL. That means, we could write this in a way that makes the
    if ((flags & O_TMPFILE_MASK) != O_TMPFILE) check superfluous but let's
    not do that. The check documents the peculiar aspects of O_TMPFILE quite
    nicely and can serve as a reminder how easy it is to break.

But I'm not married to keeping it and it could be misleading.

> 
> 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.

Yes.

> 
> 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.

Yeah, see above why I did that originally.

> 
> 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.

But yes, that is a valid complaint so - without having tested - sm like:

diff --git a/fs/open.c b/fs/open.c
index 4401a73d4032..3c5227d84cfe 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -1195,6 +1195,13 @@ inline int build_open_flags(const struct open_how *how, struct open_flags *op)
                op->mode = 0;
        }

+       /*
+        * Block nonsensical creation requests and ensure that O_CREAT isn't
+        * slipped alongside O_TMPFILE which relies on O_DIRECTORY.
+        */
+       if ((flags & (O_DIRECTORY | O_CREAT)) == (O_DIRECTORY | O_CREAT))
+               return -EINVAL;
+
        /*
         * In order to ensure programs get explicit errors when trying to use
         * O_TMPFILE on old kernels, O_TMPFILE is implemented such that it
@@ -1202,7 +1209,7 @@ inline int build_open_flags(const struct open_how *how, struct open_flags *op)
         * have to require userspace to explicitly set it.
         */
        if (flags & __O_TMPFILE) {
-               if ((flags & O_TMPFILE_MASK) != O_TMPFILE)
+               if ((flags & O_TMPFILE) != O_TMPFILE)
                        return -EINVAL;
                if (!(acc_mode & MAY_WRITE))
                        return -EINVAL;
diff --git a/include/uapi/asm-generic/fcntl.h b/include/uapi/asm-generic/fcntl.h
index 1ecdb911add8..80f37a0d40d7 100644
--- a/include/uapi/asm-generic/fcntl.h
+++ b/include/uapi/asm-generic/fcntl.h
@@ -91,7 +91,6 @@

 /* a horrid kludge trying to make sure that this will fail on old kernels */
 #define O_TMPFILE (__O_TMPFILE | O_DIRECTORY)
-#define O_TMPFILE_MASK (__O_TMPFILE | O_DIRECTORY | O_CREAT)

 #ifndef O_NDELAY
 #define O_NDELAY       O_NONBLOCK
diff --git a/tools/include/uapi/asm-generic/fcntl.h b/tools/include/uapi/asm-generic/fcntl.h
index b02c8e0f4057..1c7a0f6632c0 100644
--- a/tools/include/uapi/asm-generic/fcntl.h
+++ b/tools/include/uapi/asm-generic/fcntl.h
@@ -91,7 +91,6 @@

 /* a horrid kludge trying to make sure that this will fail on old kernels */
 #define O_TMPFILE (__O_TMPFILE | O_DIRECTORY)
-#define O_TMPFILE_MASK (__O_TMPFILE | O_DIRECTORY | O_CREAT)

 #ifndef O_NDELAY
 #define O_NDELAY       O_NONBLOCK
--
2.34.1