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

[Eric Biggers <ebiggers@kernel.org>]

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