Re: [f2fs-dev] [PATCH] Revert "ext4 crypto: fix to check feature status before get policy"

[Chao Yu]

Sorry to introduce such issue... :(

On 2019/10/31 3:02, Eric Biggers wrote:
> On Wed, Oct 30, 2019 at 10:51:20AM -0700, Doug Anderson wrote:
>> Hi,
>>
>> On Wed, Oct 30, 2019 at 10:38 AM Eric Biggers  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 
 ---

  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 

Re: [f2fs-dev] [PATCH] Revert "ext4 crypto: fix to check feature status before get policy"

[Theodore Y. Ts'o]

On Sat, Nov 02, 2019 at 03:10:17PM -0700, Guenter Roeck wrote:
> 
> This change is now in our code base:
> 
> https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/5c5b06fded399013b9cce3d504c3d968ee84ab8b
> 
> If the revert has not made it upstream, I would suggest to hold it off
> for the time being. I'll do more testing next week, but as it looks
> like it may no longer be needed, at least not from a Chrome OS
> perspective.

Thanks, I'll hold off on requesting a pull request from Linus for this
change.

- Ted


___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] Revert "ext4 crypto: fix to check feature status before get policy"

[Guenter Roeck via Linux-f2fs-devel]

On Fri, Nov 1, 2019 at 11:17 AM Guenter Roeck  wrote:
[ ... ]
> > Ah, I think I found it:
> >
> > https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/2cbdedd5eca0a57d9596671a99da5fab8e60722b/sys-apps/upstart/files/upstart-1.2-dircrypto.patch
> >
> > The init process does EXT4_IOC_GET_ENCRYPTION_POLICY on /, and if the error 
> > is
> > EOPNOTSUPP, it skips creating the "dircrypto" keyring.  So then cryptohome 
> > can't
> > add keys later.  (Note the error message you got, "Error adding dircrypto 
> > key".)
> >
> > So it looks like the kernel patch broke both that and
> > ext4_dir_encryption_supported().
> >
>
> ext4_dir_encryption_supported() was already changed to use the sysfs
> file, and changing the upstart code to check the sysfs file does
> indeed fix the problem for good. I'll do some more tests and push the
> necessary changes into our code base if I don't hit some other issue.
>

This change is now in our code base:

https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/5c5b06fded399013b9cce3d504c3d968ee84ab8b

If the revert has not made it upstream, I would suggest to hold it off
for the time being. I'll do more testing next week, but as it looks
like it may no longer be needed, at least not from a Chrome OS
perspective.

Guenter


___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] Revert "ext4 crypto: fix to check feature status before get policy"

[Guenter Roeck via Linux-f2fs-devel]

On Thu, Oct 31, 2019 at 9:36 PM Eric Biggers  wrote:
>
> On Thu, Oct 31, 2019 at 10:52:19AM -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Wed, Oct 30, 2019 at 2:59 PM Doug Anderson  wrote:
> > >
> > > Hi,
> > >
> > > On Wed, Oct 30, 2019 at 1:57 PM Eric Biggers  wrote:
> > > >
> > > > FWIW, from reading the Chrome OS code, I think the code you linked to 
> > > > isn't
> > > > where the breakage actually is.  I think it's actually at
> > > > https://chromium.googlesource.com/chromiumos/platform2/+/refs/heads/master/chromeos-common-script/share/chromeos-common.sh#375
> > > > ... where an init script is using the error message printed by 'e4crypt
> > > > get_policy' to decide whether to add -O encrypt to the filesystem or 
> > > > not.
> > > >
> > > > It really should check instead:
> > > >
> > > > [ -e /sys/fs/ext4/features/encryption ]
> > >
> > > OK, I filed  and CCed all the people listed
> > > in the cryptohome "OWNERS" file.  Hopefully one of them can pick this
> > > up as a general cleanup.  Thanks!
> >
> > Just to follow-up: I did a quick test here to see if I could fix
> > "chromeos-common.sh" as you suggested.  Then I got rid of the Revert
> > and tried to login.  No joy.
> >
> > Digging a little deeper, the ext4_dir_encryption_supported() function
> > is called in two places:
> > * chromeos-install
> > * chromeos_startup
> >
> > In my test case I had a machine that I'd already logged into (on a
> > previous kernel version) and I was trying to log into it a second
> > time.  Thus there's no way that chromeos-install could be involved.
> > Looking at chromeos_startup:
> >
> > https://chromium.googlesource.com/chromiumos/platform2/+/refs/heads/master/init/chromeos_startup
> >
> > ...the function is only used for setting up the "encrypted stateful"
> > partition.  That wasn't where my failure was.  My failure was with
> > logging in AKA with cryptohome.  Thus I think it's plausible that my
> > original commit message pointing at cryptohome may have been correct.
> > It's possible that there were _also_ problems with encrypted stateful
> > that I wasn't noticing, but if so they were not the only problems.
> >
> > It still may be wise to make Chrome OS use different tests, but it
> > might not be quite as simple as hoped...
> >
>
> Ah, I think I found it:
>
> https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/2cbdedd5eca0a57d9596671a99da5fab8e60722b/sys-apps/upstart/files/upstart-1.2-dircrypto.patch
>
> The init process does EXT4_IOC_GET_ENCRYPTION_POLICY on /, and if the error is
> EOPNOTSUPP, it skips creating the "dircrypto" keyring.  So then cryptohome 
> can't
> add keys later.  (Note the error message you got, "Error adding dircrypto 
> key".)
>
> So it looks like the kernel patch broke both that and
> ext4_dir_encryption_supported().
>

ext4_dir_encryption_supported() was already changed to use the sysfs
file, and changing the upstart code to check the sysfs file does
indeed fix the problem for good. I'll do some more tests and push the
necessary changes into our code base if I don't hit some other issue.

> I don't see how it could have broken cryptohome by itself, since AFAICS
> cryptohome only uses EXT4_IOC_GET_ENCRYPTION_POLICY on the partition which is
> supposed to have the 'encrypt' feature set.
>
Yes, indeed it seems as if that is unrelated.

Guenter


___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] Revert "ext4 crypto: fix to check feature status before get policy"

[Guenter Roeck via Linux-f2fs-devel]

On Thu, Oct 31, 2019 at 9:36 PM Eric Biggers  wrote:
>
> On Thu, Oct 31, 2019 at 10:52:19AM -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Wed, Oct 30, 2019 at 2:59 PM Doug Anderson  wrote:
> > >
> > > Hi,
> > >
> > > On Wed, Oct 30, 2019 at 1:57 PM Eric Biggers  wrote:
> > > >
> > > > FWIW, from reading the Chrome OS code, I think the code you linked to 
> > > > isn't
> > > > where the breakage actually is.  I think it's actually at
> > > > https://chromium.googlesource.com/chromiumos/platform2/+/refs/heads/master/chromeos-common-script/share/chromeos-common.sh#375
> > > > ... where an init script is using the error message printed by 'e4crypt
> > > > get_policy' to decide whether to add -O encrypt to the filesystem or 
> > > > not.
> > > >
> > > > It really should check instead:
> > > >
> > > > [ -e /sys/fs/ext4/features/encryption ]
> > >
> > > OK, I filed  and CCed all the people listed
> > > in the cryptohome "OWNERS" file.  Hopefully one of them can pick this
> > > up as a general cleanup.  Thanks!
> >
> > Just to follow-up: I did a quick test here to see if I could fix
> > "chromeos-common.sh" as you suggested.  Then I got rid of the Revert
> > and tried to login.  No joy.
> >
> > Digging a little deeper, the ext4_dir_encryption_supported() function
> > is called in two places:
> > * chromeos-install
> > * chromeos_startup
> >
> > In my test case I had a machine that I'd already logged into (on a
> > previous kernel version) and I was trying to log into it a second
> > time.  Thus there's no way that chromeos-install could be involved.
> > Looking at chromeos_startup:
> >
> > https://chromium.googlesource.com/chromiumos/platform2/+/refs/heads/master/init/chromeos_startup
> >
> > ...the function is only used for setting up the "encrypted stateful"
> > partition.  That wasn't where my failure was.  My failure was with
> > logging in AKA with cryptohome.  Thus I think it's plausible that my
> > original commit message pointing at cryptohome may have been correct.
> > It's possible that there were _also_ problems with encrypted stateful
> > that I wasn't noticing, but if so they were not the only problems.
> >
> > It still may be wise to make Chrome OS use different tests, but it
> > might not be quite as simple as hoped...
> >
>
> Ah, I think I found it:
>
> https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/2cbdedd5eca0a57d9596671a99da5fab8e60722b/sys-apps/upstart/files/upstart-1.2-dircrypto.patch
>
> The init process does EXT4_IOC_GET_ENCRYPTION_POLICY on /, and if the error is
> EOPNOTSUPP, it skips creating the "dircrypto" keyring.  So then cryptohome 
> can't
> add keys later.  (Note the error message you got, "Error adding dircrypto 
> key".)
>

Good catch. I'll try replacing that with a check for the sysfs flag
and see if that does the trick.

Guenter

> So it looks like the kernel patch broke both that and
> ext4_dir_encryption_supported().
>
> I don't see how it could have broken cryptohome by itself, since AFAICS
> cryptohome only uses EXT4_IOC_GET_ENCRYPTION_POLICY on the partition which is
> supposed to have the 'encrypt' feature set.
>
> - Eric


___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] Revert "ext4 crypto: fix to check feature status before get policy"

[Eric Biggers]

On Thu, Oct 31, 2019 at 10:52:19AM -0700, Doug Anderson wrote:
> Hi,
> 
> On Wed, Oct 30, 2019 at 2:59 PM Doug Anderson  wrote:
> >
> > Hi,
> >
> > On Wed, Oct 30, 2019 at 1:57 PM Eric Biggers  wrote:
> > >
> > > FWIW, from reading the Chrome OS code, I think the code you linked to 
> > > isn't
> > > where the breakage actually is.  I think it's actually at
> > > https://chromium.googlesource.com/chromiumos/platform2/+/refs/heads/master/chromeos-common-script/share/chromeos-common.sh#375
> > > ... where an init script is using the error message printed by 'e4crypt
> > > get_policy' to decide whether to add -O encrypt to the filesystem or not.
> > >
> > > It really should check instead:
> > >
> > > [ -e /sys/fs/ext4/features/encryption ]
> >
> > OK, I filed  and CCed all the people listed
> > in the cryptohome "OWNERS" file.  Hopefully one of them can pick this
> > up as a general cleanup.  Thanks!
> 
> Just to follow-up: I did a quick test here to see if I could fix
> "chromeos-common.sh" as you suggested.  Then I got rid of the Revert
> and tried to login.  No joy.
> 
> Digging a little deeper, the ext4_dir_encryption_supported() function
> is called in two places:
> * chromeos-install
> * chromeos_startup
> 
> In my test case I had a machine that I'd already logged into (on a
> previous kernel version) and I was trying to log into it a second
> time.  Thus there's no way that chromeos-install could be involved.
> Looking at chromeos_startup:
> 
> https://chromium.googlesource.com/chromiumos/platform2/+/refs/heads/master/init/chromeos_startup
> 
> ...the function is only used for setting up the "encrypted stateful"
> partition.  That wasn't where my failure was.  My failure was with
> logging in AKA with cryptohome.  Thus I think it's plausible that my
> original commit message pointing at cryptohome may have been correct.
> It's possible that there were _also_ problems with encrypted stateful
> that I wasn't noticing, but if so they were not the only problems.
> 
> It still may be wise to make Chrome OS use different tests, but it
> might not be quite as simple as hoped...
> 

Ah, I think I found it:

https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/2cbdedd5eca0a57d9596671a99da5fab8e60722b/sys-apps/upstart/files/upstart-1.2-dircrypto.patch

The init process does EXT4_IOC_GET_ENCRYPTION_POLICY on /, and if the error is
EOPNOTSUPP, it skips creating the "dircrypto" keyring.  So then cryptohome can't
add keys later.  (Note the error message you got, "Error adding dircrypto key".)

So it looks like the kernel patch broke both that and
ext4_dir_encryption_supported().

I don't see how it could have broken cryptohome by itself, since AFAICS
cryptohome only uses EXT4_IOC_GET_ENCRYPTION_POLICY on the partition which is
supposed to have the 'encrypt' feature set.

- Eric


___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] Revert "ext4 crypto: fix to check feature status before get policy"

[Doug Anderson]

Hi,

On Wed, Oct 30, 2019 at 2:59 PM Doug Anderson  wrote:
>
> Hi,
>
> On Wed, Oct 30, 2019 at 1:57 PM Eric Biggers  wrote:
> >
> > FWIW, from reading the Chrome OS code, I think the code you linked to isn't
> > where the breakage actually is.  I think it's actually at
> > https://chromium.googlesource.com/chromiumos/platform2/+/refs/heads/master/chromeos-common-script/share/chromeos-common.sh#375
> > ... where an init script is using the error message printed by 'e4crypt
> > get_policy' to decide whether to add -O encrypt to the filesystem or not.
> >
> > It really should check instead:
> >
> > [ -e /sys/fs/ext4/features/encryption ]
>
> OK, I filed  and CCed all the people listed
> in the cryptohome "OWNERS" file.  Hopefully one of them can pick this
> up as a general cleanup.  Thanks!

Just to follow-up: I did a quick test here to see if I could fix
"chromeos-common.sh" as you suggested.  Then I got rid of the Revert
and tried to login.  No joy.

Digging a little deeper, the ext4_dir_encryption_supported() function
is called in two places:
* chromeos-install
* chromeos_startup

In my test case I had a machine that I'd already logged into (on a
previous kernel version) and I was trying to log into it a second
time.  Thus there's no way that chromeos-install could be involved.
Looking at chromeos_startup:

https://chromium.googlesource.com/chromiumos/platform2/+/refs/heads/master/init/chromeos_startup

...the function is only used for setting up the "encrypted stateful"
partition.  That wasn't where my failure was.  My failure was with
logging in AKA with cryptohome.  Thus I think it's plausible that my
original commit message pointing at cryptohome may have been correct.
It's possible that there were _also_ problems with encrypted stateful
that I wasn't noticing, but if so they were not the only problems.

It still may be wise to make Chrome OS use different tests, but it
might not be quite as simple as hoped...

-Doug


___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] Revert "ext4 crypto: fix to check feature status before get policy"

[Doug Anderson]

Hi,

On Wed, Oct 30, 2019 at 1:57 PM Eric Biggers  wrote:
>
> FWIW, from reading the Chrome OS code, I think the code you linked to isn't
> where the breakage actually is.  I think it's actually at
> https://chromium.googlesource.com/chromiumos/platform2/+/refs/heads/master/chromeos-common-script/share/chromeos-common.sh#375
> ... where an init script is using the error message printed by 'e4crypt
> get_policy' to decide whether to add -O encrypt to the filesystem or not.
>
> It really should check instead:
>
> [ -e /sys/fs/ext4/features/encryption ]

OK, I filed  and CCed all the people listed
in the cryptohome "OWNERS" file.  Hopefully one of them can pick this
up as a general cleanup.  Thanks!

-Doug


___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] Revert "ext4 crypto: fix to check feature status before get policy"

[Eric Biggers]

On Wed, Oct 30, 2019 at 10:51:20AM -0700, Doug Anderson wrote:
> Hi,
> 
> On Wed, Oct 30, 2019 at 10:38 AM Eric Biggers  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 
> > > ---
> > >
> > >  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