($INBOX_DIR/description missing)
 help / color / Atom feed
* [PATCH v2] Revert "ext4 crypto: fix to check feature status before get policy"
@ 2019-10-30 21:51 Eric Biggers
  2019-10-31 16:45 ` Theodore Y. Ts'o
  0 siblings, 1 reply; 2+ messages in thread
From: Eric Biggers @ 2019-10-30 21:51 UTC (permalink / raw)
  To: linux-fscrypt
  Cc: linux-ext4, linux-f2fs-devel, Douglas Anderson, Gwendal Grignou,
	Ryo Hashimoto, groeck, apronin, sukhomlinov, Chao Yu,
	Theodore Ts'o

From: Douglas Anderson <dianders@chromium.org>

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][2] 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).

[1] https://chromium.googlesource.com/chromiumos/platform2/+/5993e5c2c2439d7a144863e9c7622736d72771d5/chromeos-common-script/share/chromeos-common.sh#375
[2] https://crbug.com/1018265

[EB: Note, this revert restores an inconsistency between ext4 and f2fs
 and restores the partially incorrect documentation.  Later we should
 try fixing the inconsistency the other way, by changing f2fs instead
 -- or if that won't work either, at least fixing the documentation.

 Also fixed link 1 above to point to the code which actually broke.]

Fixes: 0642ea2409f3 ("ext4 crypto: fix to check feature status before get policy")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Signed-off-by: Eric Biggers <ebiggers@google.com>
---

v2: improved commit message.

 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);
 
 	case FS_IOC_GET_ENCRYPTION_POLICY_EX:
-- 
2.23.0


^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [PATCH v2] Revert "ext4 crypto: fix to check feature status before get policy"
  2019-10-30 21:51 [PATCH v2] Revert "ext4 crypto: fix to check feature status before get policy" Eric Biggers
@ 2019-10-31 16:45 ` Theodore Y. Ts'o
  0 siblings, 0 replies; 2+ messages in thread
From: Theodore Y. Ts'o @ 2019-10-31 16:45 UTC (permalink / raw)
  To: Eric Biggers
  Cc: linux-fscrypt, linux-ext4, linux-f2fs-devel, Douglas Anderson,
	Gwendal Grignou, Ryo Hashimoto, groeck, apronin, sukhomlinov,
	Chao Yu

On Wed, Oct 30, 2019 at 02:51:38PM -0700, Eric Biggers wrote:
> From: Douglas Anderson <dianders@chromium.org>
> 
> 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][2] 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).

The rule is that if someone complains, we have to revert.  Douglas's
email counts as a complaint, so we should revert.  That being said, if
ChromeOS (userspace) changes to using /sys/fs/ext4/features/encryption
to determine whether or not the kernel supports encryption, then we
can in the future change the error code to make things consistent with
f2fs.

This looks good, I'll pull it into ext4 git tree.

						- Ted

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, back to index

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-30 21:51 [PATCH v2] Revert "ext4 crypto: fix to check feature status before get policy" Eric Biggers
2019-10-31 16:45 ` Theodore Y. Ts'o

($INBOX_DIR/description missing)

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-fscrypt/0 linux-fscrypt/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-fscrypt linux-fscrypt/ https://lore.kernel.org/linux-fscrypt \
		linux-fscrypt@vger.kernel.org
	public-inbox-index linux-fscrypt

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-fscrypt


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git