Linux has traditionally accepted random garbage in the flags argument to
the open syscall (including the later added openat). This really harms
when adding new flags, because applications can't just probe for the
flag to actually work. While rejecting unknown flags is an ABI change
strictly speaking I can't see what would actually get broken by it
in practice, so by the Linux rules it might not be an issue.
Below is the trivial series to reject unknown flags. If this is not
acceptable there migh be some other ways, although they seem ugly:
(a) add a new openat2 system call that enforces this behavior, and
hope all majors libcs switch to using that by default to implement
open(3).
(b) add a new personality flag to enforce this behavior (or maybe
opt in by default and allow admins to opt out of it)
Add a central define for all valid open flags, and use it in the uniqueness
check.
Signed-off-by: Christoph Hellwig <[email protected]>
---
fs/fcntl.c | 14 ++++----------
include/linux/fcntl.h | 6 ++++++
2 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/fs/fcntl.c b/fs/fcntl.c
index be8fbe289087..de1b16bb6a29 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -742,16 +742,10 @@ static int __init fcntl_init(void)
* Exceptions: O_NONBLOCK is a two bit define on parisc; O_NDELAY
* is defined as O_NONBLOCK on some platforms and not on others.
*/
- BUILD_BUG_ON(21 - 1 /* for O_RDONLY being 0 */ != HWEIGHT32(
- O_RDONLY | O_WRONLY | O_RDWR |
- O_CREAT | O_EXCL | O_NOCTTY |
- O_TRUNC | O_APPEND | /* O_NONBLOCK | */
- __O_SYNC | O_DSYNC | FASYNC |
- O_DIRECT | O_LARGEFILE | O_DIRECTORY |
- O_NOFOLLOW | O_NOATIME | O_CLOEXEC |
- __FMODE_EXEC | O_PATH | __O_TMPFILE |
- __FMODE_NONOTIFY
- ));
+ BUILD_BUG_ON(21 - 1 /* for O_RDONLY being 0 */ !=
+ HWEIGHT32(
+ (VALID_OPEN_FLAGS & ~(O_NONBLOCK | O_NDELAY)) |
+ __FMODE_EXEC | __FMODE_NONOTIFY));
fasync_cache = kmem_cache_create("fasync_cache",
sizeof(struct fasync_struct), 0, SLAB_PANIC, NULL);
diff --git a/include/linux/fcntl.h b/include/linux/fcntl.h
index 76ce329e656d..1b48d9c9a561 100644
--- a/include/linux/fcntl.h
+++ b/include/linux/fcntl.h
@@ -3,6 +3,12 @@
#include <uapi/linux/fcntl.h>
+/* list of all valid flags for the open/openat flags argument: */
+#define VALID_OPEN_FLAGS \
+ (O_RDONLY | O_WRONLY | O_RDWR | O_CREAT | O_EXCL | O_NOCTTY | O_TRUNC | \
+ O_APPEND | O_NDELAY | O_NONBLOCK | O_NDELAY | __O_SYNC | O_DSYNC | \
+ FASYNC | O_DIRECT | O_LARGEFILE | O_DIRECTORY | O_NOFOLLOW | \
+ O_NOATIME | O_CLOEXEC | O_PATH | __O_TMPFILE)
#ifndef force_o_largefile
#define force_o_largefile() (BITS_PER_LONG != 32)
--
2.11.0
This way userspace can probe for actually supported flags.
Signed-off-by: Christoph Hellwig <[email protected]>
---
fs/open.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/fs/open.c b/fs/open.c
index 949cef29c3bb..9106ed7310f0 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -900,6 +900,9 @@ static inline int build_open_flags(int flags, umode_t mode, struct open_flags *o
int lookup_flags = 0;
int acc_mode = ACC_MODE(flags);
+ if (flags & ~VALID_OPEN_FLAGS)
+ return -EINVAL;
+
if (flags & (O_CREAT | __O_TMPFILE))
op->mode = (mode & S_IALLUGO) | S_IFREG;
else
--
2.11.0
On Thu, Mar 30, 2017 at 9:33 AM, Christoph Hellwig <[email protected]> wrote:
> This way userspace can probe for actually supported flags.
No. Not this way.
First off, since we've never checked the flags, it really is likely
that somebody just by mistake passes in garbage.
So it might cause a regression, which means we might need to revert
it, which in turn means that we sure as hell do *not* want to
encourage _other_ people to then use this to "probe" the accepted
flags.
Secondly, since we know old kernels don't test the flags, it is
*doubly* stupid to then talk about "probing accepted flags".
So the whole concept of probing is pure and utter f*cking garbage.
So get that idiotic idea out of your head.
What might be acceptable is to say "we should have not accepted random
flags to begin with", and add this error case, but realize that
probing for those flags is completely idiotic and moronic.
Once you do that, you can then say "to make it easier to see if
somebody might have passed in garbage that just happened to work, we
can add a WARN_ON_ONCE()" for this case. That has the added advantage
that it hopefully makes people understand just how stipid that idiotic
"probe flags" idea was.
Anyway, big NAK on this idiotic patch series, since as is the whole
concept and reasoning for it is crazy crap.
People, you need to really understand and INTERNALIZE that backwards
compatibility is important.
You need to understand it so well that you go "wow, this whole idea
about probing was obviously shit".
Really.
Linus
On Thu, Mar 30, 2017 at 9:33 AM, Christoph Hellwig <[email protected]> wrote:
This really harms
> when adding new flags, because applications can't just probe for the
> flag to actually work.
Side note: this whole argument is also incredibly idiotic from the
very beginning, regardless of the backwards compatibility issue.
But probing for flags is why we *could* add things like O_NOATIME etc
- exactly because it "just worked" with old kernels, and people could
just use the new flags knowing that it was a no-op on old kernels.
The whole concept of "probing for supported features" is very suspect.
It's a bad bad idea. Don't do it.
What kind of new flag did you even have in mind that would have such
broken semantics that it would completely change the other flags?
Becuase now I'm starting to think that the whole series has an even
deeper bug: stupid new features that were badly thought out and not
even described.
Linus
On Thu, Mar 30, 2017 at 10:08:10AM -0700, Linus Torvalds wrote:
> What kind of new flag did you even have in mind that would have such
> broken semantics that it would completely change the other flags?
> Becuase now I'm starting to think that the whole series has an even
> deeper bug: stupid new features that were badly thought out and not
> even described.
Failure atomic file updates, aka O_ATOMIC:
https://lwn.net/Articles/573092/
Currently the way to probe for it is a new ioctl to check if atomicy
is offered. This should work, but it's rather fragile..
On Thu, Mar 30, 2017 at 10:21 AM, Christoph Hellwig <[email protected]> wrote:
>
> Failure atomic file updates, aka O_ATOMIC:
>
> https://lwn.net/Articles/573092/
>
> Currently the way to probe for it is a new ioctl to check if atomicy
> is offered. This should work, but it's rather fragile..
So quite frankly, I'd much rather see that people who really want to
check would instead just
fd = open(... O_ATOMIC);
if (fd < 0)
.. regular error handling ..
/* Did we actually get O_ATOMIC? */
if (!(O_ATOMIC & fnctl(fd, F_GETFL, NULL)))
.. warn about lack of O_ATOMIC ..
because I suspect that you will find users that might *want* atomic
behavior, but in the absence of atomicity guarantees will want to
still be able to do IO.
The above kind of model seems much more straightforward, and has no
backwards/forwards compatibility issues I can see.
I'm assuming you'd also possible want to be able to use F_SETFL to set
O_ATOMIC after the fact (independently of the open - I could see tools
like "dd" growing an atomic flag and setting it on stdout), so the
F_GETFL interface seems natural for that reason too.
Linus
On Thu, Mar 30, 2017 at 11:19:53AM -0700, Linus Torvalds wrote:
> So quite frankly, I'd much rather see that people who really want to
> check would instead just
>
> fd = open(... O_ATOMIC);
> if (fd < 0)
> .. regular error handling ..
>
> /* Did we actually get O_ATOMIC? */
> if (!(O_ATOMIC & fnctl(fd, F_GETFL, NULL)))
> .. warn about lack of O_ATOMIC ..
>
> because I suspect that you will find users that might *want* atomic
> behavior, but in the absence of atomicity guarantees will want to
> still be able to do IO.
That would be nice, but still won't work as we blindly copy f_flags
into F_GETFL, not even masking our internal FMODE_ bits.
On Thu, Mar 30, 2017 at 11:26 AM, Christoph Hellwig <[email protected]> wrote:
>
> That would be nice, but still won't work as we blindly copy f_flags
> into F_GETFL, not even masking our internal FMODE_ bits.
Ok, *that* is just silly of us, and we could try to just fix, and even backport.
There's no possible valid use I could see where that should break
(famous last words - user code does some damn odd things at times).
Of course, that won't fix old kernels that are out there, but then
neither would your original patch...
Side note: I think you *can* detect the O_ATOMIC support by using
F_SETFL, because F_SETFL only allows you to change flags that we
recognize. So somebody who really wants to *guarantee* that O_ATOMIC
is there and honored even with old kernels could presumable do
something like
fd = open(..); // *no* O_ATOMIC
fcnt(fd, F_SETFL, O_ATOMIC);
if (fcnt(fd, F_GETFL, NULL) & O_ATOMIC)
// Yay! We actually got it
else
// I guess we need to fall back on old behavior
although I agree that that is ridiculously inconvenient and not a
great thing, and it's worth trying to aim for some better model.
Linus
On 03/30/2017 11:19 AM, Linus Torvalds wrote:
> like "dd" growing an atomic flag and setting it on stdout),
'dd' typically assumes that if a flag O_FOO is not #defined by
<fcntl.h>, then 'dd' can "#define O_FOO 0" and the rest of dd's code can
assume O_FOO works "well enough" on older systems. I hope this
backward-compatibility hack will suffice for O_ATOMIC too.
> I'm assuming you'd also possible want to be able to use F_SETFL to set
> O_ATOMIC after the fact
Just for fun, one thread can set O_ATOMIC at the same time another
thread is doing a 'write'....
On Thu, Mar 30, 2017 at 12:02 PM, Paul Eggert <[email protected]> wrote:
>
>> I'm assuming you'd also possible want to be able to use F_SETFL to set
>> O_ATOMIC after the fact
>
> Just for fun, one thread can set O_ATOMIC at the same time another thread is
> doing a 'write'....
I'm sure that falls under "if you break it, you get to keep both
pieces". IOW, I don't think anybody will ever say that the concurrent
write has to have some particular semantics wrt the concurrent
O_ATOMIC. Maybe *part* of the write will be done with some semantics,
and part of the write will be done with other semantics.
My guess is that there is going to be very few O_ATOMIC users anyway,
and they'll very carefully set it once and test it (or not even test
it - just make it be a configuration flag and tell people "don't ask
for O_ATOMIC if your system doesn't support it")
Linus
* Linus Torvalds:
> But probing for flags is why we *could* add things like O_NOATIME etc
> - exactly because it "just worked" with old kernels, and people could
> just use the new flags knowing that it was a no-op on old kernels.
Right, it mostly works for the flags we added.
> The whole concept of "probing for supported features" is very suspect.
> It's a bad bad idea. Don't do it.
It's vastly preferable to hard-coding kernel version dependencies in
source or object code. Particularly if you want to move applications
between different kernel versions (which seems to be all the rage
right now).
The problem with probing for flags is that it's kind of hard to see
which flag causes the failure. Maybe application code could
retroactively apply it with fcntl. This is what we did when there was
no support for O_CLOEXEC, but this was easy because there has been
FD_CLOEXEC in fcntl since basically forever.
> What kind of new flag did you even have in mind that would have such
> broken semantics that it would completely change the other flags?
I think we had to go through some gymnastics to get the desired
behavior for O_TMPFILE and O_PATH.
There's another consideration. open/openat flags are very special in
one regard: glibc tries to implement the system call wrapper in
standard C, with proper <stdarg.h> processing. This means that the
wrapper has to determine whether there is a mode argument or not, and
avoid the va_arg call if it is missing. Consequently, we parse the
flag argument to see if it implies the need for a mode. This broke
with O_TMPFILE.
We don't do that for fcntl, which has exactly the same issue because
F_GETFD has an empty variable argument list. We just call va_arg
beyond the end of the argument list in the F_GETFD case. We don't
check the command argument and use it to adjust the number of va_arg
calls. We should do the same for open/openat because it obviously
works for all architectures we care about in the fcntl case. (This is
independent if there is going to be another system call with different
semantics for handling unknown flags.)
On 03/30/2017 09:45 PM, Linus Torvalds wrote:
> On Thu, Mar 30, 2017 at 11:26 AM, Christoph Hellwig <[email protected]> wrote:
>>
>> That would be nice, but still won't work as we blindly copy f_flags
>> into F_GETFL, not even masking our internal FMODE_ bits.
>
> Ok, *that* is just silly of us, and we could try to just fix, and even backport.
>
> There's no possible valid use I could see where that should break
> (famous last words - user code does some damn odd things at times).
>
> Of course, that won't fix old kernels that are out there, but then
> neither would your original patch...
>
> Side note: I think you *can* detect the O_ATOMIC support by using
> F_SETFL, because F_SETFL only allows you to change flags that we
> recognize. So somebody who really wants to *guarantee* that O_ATOMIC
> is there and honored even with old kernels could presumable do
> something like
>
> fd = open(..); // *no* O_ATOMIC
> fcnt(fd, F_SETFL, O_ATOMIC);
> if (fcnt(fd, F_GETFL, NULL) & O_ATOMIC)
> // Yay! We actually got it
> else
> // I guess we need to fall back on old behavior
>
> although I agree that that is ridiculously inconvenient and not a
> great thing, and it's worth trying to aim for some better model.
>
Perhaps in that case it is time for an F_GETFL2 an F_GET_REAL_FL
that gives you the nice simple user code Linus wanted for new applications.
and solves forward and backwords for applications and Kernels?
Just my $0.017
Boaz
> Linus
>