[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20230322101710.6rziolp4sqooqfwq@wittgenstein>
Date: Wed, 22 Mar 2023 11:17:10 +0100
From: Christian Brauner <brauner@...nel.org>
To: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: Pedro Falcato <pedro.falcato@...il.com>,
Alexander Viro <viro@...iv.linux.org.uk>,
linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
Aleksa Sarai <cyphar@...har.com>
Subject: Re: [PATCH] do_open(): Fix O_DIRECTORY | O_CREAT behavior
On Tue, Mar 21, 2023 at 02:47:55PM -0700, Linus Torvalds wrote:
> On Tue, Mar 21, 2023 at 1:16 PM Christian Brauner <brauner@...nel.org> 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@mail.gmail.com/
>
> 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..
I don't feel strongly about this either and your points are valid. So I
incorporated that and updated the comments in the code. In case you'd like to
take another look I've now put this up at:
The following changes since commit e8d018dd0257f744ca50a729e3d042cf2ec9da65:
Linux 6.3-rc3 (2023-03-19 13:27:55 -0700)
are available in the Git repository at:
ssh://git@...olite.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping.git tags/vfs.open.directory.creat.einval
for you to fetch changes up to 43b450632676fb60e9faeddff285d9fac94a4f58:
open: return EINVAL for O_DIRECTORY | O_CREAT (2023-03-22 11:06:55 +0100)
----------------------------------------------------------------
vfs.open.directory.creat.einval
----------------------------------------------------------------
Christian Brauner (1):
open: return EINVAL for O_DIRECTORY | O_CREAT
fs/open.c | 18 +++++++++++++-----
include/uapi/asm-generic/fcntl.h | 1 -
tools/include/uapi/asm-generic/fcntl.h | 1 -
3 files changed, 13 insertions(+), 7 deletions(-)
Powered by blists - more mailing lists