On Sat, Dec 23, 2023 at 6:23 AM Gabriel Krisman Bertazi <krisman@xxxxxxx> wrote: > > Eric Biggers <ebiggers@xxxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxx> > --- > 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.
