From: Omar Sandoval <osandov@osandov.com>
To: Aleksa Sarai <cyphar@cyphar.com>
Cc: Amir Goldstein <amir73il@gmail.com>,
linux-fsdevel <linux-fsdevel@vger.kernel.org>,
Linux Btrfs <linux-btrfs@vger.kernel.org>,
Dave Chinner <david@fromorbit.com>, Jann Horn <jannh@google.com>,
Linux API <linux-api@vger.kernel.org>,
kernel-team@fb.com, Theodore Tso <tytso@mit.edu>
Subject: Re: [PATCH man-pages] Document encoded I/O
Date: Wed, 30 Oct 2019 15:46:06 -0700 [thread overview]
Message-ID: <20191030224606.GF326591@vader> (raw)
In-Reply-To: <20191023121203.pozm2xzrbxmcqpbr@yavin.dot.cyphar.com>
On Wed, Oct 23, 2019 at 11:12:03PM +1100, Aleksa Sarai wrote:
> On 2019-10-23, Amir Goldstein <amir73il@gmail.com> 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().
next prev parent reply other threads:[~2019-10-30 22:46 UTC|newest]
Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-15 18:42 [RFC PATCH v2 0/5] fs: interface for directly reading/writing compressed data Omar Sandoval
2019-10-15 18:42 ` [PATCH man-pages] Document encoded I/O Omar Sandoval
2019-10-20 23:05 ` [RFC PATCH v2 0/5] fs: interface for directly reading/writing compressed data Dave Chinner
2019-10-21 19:04 ` Omar Sandoval
2019-10-21 6:18 ` [PATCH man-pages] Document encoded I/O Amir Goldstein
2019-10-21 18:53 ` Omar Sandoval
2019-10-22 6:40 ` Amir Goldstein
2019-10-23 4:44 ` Aleksa Sarai
2019-10-23 6:06 ` Amir Goldstein
2019-10-23 12:12 ` Aleksa Sarai
2019-10-30 22:46 ` Omar Sandoval [this message]
2019-10-30 22:57 ` Omar Sandoval
2019-10-15 18:42 ` [RFC PATCH v2 1/5] fs: add O_ENCODED open flag Omar Sandoval
2019-10-19 4:50 ` Aleksa Sarai
2019-10-23 4:46 ` Aleksa Sarai
2019-10-30 22:55 ` Omar Sandoval
2019-10-30 23:17 ` Aleksa Sarai
2019-10-15 18:42 ` [RFC PATCH v2 2/5] fs: add RWF_ENCODED for reading/writing compressed data Omar Sandoval
2019-10-16 9:50 ` Nikolay Borisov
2019-10-18 22:19 ` Omar Sandoval
2019-10-19 5:01 ` Aleksa Sarai
2019-10-21 18:28 ` Darrick J. Wong
2019-10-21 18:38 ` Aleksa Sarai
2019-10-21 19:00 ` Darrick J. Wong
2019-10-22 1:37 ` Aleksa Sarai
2019-10-30 22:21 ` Omar Sandoval
2019-10-22 2:02 ` Aleksa Sarai
2019-10-30 22:26 ` Omar Sandoval
2019-10-30 23:11 ` Aleksa Sarai
2019-10-21 19:07 ` Omar Sandoval
2019-10-15 18:42 ` [RFC PATCH v2 3/5] btrfs: generalize btrfs_lookup_bio_sums_dio() Omar Sandoval
2019-10-16 9:22 ` Nikolay Borisov
2019-10-18 22:19 ` Omar Sandoval
2019-10-15 18:42 ` [RFC PATCH v2 4/5] btrfs: implement RWF_ENCODED reads Omar Sandoval
2019-10-16 11:10 ` Nikolay Borisov
2019-10-18 22:23 ` Omar Sandoval
2019-10-15 18:42 ` [RFC PATCH v2 5/5] btrfs: implement RWF_ENCODED writes Omar Sandoval
2019-10-16 10:44 ` Nikolay Borisov
2019-10-18 22:55 ` Omar Sandoval
2019-10-18 23:33 ` Omar Sandoval
2019-10-21 13:14 ` David Sterba
2019-10-21 18:05 ` Omar Sandoval
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20191030224606.GF326591@vader \
--to=osandov@osandov.com \
--cc=amir73il@gmail.com \
--cc=cyphar@cyphar.com \
--cc=david@fromorbit.com \
--cc=jannh@google.com \
--cc=kernel-team@fb.com \
--cc=linux-api@vger.kernel.org \
--cc=linux-btrfs@vger.kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=tytso@mit.edu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).