[Apologies for the quick spin of a v2. The only difference are a couple
fixes to the build when CONFIG_UNICODE=n caught by LKP and detailed in
each patch changelog.]
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 (8):
dcache: Add helper to disable d_revalidate for a specific dentry
fscrypt: Drop d_revalidate if key is available
libfs: Merge encrypted_ci_dentry_ops and ci_dentry_ops
libfs: Expose generic_ci_dentry_ops outside of libfs
ext4: Set the case-insensitive dentry operations through ->s_d_op
f2fs: Set the case-insensitive dentry operations through ->s_d_op
libfs: Don't support setting casefold operations during lookup
fscrypt: Move d_revalidate configuration back into fscrypt
fs/crypto/fname.c | 9 +++++-
fs/crypto/hooks.c | 8 +++++
fs/dcache.c | 10 +++++++
fs/ext4/namei.c | 1 -
fs/ext4/super.c | 5 ++++
fs/f2fs/namei.c | 1 -
fs/f2fs/super.c | 5 ++++
fs/libfs.c | 66 ++---------------------------------------
fs/ubifs/dir.c | 1 -
include/linux/dcache.h | 1 +
include/linux/fs.h | 2 +-
include/linux/fscrypt.h | 10 +++----
12 files changed, 45 insertions(+), 74 deletions(-)
--
2.43.0
Case-insensitive wants d_compare/d_hash for every dentry in the
filesystem, while fscrypt needs d_revalidate only for DCACHE_NOKEY_NAME.
This means we currently can't use sb->s_d_op to set case-insensitive
hooks in fscrypt+case-insensitive filesystems without paying the cost to
call d_revalidate for every dentry in the filesystem.
In preparation to doing exactly that, add a way to disable d_revalidate
later.
Suggested-by: Al Viro <[email protected]>
Signed-off-by: Gabriel Krisman Bertazi <[email protected]>
---
fs/dcache.c | 10 ++++++++++
include/linux/dcache.h | 1 +
2 files changed, 11 insertions(+)
diff --git a/fs/dcache.c b/fs/dcache.c
index c82ae731df9a..1f5464cd3bd1 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1911,6 +1911,16 @@ struct dentry *d_alloc_name(struct dentry *parent, const char *name)
}
EXPORT_SYMBOL(d_alloc_name);
+void d_set_always_valid(struct dentry *dentry)
+{
+ if (!(dentry->d_flags & DCACHE_OP_REVALIDATE))
+ return;
+
+ spin_lock(&dentry->d_lock);
+ dentry->d_flags &= ~DCACHE_OP_REVALIDATE;
+ spin_unlock(&dentry->d_lock);
+}
+
void d_set_d_op(struct dentry *dentry, const struct dentry_operations *op)
{
WARN_ON_ONCE(dentry->d_op);
diff --git a/include/linux/dcache.h b/include/linux/dcache.h
index 3da2f0545d5d..d2ce151b2d8e 100644
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -225,6 +225,7 @@ extern struct dentry * d_instantiate_anon(struct dentry *, struct inode *);
extern void __d_drop(struct dentry *dentry);
extern void d_drop(struct dentry *dentry);
extern void d_delete(struct dentry *);
+extern void d_set_always_valid(struct dentry *dentry);
extern void d_set_d_op(struct dentry *dentry, const struct dentry_operations *op);
/* allocate/de-allocate */
--
2.43.0
fscrypt dentries are always valid once the key is available. Since the
key cannot be removed without evicting the dentry, we don't need to keep
retrying to revalidate it.
Signed-off-by: Gabriel Krisman Bertazi <[email protected]>
---
fs/crypto/fname.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c
index 7b3fc189593a..0457ba2d7d76 100644
--- a/fs/crypto/fname.c
+++ b/fs/crypto/fname.c
@@ -591,8 +591,15 @@ int fscrypt_d_revalidate(struct dentry *dentry, unsigned int flags)
* reverting to no-key names without evicting the directory's inode
* -- which implies eviction of the dentries in the directory.
*/
- if (!(dentry->d_flags & DCACHE_NOKEY_NAME))
+ if (!(dentry->d_flags & DCACHE_NOKEY_NAME)) {
+ /*
+ * If fscrypt is the only feature requiring
+ * revalidation for this dentry, we can just disable it.
+ */
+ if (dentry->d_op->d_revalidate == &fscrypt_d_revalidate)
+ d_set_always_valid(dentry);
return 1;
+ }
/*
* No-key name; valid if the directory's key is still unavailable.
--
2.43.0
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 <[email protected]>
---
fs/libfs.c | 34 ++++++----------------------------
1 file changed, 6 insertions(+), 28 deletions(-)
diff --git a/fs/libfs.c b/fs/libfs.c
index e9440d55073c..52c944173e57 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -1768,6 +1768,9 @@ 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,
+#if defined(CONFIG_FS_ENCRYPTION)
+ .d_revalidate = fscrypt_d_revalidate,
+#endif
};
#endif
@@ -1777,14 +1780,6 @@ static const struct dentry_operations generic_encrypted_dentry_ops = {
};
#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,
- .d_revalidate = fscrypt_d_revalidate,
-};
-#endif
-
/**
* generic_set_encrypted_ci_d_ops - helper for setting d_ops for given dentry
* @dentry: dentry to set ops on
@@ -1801,38 +1796,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
In preparation to allow filesystems to set this through sb->s_d_op,
expose the symbol directly to the filesystems.
Signed-off-by: Gabriel Krisman Bertazi <[email protected]>
---
fs/libfs.c | 2 +-
include/linux/fs.h | 1 +
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/fs/libfs.c b/fs/libfs.c
index 52c944173e57..b8ecada3a5b2 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -1765,7 +1765,7 @@ static int generic_ci_d_hash(const struct dentry *dentry, struct qstr *str)
return 0;
}
-static const struct dentry_operations generic_ci_dentry_ops = {
+const struct dentry_operations generic_ci_dentry_ops = {
.d_hash = generic_ci_d_hash,
.d_compare = generic_ci_d_compare,
#if defined(CONFIG_FS_ENCRYPTION)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 98b7a7a8c42e..887a27d07f96 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 const struct dentry_operations generic_ci_dentry_ops;
int may_setattr(struct mnt_idmap *idmap, struct inode *inode,
unsigned int ia_valid);
--
2.43.0
All dentries in a case-insensitive filesystem have the same set of
dentry operations. Therefore, we should let VFS propagate them from
sb->s_d_op d_alloc instead of setting at lookup time.
This was already the case 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. Let's revert to the previous
implementation.
Suggested-by: Al Viro <[email protected]>
Signed-off-by: Gabriel Krisman Bertazi <[email protected]>
---
changes since v1:
- Fix CONFIG_UNICODE=n build (lkp)
---
fs/ext4/namei.c | 6 +++++-
fs/ext4/super.c | 5 +++++
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index d252935f9c8a..463e73fb5bf0 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -1762,7 +1762,11 @@ 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);
+
+ /* Case-insensitive volumes set dentry ops through sb->s_d_op. */
+ if (!dir->i_sb->s_d_op)
+ 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..e0680d0641aa 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -5493,6 +5493,11 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
goto failed_mount4;
}
+#if IS_ENABLED(CONFIG_UNICODE)
+ if (sb->s_encoding)
+ sb->s_d_op = &generic_ci_dentry_ops;
+#endif
+
sb->s_root = d_make_root(root);
if (!sb->s_root) {
ext4_msg(sb, KERN_ERR, "get root dentry failed");
--
2.43.0
All dentries in a case-insensitive filesystem have the same set of
dentry operations. Therefore, we should let VFS propagate them from
sb->s_d_op d_alloc instead of setting at lookup time.
This was already the case 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. Let's revert to the previous
implementation.
Suggested-by: Al Viro <[email protected]>
Signed-off-by: Gabriel Krisman Bertazi <[email protected]>
---
changes since v1:
- Fix CONFIG_UNICODE=n build (lkp)
---
fs/f2fs/namei.c | 6 +++++-
fs/f2fs/super.c | 5 +++++
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
index d0053b0284d8..6aec21f0b5d6 100644
--- a/fs/f2fs/namei.c
+++ b/fs/f2fs/namei.c
@@ -532,7 +532,11 @@ 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);
+
+ /* Case-insensitive volumes set dentry ops through sb->s_d_op. */
+ if (!dir->i_sb->s_d_op)
+ 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..8d1abd7f3a6f 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -4663,6 +4663,11 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
goto free_node_inode;
}
+#if IS_ENABLED(CONFIG_UNICODE)
+ if (sb->s_encoding)
+ sb->s_d_op = &generic_ci_dentry_ops;
+#endif
+
sb->s_root = d_make_root(root); /* allocate root dentry */
if (!sb->s_root) {
err = -ENOMEM;
--
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 filesystems, it should be configured through ->s_d_op.
Signed-off-by: Gabriel Krisman Bertazi <[email protected]>
---
fs/libfs.c | 15 ---------------
1 file changed, 15 deletions(-)
diff --git a/fs/libfs.c b/fs/libfs.c
index b8ecada3a5b2..41c02c003265 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -1784,27 +1784,12 @@ static const struct dentry_operations generic_encrypted_dentry_ops = {
* 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);
--
2.43.0
This partially reverts commit bb9cd9106b22 ("fscrypt: Have filesystems
handle their d_ops"), which moved this handler out of fscrypt and into
the filesystems, in preparation to support casefold and fscrypt
combinations. Now that we set casefolding operations through
->s_d_op, move this back into fscrypt, where it belongs, but take care
to handle filesystems that set their own sb->s_d_op.
Signed-off-by: Gabriel Krisman Bertazi <[email protected]>
---
changes since v1:
- Fix unused definition warning (lkp)
---
fs/crypto/hooks.c | 8 ++++++++
fs/ext4/namei.c | 5 -----
fs/f2fs/namei.c | 5 -----
fs/libfs.c | 25 -------------------------
fs/ubifs/dir.c | 1 -
include/linux/fs.h | 1 -
include/linux/fscrypt.h | 10 +++++-----
7 files changed, 13 insertions(+), 42 deletions(-)
diff --git a/fs/crypto/hooks.c b/fs/crypto/hooks.c
index 52504dd478d3..166837d5af29 100644
--- a/fs/crypto/hooks.c
+++ b/fs/crypto/hooks.c
@@ -94,6 +94,10 @@ int __fscrypt_prepare_rename(struct inode *old_dir, struct dentry *old_dentry,
}
EXPORT_SYMBOL_GPL(__fscrypt_prepare_rename);
+static const struct dentry_operations fscrypt_dentry_ops = {
+ .d_revalidate = fscrypt_d_revalidate,
+};
+
int __fscrypt_prepare_lookup(struct inode *dir, struct dentry *dentry,
struct fscrypt_name *fname)
{
@@ -106,6 +110,10 @@ int __fscrypt_prepare_lookup(struct inode *dir, struct dentry *dentry,
spin_lock(&dentry->d_lock);
dentry->d_flags |= DCACHE_NOKEY_NAME;
spin_unlock(&dentry->d_lock);
+
+ /* Give preference to the filesystem hooks, if any. */
+ if (!dentry->d_op)
+ d_set_d_op(dentry, &fscrypt_dentry_ops);
}
return err;
}
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 463e73fb5bf0..3f0b853a371e 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -1762,11 +1762,6 @@ static struct buffer_head *ext4_lookup_entry(struct inode *dir,
struct buffer_head *bh;
err = ext4_fname_prepare_lookup(dir, dentry, &fname);
-
- /* Case-insensitive volumes set dentry ops through sb->s_d_op. */
- if (!dir->i_sb->s_d_op)
- generic_set_encrypted_ci_d_ops(dentry);
-
if (err == -ENOENT)
return NULL;
if (err)
diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
index 6aec21f0b5d6..b40c6c393bd6 100644
--- a/fs/f2fs/namei.c
+++ b/fs/f2fs/namei.c
@@ -532,11 +532,6 @@ static struct dentry *f2fs_lookup(struct inode *dir, struct dentry *dentry,
}
err = f2fs_prepare_lookup(dir, dentry, &fname);
-
- /* Case-insensitive volumes set dentry ops through sb->s_d_op. */
- if (!dir->i_sb->s_d_op)
- generic_set_encrypted_ci_d_ops(dentry);
-
if (err == -ENOENT)
goto out_splice;
if (err)
diff --git a/fs/libfs.c b/fs/libfs.c
index 41c02c003265..a04d6c1ad77a 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -1774,31 +1774,6 @@ const struct dentry_operations generic_ci_dentry_ops = {
};
#endif
-#ifdef CONFIG_FS_ENCRYPTION
-static const struct dentry_operations generic_encrypted_dentry_ops = {
- .d_revalidate = fscrypt_d_revalidate,
-};
-#endif
-
-/**
- * generic_set_encrypted_ci_d_ops - helper for setting d_ops for given dentry
- * @dentry: dentry to set ops on
- *
- * 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)
-{
-#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);
-
/**
* inode_maybe_inc_iversion - increments i_version
* @inode: inode with the i_version that should be updated
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/include/linux/fs.h b/include/linux/fs.h
index 887a27d07f96..e5ae21f9f637 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 const struct dentry_operations generic_ci_dentry_ops;
int may_setattr(struct mnt_idmap *idmap, struct inode *inode,
diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
index 12f9e455d569..97a11280c2bd 100644
--- a/include/linux/fscrypt.h
+++ b/include/linux/fscrypt.h
@@ -961,11 +961,11 @@ static inline int fscrypt_prepare_rename(struct inode *old_dir,
* key is available, then the lookup is assumed to be by plaintext name;
* otherwise, it is assumed to be by no-key name.
*
- * This will set DCACHE_NOKEY_NAME on the dentry if the lookup is by no-key
- * name. In this case the filesystem must assign the dentry a dentry_operations
- * which contains fscrypt_d_revalidate (or contains a d_revalidate method that
- * calls fscrypt_d_revalidate), so that the dentry will be invalidated if the
- * directory's encryption key is later added.
+ * This also optionally installs a custom ->d_revalidate() method which will
+ * invalidate the dentry if it was created without the key and the key is later
+ * added. If the filesystem provides its own ->d_op hooks, they will be used
+ * instead, but then the filesystem must make sure to call fscrypt_d_revalidate
+ * in its d_revalidate hook, to check if fscrypt considers the dentry stale.
*
* Return: 0 on success; -ENOENT if the directory's key is unavailable but the
* filename isn't a valid no-key name, so a negative dentry should be created;
--
2.43.0
On Fri, Dec 15, 2023 at 04:16:03PM -0500, Gabriel Krisman Bertazi wrote:
> +#if defined(CONFIG_FS_ENCRYPTION)
> + .d_revalidate = fscrypt_d_revalidate,
> +#endif
#ifdef CONFIG_FS_ENCRYPTION, since it's a bool.
- Eric
On Fri, Dec 15, 2023 at 04:16:04PM -0500, Gabriel Krisman Bertazi wrote:
> In preparation to allow filesystems to set this through sb->s_d_op,
> expose the symbol directly to the filesystems.
>
> Signed-off-by: Gabriel Krisman Bertazi <[email protected]>
> ---
> fs/libfs.c | 2 +-
> include/linux/fs.h | 1 +
> 2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/fs/libfs.c b/fs/libfs.c
> index 52c944173e57..b8ecada3a5b2 100644
> --- a/fs/libfs.c
> +++ b/fs/libfs.c
> @@ -1765,7 +1765,7 @@ static int generic_ci_d_hash(const struct dentry *dentry, struct qstr *str)
> return 0;
> }
>
> -static const struct dentry_operations generic_ci_dentry_ops = {
> +const struct dentry_operations generic_ci_dentry_ops = {
> .d_hash = generic_ci_d_hash,
> .d_compare = generic_ci_d_compare,
This needs an EXPORT_SYMBOL_GPL(), since the filesystems that will use this can
be loadable modules.
- Eric
On Fri, Dec 15, 2023 at 04:16:02PM -0500, Gabriel Krisman Bertazi wrote:
> fscrypt dentries are always valid once the key is available. Since the
> key cannot be removed without evicting the dentry, we don't need to keep
> retrying to revalidate it.
>
> Signed-off-by: Gabriel Krisman Bertazi <[email protected]>
IIUC, this patch minimizes the overhead of fscrypt_d_revalidate() both for
encrypted and unencrypted dentries. That's what's needed (seeing as this series
makes fscrypt_d_revalidate be installed on unencrypted dentries), but the commit
message only mentions the encrypted case. It would be helpful to mention both.
> ---
> fs/crypto/fname.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c
> index 7b3fc189593a..0457ba2d7d76 100644
> --- a/fs/crypto/fname.c
> +++ b/fs/crypto/fname.c
> @@ -591,8 +591,15 @@ int fscrypt_d_revalidate(struct dentry *dentry, unsigned int flags)
> * reverting to no-key names without evicting the directory's inode
> * -- which implies eviction of the dentries in the directory.
> */
> - if (!(dentry->d_flags & DCACHE_NOKEY_NAME))
> + if (!(dentry->d_flags & DCACHE_NOKEY_NAME)) {
> + /*
> + * If fscrypt is the only feature requiring
> + * revalidation for this dentry, we can just disable it.
> + */
> + if (dentry->d_op->d_revalidate == &fscrypt_d_revalidate)
No need for the & in &fscrypt_d_revalidate.
- Eric
On Fri, Dec 15, 2023 at 04:16:08PM -0500, Gabriel Krisman Bertazi wrote:
> int __fscrypt_prepare_lookup(struct inode *dir, struct dentry *dentry,
> struct fscrypt_name *fname)
> {
> @@ -106,6 +110,10 @@ int __fscrypt_prepare_lookup(struct inode *dir, struct dentry *dentry,
> spin_lock(&dentry->d_lock);
> dentry->d_flags |= DCACHE_NOKEY_NAME;
> spin_unlock(&dentry->d_lock);
> +
> + /* Give preference to the filesystem hooks, if any. */
> + if (!dentry->d_op)
> + d_set_d_op(dentry, &fscrypt_dentry_ops);
I think that fscrypt_prepare_lookup_partial() should get this too, since it sets
DCACHE_NOKEY_NAME too (though currently the only filesystem that calls it has
its own d_revalidate anyway).
- Eric
On Fri, Dec 15, 2023 at 04:16:00PM -0500, Gabriel Krisman Bertazi wrote:
> [Apologies for the quick spin of a v2. The only difference are a couple
> fixes to the build when CONFIG_UNICODE=n caught by LKP and detailed in
> each patch changelog.]
>
> 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 (8):
> dcache: Add helper to disable d_revalidate for a specific dentry
> fscrypt: Drop d_revalidate if key is available
> libfs: Merge encrypted_ci_dentry_ops and ci_dentry_ops
> libfs: Expose generic_ci_dentry_ops outside of libfs
> ext4: Set the case-insensitive dentry operations through ->s_d_op
> f2fs: Set the case-insensitive dentry operations through ->s_d_op
> libfs: Don't support setting casefold operations during lookup
> fscrypt: Move d_revalidate configuration back into fscrypt
Thanks Gabriel, this series looks good. Sorry that we missed this when adding
the support for encrypt+casefold.
It's slightly awkward that some lines of code added by patches 5-6 are removed
in patch 8. These changes look very hard to split up, though, so you've
probably done about the best that can be done.
One question/request: besides performance, the other reason we're so careful
about minimizing when ->d_revalidate is set for fscrypt is so that overlayfs
works on encrypted directories. This is because overlayfs is not compatible
with ->d_revalidate. I think your solution still works for that, since
DCACHE_OP_REVALIDATE will be cleared after the first call to
fscrypt_d_revalidate(), and when checking for usupported dentries overlayfs does
indeed check for DCACHE_OP_REVALIDATE instead of ->d_revalidate directly.
However, that does rely on that very first call to ->d_revalidate actually
happening before the check is done. It would be nice to verify that
overlayfs+fscrypt indeed continues to work, and explicitly mention this
somewhere (I don't see any mention of overlayfs+fscrypt in the series).
- Eric
On Fri, Dec 15, 2023 at 04:16:02PM -0500, Gabriel Krisman Bertazi wrote:
> fscrypt dentries are always valid once the key is available. Since the
> key cannot be removed without evicting the dentry, we don't need to keep
> retrying to revalidate it.
>
> Signed-off-by: Gabriel Krisman Bertazi <[email protected]>
> ---
> fs/crypto/fname.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c
> index 7b3fc189593a..0457ba2d7d76 100644
> --- a/fs/crypto/fname.c
> +++ b/fs/crypto/fname.c
> @@ -591,8 +591,15 @@ int fscrypt_d_revalidate(struct dentry *dentry, unsigned int flags)
> * reverting to no-key names without evicting the directory's inode
> * -- which implies eviction of the dentries in the directory.
> */
> - if (!(dentry->d_flags & DCACHE_NOKEY_NAME))
> + if (!(dentry->d_flags & DCACHE_NOKEY_NAME)) {
> + /*
> + * If fscrypt is the only feature requiring
> + * revalidation for this dentry, we can just disable it.
> + */
> + if (dentry->d_op->d_revalidate == &fscrypt_d_revalidate)
Umm... What about ceph? IOW, why do we care how had we gotten to that
function - directly via ->d_revalidate() or from ->d_revalidate() instance?
On Thu, Dec 21, 2023 at 07:14:02AM +0000, Al Viro wrote:
> On Fri, Dec 15, 2023 at 04:16:02PM -0500, Gabriel Krisman Bertazi wrote:
> > fscrypt dentries are always valid once the key is available. Since the
> > key cannot be removed without evicting the dentry, we don't need to keep
> > retrying to revalidate it.
> >
> > Signed-off-by: Gabriel Krisman Bertazi <[email protected]>
> > ---
> > fs/crypto/fname.c | 9 ++++++++-
> > 1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c
> > index 7b3fc189593a..0457ba2d7d76 100644
> > --- a/fs/crypto/fname.c
> > +++ b/fs/crypto/fname.c
> > @@ -591,8 +591,15 @@ int fscrypt_d_revalidate(struct dentry *dentry, unsigned int flags)
> > * reverting to no-key names without evicting the directory's inode
> > * -- which implies eviction of the dentries in the directory.
> > */
> > - if (!(dentry->d_flags & DCACHE_NOKEY_NAME))
> > + if (!(dentry->d_flags & DCACHE_NOKEY_NAME)) {
> > + /*
> > + * If fscrypt is the only feature requiring
> > + * revalidation for this dentry, we can just disable it.
> > + */
> > + if (dentry->d_op->d_revalidate == &fscrypt_d_revalidate)
>
> Umm... What about ceph? IOW, why do we care how had we gotten to that
> function - directly via ->d_revalidate() or from ->d_revalidate() instance?
Nevermind.
On Fri, Dec 15, 2023 at 04:16:08PM -0500, Gabriel Krisman Bertazi wrote:
> +static const struct dentry_operations fscrypt_dentry_ops = {
> + .d_revalidate = fscrypt_d_revalidate,
> +};
> +
> int __fscrypt_prepare_lookup(struct inode *dir, struct dentry *dentry,
> struct fscrypt_name *fname)
> {
> @@ -106,6 +110,10 @@ int __fscrypt_prepare_lookup(struct inode *dir, struct dentry *dentry,
> spin_lock(&dentry->d_lock);
> dentry->d_flags |= DCACHE_NOKEY_NAME;
> spin_unlock(&dentry->d_lock);
> +
> + /* Give preference to the filesystem hooks, if any. */
> + if (!dentry->d_op)
> + d_set_d_op(dentry, &fscrypt_dentry_ops);
> }
> return err;
Hmm... Could we simply set ->s_d_op to &fscrypt_dentry_ops in non-ci case
*AND* have __fscrypt_prepare_lookup() clear DCACHE_OP_REVALIDATE in case
when it's not setting DCACHE_NOKEY_NAME and ->d_op->d_revalidate is
equal to fscrypt_d_revalidate? I mean,
spin_lock(&dentry->d_lock);
if (fname->is_nokey_name)
dentry->d_flags |= DCACHE_NOKEY_NAME;
else if (dentry->d_flags & DCACHE_OP_REVALIDATE &&
dentry->d_op->d_revalidate == fscrypt_d_revalidate)
dentry->d_flags &= ~DCACHE_OP_REVALIDATE;
spin_unlock(&dentry->d_lock);
here + always set ->s_d_op for ext4 and friends (conditional upon
the CONFIG_UNICODE).
No encryption - fine, you get ->is_nokey_name false from the very
beginning, DCACHE_OP_REVALIDATE is cleared and VFS won't ever call
->d_revalidate(); not even the first time.
Yes, you pay minimal price in dentry_unlink_inode() when we hit
if (dentry->d_op && dentry->d_op->d_iput)
and bugger off after the second fetch instead of the first one.
I would be quite surprised if it turns out to be measurable,
but if it is, we can always add DCACHE_OP_IPUT to flags.
Similar for ->d_op->d_release (called in the end of
__dentry_kill()). Again, that only makes sense if we get
a measurable overhead from that.
On Thu, Dec 21, 2023 at 07:39:40AM +0000, Al Viro wrote:
> Hmm... Could we simply set ->s_d_op to &fscrypt_dentry_ops in non-ci case
> *AND* have __fscrypt_prepare_lookup() clear DCACHE_OP_REVALIDATE in case
> when it's not setting DCACHE_NOKEY_NAME and ->d_op->d_revalidate is
> equal to fscrypt_d_revalidate? I mean,
>
> spin_lock(&dentry->d_lock);
> if (fname->is_nokey_name)
> dentry->d_flags |= DCACHE_NOKEY_NAME;
> else if (dentry->d_flags & DCACHE_OP_REVALIDATE &&
> dentry->d_op->d_revalidate == fscrypt_d_revalidate)
> dentry->d_flags &= ~DCACHE_OP_REVALIDATE;
> spin_unlock(&dentry->d_lock);
>
> here + always set ->s_d_op for ext4 and friends (conditional upon
> the CONFIG_UNICODE).
>
> No encryption - fine, you get ->is_nokey_name false from the very
> beginning, DCACHE_OP_REVALIDATE is cleared and VFS won't ever call
> ->d_revalidate(); not even the first time.
>
> Yes, you pay minimal price in dentry_unlink_inode() when we hit
> if (dentry->d_op && dentry->d_op->d_iput)
> and bugger off after the second fetch instead of the first one.
> I would be quite surprised if it turns out to be measurable,
> but if it is, we can always add DCACHE_OP_IPUT to flags.
> Similar for ->d_op->d_release (called in the end of
> __dentry_kill()). Again, that only makes sense if we get
> a measurable overhead from that.
fscrypt_prepare_lookup() handles unencrypted directories inline, without calling
__fscrypt_prepare_lookup() which is only for encrypted directories. So the
logic to clear DCACHE_OP_REVALIDATE would need to be there too.
- Eric
Eric Biggers <[email protected]> writes:
> On Fri, Dec 15, 2023 at 04:16:00PM -0500, Gabriel Krisman Bertazi wrote:
>> [Apologies for the quick spin of a v2. The only difference are a couple
>> fixes to the build when CONFIG_UNICODE=n caught by LKP and detailed in
>> each patch changelog.]
>>
>> 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 (8):
>> dcache: Add helper to disable d_revalidate for a specific dentry
>> fscrypt: Drop d_revalidate if key is available
>> libfs: Merge encrypted_ci_dentry_ops and ci_dentry_ops
>> libfs: Expose generic_ci_dentry_ops outside of libfs
>> ext4: Set the case-insensitive dentry operations through ->s_d_op
>> f2fs: Set the case-insensitive dentry operations through ->s_d_op
>> libfs: Don't support setting casefold operations during lookup
>> fscrypt: Move d_revalidate configuration back into fscrypt
>
> Thanks Gabriel, this series looks good. Sorry that we missed this when adding
> the support for encrypt+casefold.
>
> It's slightly awkward that some lines of code added by patches 5-6 are removed
> in patch 8. These changes look very hard to split up, though, so you've
> probably done about the best that can be done.
>
> One question/request: besides performance, the other reason we're so careful
> about minimizing when ->d_revalidate is set for fscrypt is so that overlayfs
> works on encrypted directories. This is because overlayfs is not compatible
> with ->d_revalidate. I think your solution still works for that, since
> DCACHE_OP_REVALIDATE will be cleared after the first call to
> fscrypt_d_revalidate(), and when checking for usupported dentries overlayfs does
> indeed check for DCACHE_OP_REVALIDATE instead of ->d_revalidate directly.
> However, that does rely on that very first call to ->d_revalidate actually
> happening before the check is done. It would be nice to verify that
> overlayfs+fscrypt indeed continues to work, and explicitly mention this
> somewhere (I don't see any mention of overlayfs+fscrypt in the series).
Hi Eric,
From my testing, overlayfs+fscrypt should work fine. I tried mounting
it on top of encrypted directories, with and without key, and will add
this information to the commit message. If we adopt the suggestion from
Al in the other subthread, even better, we won't need the first
d_revalidate to happen before the check, so I'll adopt that.
While looking into overlayfs, I found another reason we would need this
patchset. A side effect of not configuring ->d_op through s_d_op is
that the root dentry won't have d_op set. While this is fine for
casefold, because we forbid the root directory from being
case-insensitive, we can trick overlayfs into mounting a
filesystem it can't handle.
Even with this merged, and as Christian said in another thread regarding
ecryptfs, we should handle this explicitly. Something like below.
Amir, would you consider this for -rc8?
-- >8 --
Subject: [PATCH] ovl: Reject mounting case-insensitive filesystems
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.
Fixes: bb9cd9106b22 ("fscrypt: Have filesystems handle their d_ops")
Signed-off-by: Gabriel Krisman Bertazi <[email protected]>
---
fs/overlayfs/params.c | 2 ++
include/linux/fs.h | 9 +++++++++
2 files changed, 11 insertions(+)
diff --git a/fs/overlayfs/params.c b/fs/overlayfs/params.c
index 3fe2dde1598f..99495f079644 100644
--- a/fs/overlayfs/params.c
+++ b/fs/overlayfs/params.c
@@ -286,6 +286,8 @@ static int ovl_mount_dir_check(struct fs_context *fc, const struct path *path,
if (!d_is_dir(path->dentry))
return invalfc(fc, "%s is not a directory", name);
+ if (sb_has_encoding(path->mnt->mnt_sb))
+ return invalfc(fc, "caseless 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
On Sat, Dec 23, 2023 at 6:23 AM Gabriel Krisman Bertazi <[email protected]> wrote:
>
> Eric Biggers <[email protected]> writes:
>
> > On Fri, Dec 15, 2023 at 04:16:00PM -0500, Gabriel Krisman Bertazi wrote:
> >> [Apologies for the quick spin of a v2. The only difference are a couple
> >> fixes to the build when CONFIG_UNICODE=n caught by LKP and detailed in
> >> each patch changelog.]
> >>
> >> 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 (8):
> >> dcache: Add helper to disable d_revalidate for a specific dentry
> >> fscrypt: Drop d_revalidate if key is available
> >> libfs: Merge encrypted_ci_dentry_ops and ci_dentry_ops
> >> libfs: Expose generic_ci_dentry_ops outside of libfs
> >> ext4: Set the case-insensitive dentry operations through ->s_d_op
> >> f2fs: Set the case-insensitive dentry operations through ->s_d_op
> >> libfs: Don't support setting casefold operations during lookup
> >> fscrypt: Move d_revalidate configuration back into fscrypt
> >
> > Thanks Gabriel, this series looks good. Sorry that we missed this when adding
> > the support for encrypt+casefold.
> >
> > It's slightly awkward that some lines of code added by patches 5-6 are removed
> > in patch 8. These changes look very hard to split up, though, so you've
> > probably done about the best that can be done.
> >
> > One question/request: besides performance, the other reason we're so careful
> > about minimizing when ->d_revalidate is set for fscrypt is so that overlayfs
> > works on encrypted directories. This is because overlayfs is not compatible
> > with ->d_revalidate. I think your solution still works for that, since
> > DCACHE_OP_REVALIDATE will be cleared after the first call to
> > fscrypt_d_revalidate(), and when checking for usupported dentries overlayfs does
> > indeed check for DCACHE_OP_REVALIDATE instead of ->d_revalidate directly.
> > However, that does rely on that very first call to ->d_revalidate actually
> > happening before the check is done. It would be nice to verify that
> > overlayfs+fscrypt indeed continues to work, and explicitly mention this
> > somewhere (I don't see any mention of overlayfs+fscrypt in the series).
>
> Hi Eric,
>
> From my testing, overlayfs+fscrypt should work fine. I tried mounting
> it on top of encrypted directories, with and without key, and will add
> this information to the commit message. If we adopt the suggestion from
> Al in the other subthread, even better, we won't need the first
> d_revalidate to happen before the check, so I'll adopt that.
>
> While looking into overlayfs, I found another reason we would need this
> patchset. A side effect of not configuring ->d_op through s_d_op is
> that the root dentry won't have d_op set. While this is fine for
> casefold, because we forbid the root directory from being
> case-insensitive, we can trick overlayfs into mounting a
> filesystem it can't handle.
>
> Even with this merged, and as Christian said in another thread regarding
> ecryptfs, we should handle this explicitly. Something like below.
>
> Amir, would you consider this for -rc8?
IIUC, this fixes a regression from v5.10 with a very low likelihood of
impact on anyone in the real world, so what's the rush?
I would rather that you send this fix along with your patch set.
Feel free to add:
Acked-by: Amir Goldstein <[email protected]>
after fixing nits below
>
> -- >8 --
> Subject: [PATCH] ovl: Reject mounting case-insensitive filesystems
>
> 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.
>
> Fixes: bb9cd9106b22 ("fscrypt: Have filesystems handle their d_ops")
> Signed-off-by: Gabriel Krisman Bertazi <[email protected]>
> ---
> fs/overlayfs/params.c | 2 ++
> include/linux/fs.h | 9 +++++++++
> 2 files changed, 11 insertions(+)
>
> diff --git a/fs/overlayfs/params.c b/fs/overlayfs/params.c
> index 3fe2dde1598f..99495f079644 100644
> --- a/fs/overlayfs/params.c
> +++ b/fs/overlayfs/params.c
> @@ -286,6 +286,8 @@ static int ovl_mount_dir_check(struct fs_context *fc, const struct path *path,
> if (!d_is_dir(path->dentry))
> return invalfc(fc, "%s is not a directory", name);
>
Please add a comment to explain why this is needed to prevent post
mount lookup failures.
> + if (sb_has_encoding(path->mnt->mnt_sb))
> + return invalfc(fc, "caseless filesystem on %s not supported", name);
>
I have not seen you use the term "caseless" on the list since 2018. old habits?
Please use the term "case-insensitive" and please move the
ovl_dentry_weird() check
below this one, because when trying to mount overlayfs over non-root
case-insensitive
directory, the more specific error message is more useful.
Thanks,
Amir.
[CC: overlayfs]
On Sat, Dec 23, 2023 at 8:20 AM Amir Goldstein <[email protected]> wrote:
>
> On Sat, Dec 23, 2023 at 6:23 AM Gabriel Krisman Bertazi <[email protected]> wrote:
> >
> > Eric Biggers <[email protected]> writes:
> >
> > > On Fri, Dec 15, 2023 at 04:16:00PM -0500, Gabriel Krisman Bertazi wrote:
> > >> [Apologies for the quick spin of a v2. The only difference are a couple
> > >> fixes to the build when CONFIG_UNICODE=n caught by LKP and detailed in
> > >> each patch changelog.]
> > >>
> > >> 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 theandand 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 (8):
> > >> dcache: Add helper to disable d_revalidate for a specific dentry
> > >> fscrypt: Drop d_revalidate if key is available
> > >> libfs: Merge encrypted_ci_dentry_ops and ci_dentry_ops
> > >> libfs: Expose generic_ci_dentry_ops outside of libfs
> > >> ext4: Set the case-insensitive dentry operations through ->s_d_op
> > >> f2fs: Set the case-insensitive dentry operations through ->s_d_op
> > >> libfs: Don't support setting casefold operations during lookup
> > >> fscrypt: Move d_revalidate configuration back into fscrypt
> > >
> > > Thanks Gabriel, this series looks good. Sorry that we missed this when adding
> > > the support for encrypt+casefold.
> > >
> > > It's slightly awkward that some lines of code added by patches 5-6 are removed
> > > in patch 8. These changes look very hard to split up, though, so you've
> > > probably done about the best that can be done.
> > >
> > > One question/request: besides performance, the other reason we're so careful
> > > about minimizing when ->d_revalidate is set for fscrypt is so that overlayfs
> > > works on encrypted directories. This is because overlayfs is not compatible
> > > with ->d_revalidate. I think your solution still works for that, since
> > > DCACHE_OP_REVALIDATE will be cleared after the first call to
> > > fscrypt_d_revalidate(), and when checking for usupported dentries overlayfs does
> > > indeed check for DCACHE_OP_REVALIDATE instead of ->d_revalidate directly.
> > > However, that does rely on that very first call to ->d_revalidate actually
> > > happening before the check is done. It would be nice to verify that
> > > overlayfs+fscrypt indeed continues to work, and explicitly mention this
> > > somewhere (I don't see any mention of overlayfs+fscrypt in the series).
> >
> > Hi Eric,
> >
> > From my testing, overlayfs+fscrypt should work fine. I tried mounting
> > it on top of encrypted directories, with and without key, and will add
> > this information to the commit message. If we adopt the suggestion from
> > Al in the other subthread, even better, we won't need the first
> > d_revalidate to happen before the check, so I'll adopt that.
> >
> > While looking into overlayfs, I found another reason we would need this
> > patchset. A side effect of not configuring ->d_op through s_d_op is
> > that the root dentry won't have d_op set. While this is fine for
> > casefold, because we forbid the root directory from being
> > case-insensitive, we can trick overlayfs into mounting a
> > filesystem it can't handle.
> >
> > Even with this merged, and as Christian said in another thread regarding
> > ecryptfs, we should handle this explicitly. Something like below.
> >
> > Amir, would you consider this for -rc8?
>
> IIUC, this fixes a regression from v5.10 with a very low likelihood of
> impact on anyone in the real world, so what's the rush?
> I would rather that you send this fix along with your patch set.
>
> Feel free to add:
>
> Acked-by: Amir Goldstein <[email protected]>
>
> after fixing nits below
>
> >
> > -- >8 --
> > Subject: [PATCH] ovl: Reject mounting case-insensitive filesystems
> >
> > 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.
> >
> > Fixes: bb9cd9106b22 ("fscrypt: Have filesystems handle their d_ops")
> > Signed-off-by: Gabriel Krisman Bertazi <[email protected]>
> > ---
> > fs/overlayfs/params.c | 2 ++
> > include/linux/fs.h | 9 +++++++++
> > 2 files changed, 11 insertions(+)
> >
> > diff --git a/fs/overlayfs/params.c b/fs/overlayfs/params.c
> > index 3fe2dde1598f..99495f079644 100644
> > --- a/fs/overlayfs/params.c
> > +++ b/fs/overlayfs/params.c
> > @@ -286,6 +286,8 @@ static int ovl_mount_dir_check(struct fs_context *fc, const struct path *path,
> > if (!d_is_dir(path->dentry))
> > return invalfc(fc, "%s is not a directory", name);
> >
>
> Please add a comment to explain why this is needed to prevent post
> mount lookup failures.
>
> > + if (sb_has_encoding(path->mnt->mnt_sb))
> > + return invalfc(fc, "caseless filesystem on %s not supported", name);
> >
>
> I have not seen you use the term "caseless" on the list since 2018. old habits?
> Please use the term "case-insensitive" and please move the
> ovl_dentry_weird() check
> below this one, because when trying to mount overlayfs over non-root
> case-insensitive
> directory, the more specific error message is more useful.
>
> Thanks,
> Amir.
Amir Goldstein <[email protected]> writes:
>> Amir, would you consider this for -rc8?
>
> IIUC, this fixes a regression from v5.10 with a very low likelihood of
> impact on anyone in the real world, so what's the rush?
> I would rather that you send this fix along with your patch set.
>
> Feel free to add:
>
> Acked-by: Amir Goldstein <[email protected]>
>
> after fixing nits below
Thanks for your review.
It is fine to wait, and I'll turn it into part of this series, with
your ack, after fixing the details you pointed out.
Thanks,
--
Gabriel Krisman Bertazi