Received-SPF: pass (google.com: domain of linux-ext4+bounces-1034-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Tue, 30 Jan 2024 16:55:10 -0800
From: Eric Biggers <ebiggers@kernel.org>
To: Gabriel Krisman Bertazi <krisman@suse.de>
Cc: viro@zeniv.linux.org.uk, jaegeuk@kernel.org, tytso@mit.edu,
	amir73il@gmail.com, linux-ext4@vger.kernel.org,
	linux-f2fs-devel@lists.sourceforge.net,
	linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH v5 06/12] fscrypt: Ignore plaintext dentries during d_move
Message-ID: <20240131005510.GD2020@sol.localdomain>
References: <20240129204330.32346-1-krisman@suse.de>
 <20240129204330.32346-7-krisman@suse.de>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240129204330.32346-7-krisman@suse.de>

On Mon, Jan 29, 2024 at 05:43:24PM -0300, Gabriel Krisman Bertazi wrote:
> Now that we do more than just clear the DCACHE_NOKEY_NAME in
> fscrypt_handle_d_move, skip it entirely for plaintext dentries, to avoid
> extra costs.
> 
> Note that VFS will call this function for any dentry, whether the volume
> has fscrypt on not.  But, since we only care about DCACHE_NOKEY_NAME, we
> can check for that, to avoid touching the superblock for other fields
> that identify a fscrypt volume.
> 
> Note also that fscrypt_handle_d_move is hopefully inlined back into
> __d_move, so the call cost is not significant.  Considering that
> DCACHE_NOKEY_NAME is a fscrypt-specific flag, we do the check in fscrypt
> code instead of the caller.
> 
> Signed-off-by: Gabriel Krisman Bertazi <krisman@suse.de>
> 
> ---
> Changes since v4:
>   - Check based on the dentry itself (eric)
> ---
>  include/linux/fscrypt.h | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
> index c1e285053b3e..ab668760d63e 100644
> --- a/include/linux/fscrypt.h
> +++ b/include/linux/fscrypt.h
> @@ -232,6 +232,15 @@ static inline bool fscrypt_needs_contents_encryption(const struct inode *inode)
>   */
>  static inline void fscrypt_handle_d_move(struct dentry *dentry)
>  {
> +	/*
> +	 * VFS calls fscrypt_handle_d_move even for non-fscrypt
> +	 * filesystems.  Since we only care about DCACHE_NOKEY_NAME
> +	 * dentries here, check that to bail out quickly, if possible.
> +	 */
> +	if (!(dentry->d_flags & DCACHE_NOKEY_NAME))
> +		return;

I think you're over-complicating this a bit.  This should be merged with patch
5, since this is basically fixing patch 5, and the end result should look like:

/*
 * When d_splice_alias() moves a directory's no-key alias to its plaintext alias
 * as a result of the encryption key being added, DCACHE_NOKEY_NAME must be
 * cleared and there might be an opportunity to disable d_revalidate.  Note that
 * we don't have to support the inverse operation because fscrypt doesn't allow
 * no-key names to be the source or target of a rename().
 */
static inline void fscrypt_handle_d_move(struct dentry *dentry)
{
	if (dentry->d_flags & DCACHE_NOKEY_NAME) {
		dentry->d_flags &= ~DCACHE_NOKEY_NAME;
		if (dentry->d_op->d_revalidate == fscrypt_d_revalidate)
			dentry->d_flags &= ~DCACHE_OP_REVALIDATE;
	}
}

Note that checking for NULL dentry->d_op is not necessary, since it's already
been verified that DCACHE_NOKEY_NAME is set, which means fscrypt is in use,
which means that there are dentry_operations.

- Eric