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
