On Fri, Aug 11, 2023 at 08:41:37PM -0400, Gabriel Krisman Bertazi wrote:
> In preparation to use it in ecryptfs, move needs_casefolding into a
> public header and give it a namespaced name.
>
> Signed-off-by: Gabriel Krisman Bertazi <krisman@xxxxxxx>
> ---
> fs/libfs.c | 14 ++------------
> include/linux/fs.h | 21 +++++++++++++++++++++
> 2 files changed, 23 insertions(+), 12 deletions(-)
>
> diff --git a/fs/libfs.c b/fs/libfs.c
> index 5b851315eeed..8d0b64cfd5da 100644
> --- a/fs/libfs.c
> +++ b/fs/libfs.c
> @@ -1381,16 +1381,6 @@ bool is_empty_dir_inode(struct inode *inode)
> }
>
> #if IS_ENABLED(CONFIG_UNICODE)
> -/*
> - * Determine if the name of a dentry should be casefolded.
> - *
> - * Return: if names will need casefolding
> - */
> -static bool needs_casefold(const struct inode *dir)
> -{
> - return IS_CASEFOLDED(dir) && dir->i_sb->s_encoding;
> -}
> -
> /**
> * generic_ci_d_compare - generic d_compare implementation for casefolding filesystems
> * @dentry: dentry whose name we are checking against
> @@ -1411,7 +1401,7 @@ static int generic_ci_d_compare(const struct dentry *dentry, unsigned int len,
> char strbuf[DNAME_INLINE_LEN];
> int ret;
>
> - if (!dir || !needs_casefold(dir))
> + if (!dir || !dir_is_casefolded(dir))
> goto fallback;
> /*
> * If the dentry name is stored in-line, then it may be concurrently
> @@ -1453,7 +1443,7 @@ static int generic_ci_d_hash(const struct dentry *dentry, struct qstr *str)
> const struct unicode_map *um = sb->s_encoding;
> int ret = 0;
>
> - if (!dir || !needs_casefold(dir))
> + if (!dir || !dir_is_casefolded(dir))
> return 0;
>
> ret = utf8_casefold_hash(um, dentry, str);
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 6867512907d6..e3b631c6d24a 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -3213,6 +3213,27 @@ static inline bool dir_relax_shared(struct inode *inode)
> return !IS_DEADDIR(inode);
> }
>
> +/**
> + * dir_is_casefolded - Safely determine if filenames inside of a
> + * directory should be casefolded.
> + * @dir: The directory inode to be checked
> + *
> + * Filesystems should usually rely on this instead of checking the
> + * S_CASEFOLD flag directly when handling inodes, to avoid surprises
> + * with corrupted volumes. Checking i_sb->s_encoding ensures the
> + * filesystem knows how to deal with case-insensitiveness.
> + *
> + * Return: if names will need casefolding
> + */
> +static inline bool dir_is_casefolded(const struct inode *dir)
> +{
> +#if IS_ENABLED(CONFIG_UNICODE)
> + return IS_CASEFOLDED(dir) && dir->i_sb->s_encoding;
> +#else
> + return false;
> +#endif
> +}
To be honest I've always been confused about why the ->s_encoding check is
there. It looks like Ted added it in 6456ca6520ab ("ext4: fix kernel oops
caused by spurious casefold flag") to address a fuzzing report for a filesystem
that had a casefolded directory but didn't have the casefold feature flag set.
It seems like an unnecessarily complex fix, though. The filesystem should just
reject the inode earlier, in __ext4_iget(). And likewise for f2fs. Then no
other code has to worry about this problem.
Actually, f2fs already does it, as I added it in commit f6322f3f1212:
if ((fi->i_flags & F2FS_CASEFOLD_FL) && !f2fs_sb_has_casefold(sbi)) {
set_sbi_flag(sbi, SBI_NEED_FSCK);
f2fs_warn(sbi, "%s: inode (ino=%lx) has casefold flag, but casefold feature is off",
__func__, inode->i_ino);
return false;
}
So just __ext4_iget() needs to be fixed. I think we should consider doing that
before further entrenching all the extra ->s_encoding checks.
Also I don't understand why this needs to be part of your patch series anyway.
Shouldn't eCryptfs check IS_CASEFOLDED() anyway?
- Eric
