From: Doug Anderson <dianders@chromium.org>
To: Eric Biggers <ebiggers@kernel.org>
Cc: Gwendal Grignou <gwendal@chromium.org>, Chao Yu <chao@kernel.org>,
Ryo Hashimoto <hashimoto@chromium.org>,
Vadim Sukhomlinov <sukhomlinov@google.com>,
Guenter Roeck <groeck@chromium.org>,
apronin@chromium.org, linux-doc@vger.kernel.org,
Andreas Dilger <adilger.kernel@dilger.ca>,
"Theodore Y. Ts'o" <tytso@mit.edu>,
Jonathan Corbet <corbet@lwn.net>,
LKML <linux-kernel@vger.kernel.org>,
Jaegeuk Kim <jaegeuk@kernel.org>,
linux-fscrypt@vger.kernel.org, linux-ext4@vger.kernel.org
Subject: Re: [PATCH] Revert "ext4 crypto: fix to check feature status before get policy"
Date: Wed, 30 Oct 2019 10:51:20 -0700 [thread overview]
Message-ID: <CAD=FV=Uzma+eSGG1S1Aq6s3QdMNh4J-c=g-5uhB=0XBtkAawcA@mail.gmail.com> (raw)
In-Reply-To: <20191030173758.GC693@sol.localdomain>
Hi,
On Wed, Oct 30, 2019 at 10:38 AM Eric Biggers <ebiggers@kernel.org> wrote:
>
> Hi Douglas,
>
> On Wed, Oct 30, 2019 at 10:06:25AM -0700, Douglas Anderson wrote:
> > This reverts commit 0642ea2409f3 ("ext4 crypto: fix to check feature
> > status before get policy").
> >
> > The commit made a clear and documented ABI change that is not backward
> > compatible. There exists userspace code [1] that relied on the old
> > behavior and is now broken.
> >
> > While we could entertain the idea of updating the userspace code to
> > handle the ABI change, it's my understanding that in general ABI
> > changes that break userspace are frowned upon (to put it nicely).
> >
> > NOTE: if we for some reason do decide to entertain the idea of
> > allowing the ABI change and updating userspace, I'd appreciate any
> > help on how we should make the change. Specifically the old code
> > relied on the different return values to differentiate between
> > "KeyState::NO_KEY" and "KeyState::NOT_SUPPORTED". I'm no expert on
> > the ext4 encryption APIs (I just ended up here tracking down the
> > regression [2]) so I'd need a bit of handholding from someone.
> >
> > [1] https://chromium.googlesource.com/chromiumos/platform2/+/refs/heads/master/cryptohome/dircrypto_util.cc#73
> > [2] https://crbug.com/1018265
> >
> > Fixes: 0642ea2409f3 ("ext4 crypto: fix to check feature status before get policy")
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> >
> > Documentation/filesystems/fscrypt.rst | 3 +--
> > fs/ext4/ioctl.c | 2 --
> > 2 files changed, 1 insertion(+), 4 deletions(-)
> >
> > diff --git a/Documentation/filesystems/fscrypt.rst b/Documentation/filesystems/fscrypt.rst
> > index 8a0700af9596..4289c29d7c5a 100644
> > --- a/Documentation/filesystems/fscrypt.rst
> > +++ b/Documentation/filesystems/fscrypt.rst
> > @@ -562,8 +562,7 @@ FS_IOC_GET_ENCRYPTION_POLICY_EX can fail with the following errors:
> > or this kernel is too old to support FS_IOC_GET_ENCRYPTION_POLICY_EX
> > (try FS_IOC_GET_ENCRYPTION_POLICY instead)
> > - ``EOPNOTSUPP``: the kernel was not configured with encryption
> > - support for this filesystem, or the filesystem superblock has not
> > - had encryption enabled on it
> > + support for this filesystem
> > - ``EOVERFLOW``: the file is encrypted and uses a recognized
> > encryption policy version, but the policy struct does not fit into
> > the provided buffer
> > diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
> > index 0b7f316fd30f..13d97fb797b4 100644
> > --- a/fs/ext4/ioctl.c
> > +++ b/fs/ext4/ioctl.c
> > @@ -1181,8 +1181,6 @@ long ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> > #endif
> > }
> > case EXT4_IOC_GET_ENCRYPTION_POLICY:
> > - if (!ext4_has_feature_encrypt(sb))
> > - return -EOPNOTSUPP;
> > return fscrypt_ioctl_get_policy(filp, (void __user *)arg);
> >
>
> Thanks for reporting this. Can you elaborate on exactly why returning
> EOPNOTSUPP breaks things in the Chrome OS code? Since encryption is indeed not
> supported, why isn't "KeyState::NOT_SUPPORTED" correct?
I guess all I know is from the cryptohome source code I sent a link
to, which I'm not a super expert in. Did you get a chance to take a
look at that? As far as I can tell the code is doing something like
this:
1. If I see EOPNOTSUPP then this must be a kernel without ext4 crypto.
Fallback to using the old-style ecryptfs.
2. If I see ENODATA then this is a kernel with ext4 crypto but there's
no key yet. We should set a key and (if necessarily) enable crypto on
the filesystem.
3. If I see no error then we're already good.
> Note that the state after this revert will be:
>
> - FS_IOC_GET_ENCRYPTION_POLICY on ext4 => ENODATA
> - FS_IOC_GET_ENCRYPTION_POLICY on f2fs => EOPNOTSUPP
> - FS_IOC_GET_ENCRYPTION_POLICY_EX on ext4 => EOPNOTSUPP
> - FS_IOC_GET_ENCRYPTION_POLICY_EX on f2fs => EOPNOTSUPP
>
> So if this code change is made, the documentation would need to be updated to
> explain that the error code from FS_IOC_GET_ENCRYPTION_POLICY is
> filesystem-specific (which we'd really like to avoid...), and that
> FS_IOC_GET_ENCRYPTION_POLICY_EX handles this case differently. Or else the
> other three would need to be changed to ENODATA -- which for
> FS_IOC_GET_ENCRYPTION_POLICY on f2fs would be an ABI break in its own right,
> though it's possible that no one would notice.
>
> Is your proposal to keep the error filesystem-specific for now?
I guess I'd have to leave it up to the people who know this better.
Mostly I just saw this as an ABI change breaking userspace which to me
means revert. I have very little background here to make good
decisions about the right way to move forward.
> BTW, the crbug.com link is not publicly viewable, so should not be included in
> the commit message.
My apologies. It's public now. Annoyingly they've been experimenting
with making bugs on crbug.com private by default (argh) and I didn't
notice.
-Doug
next prev parent reply other threads:[~2019-10-30 17:51 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-30 17:06 [PATCH] Revert "ext4 crypto: fix to check feature status before get policy" Douglas Anderson
2019-10-30 17:37 ` Eric Biggers
2019-10-30 17:51 ` Doug Anderson [this message]
2019-10-30 19:02 ` Eric Biggers
2019-10-30 20:57 ` Eric Biggers
2019-10-30 21:59 ` Doug Anderson
2019-10-31 17:52 ` Doug Anderson
2019-11-01 4:36 ` Eric Biggers
2019-11-01 13:32 ` Guenter Roeck
2019-11-01 18:17 ` Guenter Roeck
2019-11-02 22:10 ` Guenter Roeck
2019-11-03 13:19 ` Theodore Y. Ts'o
2019-11-04 7:45 ` Chao Yu
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='CAD=FV=Uzma+eSGG1S1Aq6s3QdMNh4J-c=g-5uhB=0XBtkAawcA@mail.gmail.com' \
--to=dianders@chromium.org \
--cc=adilger.kernel@dilger.ca \
--cc=apronin@chromium.org \
--cc=chao@kernel.org \
--cc=corbet@lwn.net \
--cc=ebiggers@kernel.org \
--cc=groeck@chromium.org \
--cc=gwendal@chromium.org \
--cc=hashimoto@chromium.org \
--cc=jaegeuk@kernel.org \
--cc=linux-doc@vger.kernel.org \
--cc=linux-ext4@vger.kernel.org \
--cc=linux-fscrypt@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=sukhomlinov@google.com \
--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).