diff mbox series

[v4,19/46] btrfs: turn on inlinecrypt mount option for encrypt

Message ID a3f216c6e951b8d1b3cb9b96dcd6d44e1c19bd9b.1701468306.git.josef@toxicpanda.com (mailing list archive)
State New, archived
Headers show
Series btrfs: add fscrypt support | expand

Commit Message

Josef Bacik Dec. 1, 2023, 10:11 p.m. UTC 
From: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>

fscrypt's extent encryption requires the use of inline encryption or the
software fallback that the block layer provides; it is rather
complicated to allow software encryption with extent encryption due to
the timing of memory allocations. Thus, if btrfs has ever had a
encrypted file, or when encryption is enabled on a directory, update the
mount flags to include inlinecrypt.

Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/ioctl.c |  3 +++
 fs/btrfs/super.c | 10 ++++++++++
 2 files changed, 13 insertions(+)

Comments

Eric Biggers Dec. 5, 2023, 5:41 a.m. UTC | #1 
On Fri, Dec 01, 2023 at 05:11:16PM -0500, Josef Bacik wrote:
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 0e8e2ca48a2e..48d751011d07 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -4585,6 +4585,9 @@ long btrfs_ioctl(struct file *file, unsigned int
>  		 * state persists.
>  		 */
>  		btrfs_set_fs_incompat(fs_info, ENCRYPT);
> +		if (!(inode->i_sb->s_flags & SB_INLINECRYPT)) {
> +			inode->i_sb->s_flags |= SB_INLINECRYPT;
> +		}
>  		return fscrypt_ioctl_set_policy(file, (const void __user *)arg);
>  	}

Multiple tasks can execute ioctls at the same time, so isn't the lockless
modification of 's_flags' above a data race?

Maybe you should just set SB_INLINECRYPT at mount time only, regardless of the
ENCRYPT feature, so that it doesn't have to be enabled later.

> +	if (btrfs_fs_incompat(fs_info, ENCRYPT)) {
> +		if (IS_ENABLED(CONFIG_FS_ENCRYPTION_INLINE_CRYPT)) {
> +			sb->s_flags |= SB_INLINECRYPT;
> +		} else {
> +			btrfs_err(fs_info, "encryption not supported");
> +			err = -EINVAL;
> +			goto fail_close;
> +		}
> +	}

Why CONFIG_FS_ENCRYPTION_INLINE_CRYPT instead of CONFIG_FS_ENCRYPTION?  I think
you need to make CONFIG_FS_ENCRYPTION select CONFIG_FS_ENCRYPTION_INLINE_CRYPT
when btrfs is enabled anyway, right?

Also, should the error message be clearer?  Like "filesystem has encrypt feature
but kernel doesn't support encryption", or something like that.  Actually,
should that case even be an error?  ext4, for example, allows a filesystem with
the encrypt feature to be mounted even when the kernel doesn't support
encryption.  (It doesn't allow access to encrypted files, of course.)

- Eric
diff mbox series

Patch

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 0e8e2ca48a2e..48d751011d07 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -4585,6 +4585,9 @@  long btrfs_ioctl(struct file *file, unsigned int
 		 * state persists.
 		 */
 		btrfs_set_fs_incompat(fs_info, ENCRYPT);
+		if (!(inode->i_sb->s_flags & SB_INLINECRYPT)) {
+			inode->i_sb->s_flags |= SB_INLINECRYPT;
+		}
 		return fscrypt_ioctl_set_policy(file, (const void __user *)arg);
 	}
 	case FS_IOC_GET_ENCRYPTION_POLICY:
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 39a338e92115..462a23db26af 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -946,6 +946,16 @@  static int btrfs_fill_super(struct super_block *sb,
 		return err;
 	}
 
+	if (btrfs_fs_incompat(fs_info, ENCRYPT)) {
+		if (IS_ENABLED(CONFIG_FS_ENCRYPTION_INLINE_CRYPT)) {
+			sb->s_flags |= SB_INLINECRYPT;
+		} else {
+			btrfs_err(fs_info, "encryption not supported");
+			err = -EINVAL;
+			goto fail_close;
+		}
+	}
+
 	inode = btrfs_iget(sb, BTRFS_FIRST_FREE_OBJECTID, fs_info->fs_root);
 	if (IS_ERR(inode)) {
 		err = PTR_ERR(inode);