date:20191103

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

2019-11-03 Thread 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 10/10] errno.h: Provide EFSCORRUPTED for everybody

2019-11-03 Thread Gao Xiang

On Sun, Nov 03, 2019 at 08:45:06PM -0500, Valdis Kletnieks wrote:
> There's currently 6 filesystems that have the same #define. Move it
> into errno.h so it's defined in just one place.
> 
> Signed-off-by: Valdis Kletnieks 
> Acked-by: Darrick J. Wong 
> Reviewed-by: Jan Kara 
> Acked-by: Theodore Ts'o 

For EROFS part,

Acked-by: Gao Xiang 

> ---
>  drivers/staging/exfat/exfat.h| 2 --
>  fs/erofs/internal.h  | 2 --
>  fs/ext4/ext4.h   | 1 -
>  fs/f2fs/f2fs.h   | 1 -
>  fs/xfs/xfs_linux.h   | 1 -
>  include/linux/jbd2.h | 1 -
>  include/uapi/asm-generic/errno.h | 1 +
>  7 files changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/drivers/staging/exfat/exfat.h b/drivers/staging/exfat/exfat.h
> index 72cf40e123de..58b091a077e8 100644
> --- a/drivers/staging/exfat/exfat.h
> +++ b/drivers/staging/exfat/exfat.h
> @@ -30,8 +30,6 @@
>  #undef DEBUG
>  #endif
>  
> -#define EFSCORRUPTED EUCLEAN /* Filesystem is corrupted */
> -
>  #define DENTRY_SIZE  32  /* dir entry size */
>  #define DENTRY_SIZE_BITS 5
>  
> diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
> index 544a453f3076..3980026a8882 100644
> --- a/fs/erofs/internal.h
> +++ b/fs/erofs/internal.h
> @@ -425,7 +425,5 @@ static inline int z_erofs_init_zip_subsystem(void) { 
> return 0; }
>  static inline void z_erofs_exit_zip_subsystem(void) {}
>  #endif   /* !CONFIG_EROFS_FS_ZIP */
>  
> -#define EFSCORRUPTEDEUCLEAN /* Filesystem is corrupted */
> -
>  #endif   /* __EROFS_INTERNAL_H */
>  
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 03db3e71676c..a86c2585457d 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -3396,6 +3396,5 @@ static inline int ext4_buffer_uptodate(struct 
> buffer_head *bh)
>  #endif   /* __KERNEL__ */
>  
>  #define EFSBADCRCEBADMSG /* Bad CRC detected */
> -#define EFSCORRUPTED EUCLEAN /* Filesystem is corrupted */
>  
>  #endif   /* _EXT4_H */
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 4024790028aa..04ebe77569a3 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -3752,6 +3752,5 @@ static inline bool is_journalled_quota(struct 
> f2fs_sb_info *sbi)
>  }
>  
>  #define EFSBADCRCEBADMSG /* Bad CRC detected */
> -#define EFSCORRUPTED EUCLEAN /* Filesystem is corrupted */
>  
>  #endif /* _LINUX_F2FS_H */
> diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h
> index ca15105681ca..3409d02a7d21 100644
> --- a/fs/xfs/xfs_linux.h
> +++ b/fs/xfs/xfs_linux.h
> @@ -123,7 +123,6 @@ typedef __u32 xfs_nlink_t;
>  
>  #define ENOATTR  ENODATA /* Attribute not found */
>  #define EWRONGFS EINVAL  /* Mount with wrong filesystem type */
> -#define EFSCORRUPTED EUCLEAN /* Filesystem is corrupted */
>  #define EFSBADCRCEBADMSG /* Bad CRC detected */
>  
>  #define SYNCHRONIZE()barrier()
> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
> index 603fbc4e2f70..69411d7e0431 100644
> --- a/include/linux/jbd2.h
> +++ b/include/linux/jbd2.h
> @@ -1657,6 +1657,5 @@ static inline tid_t  
> jbd2_get_latest_transaction(journal_t *journal)
>  #endif   /* __KERNEL__ */
>  
>  #define EFSBADCRCEBADMSG /* Bad CRC detected */
> -#define EFSCORRUPTED EUCLEAN /* Filesystem is corrupted */
>  
>  #endif   /* _LINUX_JBD2_H */
> diff --git a/include/uapi/asm-generic/errno.h 
> b/include/uapi/asm-generic/errno.h
> index cf9c51ac49f9..1d5ffdf54cb0 100644
> --- a/include/uapi/asm-generic/errno.h
> +++ b/include/uapi/asm-generic/errno.h
> @@ -98,6 +98,7 @@
>  #define  EINPROGRESS 115 /* Operation now in progress */
>  #define  ESTALE  116 /* Stale file handle */
>  #define  EUCLEAN 117 /* Structure needs cleaning */
> +#define  EFSCORRUPTEDEUCLEAN

BTW, minor, how about adding some comments right after EFSCORRUPTED
like the other error codes although it's now an alias...
Just my personal thought.

Thanks,
Gao Xiang

>  #define  ENOTNAM 118 /* Not a XENIX named type file */
>  #define  ENAVAIL 119 /* No XENIX semaphores available */
>  #define  EISNAM  120 /* Is a named type file */
> -- 
> 2.24.0.rc1
> 


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

[f2fs-dev] [PATCH 10/10] errno.h: Provide EFSCORRUPTED for everybody

2019-11-03 Thread Valdis Kletnieks

There's currently 6 filesystems that have the same #define. Move it
into errno.h so it's defined in just one place.

Signed-off-by: Valdis Kletnieks 
Acked-by: Darrick J. Wong 
Reviewed-by: Jan Kara 
Acked-by: Theodore Ts'o 
---
 drivers/staging/exfat/exfat.h| 2 --
 fs/erofs/internal.h  | 2 --
 fs/ext4/ext4.h   | 1 -
 fs/f2fs/f2fs.h   | 1 -
 fs/xfs/xfs_linux.h   | 1 -
 include/linux/jbd2.h | 1 -
 include/uapi/asm-generic/errno.h | 1 +
 7 files changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/staging/exfat/exfat.h b/drivers/staging/exfat/exfat.h
index 72cf40e123de..58b091a077e8 100644
--- a/drivers/staging/exfat/exfat.h
+++ b/drivers/staging/exfat/exfat.h
@@ -30,8 +30,6 @@
 #undef DEBUG
 #endif
 
-#define EFSCORRUPTED   EUCLEAN /* Filesystem is corrupted */
-
 #define DENTRY_SIZE32  /* dir entry size */
 #define DENTRY_SIZE_BITS   5
 
diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
index 544a453f3076..3980026a8882 100644
--- a/fs/erofs/internal.h
+++ b/fs/erofs/internal.h
@@ -425,7 +425,5 @@ static inline int z_erofs_init_zip_subsystem(void) { return 
0; }
 static inline void z_erofs_exit_zip_subsystem(void) {}
 #endif /* !CONFIG_EROFS_FS_ZIP */
 
-#define EFSCORRUPTEDEUCLEAN /* Filesystem is corrupted */
-
 #endif /* __EROFS_INTERNAL_H */
 
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 03db3e71676c..a86c2585457d 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -3396,6 +3396,5 @@ static inline int ext4_buffer_uptodate(struct buffer_head 
*bh)
 #endif /* __KERNEL__ */
 
 #define EFSBADCRC  EBADMSG /* Bad CRC detected */
-#define EFSCORRUPTED   EUCLEAN /* Filesystem is corrupted */
 
 #endif /* _EXT4_H */
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 4024790028aa..04ebe77569a3 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -3752,6 +3752,5 @@ static inline bool is_journalled_quota(struct 
f2fs_sb_info *sbi)
 }
 
 #define EFSBADCRC  EBADMSG /* Bad CRC detected */
-#define EFSCORRUPTED   EUCLEAN /* Filesystem is corrupted */
 
 #endif /* _LINUX_F2FS_H */
diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h
index ca15105681ca..3409d02a7d21 100644
--- a/fs/xfs/xfs_linux.h
+++ b/fs/xfs/xfs_linux.h
@@ -123,7 +123,6 @@ typedef __u32   xfs_nlink_t;
 
 #define ENOATTRENODATA /* Attribute not found */
 #define EWRONGFS   EINVAL  /* Mount with wrong filesystem type */
-#define EFSCORRUPTED   EUCLEAN /* Filesystem is corrupted */
 #define EFSBADCRC  EBADMSG /* Bad CRC detected */
 
 #define SYNCHRONIZE()  barrier()
diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
index 603fbc4e2f70..69411d7e0431 100644
--- a/include/linux/jbd2.h
+++ b/include/linux/jbd2.h
@@ -1657,6 +1657,5 @@ static inline tid_t  
jbd2_get_latest_transaction(journal_t *journal)
 #endif /* __KERNEL__ */
 
 #define EFSBADCRC  EBADMSG /* Bad CRC detected */
-#define EFSCORRUPTED   EUCLEAN /* Filesystem is corrupted */
 
 #endif /* _LINUX_JBD2_H */
diff --git a/include/uapi/asm-generic/errno.h b/include/uapi/asm-generic/errno.h
index cf9c51ac49f9..1d5ffdf54cb0 100644
--- a/include/uapi/asm-generic/errno.h
+++ b/include/uapi/asm-generic/errno.h
@@ -98,6 +98,7 @@
 #defineEINPROGRESS 115 /* Operation now in progress */
 #defineESTALE  116 /* Stale file handle */
 #defineEUCLEAN 117 /* Structure needs cleaning */
+#defineEFSCORRUPTEDEUCLEAN
 #defineENOTNAM 118 /* Not a XENIX named type file */
 #defineENAVAIL 119 /* No XENIX semaphores available */
 #defineEISNAM  120 /* Is a named type file */
-- 
2.24.0.rc1



___
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"

2019-11-03 Thread 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"

Re: [f2fs-dev] [PATCH 10/10] errno.h: Provide EFSCORRUPTED for everybody

[f2fs-dev] [PATCH 10/10] errno.h: Provide EFSCORRUPTED for everybody

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

4 matches

Site Navigation

Mail list logo

Footer information