| Message ID | 20240129204330.32346-2-krisman@suse.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Set casefold/fscrypt dentry operations through sb->s_d_op | expand |
On Mon, Jan 29, 2024 at 05:43:19PM -0300, Gabriel Krisman Bertazi wrote: > ovl: Reject mounting over case-insensitive directories Maybe: ovl: Reject mounting over rootdir of case-insensitive capable FS or: ovl: Always reject mounting over case-insensitive directories ... since as your commit message explains, overlayfs already does reject mounting over case-insensitive directories, just not in all cases. > 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. But this series changes that. Doesn't that make this overlayfs fix redundant? It does improve the error message, which is helpful, but your commit message makes it sound like it's an actual fix, not just an error message improvement. - Eric
Eric Biggers <ebiggers@kernel.org> writes: > On Mon, Jan 29, 2024 at 05:43:19PM -0300, Gabriel Krisman Bertazi wrote: >> ovl: Reject mounting over case-insensitive directories > > Maybe: > > ovl: Reject mounting over rootdir of case-insensitive capable FS > > or: > > ovl: Always reject mounting over case-insensitive directories > > ... since as your commit message explains, overlayfs already does reject > mounting over case-insensitive directories, just not in all cases. > >> 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. > > But this series changes that. Doesn't that make this overlayfs fix redundant? > It does improve the error message, which is helpful, but your commit message > makes it sound like it's an actual fix, not just an error message improvement. Yes, this series will make it redundant. But Christian Brauner had suggested we make this verification explicit instead of relying on d_ops being set, to avoid future changes breaking this again. I found the issue in ovl during testing of v2 and intended to send it separately for -rc7, but Amir asked it to be sent as part of this series. And also the error code is improved. Anyway, I'll can make this clear in the commit message
diff --git a/fs/overlayfs/params.c b/fs/overlayfs/params.c index 3fe2dde1598f..488f920f79d2 100644 --- a/fs/overlayfs/params.c +++ b/fs/overlayfs/params.c @@ -280,12 +280,20 @@ static int ovl_mount_dir_check(struct fs_context *fc, const struct path *path, { struct ovl_fs_context *ctx = fc->fs_private; - if (ovl_dentry_weird(path->dentry)) - return invalfc(fc, "filesystem on %s not supported", name); - if (!d_is_dir(path->dentry)) return invalfc(fc, "%s is not a directory", name); + /* + * Root dentries of case-insensitive capable filesystems might + * not have the dentry operations set, but still be incompatible + * with overlayfs. Check explicitly to prevent post-mount + * failures. + */ + if (sb_has_encoding(path->mnt->mnt_sb)) + return invalfc(fc, "case-insensitive capable filesystem on %s not supported", name); + + if (ovl_dentry_weird(path->dentry)) + return invalfc(fc, "filesystem on %s not supported", name); /* * Check whether upper path is read-only here to report failures diff --git a/include/linux/fs.h b/include/linux/fs.h index 98b7a7a8c42e..e6667ece5e64 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -3203,6 +3203,15 @@ extern int generic_check_addressable(unsigned, u64); extern void generic_set_encrypted_ci_d_ops(struct dentry *dentry); +static inline bool sb_has_encoding(const struct super_block *sb) +{ +#if IS_ENABLED(CONFIG_UNICODE) + return !!sb->s_encoding; +#else + return false; +#endif +} + int may_setattr(struct mnt_idmap *idmap, struct inode *inode, unsigned int ia_valid); int setattr_prepare(struct mnt_idmap *, struct dentry *, struct iattr *);
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 <krisman@suse.de> Acked-by: Amir Goldstein <amir73il@gmail.com> --- changes since v3: - Case insensitive filesystem ->Case insensitive capable filesystem (eric) - clarify patch summary line changes since v2: - Re-sort checks to trigger more descriptive error messages first (Amir) - Add code comment (Amir) --- fs/overlayfs/params.c | 14 +++++++++++--- include/linux/fs.h | 9 +++++++++ 2 files changed, 20 insertions(+), 3 deletions(-)