| Message ID | 87a5q1eecy.fsf_-_@mailhost.krisman.be |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | ovl: Reject mounting case-insensitive filesystems | expand |
On Sat, Dec 23, 2023 at 6:23 AM Gabriel Krisman Bertazi <krisman@suse.de> wrote: > > Eric Biggers <ebiggers@kernel.org> 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 <amir73il@gmail.com> 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 <krisman@suse.de> > --- > 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 <amir73il@gmail.com> wrote: > > On Sat, Dec 23, 2023 at 6:23 AM Gabriel Krisman Bertazi <krisman@suse.de> wrote: > > > > Eric Biggers <ebiggers@kernel.org> 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 <amir73il@gmail.com> > > 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 <krisman@suse.de> > > --- > > 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 <amir73il@gmail.com> 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 <amir73il@gmail.com> > > 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,
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 *);