| Message ID | 20231215211608.6449-9-krisman@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Revert setting casefolding dentry operations through s_d_op | expand |
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: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
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;
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 <krisman@suse.de> --- 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(-)