Hi,
v6 of this patchset applying the comments from Eric and the suggestion from
Christian. Thank you for your feedback.
Eric, since this is getting close to merging, how do you want to handle
it? I will take patch 1 through my tree already, are you fine if I merge
this through unicode or do you want to take it through fscrypt?
As usual, this survived fstests on ext4 and f2fs.
Thank you,
---
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 (10):
ovl: Always reject mounting over case-insensitive directories
fscrypt: Factor out a helper to configure the lookup dentry
fscrypt: Drop d_revalidate for valid dentries during lookup
fscrypt: Drop d_revalidate once the key is added
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 | 15 ++++------
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 | 61 +++++++++++++++++++++++++++++++++++-----
11 files changed, 100 insertions(+), 69 deletions(-)
--
2.43.0
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 <[email protected]>
---
changes since v6
- Use inline comparison for is_nokey_name (eric)
- rename fscrypt_prepare_lookup_dentry->fscrypt_prepare_dentry (eric)
---
fs/crypto/hooks.c | 15 +++++----------
include/linux/fscrypt.h | 10 ++++++++++
2 files changed, 15 insertions(+), 10 deletions(-)
diff --git a/fs/crypto/hooks.c b/fs/crypto/hooks.c
index 52504dd478d3..104771c3d3f6 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_dentry(dentry, fname->is_nokey_name);
+
return err;
}
EXPORT_SYMBOL_GPL(__fscrypt_prepare_lookup);
@@ -131,12 +128,10 @@ 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 = (!err && !fscrypt_has_encryption_key(dir));
+
+ fscrypt_prepare_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..47567a6a4f9d 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_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
Unencrypted and encrypted-dentries where the key is available don't need
to be revalidated by fscrypt, since they don't go stale from under VFS
and the key cannot be removed for the encrypted case without evicting
the dentry. Disable their d_revalidate hook on the first lookup, to
avoid repeated revalidation later. This is done in preparation to always
configuring d_op through sb->s_d_op.
The only part detail is that, since the filesystem might have other
features that require revalidation, we only apply this optimization if
the d_revalidate handler is fscrypt_d_revalidate itself.
Finally, we need to clean the dentry->flags even for unencrypted
dentries, so the ->d_lock might be acquired even for them. In order to
avoid doing it for filesystems that don't care about fscrypt at all, we
peek ->d_flags without the lock at first, and only acquire it if we
actually need to write the flag.
Signed-off-by: Gabriel Krisman Bertazi <[email protected]>
---
changes since v5
- d_set_always_valid -> d_revalidate (eric)
- Avoid acquiring the lock for !fscrypt-capable filesystems (eric, Christian)
---
include/linux/fscrypt.h | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
index 47567a6a4f9d..d1f17b90c30f 100644
--- a/include/linux/fscrypt.h
+++ b/include/linux/fscrypt.h
@@ -951,10 +951,29 @@ static inline int fscrypt_prepare_rename(struct inode *old_dir,
static inline void fscrypt_prepare_dentry(struct dentry *dentry,
bool is_nokey_name)
{
+ /*
+ * This code tries to only take ->d_lock when necessary to write
+ * to ->d_flags. We shouldn't be peeking on d_flags for
+ * DCACHE_OP_REVALIDATE unlocked, but in the unlikely case
+ * there is a race, the worst it can happen is that we fail to
+ * unset DCACHE_OP_REVALIDATE and pay the cost of an extra
+ * d_revalidate.
+ */
if (is_nokey_name) {
spin_lock(&dentry->d_lock);
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.
+ */
+ spin_lock(&dentry->d_lock);
+ dentry->d_flags &= ~DCACHE_OP_REVALIDATE;
+ spin_unlock(&dentry->d_lock);
}
}
@@ -992,6 +1011,9 @@ static inline int fscrypt_prepare_lookup(struct inode *dir,
fname->usr_fname = &dentry->d_name;
fname->disk_name.name = (unsigned char *)dentry->d_name.name;
fname->disk_name.len = dentry->d_name.len;
+
+ fscrypt_prepare_dentry(dentry, false);
+
return 0;
}
--
2.43.0
In preparation to get case-insensitive dentry operations from sb->s_d_op
again, use the same structure with and without fscrypt.
Signed-off-by: Gabriel Krisman Bertazi <[email protected]>
---
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
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 <[email protected]>
Acked-by: Theodore Ts'o <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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. Yes, this will be solved when we move d_op configuration back
to ->s_d_op. Yet, we better have an explicit fix to avoid messing up
again.
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 <[email protected]>
Acked-by: Amir Goldstein <[email protected]>
---
changes since v5:
- summary line again (eric)
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
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.
Finally, 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.
Signed-off-by: Gabriel Krisman Bertazi <[email protected]>
---
Changes since v5:
- Merge with another patch(eric)
- revert conditional check (eric)
- drop comment (eric)
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 | 29 ++++++++++++++++++++++-------
1 file changed, 22 insertions(+), 7 deletions(-)
diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
index d1f17b90c30f..4283997c1bfd 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,29 @@ 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;
+ /*
+ * VFS calls fscrypt_handle_d_move even for non-fscrypt
+ * filesystems.
+ */
+ if (dentry->d_flags & DCACHE_NOKEY_NAME) {
+ dentry->d_flags &= ~DCACHE_NOKEY_NAME;
+
+ /*
+ * Other filesystem features might be handling dentry
+ * revalidation, in which case it cannot be disabled.
+ */
+ if (dentry->d_op->d_revalidate == fscrypt_d_revalidate)
+ dentry->d_flags &= ~DCACHE_OP_REVALIDATE;
+ }
}
/**
@@ -368,7 +384,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
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 <[email protected]>
---
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
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 capable filesystems, it should be propagated through
->s_d_op.
Signed-off-by: Gabriel Krisman Bertazi <[email protected]>
---
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
On Mon, Feb 12, 2024 at 09:13:13PM -0500, Gabriel Krisman Bertazi wrote:
> Both fscrypt_prepare_lookup_dentry_partial and
> fscrypt_prepare_lookup_dentry
Neither of these functions exist.
- Eric
On Mon, Feb 12, 2024 at 09:13:14PM -0500, Gabriel Krisman Bertazi wrote:
> Finally, we need to clean the dentry->flags even for unencrypted
> dentries, so the ->d_lock might be acquired even for them. In order to
might => must?
> diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
> index 47567a6a4f9d..d1f17b90c30f 100644
> --- a/include/linux/fscrypt.h
> +++ b/include/linux/fscrypt.h
> @@ -951,10 +951,29 @@ static inline int fscrypt_prepare_rename(struct inode *old_dir,
> static inline void fscrypt_prepare_dentry(struct dentry *dentry,
> bool is_nokey_name)
> {
> + /*
> + * This code tries to only take ->d_lock when necessary to write
> + * to ->d_flags. We shouldn't be peeking on d_flags for
> + * DCACHE_OP_REVALIDATE unlocked, but in the unlikely case
> + * there is a race, the worst it can happen is that we fail to
> + * unset DCACHE_OP_REVALIDATE and pay the cost of an extra
> + * d_revalidate.
> + */
> if (is_nokey_name) {
> spin_lock(&dentry->d_lock);
> 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.
> + */
> + spin_lock(&dentry->d_lock);
> + dentry->d_flags &= ~DCACHE_OP_REVALIDATE;
> + spin_unlock(&dentry->d_lock);
> }
> }
Does this all get optimized out when !CONFIG_FS_ENCRYPTION?
As-is, I don't think the d_revalidate part will be optimized out.
You may need to create a !CONFIG_FS_ENCRYPTION stub explicitly.
- Eric
On Mon, Feb 12, 2024 at 09:13:15PM -0500, Gabriel Krisman Bertazi wrote:
> 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.
>
> Finally, 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.
>
> Signed-off-by: Gabriel Krisman Bertazi <[email protected]>
I think this explanation misses an important point, which is that it's only
*directories* where a no-key dentry can become the regular dentry. The VFS does
the move because it only allows one dentry to exist per directory.
For nondirectories, the dentries don't get reused and this patch is irrelevant.
(Of course, there's no point in making fscrypt_handle_d_move() check whether the
dentry is a directory, since checking DCACHE_NOKEY_NAME is sufficient.)
The diff itself looks good -- thanks.
- Eric
On Mon, Feb 12, 2024 at 09:13:11PM -0500, Gabriel Krisman Bertazi wrote:
> Hi,
>
> v6 of this patchset applying the comments from Eric and the suggestion from
> Christian. Thank you for your feedback.
>
> Eric, since this is getting close to merging, how do you want to handle
> it? I will take patch 1 through my tree already, are you fine if I merge
> this through unicode or do you want to take it through fscrypt?
>
> As usual, this survived fstests on ext4 and f2fs.
I think you should just take the whole series through the unicode tree.
If I understand correctly, this series is really about fixing things for
casefolding support, not fscrypt support, right? There is a lot of interaction
between the two, but ultimately it's casefold that gets fixed.
- Eric
On Wed, Feb 14, 2024 at 04:16:31PM -0800, Eric Biggers wrote:
> On Mon, Feb 12, 2024 at 09:13:15PM -0500, Gabriel Krisman Bertazi wrote:
> > 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.
> >
> > Finally, 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.
> >
> > Signed-off-by: Gabriel Krisman Bertazi <[email protected]>
>
> I think this explanation misses an important point, which is that it's only
> *directories* where a no-key dentry can become the regular dentry. The VFS does
> the move because it only allows one dentry to exist per directory.
>
> For nondirectories, the dentries don't get reused and this patch is irrelevant.
>
> (Of course, there's no point in making fscrypt_handle_d_move() check whether the
> dentry is a directory, since checking DCACHE_NOKEY_NAME is sufficient.)
>
> The diff itself looks good -- thanks.
>
Also, do I understand correctly that this patch is a performance optimization,
not preventing a performance regression? The similar patch that precedes this
one, "fscrypt: Drop d_revalidate for valid dentries during lookup", is about
preventing a performance regression on dentries that aren't no-key. This patch
looks deceptively similar, but it only affects no-key directory dentries, which
we were already doing the fscrypt_d_revalidate for, even after the move to the
plaintext name. It's probably still a worthwhile optimization to stop doing the
fscrypt_d_revalidate when a directory dentry gets moved like that. But I want
to make sure I'm correctly understanding each patch.
- Eric
Eric Biggers <[email protected]> writes:
> On Wed, Feb 14, 2024 at 04:16:31PM -0800, Eric Biggers wrote:
>> On Mon, Feb 12, 2024 at 09:13:15PM -0500, Gabriel Krisman Bertazi wrote:
>> > 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.
>> >
>> > Finally, 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.
>> >
>> > Signed-off-by: Gabriel Krisman Bertazi <[email protected]>
>>
>> I think this explanation misses an important point, which is that it's only
>> *directories* where a no-key dentry can become the regular dentry. The VFS does
>> the move because it only allows one dentry to exist per directory.
>>
>> For nondirectories, the dentries don't get reused and this patch is irrelevant.
>>
>> (Of course, there's no point in making fscrypt_handle_d_move() check whether the
>> dentry is a directory, since checking DCACHE_NOKEY_NAME is sufficient.)
>>
>> The diff itself looks good -- thanks.
>>
>
> Also, do I understand correctly that this patch is a performance optimization,
> not preventing a performance regression? The similar patch that precedes this
> one, "fscrypt: Drop d_revalidate for valid dentries during lookup", is about
> preventing a performance regression on dentries that aren't no-key. This patch
> looks deceptively similar, but it only affects no-key directory dentries, which
> we were already doing the fscrypt_d_revalidate for, even after the move to the
> plaintext name. It's probably still a worthwhile optimization to stop doing the
> fscrypt_d_revalidate when a directory dentry gets moved like that. But I want
> to make sure I'm correctly understanding each patch.
Hi Eric,
Yes, your understanding is correct. The previous patch prevents the
regression, given that we will install d_revalidate "by default" on all
dentries when fscrypt is enabled. Once that was done, it seemed obvious
to add the optimization to also drop it when the key is added later,
which is what this patch is about.
I'll follow up with a v7 shortly. Just need to find some cycles to work
on it.
thanks,
--
Gabriel Krisman Bertazi
Eric Biggers <[email protected]> writes:
> On Mon, Feb 12, 2024 at 09:13:14PM -0500, Gabriel Krisman Bertazi wrote:
>> Finally, we need to clean the dentry->flags even for unencrypted
>> dentries, so the ->d_lock might be acquired even for them. In order to
>
> might => must?
>
>> diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
>> index 47567a6a4f9d..d1f17b90c30f 100644
>> --- a/include/linux/fscrypt.h
>> +++ b/include/linux/fscrypt.h
>> @@ -951,10 +951,29 @@ static inline int fscrypt_prepare_rename(struct inode *old_dir,
>> static inline void fscrypt_prepare_dentry(struct dentry *dentry,
>> bool is_nokey_name)
>> {
>> + /*
>> + * This code tries to only take ->d_lock when necessary to write
>> + * to ->d_flags. We shouldn't be peeking on d_flags for
>> + * DCACHE_OP_REVALIDATE unlocked, but in the unlikely case
>> + * there is a race, the worst it can happen is that we fail to
>> + * unset DCACHE_OP_REVALIDATE and pay the cost of an extra
>> + * d_revalidate.
>> + */
>> if (is_nokey_name) {
>> spin_lock(&dentry->d_lock);
>> 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.
>> + */
>> + spin_lock(&dentry->d_lock);
>> + dentry->d_flags &= ~DCACHE_OP_REVALIDATE;
>> + spin_unlock(&dentry->d_lock);
>> }
>> }
>
> Does this all get optimized out when !CONFIG_FS_ENCRYPTION?
>
> As-is, I don't think the d_revalidate part will be optimized out.
>
it seems to get optimized out:
This is ext4_lookup built with CONFIG_FS_ENCRYPTION=n
ffffffff814ca3e0 <ext4_lookup>:
ffffffff814ca3e0: e8 5b b5 c3 ff call ffffffff81105940 <__fentry__>
ffffffff814ca3e5: 41 54 push %r12
ffffffff814ca3e7: 55 push %rbp
ffffffff814ca3e8: 53 push %rbx
ffffffff814ca3e9: 48 83 ec 58 sub $0x58,%rsp
ffffffff814ca3ed: 8b 56 24 mov 0x24(%rsi),%edx
ffffffff814ca3f0: 65 48 8b 04 25 28 00 mov %gs:0x28,%rax
ffffffff814ca3f7: 00 00
ffffffff814ca3f9: 48 89 44 24 50 mov %rax,0x50(%rsp)
ffffffff814ca3fe: 31 c0 xor %eax,%eax
ffffffff814ca400: 48 c7 c0 dc ff ff ff mov $0xffffffffffffffdc,%rax
ffffffff814ca407: 81 fa ff 00 00 00 cmp $0xff,%edx
ffffffff814ca40d: 76 21 jbe ffffffff814ca430 <ext4_lookup+0x50>
ffffffff814ca40f: 48 8b 4c 24 50 mov 0x50(%rsp),%rcx
ffffffff814ca414: 65 48 33 0c 25 28 00 xor %gs:0x28,%rcx
ffffffff814ca41b: 00 00
ffffffff814ca41d: 0f 85 cd 01 00 00 jne ffffffff814ca5f0 <ext4_lookup+0x210> <- (__stack_chk_fail)
ffffffff814ca423: 48 83 c4 58 add $0x58,%rsp
ffffffff814ca427: 5b pop %rbx
ffffffff814ca428: 5d pop %rbp
ffffffff814ca429: 41 5c pop %r12
ffffffff814ca42b: e9 70 21 8b 00 jmp ffffffff81d7c5a0 <__x86_return_thunk>
ffffffff814ca430: 48 89 f3 mov %rsi,%rbx
ffffffff814ca433: 89 54 24 20 mov %edx,0x20(%rsp)
ffffffff814ca437: 48 8d 76 20 lea 0x20(%rsi),%rsi
ffffffff814ca43b: 48 8b 43 28 mov 0x28(%rbx),%rax
ffffffff814ca43f: 48 8d 54 24 10 lea 0x10(%rsp),%rdx
ffffffff814ca444: 48 89 fd mov %rdi,%rbp
ffffffff814ca447: 48 89 74 24 10 mov %rsi,0x10(%rsp)
ffffffff814ca44c: 48 89 44 24 18 mov %rax,0x18(%rsp)
ffffffff814ca451: e8 ca f0 ff ff call ffffffff814c9520 <ext4_fname_setup_ci_filename>
[..]
I had also confirmed previously that fscrypt_lookup_prepare and
fscrypt_prepare_dentry gets correctly inlined into
ext4_fname_prepare_lookup.
> You may need to create a !CONFIG_FS_ENCRYPTION stub explicitly.
But, in spite of gcc doing the right thing now, fscrypt_prepare_dentry
might grow in the future. So, if you don't mind, I will still add the
stub explicitly, as you suggested.
thanks,
--
Gabriel Krisman Bertazi