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@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.. 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@xxxxxxxxxxxxxxxxxxx/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(-)
