On Mon, Oct 21, 2019 at 9:54 PM Omar Sandoval <osandov@xxxxxxxxxxx> wrote: > > On Mon, Oct 21, 2019 at 09:18:13AM +0300, Amir Goldstein wrote: > > CC: Ted > > > > What ever happened to read/write ext4 encrypted data API? > > https://marc.info/?l=linux-ext4&m=145030599010416&w=2 > > > > Can we learn anything from the ext4 experience to improve > > the new proposed API? > > I wasn't aware of these patches, thanks for pointing them out. Ted, do > you have any thoughts about making this API work for fscrypt? > > > On Wed, Oct 16, 2019 at 12:29 AM Omar Sandoval <osandov@xxxxxxxxxxx> wrote: > > > > > > From: Omar Sandoval <osandov@xxxxxx> > > > > > > This adds a new page, rwf_encoded(7), providing an overview of encoded > > > I/O and updates fcntl(2), open(2), and preadv2(2)/pwritev2(2) to > > > reference it. > > > > > > Signed-off-by: Omar Sandoval <osandov@xxxxxx> > > > --- > > > man2/fcntl.2 | 10 +- > > > man2/open.2 | 13 ++ > > > man2/readv.2 | 46 +++++++ > > > man7/rwf_encoded.7 | 297 +++++++++++++++++++++++++++++++++++++++++++++ > > > 4 files changed, 365 insertions(+), 1 deletion(-) > > > create mode 100644 man7/rwf_encoded.7 > > > > > > diff --git a/man2/fcntl.2 b/man2/fcntl.2 > > > index fce4f4c2b..76fe9cc6f 100644 > > > --- a/man2/fcntl.2 > > > +++ b/man2/fcntl.2 > > > @@ -222,8 +222,9 @@ On Linux, this command can change only the > > > .BR O_ASYNC , > > > .BR O_DIRECT , > > > .BR O_NOATIME , > > > +.BR O_NONBLOCK , > > > and > > > -.B O_NONBLOCK > > > +.B O_ENCODED > > > flags. > > > It is not possible to change the > > > .BR O_DSYNC > > > @@ -1803,6 +1804,13 @@ Attempted to clear the > > > flag on a file that has the append-only attribute set. > > > .TP > > > .B EPERM > > > +Attempted to set the > > > +.B O_ENCODED > > > +flag and the calling process did not have the > > > +.B CAP_SYS_ADMIN > > > +capability. > > > +.TP > > > +.B EPERM > > > .I cmd > > > was > > > .BR F_ADD_SEALS , > > > diff --git a/man2/open.2 b/man2/open.2 > > > index b0f485b41..cdd3c549c 100644 > > > --- a/man2/open.2 > > > +++ b/man2/open.2 > > > @@ -421,6 +421,14 @@ was followed by a call to > > > .BR fdatasync (2)). > > > .IR "See NOTES below" . > > > .TP > > > +.B O_ENCODED > > > +Open the file with encoded I/O permissions; > > > > 1. I find the name of the flag confusing. > > Yes, most people don't read documentation so carefully (or at all) > > so they will assume O_ENCODED will affect read/write or that it > > relates to RWF_ENCODED in a similar way that O_SYNC relates > > to RWF_SYNC (i.e. logical OR and not logical AND). > > > > I am not good at naming and to prove it I will propose: > > O_PROMISCUOUS, O_MAINTENANCE, O_ALLOW_ENCODED > > Agreed, the name is misleading. I can't think of anything better than > O_ALLOW_ENCODED, so I'll go with that unless someone comes up with > something better :) > > > 2. While I see no harm in adding O_ flag to open(2) for this > > use case, I also don't see a major benefit in adding it. > > What if we only allowed setting the flag via fcntl(2) which returns > > an error on old kernels? > > Since unlike most O_ flags, O_ENCODED does NOT affect file > > i/o without additional opt-in flags, it is not standard anyway and > > therefore I find that setting it only via fcntl(2) is less error prone. > > If I make this fcntl-only, then it probably shouldn't be through > F_GETFL/F_SETFL (it'd be pretty awkward for an O_ flag to not be valid > for open(), and also awkward to mix some non-O_ flag with O_ flags for > F_GETFL/F_SETFL). So that leaves a couple of options: > > 1. Get/set it with F_GETFD/F_SETFD, which is currently only used for > FD_CLOEXEC. That also silently ignores unknown flags, but as with the > O_ flag option, I don't think that's a big deal for FD_ALLOW_ENCODED. > 2. Add a new fcntl command (F_GETFD2/F_SETFD2?). This seems like > overkill to me. > > However, both of these options are annoying to implement. Ideally, we > wouldn't have to add another flags field to struct file. But, to reuse > f_flags, we'd need to make sure that FD_ALLOW_ENCODED doesn't collide > with other O_ flags, and we'd probably want to hide it from F_GETFL. At > that point, it might as well be an O_ flag. > > It seems to me that it's more trouble than it's worth to make this not > an O_ flag, but please let me know if you see a nice way to do so. > 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; } Not pretty, but hidden inside libc, end-users won't need to be aware of this. Cheers, Amir.
