Hi,
The only difference of v3 from v2 is a fix from an issue reported by
kernel test robot in patch 4. Please consider this version instead.
The v2 has some big changes: instead of only configuring on the
case-insensitive case, we do it for case-sensitive fscrypt as well, and
disable d_revalidate as needed. This pretty much reverses the way
fscrypt operated (only enable d_revalidate for dentries that require
it), but has the advantage we can be consistent among variations of
case-insensitive/sensitive, encrypted/unencrypted configurations.
You'll find the code is simpler than v1 and v2. I dropped the dcache
patch because now we always try to disable DCACHE_OP_REVALIDATE while
holding the d_lock already, so I do it inline; I also changed the way we
drop d_revalidate when the key is made available, because we couldn't
really do it the way I originally proposed on the RCU case, which would
require falling back to non-RCU lookup just to disable d_revalidate; I
also included a patch fixing the overlayfs issue that I mentioned on the
previous thread. While unrelated to the rest of the patchset, it is a
quick fix that I might merge earlier if you are happy with it.
More details can be found in the per-patch changelog.
This survived fstests on ext4 and f2fs. I also verified that fscrypt
continues to work when combined to overlayfs as Eric requested.
..
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: Reject mounting case-insensitive filesystems
fscrypt: Share code between functions that prepare lookup
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
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/ceph/dir.c | 2 +-
fs/ceph/file.c | 2 +-
fs/crypto/hooks.c | 62 +++++++++++++++++++++--------------------
fs/ext4/namei.c | 1 -
fs/ext4/super.c | 1 +
fs/f2fs/namei.c | 1 -
fs/f2fs/super.c | 1 +
fs/libfs.c | 61 +++++++++++-----------------------------
fs/overlayfs/params.c | 13 +++++++--
fs/ubifs/dir.c | 1 -
fs/ubifs/super.c | 1 +
include/linux/fs.h | 11 +++++++-
include/linux/fscrypt.h | 51 ++++++++++++++++++++-------------
13 files changed, 106 insertions(+), 102 deletions(-)
--
2.43.0
Unencrypted and encrypted-dentries where the key is available don't need
to be revalidated with regards to fscrypt, since they don't go stale
from under VFS and the key cannot be removed for the encrypted case
without evicting the dentry. Mark them with d_set_always_valid, to
avoid unnecessary revalidation, in preparation to always configuring
d_op through sb->s_d_op.
Since the filesystem might have other features that require
revalidation, only apply this optimization if the d_revalidate handler
is fscrypt_d_revalidate itself.
Signed-off-by: Gabriel Krisman Bertazi <[email protected]>
---
fs/crypto/hooks.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/fs/crypto/hooks.c b/fs/crypto/hooks.c
index 41df986d1230..53381acc83e7 100644
--- a/fs/crypto/hooks.c
+++ b/fs/crypto/hooks.c
@@ -127,6 +127,15 @@ int fscrypt_prepare_lookup_dentry(struct inode *dir,
spin_lock(&dentry->d_lock);
if (nokey_name) {
dentry->d_flags |= DCACHE_NOKEY_NAME;
+ } else if (dentry->d_flags & DCACHE_OP_REVALIDATE &&
+ dentry->d_op->d_revalidate == fscrypt_d_revalidate) {
+ /*
+ * Unencrypted dentries and encrypted dentries where the
+ * key is available are always valid from fscrypt
+ * perspective. Avoid the cost of calling
+ * fscrypt_d_revalidate unnecessarily.
+ */
+ dentry->d_flags &= ~DCACHE_OP_REVALIDATE;
}
spin_unlock(&dentry->d_lock);
--
2.43.0
overlayfs relies on the filesystem setting DCACHE_OP_HASH or
DCACHE_OP_COMPARE to reject mounting over case-insensitive directories.
Since commit bb9cd9106b22 ("fscrypt: Have filesystems handle their
d_ops"), we set ->d_op through a hook in ->d_lookup, which
means the root dentry won't have them, causing the mount to accidentally
succeed.
In v6.7-rc7, the following sequence will succeed to mount, but any
dentry other than the root dentry will be a "weird" dentry to ovl and
fail with EREMOTE.
mkfs.ext4 -O casefold lower.img
mount -O loop lower.img lower
mount -t overlay -o lowerdir=lower,upperdir=upper,workdir=work ovl /mnt
Mounting on a subdirectory fails, as expected, because DCACHE_OP_HASH
and DCACHE_OP_COMPARE are properly set by ->lookup.
Fix by explicitly rejecting superblocks that allow case-insensitive
dentries.
While there, re-sort the entries to have more descriptive error messages
first.
Fixes: bb9cd9106b22 ("fscrypt: Have filesystems handle their d_ops")
Signed-off-by: Gabriel Krisman Bertazi <[email protected]>
Acked-by: Amir Goldstein <[email protected]>
---
changes since v2:
- Re-sort checks to trigger more descriptive error messages
first (Amir)
- Add code comment (Amir)
---
fs/overlayfs/params.c | 13 ++++++++++---
include/linux/fs.h | 9 +++++++++
2 files changed, 19 insertions(+), 3 deletions(-)
diff --git a/fs/overlayfs/params.c b/fs/overlayfs/params.c
index 3fe2dde1598f..09a4973f26f9 100644
--- a/fs/overlayfs/params.c
+++ b/fs/overlayfs/params.c
@@ -280,12 +280,19 @@ 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 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 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.
Signed-off-by: Gabriel Krisman Bertazi <[email protected]>
---
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 | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
index 3801c5c94fb6..0ba3928f3020 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)
{
@@ -230,6 +232,14 @@ static inline bool fscrypt_needs_contents_encryption(const struct inode *inode)
static inline void fscrypt_handle_d_move(struct dentry *dentry)
{
dentry->d_flags &= ~DCACHE_NOKEY_NAME;
+
+ /*
+ * Save the d_revalidate call cost during VFS operations. We
+ * can do it because, when the key is available, the dentry
+ * can't go stale and the key won't go away without eviction.
+ */
+ if (dentry->d_op && dentry->d_op->d_revalidate == fscrypt_d_revalidate)
+ dentry->d_flags &= ~DCACHE_OP_REVALIDATE;
}
/**
@@ -368,7 +378,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
Right now, we have two functions that can be called by the filesystem
during lookup to set up fscrypt internal state for the dentry and
inode under lookup:
1) fscrypt_prepare_lookup_dentry_partial: used only by ceph. It sets
encryption information in the inode and sets the dentry flag if the
key is not available.
2) fscrypt_prepare_lookup: used by everything else. Does all the
above, plus also sets struct fname.
This patch refactors the code to implement the later using the former.
This way, we'll have a single place where we set DCACHE_NOKEY_NAME, in
preparation to add more churn to that condition in the following patch.
To make the patch simpler, we now call fscrypt_get_encryption_info twice
for fscrypt_prepare_lookup, once inside fscrypt_setup_filename and once
inside fscrypt_prepare_lookup_dentry. It seems safe to do, and
considering it will bail early in the second lookup and most lookups
should go to the dcache anyway, it doesn't seem problematic for
performance. In addition, we add a function call for the unencrypted
case, also during lookup.
Apart from the above, it should have no behavior change.
I tested ext4/f2fs manually and with fstests, which yielded no regressions.
Signed-off-by: Gabriel Krisman Bertazi <[email protected]>
---
fs/ceph/dir.c | 2 +-
fs/ceph/file.c | 2 +-
fs/crypto/hooks.c | 53 ++++++++++++++++++-----------------------
include/linux/fscrypt.h | 40 +++++++++++++++++--------------
4 files changed, 47 insertions(+), 50 deletions(-)
diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
index 91709934c8b1..835421e2845d 100644
--- a/fs/ceph/dir.c
+++ b/fs/ceph/dir.c
@@ -813,7 +813,7 @@ static struct dentry *ceph_lookup(struct inode *dir, struct dentry *dentry,
if (IS_ENCRYPTED(dir)) {
bool had_key = fscrypt_has_encryption_key(dir);
- err = fscrypt_prepare_lookup_partial(dir, dentry);
+ err = fscrypt_prepare_lookup_dentry(dir, dentry);
if (err < 0)
return ERR_PTR(err);
diff --git a/fs/ceph/file.c b/fs/ceph/file.c
index 3b5aae29e944..5c9206670533 100644
--- a/fs/ceph/file.c
+++ b/fs/ceph/file.c
@@ -812,7 +812,7 @@ int ceph_atomic_open(struct inode *dir, struct dentry *dentry,
ihold(dir);
if (IS_ENCRYPTED(dir)) {
set_bit(CEPH_MDS_R_FSCRYPT_FILE, &req->r_req_flags);
- err = fscrypt_prepare_lookup_partial(dir, dentry);
+ err = fscrypt_prepare_lookup_dentry(dir, dentry);
if (err < 0)
goto out_req;
}
diff --git a/fs/crypto/hooks.c b/fs/crypto/hooks.c
index 52504dd478d3..41df986d1230 100644
--- a/fs/crypto/hooks.c
+++ b/fs/crypto/hooks.c
@@ -94,52 +94,45 @@ int __fscrypt_prepare_rename(struct inode *old_dir, struct dentry *old_dentry,
}
EXPORT_SYMBOL_GPL(__fscrypt_prepare_rename);
-int __fscrypt_prepare_lookup(struct inode *dir, struct dentry *dentry,
- struct fscrypt_name *fname)
-{
- int err = fscrypt_setup_filename(dir, &dentry->d_name, 1, fname);
-
- if (err && err != -ENOENT)
- return err;
-
- if (fname->is_nokey_name) {
- spin_lock(&dentry->d_lock);
- dentry->d_flags |= DCACHE_NOKEY_NAME;
- spin_unlock(&dentry->d_lock);
- }
- return err;
-}
-EXPORT_SYMBOL_GPL(__fscrypt_prepare_lookup);
-
/**
- * fscrypt_prepare_lookup_partial() - prepare lookup without filename setup
+ * fscrypt_prepare_lookup_dentry() - prepare lookup without filename setup
* @dir: the encrypted directory being searched
* @dentry: the dentry being looked up in @dir
*
- * This function should be used by the ->lookup and ->atomic_open methods of
- * filesystems that handle filename encryption and no-key name encoding
- * themselves and thus can't use fscrypt_prepare_lookup(). Like
- * fscrypt_prepare_lookup(), this will try to set up the directory's encryption
- * key and will set DCACHE_NOKEY_NAME on the dentry if the key is unavailable.
- * However, this function doesn't set up a struct fscrypt_name for the filename.
+ * This function should be used by the ->lookup and ->atomic_open
+ * methods of filesystems that handle filename encryption and no-key
+ * name encoding themselves and thus can't use fscrypt_prepare_lookup()
+ * directly. This will try to set up the directory's encryption key and
+ * will set DCACHE_NOKEY_NAME on the dentry if the key is unavailable.
+ * However, this function doesn't set up a struct fscrypt_name for the
+ * filename.
*
* Return: 0 on success; -errno on error. Note that the encryption key being
* unavailable is not considered an error. It is also not an error if
* the encryption policy is unsupported by this kernel; that is treated
* like the key being unavailable, so that files can still be deleted.
*/
-int fscrypt_prepare_lookup_partial(struct inode *dir, struct dentry *dentry)
+int fscrypt_prepare_lookup_dentry(struct inode *dir,
+ struct dentry *dentry)
{
- int err = fscrypt_get_encryption_info(dir, true);
+ bool nokey_name = false;
+ int err = 0;
- if (!err && !fscrypt_has_encryption_key(dir)) {
- spin_lock(&dentry->d_lock);
+ if (IS_ENCRYPTED(dir)) {
+ err = fscrypt_get_encryption_info(dir, true);
+ if (!err && !fscrypt_has_encryption_key(dir))
+ nokey_name = true;
+ }
+
+ spin_lock(&dentry->d_lock);
+ if (nokey_name) {
dentry->d_flags |= DCACHE_NOKEY_NAME;
- spin_unlock(&dentry->d_lock);
}
+ spin_unlock(&dentry->d_lock);
+
return err;
}
-EXPORT_SYMBOL_GPL(fscrypt_prepare_lookup_partial);
+EXPORT_SYMBOL_GPL(fscrypt_prepare_lookup_dentry);
int __fscrypt_prepare_readdir(struct inode *dir)
{
diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
index 12f9e455d569..3801c5c94fb6 100644
--- a/include/linux/fscrypt.h
+++ b/include/linux/fscrypt.h
@@ -382,9 +382,7 @@ int __fscrypt_prepare_link(struct inode *inode, struct inode *dir,
int __fscrypt_prepare_rename(struct inode *old_dir, struct dentry *old_dentry,
struct inode *new_dir, struct dentry *new_dentry,
unsigned int flags);
-int __fscrypt_prepare_lookup(struct inode *dir, struct dentry *dentry,
- struct fscrypt_name *fname);
-int fscrypt_prepare_lookup_partial(struct inode *dir, struct dentry *dentry);
+int fscrypt_prepare_lookup_dentry(struct inode *dir, struct dentry *dentry);
int __fscrypt_prepare_readdir(struct inode *dir);
int __fscrypt_prepare_setattr(struct dentry *dentry, struct iattr *attr);
int fscrypt_prepare_setflags(struct inode *inode,
@@ -704,14 +702,7 @@ static inline int __fscrypt_prepare_rename(struct inode *old_dir,
return -EOPNOTSUPP;
}
-static inline int __fscrypt_prepare_lookup(struct inode *dir,
- struct dentry *dentry,
- struct fscrypt_name *fname)
-{
- return -EOPNOTSUPP;
-}
-
-static inline int fscrypt_prepare_lookup_partial(struct inode *dir,
+static inline int fscrypt_prepare_lookup_dentry(struct inode *dir,
struct dentry *dentry)
{
return -EOPNOTSUPP;
@@ -975,14 +966,27 @@ static inline int fscrypt_prepare_lookup(struct inode *dir,
struct dentry *dentry,
struct fscrypt_name *fname)
{
- if (IS_ENCRYPTED(dir))
- return __fscrypt_prepare_lookup(dir, dentry, fname);
+ int ret, err = 0;
+
+ if (IS_ENCRYPTED(dir)) {
+ err = fscrypt_setup_filename(dir, &dentry->d_name, 1, fname);
+ if (err && err != -ENOENT)
+ return err;
+ } else {
+ memset(fname, 0, sizeof(*fname));
+ fname->usr_fname = &dentry->d_name;
+ fname->disk_name.name = (unsigned char *)dentry->d_name.name;
+ fname->disk_name.len = dentry->d_name.len;
+ }
- memset(fname, 0, sizeof(*fname));
- fname->usr_fname = &dentry->d_name;
- fname->disk_name.name = (unsigned char *)dentry->d_name.name;
- fname->disk_name.len = dentry->d_name.len;
- return 0;
+ /*
+ * fscrypt_prepare_lookup_dentry will succeed even if the
+ * directory key is unavailable but the filename isn't a valid
+ * no-key name (-ENOENT). Thus, propagate the previous
+ * error, if any.
+ */
+ ret = fscrypt_prepare_lookup_dentry(dir, dentry);
+ return err ? err : ret;
}
/**
--
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]>
---
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
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]>
---
fs/libfs.c | 29 +++++++++++++++++++++++++++++
include/linux/fs.h | 1 +
2 files changed, 30 insertions(+)
diff --git a/fs/libfs.c b/fs/libfs.c
index c4be0961faf0..9cd4df6969d2 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -1822,6 +1822,35 @@ 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_root 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
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/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
No filesystems depend on it anymore, and it is generally a bad idea.
Since all dentries should have the same set of dentry operations in
case-insensitive filesystems, it should be propagated through ->s_d_op.
Signed-off-by: Gabriel Krisman Bertazi <[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 9cd4df6969d2..c5c92ac76ba7 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
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
On Fri, Jan 19, 2024 at 03:47:33PM -0300, Gabriel Krisman Bertazi wrote:
> ovl: Reject mounting case-insensitive filesystems
Overlayfs doesn't mount filesystems. I think you might mean something like
reject case-insensitive lowerdirs?
> + /*
> + * Root dentries of case-insensitive 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 filesystem on %s not supported", name);
sb_has_encoding() doesn't mean that the filesystem is case-insensitive. It
means that the filesystem supports individual case-insensitive directories.
With that in mind, is this code still working as intended?
If so, can you update the comment and error message accordingly?
- Eric
On Fri, Jan 19, 2024 at 03:47:34PM -0300, Gabriel Krisman Bertazi wrote:
> To make the patch simpler, we now call fscrypt_get_encryption_info twice
> for fscrypt_prepare_lookup, once inside fscrypt_setup_filename and once
> inside fscrypt_prepare_lookup_dentry. It seems safe to do, and
> considering it will bail early in the second lookup and most lookups
> should go to the dcache anyway, it doesn't seem problematic for
> performance. In addition, we add a function call for the unencrypted
> case, also during lookup.
Unfortunately I don't think it's correct. This is basically undoing my fix
b01531db6cec ("fscrypt: fix race where ->lookup() marks plaintext dentry as
ciphertext") from several years ago.
When a lookup is done, the filesystem needs to either treat the name being
looked up as a no-key name *or* as a regular name, depending on whether the
directory's key is present. We shouldn't enable race conditions where, due to
the key being concurrently added, the name is treated as a no-key name for
filename matching purposes but a regular name for dentry validation purposes.
That can result in an anomaly where a file that exists ends up with a negative
dentry that doesn't get invalidated.
Basically, the boolean fscrypt_name::is_nokey_name that's produced by
fscrypt_setup_filename() should continue to be propagated to DCACHE_NOKEY_NAME.
- Eric
On Fri, Jan 19, 2024 at 03:47:36PM -0300, Gabriel Krisman Bertazi wrote:
> /*
> * 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().
> */
> static inline void fscrypt_handle_d_move(struct dentry *dentry)
> {
> dentry->d_flags &= ~DCACHE_NOKEY_NAME;
> +
> + /*
> + * Save the d_revalidate call cost during VFS operations. We
> + * can do it because, when the key is available, the dentry
> + * can't go stale and the key won't go away without eviction.
> + */
> + if (dentry->d_op && dentry->d_op->d_revalidate == fscrypt_d_revalidate)
> + dentry->d_flags &= ~DCACHE_OP_REVALIDATE;
> }
Is there any way to optimize this further for the case where fscrypt is not
being used? This is called unconditionally from d_move(). I've generally been
trying to avoid things like this where the fscrypt support slows things down for
everyone even when they're not using the feature.
In any case, as always please don't let function comments get outdated. Since
it now seems to just describe the DCACHE_NOKEY_NAME clearing and not the whole
function, maybe it should be moved to above that line.
- Eric
On Fri, Jan 19, 2024 at 03:47:38PM -0300, Gabriel Krisman Bertazi wrote:
> +/**
> + * 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_root to best set of dentry
s_d_op, not s_d_root.
- Eric
Eric Biggers <[email protected]> writes:
> On Fri, Jan 19, 2024 at 03:47:33PM -0300, Gabriel Krisman Bertazi wrote:
>> ovl: Reject mounting case-insensitive filesystems
>
> Overlayfs doesn't mount filesystems. I think you might mean something like
> reject case-insensitive lowerdirs?
uppers and workdir too. I'd make this:
"ovl: Reject mounting over case-insensitive filesystems"
>
>> + /*
>> + * Root dentries of case-insensitive 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 filesystem on %s not supported", name);
>
> sb_has_encoding() doesn't mean that the filesystem is case-insensitive. It
> means that the filesystem supports individual case-insensitive
> directories.
>
> With that in mind, is this code still working as intended?
>
Yes, it is. In particular, after the rest of the patchset, any dentry
will be weird and lookups will throw -EREMOTE.
> If so, can you update the comment and error message accordingly?
I'm not sure how to change and still make it readable by users. How about:
return invalfc(fc, "case-insensitive capable filesystem on %s not supported", name);
what do you think?
--
Gabriel Krisman Bertazi
Eric Biggers <[email protected]> writes:
> On Fri, Jan 19, 2024 at 03:47:34PM -0300, Gabriel Krisman Bertazi wrote:
>> To make the patch simpler, we now call fscrypt_get_encryption_info twice
>> for fscrypt_prepare_lookup, once inside fscrypt_setup_filename and once
>> inside fscrypt_prepare_lookup_dentry. It seems safe to do, and
>> considering it will bail early in the second lookup and most lookups
>> should go to the dcache anyway, it doesn't seem problematic for
>> performance. In addition, we add a function call for the unencrypted
>> case, also during lookup.
>
> Unfortunately I don't think it's correct. This is basically undoing my fix
> b01531db6cec ("fscrypt: fix race where ->lookup() marks plaintext dentry as
> ciphertext") from several years ago.
>
> When a lookup is done, the filesystem needs to either treat the name being
> looked up as a no-key name *or* as a regular name, depending on whether the
> directory's key is present. We shouldn't enable race conditions where, due to
> the key being concurrently added, the name is treated as a no-key name for
> filename matching purposes but a regular name for dentry validation purposes.
> That can result in an anomaly where a file that exists ends up with a negative
> dentry that doesn't get invalidated.
>
> Basically, the boolean fscrypt_name::is_nokey_name that's produced by
> fscrypt_setup_filename() should continue to be propagated to DCACHE_NOKEY_NAME.
I see your point. I'll drop this patch and replace it with a patch that
just merges the DCACHE_NOKEY_NAME configuration. Sadly, we gotta keep
the two variants I think.
thanks for the review
--
Gabriel Krisman Bertazi
Eric Biggers <[email protected]> writes:
> On Fri, Jan 19, 2024 at 03:47:36PM -0300, Gabriel Krisman Bertazi wrote:
>> /*
>> * 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().
>> */
>> static inline void fscrypt_handle_d_move(struct dentry *dentry)
>> {
>> dentry->d_flags &= ~DCACHE_NOKEY_NAME;
>> +
>> + /*
>> + * Save the d_revalidate call cost during VFS operations. We
>> + * can do it because, when the key is available, the dentry
>> + * can't go stale and the key won't go away without eviction.
>> + */
>> + if (dentry->d_op && dentry->d_op->d_revalidate == fscrypt_d_revalidate)
>> + dentry->d_flags &= ~DCACHE_OP_REVALIDATE;
>> }
>
> Is there any way to optimize this further for the case where fscrypt is not
> being used? This is called unconditionally from d_move(). I've generally been
> trying to avoid things like this where the fscrypt support slows things down for
> everyone even when they're not using the feature.
The problem would be figuring out whether the filesystem has fscrypt
enabled. I think we can rely on sb->s_cop always being set:
if (sb->s_cop)
fscrypt_handle_d_move(dentry);
What do you think?
--
Gabriel Krisman Bertazi
On Thu, Jan 25, 2024 at 01:55:00PM -0300, Gabriel Krisman Bertazi wrote:
> I'm not sure how to change and still make it readable by users. How about:
>
> return invalfc(fc, "case-insensitive capable filesystem on %s not supported", name);
>
> what do you think?
"case-insensitive capable" sounds good.
- Eric
On Thu, Jan 25, 2024 at 05:20:56PM -0300, Gabriel Krisman Bertazi wrote:
> Eric Biggers <[email protected]> writes:
>
> > On Fri, Jan 19, 2024 at 03:47:36PM -0300, Gabriel Krisman Bertazi wrote:
> >> /*
> >> * 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().
> >> */
> >> static inline void fscrypt_handle_d_move(struct dentry *dentry)
> >> {
> >> dentry->d_flags &= ~DCACHE_NOKEY_NAME;
> >> +
> >> + /*
> >> + * Save the d_revalidate call cost during VFS operations. We
> >> + * can do it because, when the key is available, the dentry
> >> + * can't go stale and the key won't go away without eviction.
> >> + */
> >> + if (dentry->d_op && dentry->d_op->d_revalidate == fscrypt_d_revalidate)
> >> + dentry->d_flags &= ~DCACHE_OP_REVALIDATE;
> >> }
> >
> > Is there any way to optimize this further for the case where fscrypt is not
> > being used? This is called unconditionally from d_move(). I've generally been
> > trying to avoid things like this where the fscrypt support slows things down for
> > everyone even when they're not using the feature.
>
> The problem would be figuring out whether the filesystem has fscrypt
> enabled. I think we can rely on sb->s_cop always being set:
>
> if (sb->s_cop)
> fscrypt_handle_d_move(dentry);
>
> What do you think?
That's better, I just wonder if there's an even better way. Do you need to do
this for dentries that don't have DCACHE_NOKEY_NAME set? If not, it would be
more efficient to test for DCACHE_NOKEY_NAME before clearing the flags.
- Eric
Eric Biggers <[email protected]> writes:
> On Thu, Jan 25, 2024 at 05:20:56PM -0300, Gabriel Krisman Bertazi wrote:
>> Eric Biggers <[email protected]> writes:
>>
>> > On Fri, Jan 19, 2024 at 03:47:36PM -0300, Gabriel Krisman Bertazi wrote:
>> >> /*
>> >> * 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().
>> >> */
>> >> static inline void fscrypt_handle_d_move(struct dentry *dentry)
>> >> {
>> >> dentry->d_flags &= ~DCACHE_NOKEY_NAME;
>> >> +
>> >> + /*
>> >> + * Save the d_revalidate call cost during VFS operations. We
>> >> + * can do it because, when the key is available, the dentry
>> >> + * can't go stale and the key won't go away without eviction.
>> >> + */
>> >> + if (dentry->d_op && dentry->d_op->d_revalidate == fscrypt_d_revalidate)
>> >> + dentry->d_flags &= ~DCACHE_OP_REVALIDATE;
>> >> }
>> >
>> > Is there any way to optimize this further for the case where fscrypt is not
>> > being used? This is called unconditionally from d_move(). I've generally been
>> > trying to avoid things like this where the fscrypt support slows things down for
>> > everyone even when they're not using the feature.
>>
>> The problem would be figuring out whether the filesystem has fscrypt
>> enabled. I think we can rely on sb->s_cop always being set:
>>
>> if (sb->s_cop)
>> fscrypt_handle_d_move(dentry);
>>
>> What do you think?
>
> That's better, I just wonder if there's an even better way. Do you need to do
> this for dentries that don't have DCACHE_NOKEY_NAME set? If not, it would be
> more efficient to test for DCACHE_NOKEY_NAME before clearing the flags.
I like that. We don't need it for dentries without DCACHE_NOKEY_NAME,
because those dentries have the d_revalidate disabled at lookup-time.
I raced my v4 with your comment, but I'll spin a v5 folding in this
suggestion shortly.
--
Gabriel Krisman Bertazi