[CC: overlayfs] On Sat, Dec 23, 2023 at 8:20 AM Amir Goldstein <amir73il@xxxxxxxxx> wrote: > > 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 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@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.
