On Wed, Oct 30, 2019 at 03:46:06PM -0700, Omar Sandoval wrote:
> On Wed, Oct 23, 2019 at 11:12:03PM +1100, Aleksa Sarai wrote:
> > On 2019-10-23, Amir Goldstein <amir73il@xxxxxxxxx> wrote:
> > > > >
> > > > > No, I see why you choose to add the flag to open(2).
> > > > > I have no objection.
> > > > >
> > > > > I once had a crazy thought how to add new open flags
> > > > > in a non racy manner without adding a new syscall,
> > > > > but as you wrote, this is not relevant for O_ALLOW_ENCODED.
> > > > >
> > > > > Something like:
> > > > >
> > > > > /*
> > > > > * Old kernels silently ignore unsupported open flags.
> > > > > * New kernels that gets __O_CHECK_NEWFLAGS do
> > > > > * the proper checking for unsupported flags AND set the
> > > > > * flag __O_HAVE_NEWFLAGS.
> > > > > */
> > > > > #define O_FLAG1 __O_CHECK_NEWFLAGS|__O_FLAG1
> > > > > #define O_HAVE_FLAG1 __O_HAVE_NEWFLAGS|__O_FLAG1
> > > > >
> > > > > fd = open(path, O_FLAG1);
> > > > > if (fd < 0)
> > > > > return -errno;
> > > > > flags = fcntl(fd, F_GETFL, 0);
> > > > > if (flags < 0)
> > > > > return flags;
> > > > > if ((flags & O_HAVE_FLAG1) != O_HAVE_FLAG1) {
> > > > > close(fd);
> > > > > return -EINVAL;
> > > > > }
> > > >
> > > > You don't need to add __O_HAVE_NEWFLAGS to do this -- this already works
> > > > today for userspace to check whether a flag works properly
> > > > (specifically, __O_FLAG1 will only be set if __O_FLAG1 is supported --
> > > > otherwise it gets cleared during build_open_flags).
> > >
> > > That's a behavior of quite recent kernels since
> > > 629e014bb834 fs: completely ignore unknown open flags
> > > and maybe some stable kernels. Real old kernels don't have that luxury.
> >
> > Ah okay -- so the key feature is that __O_CHECK_NEWFLAGS gets
> > transformed into __O_HAVE_NEWFLAGS (making it so that both the older and
> > current behaviours are detected). Apologies, I missed that on my first
> > read-through.
> >
> > While it is a little bit ugly, it probably wouldn't be a bad idea to
> > have something like that.
> >
> > > > The problem with adding new flags is that an *old* program running on a
> > > > *new* kernel could pass a garbage flag (__O_CHECK_NEWFLAGS for instance)
> > > > that causes an error only on the new kernel.
> > >
> > > That's a theoretic problem. Same as O_PATH|O_TMPFILE.
> > > Show me a real life program that passes garbage files to open.
> >
> > Has "that's a theoretical problem" helped when we faced this issue in
> > the past? I don't disagree that this is mostly theoretical, but I have a
> > feeling that this is an argument that won't hold water.
> >
> > As for an example of semi-garbage flag passing -- systemd passes
> > O_PATH|O_NOCTTY in several places. Yes, they're known flags (so not
> > entirely applicable to this discussion) but it's also not a meaningful
> > combination of flags and yet is permitted.
> >
> > > > The only real solution to this (and several other problems) is
> > > > openat2().
> > >
> > > No argue about that. Come on, let's get it merged ;-)
> >
> > Believe me, I'm trying. ;)
> >
> > > > As for O_ALLOW_ENCODED -- the current semantics (-EPERM if it
> > > > is set without CAP_SYS_ADMIN) *will* cause backwards compatibility
> > > > issues for programs that have garbage flags set...
> > > >
> > >
> > > Again, that's theoretical. In practice, O_ALLOW_ENCODED can work with
> > > open()/openat(). In fact, even if O_ALLOW_ENCODED gets merged after
> > > openat2(), I don't think it should be forbidden by open()/openat(),
> > > right? Do in that sense, O_ALLOW_ENCODED does not depend on openat2().
> >
> > If it's a valid open() flag it'll also be a valid openat2(2) flag. The
> > only question is whether the garbage-flag problem justifies making it a
> > no-op for open(2).
>
> Consider O_NOATIME: a (non-root) program passing this flag for files it
> didn't own would have been broken by kernel v2.6.8. Or, more recently, a
> program accidentally setting O_TMPFILE would suddenly get drastically
> different behavior on v3.11. These two flags technically broke backwards
> compatibility. I don't think it's worth the trouble to treat
> O_ALLOW_ENCODED any differently for open().
Ah, I missed that O_TMPFILE is careful to fail on old kernels. My point
still stands about O_NOATIME, though :)
