Re: [f2fs-dev] [PATCH] quota: explicitly forbid quota files from being encrypted

From: Jan Kara <jack@suse.cz>
To: Eric Biggers <ebiggers@kernel.org>
Cc: stable@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net,
	linux-fscrypt@vger.kernel.org, Jan Kara <jack@suse.com>,
	linux-fsdevel@vger.kernel.org, linux-ext4@vger.kernel.org
Subject: Re: [f2fs-dev] [PATCH] quota: explicitly forbid quota files from being encrypted
Date: Tue, 5 Sep 2023 11:11:24 +0200	[thread overview]
Message-ID: <20230905091124.giawwm3tu6fm2buq@quack3> (raw)
In-Reply-To: <20230905003227.326998-1-ebiggers@kernel.org>

On Mon 04-09-23 17:32:27, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Since commit d7e7b9af104c ("fscrypt: stop using keyrings subsystem for
> fscrypt_master_key"), xfstest generic/270 causes a WARNING when run on
> f2fs with test_dummy_encryption in the mount options:
> 
> $ kvm-xfstests -c f2fs/encrypt generic/270
> [...]
> WARNING: CPU: 1 PID: 2453 at fs/crypto/keyring.c:240 fscrypt_destroy_keyring+0x1f5/0x260
> 
> The cause of the WARNING is that not all encrypted inodes have been
> evicted before fscrypt_destroy_keyring() is called, which violates an
> assumption.  This happens because the test uses an external quota file,
> which gets automatically encrypted due to test_dummy_encryption.
> 
> Encryption of quota files has never really been supported.  On ext4,
> ext4_quota_read() does not decrypt the data, so encrypted quota files
> are always considered invalid on ext4.  On f2fs, f2fs_quota_read() uses
> the pagecache, so trying to use an encrypted quota file gets farther,
> resulting in the issue described above being possible.  But this was
> never intended to be possible, and there is no use case for it.
> 
> Therefore, make the quota support layer explicitly reject using
> IS_ENCRYPTED inodes when quotaon is attempted.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Looks good. I'll queue this patch into my tree and send it to Linus for
RC2.

								Honza

> ---
>  fs/quota/dquot.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
> index 9e72bfe8bbad9..7e268cd2727cc 100644
> --- a/fs/quota/dquot.c
> +++ b/fs/quota/dquot.c
> @@ -2339,6 +2339,20 @@ static int vfs_setup_quota_inode(struct inode *inode, int type)
>  	if (sb_has_quota_loaded(sb, type))
>  		return -EBUSY;
>  
> +	/*
> +	 * Quota files should never be encrypted.  They should be thought of as
> +	 * filesystem metadata, not user data.  New-style internal quota files
> +	 * cannot be encrypted by users anyway, but old-style external quota
> +	 * files could potentially be incorrectly created in an encrypted
> +	 * directory, hence this explicit check.  Some reasons why encrypted
> +	 * quota files don't work include: (1) some filesystems that support
> +	 * encryption don't handle it in their quota_read and quota_write, and
> +	 * (2) cleaning up encrypted quota files at unmount would need special
> +	 * consideration, as quota files are cleaned up later than user files.
> +	 */
> +	if (IS_ENCRYPTED(inode))
> +		return -EINVAL;
> +
>  	dqopt->files[type] = igrab(inode);
>  	if (!dqopt->files[type])
>  		return -EIO;
> 
> base-commit: 708283abf896dd4853e673cc8cba70acaf9bf4ea
> -- 
> 2.42.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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