[PATCH v5 00/12] Set casefold/fscrypt dentry operations through sb->s_d_op

[PATCH v5 00/12] Set casefold/fscrypt dentry operations through sb->s_d_op

[Gabriel Krisman Bertazi]

Hi,

Sorry for the quick respin. The only difference from v4 is that we
change the way we check for relevant dentries during a d_move, as
suggested by Eric.

The v5 of this patchset addresses the issues Eric pointed out in the
previous version.  The patch merging the fscrypt lookup helpers was
completely rewritten to avoid the race condition; We also now return
immediately from __fscrypt_handle_d_move; Finally, the overlayfs patch
message was improved.  Further details can be found on the changelog of
each patch.

As usual, this survived fstests on ext4 and f2fs.

---
original cover letter:

When case-insensitive and fscrypt were adapted to work together, we moved the
code that sets the dentry operations for case-insensitive dentries(d_hash and
d_compare) to happen from a helper inside ->lookup.  This is because fscrypt
wants to set d_revalidate only on some dentries, so it does it only for them in
d_revalidate.

But, case-insensitive hooks are actually set on all dentries in the filesystem,
so the natural place to do it is through s_d_op and let d_alloc handle it [1].
In addition, doing it inside the ->lookup is a problem for case-insensitive
dentries that are not created through ->lookup, like those coming
open-by-fhandle[2], which will not see the required d_ops.

This patchset therefore reverts to using sb->s_d_op to set the dentry operations
for case-insensitive filesystems.  In order to set case-insensitive hooks early
and not require every dentry to have d_revalidate in case-insensitive
filesystems, it introduces a patch suggested by Al Viro to disable d_revalidate
on some dentries on the fly.

It survives fstests encrypt and quick groups without regressions.  Based on
v6.7-rc1.

[1] https://lore.kernel.org/linux-fsdevel/20231123195327.GP38156@ZenIV/
[2] https://lore.kernel.org/linux-fsdevel/20231123171255.GN38156@ZenIV/

Gabriel Krisman Bertazi (12):
  ovl: Reject mounting over case-insensitive directories
  fscrypt: Factor out a helper to configure the lookup dentry
  fscrypt: Call fscrypt_prepare_lookup_dentry on unencrypted dentries
  fscrypt: Drop d_revalidate for valid dentries during lookup
  fscrypt: Drop d_revalidate once the key is added
  fscrypt: Ignore plaintext dentries during d_move
  libfs: Merge encrypted_ci_dentry_ops and ci_dentry_ops
  libfs: Add helper to choose dentry operations at mount-time
  ext4: Configure dentry operations at dentry-creation time
  f2fs: Configure dentry operations at dentry-creation time
  ubifs: Configure dentry operations at dentry-creation time
  libfs: Drop generic_set_encrypted_ci_d_ops

 fs/crypto/hooks.c       | 28 +++-----------
 fs/ext4/namei.c         |  1 -
 fs/ext4/super.c         |  1 +
 fs/f2fs/namei.c         |  1 -
 fs/f2fs/super.c         |  1 +
 fs/libfs.c              | 62 +++++++++---------------------
 fs/overlayfs/params.c   | 14 +++++--
 fs/ubifs/dir.c          |  1 -
 fs/ubifs/super.c        |  1 +
 include/linux/fs.h      | 11 +++++-
 include/linux/fscrypt.h | 83 ++++++++++++++++++++++++++++++-----------
 11 files changed, 108 insertions(+), 96 deletions(-)

-- 
2.43.0

[PATCH v5 01/12] ovl: Reject mounting over case-insensitive directories

[Gabriel Krisman Bertazi]

overlayfs relies on the filesystem setting DCACHE_OP_HASH or
DCACHE_OP_COMPARE to reject mounting over case-insensitive directories.

Since commit bb9cd9106b22 ("fscrypt: Have filesystems handle their
d_ops"), we set ->d_op through a hook in ->d_lookup, which
means the root dentry won't have them, causing the mount to accidentally
succeed.

In v6.7-rc7, the following sequence will succeed to mount, but any
dentry other than the root dentry will be a "weird" dentry to ovl and
fail with EREMOTE.

  mkfs.ext4 -O casefold lower.img
  mount -O loop lower.img lower
  mount -t overlay -o lowerdir=lower,upperdir=upper,workdir=work ovl /mnt

Mounting on a subdirectory fails, as expected, because DCACHE_OP_HASH
and DCACHE_OP_COMPARE are properly set by ->lookup.

Fix by explicitly rejecting superblocks that allow case-insensitive
dentries.

While there, re-sort the entries to have more descriptive error messages
first.

Fixes: bb9cd9106b22 ("fscrypt: Have filesystems handle their d_ops")
Signed-off-by: Gabriel Krisman Bertazi <krisman@suse.de>
Acked-by: Amir Goldstein <amir73il@gmail.com>

---
changes since v3:
  - Case insensitive filesystem ->Case insensitive capable
  filesystem (eric)
  - clarify patch summary line
changes since v2:
  - Re-sort checks to trigger more descriptive error messages
  first (Amir)
  - Add code comment (Amir)
---
 fs/overlayfs/params.c | 14 +++++++++++---
 include/linux/fs.h    |  9 +++++++++
 2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/fs/overlayfs/params.c b/fs/overlayfs/params.c
index 3fe2dde1598f..488f920f79d2 100644
--- a/fs/overlayfs/params.c
+++ b/fs/overlayfs/params.c
@@ -280,12 +280,20 @@ static int ovl_mount_dir_check(struct fs_context *fc, const struct path *path,
 {
 	struct ovl_fs_context *ctx = fc->fs_private;
 
-	if (ovl_dentry_weird(path->dentry))
-		return invalfc(fc, "filesystem on %s not supported", name);
-
 	if (!d_is_dir(path->dentry))
 		return invalfc(fc, "%s is not a directory", name);
 
+	/*
+	 * Root dentries of case-insensitive capable filesystems might
+	 * not have the dentry operations set, but still be incompatible
+	 * with overlayfs.  Check explicitly to prevent post-mount
+	 * failures.
+	 */
+	if (sb_has_encoding(path->mnt->mnt_sb))
+		return invalfc(fc, "case-insensitive capable filesystem on %s not supported", name);
+
+	if (ovl_dentry_weird(path->dentry))
+		return invalfc(fc, "filesystem on %s not supported", name);
 
 	/*
 	 * Check whether upper path is read-only here to report failures
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 98b7a7a8c42e..e6667ece5e64 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3203,6 +3203,15 @@ extern int generic_check_addressable(unsigned, u64);
 
 extern void generic_set_encrypted_ci_d_ops(struct dentry *dentry);
 
+static inline bool sb_has_encoding(const struct super_block *sb)
+{
+#if IS_ENABLED(CONFIG_UNICODE)
+	return !!sb->s_encoding;
+#else
+	return false;
+#endif
+}
+
 int may_setattr(struct mnt_idmap *idmap, struct inode *inode,
 		unsigned int ia_valid);
 int setattr_prepare(struct mnt_idmap *, struct dentry *, struct iattr *);
-- 
2.43.0

Re: [PATCH v5 01/12] ovl: Reject mounting over case-insensitive directories

[Eric Biggers]

On Mon, Jan 29, 2024 at 05:43:19PM -0300, Gabriel Krisman Bertazi wrote:
> ovl: Reject mounting over case-insensitive directories

Maybe:

    ovl: Reject mounting over rootdir of case-insensitive capable FS

or:

    ovl: Always reject mounting over case-insensitive directories

... since as your commit message explains, overlayfs already does reject
mounting over case-insensitive directories, just not in all cases.

> Since commit bb9cd9106b22 ("fscrypt: Have filesystems handle their
> d_ops"), we set ->d_op through a hook in ->d_lookup, which
> means the root dentry won't have them, causing the mount to accidentally
> succeed.

But this series changes that.  Doesn't that make this overlayfs fix redundant?
It does improve the error message, which is helpful, but your commit message
makes it sound like it's an actual fix, not just an error message improvement.

- Eric

Re: [PATCH v5 01/12] ovl: Reject mounting over case-insensitive directories

[Gabriel Krisman Bertazi]

Eric Biggers <ebiggers@kernel.org> writes:

> On Mon, Jan 29, 2024 at 05:43:19PM -0300, Gabriel Krisman Bertazi wrote:
>> ovl: Reject mounting over case-insensitive directories
>
> Maybe:
>
>     ovl: Reject mounting over rootdir of case-insensitive capable FS
>
> or:
>
>     ovl: Always reject mounting over case-insensitive directories
>
> ... since as your commit message explains, overlayfs already does reject
> mounting over case-insensitive directories, just not in all cases.
>
>> Since commit bb9cd9106b22 ("fscrypt: Have filesystems handle their
>> d_ops"), we set ->d_op through a hook in ->d_lookup, which
>> means the root dentry won't have them, causing the mount to accidentally
>> succeed.
>
> But this series changes that.  Doesn't that make this overlayfs fix redundant?
> It does improve the error message, which is helpful, but your commit message
> makes it sound like it's an actual fix, not just an error message improvement.

Yes, this series will make it redundant.  But Christian Brauner had
suggested we make this verification explicit instead of relying on d_ops
being set, to avoid future changes breaking this again.  I found the
issue in ovl during testing of v2 and intended to send it separately for
-rc7, but Amir asked it to be sent as part of this series.  And also the
error code is improved.

Anyway, I'll can make this clear in the commit message

-- 
Gabriel Krisman Bertazi

[PATCH v5 02/12] fscrypt: Factor out a helper to configure the lookup dentry

[Gabriel Krisman Bertazi]

Both fscrypt_prepare_lookup_dentry_partial and
fscrypt_prepare_lookup_dentry will set DCACHE_NOKEY_NAME for dentries
when the key is not available. Extract out a helper to set this flag in
a single place, in preparation to also add the optimization that will
disable ->d_revalidate if possible.

Signed-off-by: Gabriel Krisman Bertazi <krisman@suse.de>
---
 fs/crypto/hooks.c       | 18 ++++++++----------
 include/linux/fscrypt.h | 10 ++++++++++
 2 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/fs/crypto/hooks.c b/fs/crypto/hooks.c
index 52504dd478d3..71463cef08f9 100644
--- a/fs/crypto/hooks.c
+++ b/fs/crypto/hooks.c
@@ -102,11 +102,8 @@ int __fscrypt_prepare_lookup(struct inode *dir, struct dentry *dentry,
 	if (err && err != -ENOENT)
 		return err;
 
-	if (fname->is_nokey_name) {
-		spin_lock(&dentry->d_lock);
-		dentry->d_flags |= DCACHE_NOKEY_NAME;
-		spin_unlock(&dentry->d_lock);
-	}
+	fscrypt_prepare_lookup_dentry(dentry, fname->is_nokey_name);
+
 	return err;
 }
 EXPORT_SYMBOL_GPL(__fscrypt_prepare_lookup);
@@ -131,12 +128,13 @@ EXPORT_SYMBOL_GPL(__fscrypt_prepare_lookup);
 int fscrypt_prepare_lookup_partial(struct inode *dir, struct dentry *dentry)
 {
 	int err = fscrypt_get_encryption_info(dir, true);
+	bool is_nokey_name = false;
+
+	if (!err && !fscrypt_has_encryption_key(dir))
+		is_nokey_name = true;
+
+	fscrypt_prepare_lookup_dentry(dentry, is_nokey_name);
 
-	if (!err && !fscrypt_has_encryption_key(dir)) {
-		spin_lock(&dentry->d_lock);
-		dentry->d_flags |= DCACHE_NOKEY_NAME;
-		spin_unlock(&dentry->d_lock);
-	}
 	return err;
 }
 EXPORT_SYMBOL_GPL(fscrypt_prepare_lookup_partial);
diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
index 12f9e455d569..68ca8706483a 100644
--- a/include/linux/fscrypt.h
+++ b/include/linux/fscrypt.h
@@ -948,6 +948,16 @@ static inline int fscrypt_prepare_rename(struct inode *old_dir,
 	return 0;
 }
 
+static inline void fscrypt_prepare_lookup_dentry(struct dentry *dentry,
+						 bool is_nokey_name)
+{
+	if (is_nokey_name) {
+		spin_lock(&dentry->d_lock);
+		dentry->d_flags |= DCACHE_NOKEY_NAME;
+		spin_unlock(&dentry->d_lock);
+	}
+}
+
 /**
  * fscrypt_prepare_lookup() - prepare to lookup a name in a possibly-encrypted
  *			      directory
-- 
2.43.0

Re: [PATCH v5 02/12] fscrypt: Factor out a helper to configure the lookup dentry

[Eric Biggers]

On Mon, Jan 29, 2024 at 05:43:20PM -0300, Gabriel Krisman Bertazi wrote:
> Both fscrypt_prepare_lookup_dentry_partial and
> fscrypt_prepare_lookup_dentry will set DCACHE_NOKEY_NAME for dentries
> when the key is not available. 

Shouldn't this say: "Both fscrypt_prepare_lookup() and
fscrypt_prepare_lookup_partial() set DCACHE_NOKEY_NAME for dentries when the key
is not available."

> @@ -131,12 +128,13 @@ EXPORT_SYMBOL_GPL(__fscrypt_prepare_lookup);
>  int fscrypt_prepare_lookup_partial(struct inode *dir, struct dentry *dentry)
>  {
>  	int err = fscrypt_get_encryption_info(dir, true);
> +	bool is_nokey_name = false;
> +
> +	if (!err && !fscrypt_has_encryption_key(dir))
> +		is_nokey_name = true;

	bool is_nokey_name = (err == 0 && !fscrypt_has_encryption_key(dir));

> diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
> index 12f9e455d569..68ca8706483a 100644
> --- a/include/linux/fscrypt.h
> +++ b/include/linux/fscrypt.h
> @@ -948,6 +948,16 @@ static inline int fscrypt_prepare_rename(struct inode *old_dir,
>  	return 0;
>  }
>  
> +static inline void fscrypt_prepare_lookup_dentry(struct dentry *dentry,
> +						 bool is_nokey_name)

Maybe just fscrypt_prepare_dentry()?

- Eric

[PATCH v5 03/12] fscrypt: Call fscrypt_prepare_lookup_dentry on unencrypted dentries

[Gabriel Krisman Bertazi]

In preparation to dropping DCACHE_OP_REVALIDATE for dentries that
don't need it at lookup time, refactor the code to make unencrypted
denties also call fscrypt_prepare_dentry.  This makes the
non-inline __fscrypt_prepare_lookup superfulous, so drop it.

Signed-off-by: Gabriel Krisman Bertazi <krisman@suse.de>
---
 fs/crypto/hooks.c       | 14 --------------
 include/linux/fscrypt.h | 31 +++++++++++++++----------------
 2 files changed, 15 insertions(+), 30 deletions(-)

diff --git a/fs/crypto/hooks.c b/fs/crypto/hooks.c
index 71463cef08f9..eb870bc162e6 100644
--- a/fs/crypto/hooks.c
+++ b/fs/crypto/hooks.c
@@ -94,20 +94,6 @@ int __fscrypt_prepare_rename(struct inode *old_dir, struct dentry *old_dentry,
 }
 EXPORT_SYMBOL_GPL(__fscrypt_prepare_rename);
 
-int __fscrypt_prepare_lookup(struct inode *dir, struct dentry *dentry,
-			     struct fscrypt_name *fname)
-{
-	int err = fscrypt_setup_filename(dir, &dentry->d_name, 1, fname);
-
-	if (err && err != -ENOENT)
-		return err;
-
-	fscrypt_prepare_lookup_dentry(dentry, fname->is_nokey_name);
-
-	return err;
-}
-EXPORT_SYMBOL_GPL(__fscrypt_prepare_lookup);
-
 /**
  * fscrypt_prepare_lookup_partial() - prepare lookup without filename setup
  * @dir: the encrypted directory being searched
diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
index 68ca8706483a..4aaf847955c0 100644
--- a/include/linux/fscrypt.h
+++ b/include/linux/fscrypt.h
@@ -382,8 +382,6 @@ int __fscrypt_prepare_link(struct inode *inode, struct inode *dir,
 int __fscrypt_prepare_rename(struct inode *old_dir, struct dentry *old_dentry,
 			     struct inode *new_dir, struct dentry *new_dentry,
 			     unsigned int flags);
-int __fscrypt_prepare_lookup(struct inode *dir, struct dentry *dentry,
-			     struct fscrypt_name *fname);
 int fscrypt_prepare_lookup_partial(struct inode *dir, struct dentry *dentry);
 int __fscrypt_prepare_readdir(struct inode *dir);
 int __fscrypt_prepare_setattr(struct dentry *dentry, struct iattr *attr);
@@ -704,13 +702,6 @@ static inline int __fscrypt_prepare_rename(struct inode *old_dir,
 	return -EOPNOTSUPP;
 }
 
-static inline int __fscrypt_prepare_lookup(struct inode *dir,
-					   struct dentry *dentry,
-					   struct fscrypt_name *fname)
-{
-	return -EOPNOTSUPP;
-}
-
 static inline int fscrypt_prepare_lookup_partial(struct inode *dir,
 						 struct dentry *dentry)
 {
@@ -985,14 +976,22 @@ static inline int fscrypt_prepare_lookup(struct inode *dir,
 					 struct dentry *dentry,
 					 struct fscrypt_name *fname)
 {
-	if (IS_ENCRYPTED(dir))
-		return __fscrypt_prepare_lookup(dir, dentry, fname);
+	int err = 0;
+
+	if (IS_ENCRYPTED(dir)) {
+		err = fscrypt_setup_filename(dir, &dentry->d_name, 1, fname);
+		if (err && err != -ENOENT)
+			return err;
+	} else {
+		memset(fname, 0, sizeof(*fname));
+		fname->usr_fname = &dentry->d_name;
+		fname->disk_name.name = (unsigned char *)dentry->d_name.name;
+		fname->disk_name.len = dentry->d_name.len;
+	}
 
-	memset(fname, 0, sizeof(*fname));
-	fname->usr_fname = &dentry->d_name;
-	fname->disk_name.name = (unsigned char *)dentry->d_name.name;
-	fname->disk_name.len = dentry->d_name.len;
-	return 0;
+	fscrypt_prepare_lookup_dentry(dentry, fname->is_nokey_name);
+
+	return err;
 }
 
 /**
-- 
2.43.0

[PATCH v5 04/12] fscrypt: Drop d_revalidate for valid dentries during lookup

[Gabriel Krisman Bertazi]

Unencrypted and encrypted-dentries where the key is available don't need
to be revalidated with regards to fscrypt, since they don't go stale
from under VFS and the key cannot be removed for the encrypted case
without evicting the dentry.  Mark them with d_set_always_valid, to
avoid unnecessary revalidation, in preparation to always configuring
d_op through sb->s_d_op.

Since the filesystem might have other features that require
revalidation, only apply this optimization if the d_revalidate handler
is fscrypt_d_revalidate itself.

Signed-off-by: Gabriel Krisman Bertazi <krisman@suse.de>
---
 include/linux/fscrypt.h | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
index 4aaf847955c0..a22997b9f35c 100644
--- a/include/linux/fscrypt.h
+++ b/include/linux/fscrypt.h
@@ -942,11 +942,22 @@ static inline int fscrypt_prepare_rename(struct inode *old_dir,
 static inline void fscrypt_prepare_lookup_dentry(struct dentry *dentry,
 						 bool is_nokey_name)
 {
-	if (is_nokey_name) {
-		spin_lock(&dentry->d_lock);
+	spin_lock(&dentry->d_lock);
+
+	if (is_nokey_name)
 		dentry->d_flags |= DCACHE_NOKEY_NAME;
-		spin_unlock(&dentry->d_lock);
+	else if (dentry->d_flags & DCACHE_OP_REVALIDATE &&
+		 dentry->d_op->d_revalidate == fscrypt_d_revalidate) {
+		/*
+		 * Unencrypted dentries and encrypted dentries where the
+		 * key is available are always valid from fscrypt
+		 * perspective. Avoid the cost of calling
+		 * fscrypt_d_revalidate unnecessarily.
+		 */
+		dentry->d_flags &= ~DCACHE_OP_REVALIDATE;
 	}
+
+	spin_unlock(&dentry->d_lock);
 }
 
 /**
-- 
2.43.0

Re: [PATCH v5 04/12] fscrypt: Drop d_revalidate for valid dentries during lookup

[Eric Biggers]

On Mon, Jan 29, 2024 at 05:43:22PM -0300, Gabriel Krisman Bertazi wrote:
> Unencrypted and encrypted-dentries where the key is available don't need
> to be revalidated with regards to fscrypt, since they don't go stale
> from under VFS and the key cannot be removed for the encrypted case
> without evicting the dentry.  Mark them with d_set_always_valid, to

"d_set_always_valid" doesn't appear in the diff itself.

> diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
> index 4aaf847955c0..a22997b9f35c 100644
> --- a/include/linux/fscrypt.h
> +++ b/include/linux/fscrypt.h
> @@ -942,11 +942,22 @@ static inline int fscrypt_prepare_rename(struct inode *old_dir,
>  static inline void fscrypt_prepare_lookup_dentry(struct dentry *dentry,
>  						 bool is_nokey_name)
>  {
> -	if (is_nokey_name) {
> -		spin_lock(&dentry->d_lock);
> +	spin_lock(&dentry->d_lock);
> +
> +	if (is_nokey_name)
>  		dentry->d_flags |= DCACHE_NOKEY_NAME;
> -		spin_unlock(&dentry->d_lock);
> +	else if (dentry->d_flags & DCACHE_OP_REVALIDATE &&
> +		 dentry->d_op->d_revalidate == fscrypt_d_revalidate) {
> +		/*
> +		 * Unencrypted dentries and encrypted dentries where the
> +		 * key is available are always valid from fscrypt
> +		 * perspective. Avoid the cost of calling
> +		 * fscrypt_d_revalidate unnecessarily.
> +		 */
> +		dentry->d_flags &= ~DCACHE_OP_REVALIDATE;
>  	}
> +
> +	spin_unlock(&dentry->d_lock);

This makes lookups in unencrypted directories start doing the
spin_lock/spin_unlock pair.  Is that really necessary?

These changes also make the inline function fscrypt_prepare_lookup() very long
(when including the fscrypt_prepare_lookup_dentry() that's inlined into it).
The rule that I'm trying to follow is that to the extent that the fscrypt helper
functions are inlined, the inline part should be a fast path for unencrypted
directories.  Encrypted directories should be handled out-of-line.

So looking at the original fscrypt_prepare_lookup():

	static inline int fscrypt_prepare_lookup(struct inode *dir,
						 struct dentry *dentry,
						 struct fscrypt_name *fname)
	{
		if (IS_ENCRYPTED(dir))
			return __fscrypt_prepare_lookup(dir, dentry, fname);

		memset(fname, 0, sizeof(*fname));
		fname->usr_fname = &dentry->d_name;
		fname->disk_name.name = (unsigned char *)dentry->d_name.name;
		fname->disk_name.len = dentry->d_name.len;
		return 0;
	}

If you could just add the DCACHE_OP_REVALIDATE clearing for dentries in
unencrypted directories just before the "return 0;", hopefully without the
spinlock, that would be good.  Yes, that does mean that
__fscrypt_prepare_lookup() will have to handle it too, for the case of dentries
in encrypted directories, but that seems okay.

- Eric

Re: [PATCH v5 04/12] fscrypt: Drop d_revalidate for valid dentries during lookup

[Gabriel Krisman Bertazi]

Eric Biggers <ebiggers@kernel.org> writes:

> On Mon, Jan 29, 2024 at 05:43:22PM -0300, Gabriel Krisman Bertazi wrote:
>> Unencrypted and encrypted-dentries where the key is available don't need
>> to be revalidated with regards to fscrypt, since they don't go stale
>> from under VFS and the key cannot be removed for the encrypted case
>> without evicting the dentry.  Mark them with d_set_always_valid, to
>
> "d_set_always_valid" doesn't appear in the diff itself.
>
>> diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
>> index 4aaf847955c0..a22997b9f35c 100644
>> --- a/include/linux/fscrypt.h
>> +++ b/include/linux/fscrypt.h
>> @@ -942,11 +942,22 @@ static inline int fscrypt_prepare_rename(struct inode *old_dir,
>>  static inline void fscrypt_prepare_lookup_dentry(struct dentry *dentry,
>>  						 bool is_nokey_name)
>>  {
>> -	if (is_nokey_name) {
>> -		spin_lock(&dentry->d_lock);
>> +	spin_lock(&dentry->d_lock);
>> +
>> +	if (is_nokey_name)
>>  		dentry->d_flags |= DCACHE_NOKEY_NAME;
>> -		spin_unlock(&dentry->d_lock);
>> +	else if (dentry->d_flags & DCACHE_OP_REVALIDATE &&
>> +		 dentry->d_op->d_revalidate == fscrypt_d_revalidate) {
>> +		/*
>> +		 * Unencrypted dentries and encrypted dentries where the
>> +		 * key is available are always valid from fscrypt
>> +		 * perspective. Avoid the cost of calling
>> +		 * fscrypt_d_revalidate unnecessarily.
>> +		 */
>> +		dentry->d_flags &= ~DCACHE_OP_REVALIDATE;
>>  	}
>> +
>> +	spin_unlock(&dentry->d_lock);
>
> This makes lookups in unencrypted directories start doing the
> spin_lock/spin_unlock pair.  Is that really necessary?
>
> These changes also make the inline function fscrypt_prepare_lookup() very long
> (when including the fscrypt_prepare_lookup_dentry() that's inlined into it).
> The rule that I'm trying to follow is that to the extent that the fscrypt helper
> functions are inlined, the inline part should be a fast path for unencrypted
> directories.  Encrypted directories should be handled out-of-line.
>
> So looking at the original fscrypt_prepare_lookup():
>
> 	static inline int fscrypt_prepare_lookup(struct inode *dir,
> 						 struct dentry *dentry,
> 						 struct fscrypt_name *fname)
> 	{
> 		if (IS_ENCRYPTED(dir))
> 			return __fscrypt_prepare_lookup(dir, dentry, fname);
>
> 		memset(fname, 0, sizeof(*fname));
> 		fname->usr_fname = &dentry->d_name;
> 		fname->disk_name.name = (unsigned char *)dentry->d_name.name;
> 		fname->disk_name.len = dentry->d_name.len;
> 		return 0;
> 	}
>
> If you could just add the DCACHE_OP_REVALIDATE clearing for dentries in
> unencrypted directories just before the "return 0;", hopefully without the
> spinlock, that would be good.  Yes, that does mean that
> __fscrypt_prepare_lookup() will have to handle it too, for the case of dentries
> in encrypted directories, but that seems okay.

ok, will do.  IIUC, we might be able to do without the d_lock
provided there is no store tearing.

But what was the reason you need the d_lock to set DCACHE_NOKEY_NAME
during lookup?  Is there a race with parallel lookup setting d_flag that
I couldn't find? Or is it another reason?


-- 
Gabriel Krisman Bertazi

Re: [PATCH v5 04/12] fscrypt: Drop d_revalidate for valid dentries during lookup

[Eric Biggers]

On Wed, Jan 31, 2024 at 03:35:40PM -0300, Gabriel Krisman Bertazi wrote:
> Eric Biggers <ebiggers@kernel.org> writes:
> 
> > On Mon, Jan 29, 2024 at 05:43:22PM -0300, Gabriel Krisman Bertazi wrote:
> >> Unencrypted and encrypted-dentries where the key is available don't need
> >> to be revalidated with regards to fscrypt, since they don't go stale
> >> from under VFS and the key cannot be removed for the encrypted case
> >> without evicting the dentry.  Mark them with d_set_always_valid, to
> >
> > "d_set_always_valid" doesn't appear in the diff itself.
> >
> >> diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
> >> index 4aaf847955c0..a22997b9f35c 100644
> >> --- a/include/linux/fscrypt.h
> >> +++ b/include/linux/fscrypt.h
> >> @@ -942,11 +942,22 @@ static inline int fscrypt_prepare_rename(struct inode *old_dir,
> >>  static inline void fscrypt_prepare_lookup_dentry(struct dentry *dentry,
> >>  						 bool is_nokey_name)
> >>  {
> >> -	if (is_nokey_name) {
> >> -		spin_lock(&dentry->d_lock);
> >> +	spin_lock(&dentry->d_lock);
> >> +
> >> +	if (is_nokey_name)
> >>  		dentry->d_flags |= DCACHE_NOKEY_NAME;
> >> -		spin_unlock(&dentry->d_lock);
> >> +	else if (dentry->d_flags & DCACHE_OP_REVALIDATE &&
> >> +		 dentry->d_op->d_revalidate == fscrypt_d_revalidate) {
> >> +		/*
> >> +		 * Unencrypted dentries and encrypted dentries where the
> >> +		 * key is available are always valid from fscrypt
> >> +		 * perspective. Avoid the cost of calling
> >> +		 * fscrypt_d_revalidate unnecessarily.
> >> +		 */
> >> +		dentry->d_flags &= ~DCACHE_OP_REVALIDATE;
> >>  	}
> >> +
> >> +	spin_unlock(&dentry->d_lock);
> >
> > This makes lookups in unencrypted directories start doing the
> > spin_lock/spin_unlock pair.  Is that really necessary?
> >
> > These changes also make the inline function fscrypt_prepare_lookup() very long
> > (when including the fscrypt_prepare_lookup_dentry() that's inlined into it).
> > The rule that I'm trying to follow is that to the extent that the fscrypt helper
> > functions are inlined, the inline part should be a fast path for unencrypted
> > directories.  Encrypted directories should be handled out-of-line.
> >
> > So looking at the original fscrypt_prepare_lookup():
> >
> > 	static inline int fscrypt_prepare_lookup(struct inode *dir,
> > 						 struct dentry *dentry,
> > 						 struct fscrypt_name *fname)
> > 	{
> > 		if (IS_ENCRYPTED(dir))
> > 			return __fscrypt_prepare_lookup(dir, dentry, fname);
> >
> > 		memset(fname, 0, sizeof(*fname));
> > 		fname->usr_fname = &dentry->d_name;
> > 		fname->disk_name.name = (unsigned char *)dentry->d_name.name;
> > 		fname->disk_name.len = dentry->d_name.len;
> > 		return 0;
> > 	}
> >
> > If you could just add the DCACHE_OP_REVALIDATE clearing for dentries in
> > unencrypted directories just before the "return 0;", hopefully without the
> > spinlock, that would be good.  Yes, that does mean that
> > __fscrypt_prepare_lookup() will have to handle it too, for the case of dentries
> > in encrypted directories, but that seems okay.
> 
> ok, will do.  IIUC, we might be able to do without the d_lock
> provided there is no store tearing.
> 
> But what was the reason you need the d_lock to set DCACHE_NOKEY_NAME
> during lookup?  Is there a race with parallel lookup setting d_flag that
> I couldn't find? Or is it another reason?

d_flags is documented to be protected by d_lock.  So for setting
DCACHE_NOKEY_NAME, fs/crypto/ just does the safe thing of taking d_lock.  I
never really looked into whether the lock can be skipped there (i.e., whether
anything else can change d_flags while ->lookup is running), since this code
only ran for no-key names, for which performance isn't really important.

This patch would extend that locking to a new context in which it would be
executed several orders of magnitude more often.  So, making sure it's properly
optimized becomes more important.  It looks like it *might* be the case that
->lookup has exclusive access to d_flags, by virtue of having allocated the
dentry, so I'm just wondering if we can take advantage of that (or whether in
classic VFS fashion there's some edge case where that assumption is wrong).

- Eric

Re: [PATCH v5 04/12] fscrypt: Drop d_revalidate for valid dentries during lookup

[Gabriel Krisman Bertazi]

Eric Biggers <ebiggers@kernel.org> writes:

> On Wed, Jan 31, 2024 at 03:35:40PM -0300, Gabriel Krisman Bertazi wrote:
>> Eric Biggers <ebiggers@kernel.org> writes:
>> 
>> > On Mon, Jan 29, 2024 at 05:43:22PM -0300, Gabriel Krisman Bertazi wrote:
>> >> Unencrypted and encrypted-dentries where the key is available don't need
>> >> to be revalidated with regards to fscrypt, since they don't go stale
>> >> from under VFS and the key cannot be removed for the encrypted case
>> >> without evicting the dentry.  Mark them with d_set_always_valid, to
>> >
>> > "d_set_always_valid" doesn't appear in the diff itself.
>> >
>> >> diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
>> >> index 4aaf847955c0..a22997b9f35c 100644
>> >> --- a/include/linux/fscrypt.h
>> >> +++ b/include/linux/fscrypt.h
>> >> @@ -942,11 +942,22 @@ static inline int fscrypt_prepare_rename(struct inode *old_dir,
>> >>  static inline void fscrypt_prepare_lookup_dentry(struct dentry *dentry,
>> >>  						 bool is_nokey_name)
>> >>  {
>> >> -	if (is_nokey_name) {
>> >> -		spin_lock(&dentry->d_lock);
>> >> +	spin_lock(&dentry->d_lock);
>> >> +
>> >> +	if (is_nokey_name)
>> >>  		dentry->d_flags |= DCACHE_NOKEY_NAME;
>> >> -		spin_unlock(&dentry->d_lock);
>> >> +	else if (dentry->d_flags & DCACHE_OP_REVALIDATE &&
>> >> +		 dentry->d_op->d_revalidate == fscrypt_d_revalidate) {
>> >> +		/*
>> >> +		 * Unencrypted dentries and encrypted dentries where the
>> >> +		 * key is available are always valid from fscrypt
>> >> +		 * perspective. Avoid the cost of calling
>> >> +		 * fscrypt_d_revalidate unnecessarily.
>> >> +		 */
>> >> +		dentry->d_flags &= ~DCACHE_OP_REVALIDATE;
>> >>  	}
>> >> +
>> >> +	spin_unlock(&dentry->d_lock);
>> >
>> > This makes lookups in unencrypted directories start doing the
>> > spin_lock/spin_unlock pair.  Is that really necessary?
>> >
>> > These changes also make the inline function fscrypt_prepare_lookup() very long
>> > (when including the fscrypt_prepare_lookup_dentry() that's inlined into it).
>> > The rule that I'm trying to follow is that to the extent that the fscrypt helper
>> > functions are inlined, the inline part should be a fast path for unencrypted
>> > directories.  Encrypted directories should be handled out-of-line.
>> >
>> > So looking at the original fscrypt_prepare_lookup():
>> >
>> > 	static inline int fscrypt_prepare_lookup(struct inode *dir,
>> > 						 struct dentry *dentry,
>> > 						 struct fscrypt_name *fname)
>> > 	{
>> > 		if (IS_ENCRYPTED(dir))
>> > 			return __fscrypt_prepare_lookup(dir, dentry, fname);
>> >
>> > 		memset(fname, 0, sizeof(*fname));
>> > 		fname->usr_fname = &dentry->d_name;
>> > 		fname->disk_name.name = (unsigned char *)dentry->d_name.name;
>> > 		fname->disk_name.len = dentry->d_name.len;
>> > 		return 0;
>> > 	}
>> >
>> > If you could just add the DCACHE_OP_REVALIDATE clearing for dentries in
>> > unencrypted directories just before the "return 0;", hopefully without the
>> > spinlock, that would be good.  Yes, that does mean that
>> > __fscrypt_prepare_lookup() will have to handle it too, for the case of dentries
>> > in encrypted directories, but that seems okay.
>> 
>> ok, will do.  IIUC, we might be able to do without the d_lock
>> provided there is no store tearing.
>> 
>> But what was the reason you need the d_lock to set DCACHE_NOKEY_NAME
>> during lookup?  Is there a race with parallel lookup setting d_flag that
>> I couldn't find? Or is it another reason?
>
> d_flags is documented to be protected by d_lock.  So for setting
> DCACHE_NOKEY_NAME, fs/crypto/ just does the safe thing of taking d_lock.  I
> never really looked into whether the lock can be skipped there (i.e., whether
> anything else can change d_flags while ->lookup is running), since this code
> only ran for no-key names, for which performance isn't really important.

Yes, I was looking for the actual race that could happen here, and
couldn't find one. As far as I understand it, the only thing that could
see the dentry during a lookup would be a parallel lookup, but those
will be held waiting for completion in d_alloc_parallel, and won't touch
d_flags.  Currently, right after this code, we call d_set_d_op() in
generic_set_encrypted_ci_d_ops(), which will happily write d_flags without
the d_lock. If this is a problem here, we have a problem there.

What I really don't want to do is keep the lock for DCACHE_NOKEY_NAME,
but drop it for unsetting DCACHE_OP_REVALIDATE right in the same field,
without a good reason.  I get the argument that unencrypted
dentries are a much hotter path and we care more.  But the locking rules
of ->d_lookup don't change for both cases.

So, I'd rather drop the d_lock entirely in this path, not only for the
hunk I'm proposing.  It would be good to get an actual confirmation from
Al or Christian, though.

CC'ing Christian.

-- 
Gabriel Krisman Bertazi

Re: [PATCH v5 04/12] fscrypt: Drop d_revalidate for valid dentries during lookup

[Christian Brauner]

On Fri, Feb 02, 2024 at 11:50:07AM -0300, Gabriel Krisman Bertazi wrote:
> Eric Biggers <ebiggers@kernel.org> writes:
> 
> > On Wed, Jan 31, 2024 at 03:35:40PM -0300, Gabriel Krisman Bertazi wrote:
> >> Eric Biggers <ebiggers@kernel.org> writes:
> >> 
> >> > On Mon, Jan 29, 2024 at 05:43:22PM -0300, Gabriel Krisman Bertazi wrote:
> >> >> Unencrypted and encrypted-dentries where the key is available don't need
> >> >> to be revalidated with regards to fscrypt, since they don't go stale
> >> >> from under VFS and the key cannot be removed for the encrypted case
> >> >> without evicting the dentry.  Mark them with d_set_always_valid, to
> >> >
> >> > "d_set_always_valid" doesn't appear in the diff itself.
> >> >
> >> >> diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
> >> >> index 4aaf847955c0..a22997b9f35c 100644
> >> >> --- a/include/linux/fscrypt.h
> >> >> +++ b/include/linux/fscrypt.h
> >> >> @@ -942,11 +942,22 @@ static inline int fscrypt_prepare_rename(struct inode *old_dir,
> >> >>  static inline void fscrypt_prepare_lookup_dentry(struct dentry *dentry,
> >> >>  						 bool is_nokey_name)
> >> >>  {
> >> >> -	if (is_nokey_name) {
> >> >> -		spin_lock(&dentry->d_lock);
> >> >> +	spin_lock(&dentry->d_lock);
> >> >> +
> >> >> +	if (is_nokey_name)
> >> >>  		dentry->d_flags |= DCACHE_NOKEY_NAME;
> >> >> -		spin_unlock(&dentry->d_lock);
> >> >> +	else if (dentry->d_flags & DCACHE_OP_REVALIDATE &&
> >> >> +		 dentry->d_op->d_revalidate == fscrypt_d_revalidate) {
> >> >> +		/*
> >> >> +		 * Unencrypted dentries and encrypted dentries where the
> >> >> +		 * key is available are always valid from fscrypt
> >> >> +		 * perspective. Avoid the cost of calling
> >> >> +		 * fscrypt_d_revalidate unnecessarily.
> >> >> +		 */
> >> >> +		dentry->d_flags &= ~DCACHE_OP_REVALIDATE;
> >> >>  	}
> >> >> +
> >> >> +	spin_unlock(&dentry->d_lock);
> >> >
> >> > This makes lookups in unencrypted directories start doing the
> >> > spin_lock/spin_unlock pair.  Is that really necessary?
> >> >
> >> > These changes also make the inline function fscrypt_prepare_lookup() very long
> >> > (when including the fscrypt_prepare_lookup_dentry() that's inlined into it).
> >> > The rule that I'm trying to follow is that to the extent that the fscrypt helper
> >> > functions are inlined, the inline part should be a fast path for unencrypted
> >> > directories.  Encrypted directories should be handled out-of-line.
> >> >
> >> > So looking at the original fscrypt_prepare_lookup():
> >> >
> >> > 	static inline int fscrypt_prepare_lookup(struct inode *dir,
> >> > 						 struct dentry *dentry,
> >> > 						 struct fscrypt_name *fname)
> >> > 	{
> >> > 		if (IS_ENCRYPTED(dir))
> >> > 			return __fscrypt_prepare_lookup(dir, dentry, fname);
> >> >
> >> > 		memset(fname, 0, sizeof(*fname));
> >> > 		fname->usr_fname = &dentry->d_name;
> >> > 		fname->disk_name.name = (unsigned char *)dentry->d_name.name;
> >> > 		fname->disk_name.len = dentry->d_name.len;
> >> > 		return 0;
> >> > 	}
> >> >
> >> > If you could just add the DCACHE_OP_REVALIDATE clearing for dentries in
> >> > unencrypted directories just before the "return 0;", hopefully without the
> >> > spinlock, that would be good.  Yes, that does mean that
> >> > __fscrypt_prepare_lookup() will have to handle it too, for the case of dentries
> >> > in encrypted directories, but that seems okay.
> >> 
> >> ok, will do.  IIUC, we might be able to do without the d_lock
> >> provided there is no store tearing.
> >> 
> >> But what was the reason you need the d_lock to set DCACHE_NOKEY_NAME
> >> during lookup?  Is there a race with parallel lookup setting d_flag that
> >> I couldn't find? Or is it another reason?
> >
> > d_flags is documented to be protected by d_lock.  So for setting
> > DCACHE_NOKEY_NAME, fs/crypto/ just does the safe thing of taking d_lock.  I
> > never really looked into whether the lock can be skipped there (i.e., whether
> > anything else can change d_flags while ->lookup is running), since this code
> > only ran for no-key names, for which performance isn't really important.
> 
> Yes, I was looking for the actual race that could happen here, and
> couldn't find one. As far as I understand it, the only thing that could
> see the dentry during a lookup would be a parallel lookup, but those
> will be held waiting for completion in d_alloc_parallel, and won't touch
> d_flags.  Currently, right after this code, we call d_set_d_op() in
> generic_set_encrypted_ci_d_ops(), which will happily write d_flags without
> the d_lock. If this is a problem here, we have a problem there.
> 
> What I really don't want to do is keep the lock for DCACHE_NOKEY_NAME,
> but drop it for unsetting DCACHE_OP_REVALIDATE right in the same field,
> without a good reason.  I get the argument that unencrypted
> dentries are a much hotter path and we care more.  But the locking rules
> of ->d_lookup don't change for both cases.

Even if it were to work in this case I don't think it is generally safe
to do. But also, for DCACHE_OP_REVALIDATE afaict this is an
optimization. Why don't you simply accept the raciness, just like fuse
does in fuse_dentry_settime(), check for DCACHE_OP_REVALIDATE locklessly
and only take the lock if that thing is set?

Re: [PATCH v5 04/12] fscrypt: Drop d_revalidate for valid dentries during lookup

[Gabriel Krisman Bertazi]

Christian Brauner <brauner@kernel.org> writes:

> On Fri, Feb 02, 2024 at 11:50:07AM -0300, Gabriel Krisman Bertazi wrote:
>> Eric Biggers <ebiggers@kernel.org> writes:
>> 
>> > On Wed, Jan 31, 2024 at 03:35:40PM -0300, Gabriel Krisman Bertazi wrote:
>> >> Eric Biggers <ebiggers@kernel.org> writes:
>> >> 
>> >> > On Mon, Jan 29, 2024 at 05:43:22PM -0300, Gabriel Krisman Bertazi wrote:
>> >> >> Unencrypted and encrypted-dentries where the key is available don't need
>> >> >> to be revalidated with regards to fscrypt, since they don't go stale
>> >> >> from under VFS and the key cannot be removed for the encrypted case
>> >> >> without evicting the dentry.  Mark them with d_set_always_valid, to
>> >> >
>> >> > "d_set_always_valid" doesn't appear in the diff itself.
>> >> >
>> >> >> diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
>> >> >> index 4aaf847955c0..a22997b9f35c 100644
>> >> >> --- a/include/linux/fscrypt.h
>> >> >> +++ b/include/linux/fscrypt.h
>> >> >> @@ -942,11 +942,22 @@ static inline int fscrypt_prepare_rename(struct inode *old_dir,
>> >> >>  static inline void fscrypt_prepare_lookup_dentry(struct dentry *dentry,
>> >> >>  						 bool is_nokey_name)
>> >> >>  {
>> >> >> -	if (is_nokey_name) {
>> >> >> -		spin_lock(&dentry->d_lock);
>> >> >> +	spin_lock(&dentry->d_lock);
>> >> >> +
>> >> >> +	if (is_nokey_name)
>> >> >>  		dentry->d_flags |= DCACHE_NOKEY_NAME;
>> >> >> -		spin_unlock(&dentry->d_lock);
>> >> >> +	else if (dentry->d_flags & DCACHE_OP_REVALIDATE &&
>> >> >> +		 dentry->d_op->d_revalidate == fscrypt_d_revalidate) {
>> >> >> +		/*
>> >> >> +		 * Unencrypted dentries and encrypted dentries where the
>> >> >> +		 * key is available are always valid from fscrypt
>> >> >> +		 * perspective. Avoid the cost of calling
>> >> >> +		 * fscrypt_d_revalidate unnecessarily.
>> >> >> +		 */
>> >> >> +		dentry->d_flags &= ~DCACHE_OP_REVALIDATE;
>> >> >>  	}
>> >> >> +
>> >> >> +	spin_unlock(&dentry->d_lock);
>> >> >
>> >> > This makes lookups in unencrypted directories start doing the
>> >> > spin_lock/spin_unlock pair.  Is that really necessary?
>> >> >
>> >> > These changes also make the inline function fscrypt_prepare_lookup() very long
>> >> > (when including the fscrypt_prepare_lookup_dentry() that's inlined into it).
>> >> > The rule that I'm trying to follow is that to the extent that the fscrypt helper
>> >> > functions are inlined, the inline part should be a fast path for unencrypted
>> >> > directories.  Encrypted directories should be handled out-of-line.
>> >> >
>> >> > So looking at the original fscrypt_prepare_lookup():
>> >> >
>> >> > 	static inline int fscrypt_prepare_lookup(struct inode *dir,
>> >> > 						 struct dentry *dentry,
>> >> > 						 struct fscrypt_name *fname)
>> >> > 	{
>> >> > 		if (IS_ENCRYPTED(dir))
>> >> > 			return __fscrypt_prepare_lookup(dir, dentry, fname);
>> >> >
>> >> > 		memset(fname, 0, sizeof(*fname));
>> >> > 		fname->usr_fname = &dentry->d_name;
>> >> > 		fname->disk_name.name = (unsigned char *)dentry->d_name.name;
>> >> > 		fname->disk_name.len = dentry->d_name.len;
>> >> > 		return 0;
>> >> > 	}
>> >> >
>> >> > If you could just add the DCACHE_OP_REVALIDATE clearing for dentries in
>> >> > unencrypted directories just before the "return 0;", hopefully without the
>> >> > spinlock, that would be good.  Yes, that does mean that
>> >> > __fscrypt_prepare_lookup() will have to handle it too, for the case of dentries
>> >> > in encrypted directories, but that seems okay.
>> >> 
>> >> ok, will do.  IIUC, we might be able to do without the d_lock
>> >> provided there is no store tearing.
>> >> 
>> >> But what was the reason you need the d_lock to set DCACHE_NOKEY_NAME
>> >> during lookup?  Is there a race with parallel lookup setting d_flag that
>> >> I couldn't find? Or is it another reason?
>> >
>> > d_flags is documented to be protected by d_lock.  So for setting
>> > DCACHE_NOKEY_NAME, fs/crypto/ just does the safe thing of taking d_lock.  I
>> > never really looked into whether the lock can be skipped there (i.e., whether
>> > anything else can change d_flags while ->lookup is running), since this code
>> > only ran for no-key names, for which performance isn't really important.
>> 
>> Yes, I was looking for the actual race that could happen here, and
>> couldn't find one. As far as I understand it, the only thing that could
>> see the dentry during a lookup would be a parallel lookup, but those
>> will be held waiting for completion in d_alloc_parallel, and won't touch
>> d_flags.  Currently, right after this code, we call d_set_d_op() in
>> generic_set_encrypted_ci_d_ops(), which will happily write d_flags without
>> the d_lock. If this is a problem here, we have a problem there.
>> 
>> What I really don't want to do is keep the lock for DCACHE_NOKEY_NAME,
>> but drop it for unsetting DCACHE_OP_REVALIDATE right in the same field,
>> without a good reason.  I get the argument that unencrypted
>> dentries are a much hotter path and we care more.  But the locking rules
>> of ->d_lookup don't change for both cases.
>
> Even if it were to work in this case I don't think it is generally safe
> to do. But also, for DCACHE_OP_REVALIDATE afaict this is an
> optimization. Why don't you simply accept the raciness, just like fuse
> does in fuse_dentry_settime(), check for DCACHE_OP_REVALIDATE locklessly
> and only take the lock if that thing is set?

That sounds extremely reasonable.  I will follow that approach!

Thanks,

-- 
Gabriel Krisman Bertazi

[PATCH v5 05/12] fscrypt: Drop d_revalidate once the key is added

[Gabriel Krisman Bertazi]

From fscrypt perspective, once the key is available, the dentry will
remain valid until evicted for other reasons, since keyed dentries don't
require revalidation and, if the key is removed, the dentry is
forcefully evicted.  Therefore, we don't need to keep revalidating them
repeatedly.

Obviously, we can only do this if fscrypt is the only thing requiring
revalidation for a dentry.  For this reason, we only disable
d_revalidate if the .d_revalidate hook is fscrypt_d_revalidate itself.

It is safe to do it here because when moving the dentry to the
plain-text version, we are holding the d_lock.  We might race with a
concurrent RCU lookup but this is harmless because, at worst, we will
get an extra d_revalidate on the keyed dentry, which is will find the
dentry is valid.

Signed-off-by: Gabriel Krisman Bertazi <krisman@suse.de>

---
Changes since v3:
  - Fix null-ptr-deref for filesystems that don't support fscrypt (ktr)
Changes since v2:
  - Do it when moving instead of when revalidating the dentry. (me)

Changes since v1:
  - Improve commit message (Eric)
  - Drop & in function comparison (Eric)
---
 include/linux/fscrypt.h | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
index a22997b9f35c..c1e285053b3e 100644
--- a/include/linux/fscrypt.h
+++ b/include/linux/fscrypt.h
@@ -192,6 +192,8 @@ struct fscrypt_operations {
 					     unsigned int *num_devs);
 };
 
+int fscrypt_d_revalidate(struct dentry *dentry, unsigned int flags);
+
 static inline struct fscrypt_inode_info *
 fscrypt_get_inode_info(const struct inode *inode)
 {
@@ -221,15 +223,24 @@ static inline bool fscrypt_needs_contents_encryption(const struct inode *inode)
 }
 
 /*
- * 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.  Note that we don't have to support arbitrary moves of this flag
- * because fscrypt doesn't allow no-key names to be the source or target of a
- * rename().
+ * 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)
 {
 	dentry->d_flags &= ~DCACHE_NOKEY_NAME;
+
+	/*
+	 * Save the d_revalidate call cost during VFS operations.  We
+	 * can do it because, when the key is available, the dentry
+	 * can't go stale and the key won't go away without eviction.
+	 */
+	if (dentry->d_op && dentry->d_op->d_revalidate == fscrypt_d_revalidate)
+		dentry->d_flags &= ~DCACHE_OP_REVALIDATE;
 }
 
 /**
@@ -368,7 +379,6 @@ int fscrypt_fname_disk_to_usr(const struct inode *inode,
 bool fscrypt_match_name(const struct fscrypt_name *fname,
 			const u8 *de_name, u32 de_name_len);
 u64 fscrypt_fname_siphash(const struct inode *dir, const struct qstr *name);
-int fscrypt_d_revalidate(struct dentry *dentry, unsigned int flags);
 
 /* bio.c */
 bool fscrypt_decrypt_bio(struct bio *bio);
-- 
2.43.0

[PATCH v5 06/12] fscrypt: Ignore plaintext dentries during d_move

[Gabriel Krisman Bertazi]

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;
+
+	 /* Mark the dentry as a plaintext dentry. */
 	dentry->d_flags &= ~DCACHE_NOKEY_NAME;
 
 	/*
-- 
2.43.0

Re: [PATCH v5 06/12] fscrypt: Ignore plaintext dentries during d_move

[Eric Biggers]

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

[PATCH v5 07/12] libfs: Merge encrypted_ci_dentry_ops and ci_dentry_ops

[Gabriel Krisman Bertazi]

In preparation to get case-insensitive dentry operations from
sb->s_d_op again, use the same structure for case-insensitive
filesystems with and without fscrypt.

This means that on a casefolded filesystem without fscrypt, we end up
having to call fscrypt_d_revalidate once per dentry, which does the
function call extra cost.  We could avoid it by calling
d_set_always_valid in generic_set_encrypted_ci_d_ops, but this entire
function will go away in the following patches, and it is not worth the
extra complexity. Also, since the first fscrypt_d_revalidate will call
d_set_always_valid for us, we'll only have only pay the cost once, and
not per-lookup.

Signed-off-by: Gabriel Krisman Bertazi <krisman@suse.de>

---
Changes since v1:
  - fix header guard (eric)
---
 fs/libfs.c | 34 ++++++----------------------------
 1 file changed, 6 insertions(+), 28 deletions(-)

diff --git a/fs/libfs.c b/fs/libfs.c
index c2aa6fd4795c..c4be0961faf0 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -1776,19 +1776,14 @@ static int generic_ci_d_hash(const struct dentry *dentry, struct qstr *str)
 static const struct dentry_operations generic_ci_dentry_ops = {
 	.d_hash = generic_ci_d_hash,
 	.d_compare = generic_ci_d_compare,
-};
-#endif
-
 #ifdef CONFIG_FS_ENCRYPTION
-static const struct dentry_operations generic_encrypted_dentry_ops = {
 	.d_revalidate = fscrypt_d_revalidate,
+#endif
 };
 #endif
 
-#if defined(CONFIG_FS_ENCRYPTION) && IS_ENABLED(CONFIG_UNICODE)
-static const struct dentry_operations generic_encrypted_ci_dentry_ops = {
-	.d_hash = generic_ci_d_hash,
-	.d_compare = generic_ci_d_compare,
+#ifdef CONFIG_FS_ENCRYPTION
+static const struct dentry_operations generic_encrypted_dentry_ops = {
 	.d_revalidate = fscrypt_d_revalidate,
 };
 #endif
@@ -1809,38 +1804,21 @@ static const struct dentry_operations generic_encrypted_ci_dentry_ops = {
  * Encryption works differently in that the only dentry operation it needs is
  * d_revalidate, which it only needs on dentries that have the no-key name flag.
  * The no-key flag can't be set "later", so we don't have to worry about that.
- *
- * Finally, to maximize compatibility with overlayfs (which isn't compatible
- * with certain dentry operations) and to avoid taking an unnecessary
- * performance hit, we use custom dentry_operations for each possible
- * combination rather than always installing all operations.
  */
 void generic_set_encrypted_ci_d_ops(struct dentry *dentry)
 {
-#ifdef CONFIG_FS_ENCRYPTION
-	bool needs_encrypt_ops = dentry->d_flags & DCACHE_NOKEY_NAME;
-#endif
 #if IS_ENABLED(CONFIG_UNICODE)
-	bool needs_ci_ops = dentry->d_sb->s_encoding;
-#endif
-#if defined(CONFIG_FS_ENCRYPTION) && IS_ENABLED(CONFIG_UNICODE)
-	if (needs_encrypt_ops && needs_ci_ops) {
-		d_set_d_op(dentry, &generic_encrypted_ci_dentry_ops);
+	if (dentry->d_sb->s_encoding) {
+		d_set_d_op(dentry, &generic_ci_dentry_ops);
 		return;
 	}
 #endif
 #ifdef CONFIG_FS_ENCRYPTION
-	if (needs_encrypt_ops) {
+	if (dentry->d_flags & DCACHE_NOKEY_NAME) {
 		d_set_d_op(dentry, &generic_encrypted_dentry_ops);
 		return;
 	}
 #endif
-#if IS_ENABLED(CONFIG_UNICODE)
-	if (needs_ci_ops) {
-		d_set_d_op(dentry, &generic_ci_dentry_ops);
-		return;
-	}
-#endif
 }
 EXPORT_SYMBOL(generic_set_encrypted_ci_d_ops);
 
-- 
2.43.0

Re: [PATCH v5 07/12] libfs: Merge encrypted_ci_dentry_ops and ci_dentry_ops

[Eric Biggers]

On Mon, Jan 29, 2024 at 05:43:25PM -0300, Gabriel Krisman Bertazi wrote:
> In preparation to get case-insensitive dentry operations from
> sb->s_d_op again, use the same structure for case-insensitive
> filesystems with and without fscrypt.
> 
> This means that on a casefolded filesystem without fscrypt, we end up

Again, we shouldn't use the term "case-insensitive filesystem" (or casefolded
filesystem) when we actually mean "case-insensitive capable filesystem", or more
precisely "filesystem that supports case-insensitive directories".  Real
case-insensitive filesystems like FAT are not relevant here.

> Also, since the first fscrypt_d_revalidate will call d_set_always_valid for us

This is outdated.

- Eric

[PATCH v5 08/12] libfs: Add helper to choose dentry operations at mount-time

[Gabriel Krisman Bertazi]

In preparation to drop the similar helper that sets d_op at lookup time,
add a version to set the right d_op filesystem-wide, through sb->s_d_op.
The operations structures are shared across filesystems supporting
fscrypt and/or casefolding, therefore we can keep it in common libfs
code.

Signed-off-by: Gabriel Krisman Bertazi <krisman@suse.de>

---
changes since v3:
  - Fix typo in comment (Eric)
---
 fs/libfs.c         | 28 ++++++++++++++++++++++++++++
 include/linux/fs.h |  1 +
 2 files changed, 29 insertions(+)

diff --git a/fs/libfs.c b/fs/libfs.c
index c4be0961faf0..0aa388ee82ff 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -1822,6 +1822,34 @@ void generic_set_encrypted_ci_d_ops(struct dentry *dentry)
 }
 EXPORT_SYMBOL(generic_set_encrypted_ci_d_ops);
 
+/**
+ * generic_set_sb_d_ops - helper for choosing the set of
+ * filesystem-wide dentry operations for the enabled features
+ * @sb: superblock to be configured
+ *
+ * Filesystems supporting casefolding and/or fscrypt can call this
+ * helper at mount-time to configure sb->s_d_op to best set of dentry
+ * operations required for the enabled features. The helper must be
+ * called after these have been configured, but before the root dentry
+ * is created.
+ */
+void generic_set_sb_d_ops(struct super_block *sb)
+{
+#if IS_ENABLED(CONFIG_UNICODE)
+	if (sb->s_encoding) {
+		sb->s_d_op = &generic_ci_dentry_ops;
+		return;
+	}
+#endif
+#ifdef CONFIG_FS_ENCRYPTION
+	if (sb->s_cop) {
+		sb->s_d_op = &generic_encrypted_dentry_ops;
+		return;
+	}
+#endif
+}
+EXPORT_SYMBOL(generic_set_sb_d_ops);
+
 /**
  * inode_maybe_inc_iversion - increments i_version
  * @inode: inode with the i_version that should be updated
diff --git a/include/linux/fs.h b/include/linux/fs.h
index e6667ece5e64..c985d9392b61 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3202,6 +3202,7 @@ extern int generic_file_fsync(struct file *, loff_t, loff_t, int);
 extern int generic_check_addressable(unsigned, u64);
 
 extern void generic_set_encrypted_ci_d_ops(struct dentry *dentry);
+extern void generic_set_sb_d_ops(struct super_block *sb);
 
 static inline bool sb_has_encoding(const struct super_block *sb)
 {
-- 
2.43.0

[PATCH v5 09/12] ext4: Configure dentry operations at dentry-creation time

[Gabriel Krisman Bertazi]

This was already the case for case-insensitive before commit
bb9cd9106b22 ("fscrypt: Have filesystems handle their d_ops"), but it
was changed to set at lookup-time to facilitate the integration with
fscrypt.  But it's a problem because dentries that don't get created
through ->lookup() won't have any visibility of the operations.

Since fscrypt now also supports configuring dentry operations at
creation-time, do it for any encrypted and/or casefold volume,
simplifying the implementation across these features.

Signed-off-by: Gabriel Krisman Bertazi <krisman@suse.de>
---
 fs/ext4/namei.c | 1 -
 fs/ext4/super.c | 1 +
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index d252935f9c8a..3f0b853a371e 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -1762,7 +1762,6 @@ static struct buffer_head *ext4_lookup_entry(struct inode *dir,
 	struct buffer_head *bh;
 
 	err = ext4_fname_prepare_lookup(dir, dentry, &fname);
-	generic_set_encrypted_ci_d_ops(dentry);
 	if (err == -ENOENT)
 		return NULL;
 	if (err)
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index c5fcf377ab1f..de80a9cc699a 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -5493,6 +5493,7 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
 		goto failed_mount4;
 	}
 
+	generic_set_sb_d_ops(sb);
 	sb->s_root = d_make_root(root);
 	if (!sb->s_root) {
 		ext4_msg(sb, KERN_ERR, "get root dentry failed");
-- 
2.43.0

Re: [PATCH v5 09/12] ext4: Configure dentry operations at dentry-creation time

[Theodore Ts'o]

On Mon, Jan 29, 2024 at 05:43:27PM -0300, Gabriel Krisman Bertazi wrote:
> This was already the case for case-insensitive before commit
> bb9cd9106b22 ("fscrypt: Have filesystems handle their d_ops"), but it
> was changed to set at lookup-time to facilitate the integration with
> fscrypt.  But it's a problem because dentries that don't get created
> through ->lookup() won't have any visibility of the operations.
> 
> Since fscrypt now also supports configuring dentry operations at
> creation-time, do it for any encrypted and/or casefold volume,
> simplifying the implementation across these features.
> 
> Signed-off-by: Gabriel Krisman Bertazi <krisman@suse.de>

Acked-by: Theodore Ts'o <tytso@mit.edu>

[PATCH v5 10/12] f2fs: Configure dentry operations at dentry-creation time

[Gabriel Krisman Bertazi]

This was already the case for case-insensitive before commit
bb9cd9106b22 ("fscrypt: Have filesystems handle their d_ops"), but it
was changed to set at lookup-time to facilitate the integration with
fscrypt.  But it's a problem because dentries that don't get created
through ->lookup() won't have any visibility of the operations.

Since fscrypt now also supports configuring dentry operations at
creation-time, do it for any encrypted and/or casefold volume,
simplifying the implementation across these features.

Signed-off-by: Gabriel Krisman Bertazi <krisman@suse.de>
---
 fs/f2fs/namei.c | 1 -
 fs/f2fs/super.c | 1 +
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
index d0053b0284d8..b40c6c393bd6 100644
--- a/fs/f2fs/namei.c
+++ b/fs/f2fs/namei.c
@@ -532,7 +532,6 @@ static struct dentry *f2fs_lookup(struct inode *dir, struct dentry *dentry,
 	}
 
 	err = f2fs_prepare_lookup(dir, dentry, &fname);
-	generic_set_encrypted_ci_d_ops(dentry);
 	if (err == -ENOENT)
 		goto out_splice;
 	if (err)
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 033af907c3b1..abfdb6e25b1c 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -4663,6 +4663,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 		goto free_node_inode;
 	}
 
+	generic_set_sb_d_ops(sb);
 	sb->s_root = d_make_root(root); /* allocate root dentry */
 	if (!sb->s_root) {
 		err = -ENOMEM;
-- 
2.43.0

[PATCH v5 11/12] ubifs: Configure dentry operations at dentry-creation time

[Gabriel Krisman Bertazi]

fscrypt now supports configuring dentry operations at dentry-creation
time through the preset sb->s_d_op, instead of at lookup time.
Enable this in ubifs, since the lookup-time mechanism is going away.

Signed-off-by: Gabriel Krisman Bertazi <krisman@suse.de>
---
 fs/ubifs/dir.c   | 1 -
 fs/ubifs/super.c | 1 +
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
index 3b13c648d490..51b9a10a9851 100644
--- a/fs/ubifs/dir.c
+++ b/fs/ubifs/dir.c
@@ -205,7 +205,6 @@ static struct dentry *ubifs_lookup(struct inode *dir, struct dentry *dentry,
 	dbg_gen("'%pd' in dir ino %lu", dentry, dir->i_ino);
 
 	err = fscrypt_prepare_lookup(dir, dentry, &nm);
-	generic_set_encrypted_ci_d_ops(dentry);
 	if (err == -ENOENT)
 		return d_splice_alias(NULL, dentry);
 	if (err)
diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
index 09e270d6ed02..304646b03e99 100644
--- a/fs/ubifs/super.c
+++ b/fs/ubifs/super.c
@@ -2239,6 +2239,7 @@ static int ubifs_fill_super(struct super_block *sb, void *data, int silent)
 		goto out_umount;
 	}
 
+	generic_set_sb_d_ops(sb);
 	sb->s_root = d_make_root(root);
 	if (!sb->s_root) {
 		err = -ENOMEM;
-- 
2.43.0

[PATCH v5 12/12] libfs: Drop generic_set_encrypted_ci_d_ops

[Gabriel Krisman Bertazi]

No filesystems depend on it anymore, and it is generally a bad idea.
Since all dentries should have the same set of dentry operations in
case-insensitive filesystems, it should be propagated through ->s_d_op.

Signed-off-by: Gabriel Krisman Bertazi <krisman@suse.de>
---
 fs/libfs.c         | 34 ----------------------------------
 include/linux/fs.h |  1 -
 2 files changed, 35 deletions(-)

diff --git a/fs/libfs.c b/fs/libfs.c
index 0aa388ee82ff..35124987f162 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -1788,40 +1788,6 @@ static const struct dentry_operations generic_encrypted_dentry_ops = {
 };
 #endif
 
-/**
- * generic_set_encrypted_ci_d_ops - helper for setting d_ops for given dentry
- * @dentry:	dentry to set ops on
- *
- * Casefolded directories need d_hash and d_compare set, so that the dentries
- * contained in them are handled case-insensitively.  Note that these operations
- * are needed on the parent directory rather than on the dentries in it, and
- * while the casefolding flag can be toggled on and off on an empty directory,
- * dentry_operations can't be changed later.  As a result, if the filesystem has
- * casefolding support enabled at all, we have to give all dentries the
- * casefolding operations even if their inode doesn't have the casefolding flag
- * currently (and thus the casefolding ops would be no-ops for now).
- *
- * Encryption works differently in that the only dentry operation it needs is
- * d_revalidate, which it only needs on dentries that have the no-key name flag.
- * The no-key flag can't be set "later", so we don't have to worry about that.
- */
-void generic_set_encrypted_ci_d_ops(struct dentry *dentry)
-{
-#if IS_ENABLED(CONFIG_UNICODE)
-	if (dentry->d_sb->s_encoding) {
-		d_set_d_op(dentry, &generic_ci_dentry_ops);
-		return;
-	}
-#endif
-#ifdef CONFIG_FS_ENCRYPTION
-	if (dentry->d_flags & DCACHE_NOKEY_NAME) {
-		d_set_d_op(dentry, &generic_encrypted_dentry_ops);
-		return;
-	}
-#endif
-}
-EXPORT_SYMBOL(generic_set_encrypted_ci_d_ops);
-
 /**
  * generic_set_sb_d_ops - helper for choosing the set of
  * filesystem-wide dentry operations for the enabled features
diff --git a/include/linux/fs.h b/include/linux/fs.h
index c985d9392b61..c0cfc53f95bb 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3201,7 +3201,6 @@ extern int generic_file_fsync(struct file *, loff_t, loff_t, int);
 
 extern int generic_check_addressable(unsigned, u64);
 
-extern void generic_set_encrypted_ci_d_ops(struct dentry *dentry);
 extern void generic_set_sb_d_ops(struct super_block *sb);
 
 static inline bool sb_has_encoding(const struct super_block *sb)
-- 
2.43.0