| Message ID | 20201119060904.463807-3-drosen@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Add support for Encryption and Casefolding in F2FS | expand |
Daniel Rosenberg <drosen@google.com> writes: > This shifts the responsibility of setting up dentry operations from > fscrypt to the individual filesystems, allowing them to have their own > operations while still setting fscrypt's d_revalidate as appropriate. > > Most filesystems can just use generic_set_encrypted_ci_d_ops, unless > they have their own specific dentry operations as well. That operation > will set the minimal d_ops required under the circumstances. > > Since the fscrypt d_ops are set later on, we must set all d_ops there, > since we cannot adjust those later on. This should not result in any > change in behavior. > > Signed-off-by: Daniel Rosenberg <drosen@google.com> > Acked-by: Eric Biggers <ebiggers@google.com> > --- > fs/crypto/fname.c | 4 ---- > fs/crypto/fscrypt_private.h | 1 - > fs/crypto/hooks.c | 1 - > fs/ext4/dir.c | 7 ------- > fs/ext4/ext4.h | 4 ---- > fs/ext4/namei.c | 1 + > fs/ext4/super.c | 5 ----- > fs/f2fs/dir.c | 7 ------- > fs/f2fs/f2fs.h | 3 --- > fs/f2fs/namei.c | 1 + > fs/f2fs/super.c | 1 - > fs/ubifs/dir.c | 1 + > include/linux/fscrypt.h | 7 +++++-- > 13 files changed, 8 insertions(+), 35 deletions(-) > > diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c > index 1fbe6c24d705..cb3cfa6329ba 100644 > --- a/fs/crypto/fname.c > +++ b/fs/crypto/fname.c > @@ -570,7 +570,3 @@ int fscrypt_d_revalidate(struct dentry *dentry, unsigned int flags) > return valid; > } > EXPORT_SYMBOL_GPL(fscrypt_d_revalidate); > - > -const struct dentry_operations fscrypt_d_ops = { > - .d_revalidate = fscrypt_d_revalidate, > -}; > diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h > index 4f5806a3b73d..df9c48c1fbf7 100644 > --- a/fs/crypto/fscrypt_private.h > +++ b/fs/crypto/fscrypt_private.h > @@ -294,7 +294,6 @@ int fscrypt_fname_encrypt(const struct inode *inode, const struct qstr *iname, > bool fscrypt_fname_encrypted_size(const union fscrypt_policy *policy, > u32 orig_len, u32 max_len, > u32 *encrypted_len_ret); > -extern const struct dentry_operations fscrypt_d_ops; > > /* hkdf.c */ > > diff --git a/fs/crypto/hooks.c b/fs/crypto/hooks.c > index 20b0df47fe6a..9006fa983335 100644 > --- a/fs/crypto/hooks.c > +++ b/fs/crypto/hooks.c > @@ -117,7 +117,6 @@ int __fscrypt_prepare_lookup(struct inode *dir, struct dentry *dentry, > spin_lock(&dentry->d_lock); > dentry->d_flags |= DCACHE_NOKEY_NAME; > spin_unlock(&dentry->d_lock); > - d_set_d_op(dentry, &fscrypt_d_ops); > } > return err; > } > diff --git a/fs/ext4/dir.c b/fs/ext4/dir.c > index ca50c90adc4c..e757319a4472 100644 > --- a/fs/ext4/dir.c > +++ b/fs/ext4/dir.c > @@ -667,10 +667,3 @@ const struct file_operations ext4_dir_operations = { > .open = ext4_dir_open, > .release = ext4_release_dir, > }; > - > -#ifdef CONFIG_UNICODE > -const struct dentry_operations ext4_dentry_ops = { > - .d_hash = generic_ci_d_hash, > - .d_compare = generic_ci_d_compare, > -}; > -#endif > diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h > index bf9429484462..ad77f01d9e20 100644 > --- a/fs/ext4/ext4.h > +++ b/fs/ext4/ext4.h > @@ -3380,10 +3380,6 @@ static inline void ext4_unlock_group(struct super_block *sb, > /* dir.c */ > extern const struct file_operations ext4_dir_operations; > > -#ifdef CONFIG_UNICODE > -extern const struct dentry_operations ext4_dentry_ops; > -#endif > - > /* file.c */ > extern const struct inode_operations ext4_file_inode_operations; > extern const struct file_operations ext4_file_operations; > diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c > index 33509266f5a0..12a417ff5648 100644 > --- a/fs/ext4/namei.c > +++ b/fs/ext4/namei.c > @@ -1614,6 +1614,7 @@ static struct buffer_head *ext4_lookup_entry(struct inode *dir, > struct buffer_head *bh; > > err = ext4_fname_prepare_lookup(dir, dentry, &fname); > + generic_set_encrypted_ci_d_ops(dentry); > if (err == -ENOENT) > return NULL; > if (err) > diff --git a/fs/ext4/super.c b/fs/ext4/super.c > index 6633b20224d5..0288bedf46e1 100644 > --- a/fs/ext4/super.c > +++ b/fs/ext4/super.c > @@ -4968,11 +4968,6 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent) > goto failed_mount4; > } > > -#ifdef CONFIG_UNICODE > - if (sb->s_encoding) > - sb->s_d_op = &ext4_dentry_ops; > -#endif This change has the side-effect of removing the capability of the root directory from being case-insensitive. It is not a backward incompatible change because there is no way to make the root directory CI at the moment (it is never empty). But this restriction seems artificial. Is there a real reason to prevent the root inode from being case-insensitive? > - > sb->s_root = d_make_root(root); > if (!sb->s_root) { > ext4_msg(sb, KERN_ERR, "get root dentry failed"); > diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c > index 4b9ef8bbfa4a..71fdf5076461 100644 > --- a/fs/f2fs/dir.c > +++ b/fs/f2fs/dir.c > @@ -1099,10 +1099,3 @@ const struct file_operations f2fs_dir_operations = { > .compat_ioctl = f2fs_compat_ioctl, > #endif > }; > - > -#ifdef CONFIG_UNICODE > -const struct dentry_operations f2fs_dentry_ops = { > - .d_hash = generic_ci_d_hash, > - .d_compare = generic_ci_d_compare, > -}; > -#endif > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > index cb700d797296..62b4f31d30e2 100644 > --- a/fs/f2fs/f2fs.h > +++ b/fs/f2fs/f2fs.h > @@ -3767,9 +3767,6 @@ static inline void f2fs_update_sit_info(struct f2fs_sb_info *sbi) {} > #endif > > extern const struct file_operations f2fs_dir_operations; > -#ifdef CONFIG_UNICODE > -extern const struct dentry_operations f2fs_dentry_ops; > -#endif > extern const struct file_operations f2fs_file_operations; > extern const struct inode_operations f2fs_file_inode_operations; > extern const struct address_space_operations f2fs_dblock_aops; > diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c > index 8fa37d1434de..6edb1ab579a1 100644 > --- a/fs/f2fs/namei.c > +++ b/fs/f2fs/namei.c > @@ -497,6 +497,7 @@ static struct dentry *f2fs_lookup(struct inode *dir, struct dentry *dentry, > } > > err = f2fs_prepare_lookup(dir, dentry, &fname); > + generic_set_encrypted_ci_d_ops(dentry); > if (err == -ENOENT) > goto out_splice; > if (err) > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c > index 00eff2f51807..f51d52591c99 100644 > --- a/fs/f2fs/super.c > +++ b/fs/f2fs/super.c > @@ -3427,7 +3427,6 @@ static int f2fs_setup_casefold(struct f2fs_sb_info *sbi) > > sbi->sb->s_encoding = encoding; > sbi->sb->s_encoding_flags = encoding_flags; > - sbi->sb->s_d_op = &f2fs_dentry_ops; > } > #else > if (f2fs_sb_has_casefold(sbi)) { > diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c > index 155521e51ac5..7a920434d741 100644 > --- a/fs/ubifs/dir.c > +++ b/fs/ubifs/dir.c > @@ -203,6 +203,7 @@ static struct dentry *ubifs_lookup(struct inode *dir, struct dentry *dentry, > dbg_gen("'%pd' in dir ino %lu", dentry, dir->i_ino); > > err = fscrypt_prepare_lookup(dir, dentry, &nm); > + generic_set_encrypted_ci_d_ops(dentry); > if (err == -ENOENT) > return d_splice_alias(NULL, dentry); > if (err) > diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h > index a8f7a43f031b..e72f80482671 100644 > --- a/include/linux/fscrypt.h > +++ b/include/linux/fscrypt.h > @@ -741,8 +741,11 @@ static inline int fscrypt_prepare_rename(struct inode *old_dir, > * directory's encryption key is available, then the lookup is assumed to be by > * plaintext name; otherwise, it is assumed to be by no-key name. > * > - * This also installs a custom ->d_revalidate() method which will invalidate the > - * dentry if it was created without the key and the key is later added. > + * This will set DCACHE_NOKEY_NAME on the dentry if the lookup is by no-key > + * name. In this case the filesystem must assign the dentry a dentry_operations > + * which contains fscrypt_d_revalidate (or contains a d_revalidate method that > + * calls fscrypt_d_revalidate), so that the dentry will be invalidated if the > + * directory's encryption key is later added. > * > * Return: 0 on success; -ENOENT if the directory's key is unavailable but the > * filename isn't a valid no-key name, so a negative dentry should be created;
Hi all, On Thu, Nov 19, 2020 at 06:09:03AM +0000, Daniel Rosenberg wrote: > This shifts the responsibility of setting up dentry operations from > fscrypt to the individual filesystems, allowing them to have their own > operations while still setting fscrypt's d_revalidate as appropriate. > > Most filesystems can just use generic_set_encrypted_ci_d_ops, unless > they have their own specific dentry operations as well. That operation > will set the minimal d_ops required under the circumstances. > > Since the fscrypt d_ops are set later on, we must set all d_ops there, > since we cannot adjust those later on. This should not result in any > change in behavior. > > Signed-off-by: Daniel Rosenberg <drosen@google.com> > Acked-by: Eric Biggers <ebiggers@google.com> > --- ... > extern const struct file_operations ext4_dir_operations; > > -#ifdef CONFIG_UNICODE > -extern const struct dentry_operations ext4_dentry_ops; > -#endif > - > /* file.c */ > extern const struct inode_operations ext4_file_inode_operations; > extern const struct file_operations ext4_file_operations; > diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c > index 33509266f5a0..12a417ff5648 100644 > --- a/fs/ext4/namei.c > +++ b/fs/ext4/namei.c > @@ -1614,6 +1614,7 @@ static struct buffer_head *ext4_lookup_entry(struct inode *dir, > struct buffer_head *bh; > > err = ext4_fname_prepare_lookup(dir, dentry, &fname); > + generic_set_encrypted_ci_d_ops(dentry); One thing might be worth noticing is that currently overlayfs might not work properly when dentry->d_sb->s_encoding is set even only some subdirs are CI-enabled but the others not, see generic_set_encrypted_ci_d_ops(), ovl_mount_dir_noesc => ovl_dentry_weird() For more details, see: https://android-review.googlesource.com/c/device/linaro/hikey/+/1483316/2#message-2e1f6ab0010a3e35e7d8effea73f60341f84ee4d Just found it by chance (and not sure if it's vital for now), and a kind reminder about this. Thanks, Gao Xiang
On Sat, Nov 21, 2020 at 11:45:41PM -0500, Gabriel Krisman Bertazi wrote: > > diff --git a/fs/ext4/super.c b/fs/ext4/super.c > > index 6633b20224d5..0288bedf46e1 100644 > > --- a/fs/ext4/super.c > > +++ b/fs/ext4/super.c > > @@ -4968,11 +4968,6 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent) > > goto failed_mount4; > > } > > > > -#ifdef CONFIG_UNICODE > > - if (sb->s_encoding) > > - sb->s_d_op = &ext4_dentry_ops; > > -#endif > > This change has the side-effect of removing the capability of the root > directory from being case-insensitive. It is not a backward > incompatible change because there is no way to make the root directory > CI at the moment (it is never empty). But this restriction seems > artificial. Is there a real reason to prevent the root inode from being > case-insensitive? > The problem is that the "lost+found" directory is special in that e2fsck needs to be able to find it. That's the reason why ext4 doesn't allow the root directory to be encrypted. (And encrypting the root directory isn't really useful anyway, since if the goal is to encrypt a whole filesystem with one key, dm-crypt is a better solution.) Casefolding is a bit less problematic than encryption. But it still doesn't entirely work, as e.g. if you name the directory "LOST+FOUND" instead (the directory is casefolded after all...), then e2fsck can't find it. Unless there's a real use case for the root directory being casefolded and people are willing to fix e2fsck, I think we should just make ext4 return an error when setting the casefold flag on the root directory, like it does when trying to enable encryption on the root directory. - Eric
On Sun, Nov 22, 2020 at 01:12:18PM +0800, Gao Xiang wrote: > Hi all, > > On Thu, Nov 19, 2020 at 06:09:03AM +0000, Daniel Rosenberg wrote: > > This shifts the responsibility of setting up dentry operations from > > fscrypt to the individual filesystems, allowing them to have their own > > operations while still setting fscrypt's d_revalidate as appropriate. > > > > Most filesystems can just use generic_set_encrypted_ci_d_ops, unless > > they have their own specific dentry operations as well. That operation > > will set the minimal d_ops required under the circumstances. > > > > Since the fscrypt d_ops are set later on, we must set all d_ops there, > > since we cannot adjust those later on. This should not result in any > > change in behavior. > > > > Signed-off-by: Daniel Rosenberg <drosen@google.com> > > Acked-by: Eric Biggers <ebiggers@google.com> > > --- > > ... > > > extern const struct file_operations ext4_dir_operations; > > > > -#ifdef CONFIG_UNICODE > > -extern const struct dentry_operations ext4_dentry_ops; > > -#endif > > - > > /* file.c */ > > extern const struct inode_operations ext4_file_inode_operations; > > extern const struct file_operations ext4_file_operations; > > diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c > > index 33509266f5a0..12a417ff5648 100644 > > --- a/fs/ext4/namei.c > > +++ b/fs/ext4/namei.c > > @@ -1614,6 +1614,7 @@ static struct buffer_head *ext4_lookup_entry(struct inode *dir, > > struct buffer_head *bh; > > > > err = ext4_fname_prepare_lookup(dir, dentry, &fname); > > + generic_set_encrypted_ci_d_ops(dentry); > > One thing might be worth noticing is that currently overlayfs might > not work properly when dentry->d_sb->s_encoding is set even only some > subdirs are CI-enabled but the others not, see generic_set_encrypted_ci_d_ops(), > ovl_mount_dir_noesc => ovl_dentry_weird() > > For more details, see: > https://android-review.googlesource.com/c/device/linaro/hikey/+/1483316/2#message-2e1f6ab0010a3e35e7d8effea73f60341f84ee4d > > Just found it by chance (and not sure if it's vital for now), and > a kind reminder about this. > Yes, overlayfs doesn't work on ext4 or f2fs filesystems that have the casefold feature enabled, regardless of which directories are actually using casefolding. This is an existing limitation which was previously discussed, e.g. at https://lkml.kernel.org/linux-ext4/CAOQ4uxgPXBazE-g2v=T_vOvnr_f0ZHyKYZ4wvn7A3ePatZrhnQ@mail.gmail.com/T/#u and https://lkml.kernel.org/linux-ext4/20191203051049.44573-1-drosen@google.com/T/#u. Gabriel and Daniel, is one of you still looking into fixing this? IIUC, the current thinking is that when the casefolding flag is set on a directory, it's too late to assign dentry_operations at that point. But what if all child dentries (which must be negative) are invalidated first, and also the filesystem forbids setting the casefold flag on encrypted directories that are accessed via a no-key name (so that fscrypt_d_revalidate isn't needed -- i.e. the directory would only go from "no d_ops" to "generic_ci_dentry_ops", not from "generic_encrypted_dentry_ops" to "generic_encrypted_ci_dentry_ops")? - Eric
On Mon, Nov 23, 2020 at 02:51:44PM -0800, Eric Biggers wrote: > On Sun, Nov 22, 2020 at 01:12:18PM +0800, Gao Xiang wrote: > > Hi all, > > > > On Thu, Nov 19, 2020 at 06:09:03AM +0000, Daniel Rosenberg wrote: > > > This shifts the responsibility of setting up dentry operations from > > > fscrypt to the individual filesystems, allowing them to have their own > > > operations while still setting fscrypt's d_revalidate as appropriate. > > > > > > Most filesystems can just use generic_set_encrypted_ci_d_ops, unless > > > they have their own specific dentry operations as well. That operation > > > will set the minimal d_ops required under the circumstances. > > > > > > Since the fscrypt d_ops are set later on, we must set all d_ops there, > > > since we cannot adjust those later on. This should not result in any > > > change in behavior. > > > > > > Signed-off-by: Daniel Rosenberg <drosen@google.com> > > > Acked-by: Eric Biggers <ebiggers@google.com> > > > --- > > > > ... > > > > > extern const struct file_operations ext4_dir_operations; > > > > > > -#ifdef CONFIG_UNICODE > > > -extern const struct dentry_operations ext4_dentry_ops; > > > -#endif > > > - > > > /* file.c */ > > > extern const struct inode_operations ext4_file_inode_operations; > > > extern const struct file_operations ext4_file_operations; > > > diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c > > > index 33509266f5a0..12a417ff5648 100644 > > > --- a/fs/ext4/namei.c > > > +++ b/fs/ext4/namei.c > > > @@ -1614,6 +1614,7 @@ static struct buffer_head *ext4_lookup_entry(struct inode *dir, > > > struct buffer_head *bh; > > > > > > err = ext4_fname_prepare_lookup(dir, dentry, &fname); > > > + generic_set_encrypted_ci_d_ops(dentry); > > > > One thing might be worth noticing is that currently overlayfs might > > not work properly when dentry->d_sb->s_encoding is set even only some > > subdirs are CI-enabled but the others not, see generic_set_encrypted_ci_d_ops(), > > ovl_mount_dir_noesc => ovl_dentry_weird() > > > > For more details, see: > > https://android-review.googlesource.com/c/device/linaro/hikey/+/1483316/2#message-2e1f6ab0010a3e35e7d8effea73f60341f84ee4d > > > > Just found it by chance (and not sure if it's vital for now), and > > a kind reminder about this. > > > > Yes, overlayfs doesn't work on ext4 or f2fs filesystems that have the casefold > feature enabled, regardless of which directories are actually using casefolding. > This is an existing limitation which was previously discussed, e.g. at > https://lkml.kernel.org/linux-ext4/CAOQ4uxgPXBazE-g2v=T_vOvnr_f0ZHyKYZ4wvn7A3ePatZrhnQ@mail.gmail.com/T/#u > and > https://lkml.kernel.org/linux-ext4/20191203051049.44573-1-drosen@google.com/T/#u. > > Gabriel and Daniel, is one of you still looking into fixing this? IIUC, the > current thinking is that when the casefolding flag is set on a directory, it's > too late to assign dentry_operations at that point. But what if all child > dentries (which must be negative) are invalidated first, and also the filesystem > forbids setting the casefold flag on encrypted directories that are accessed via > a no-key name (so that fscrypt_d_revalidate isn't needed -- i.e. the directory > would only go from "no d_ops" to "generic_ci_dentry_ops", not from > "generic_encrypted_dentry_ops" to "generic_encrypted_ci_dentry_ops")? From my limited knowledge about VFS, I think that is practical as well, since we don't have sub-sub-dirs since all sub-dirs are negative dentries for empty dirs. And if casefold ioctl is "dir inode locked", I think that would be fine (?) I don't check the code though. Thanks, Gao Xiang > > - Eric >
Eric Biggers <ebiggers@kernel.org> writes: > On Sat, Nov 21, 2020 at 11:45:41PM -0500, Gabriel Krisman Bertazi wrote: >> > diff --git a/fs/ext4/super.c b/fs/ext4/super.c >> > index 6633b20224d5..0288bedf46e1 100644 >> > --- a/fs/ext4/super.c >> > +++ b/fs/ext4/super.c >> > @@ -4968,11 +4968,6 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent) >> > goto failed_mount4; >> > } >> > >> > -#ifdef CONFIG_UNICODE >> > - if (sb->s_encoding) >> > - sb->s_d_op = &ext4_dentry_ops; >> > -#endif >> >> This change has the side-effect of removing the capability of the root >> directory from being case-insensitive. It is not a backward >> incompatible change because there is no way to make the root directory >> CI at the moment (it is never empty). But this restriction seems >> artificial. Is there a real reason to prevent the root inode from being >> case-insensitive? >> > > The problem is that the "lost+found" directory is special in that e2fsck needs > to be able to find it. > > That's the reason why ext4 doesn't allow the root directory to be encrypted. > (And encrypting the root directory isn't really useful anyway, since if the goal > is to encrypt a whole filesystem with one key, dm-crypt is a better solution.) > > Casefolding is a bit less problematic than encryption. But it still doesn't > entirely work, as e.g. if you name the directory "LOST+FOUND" instead (the > directory is casefolded after all...), then e2fsck can't find it. > > Unless there's a real use case for the root directory being casefolded and > people are willing to fix e2fsck, I think we should just make ext4 return an > error when setting the casefold flag on the root directory, like it does when > trying to enable encryption on the root directory. I don't have a use case where I need a root directory to be CI. In fact, when I first implemented CI, I did want to block the root directory from being made CI, just to prevent people from doing "chattr +F /" and complaining afterwards when /usr/lib breaks. My concern with the curent patch was whether this side-effect was considered, but I'm happy with either semantics.
Eric Biggers <ebiggers@kernel.org> writes: > On Sun, Nov 22, 2020 at 01:12:18PM +0800, Gao Xiang wrote: >> Hi all, >> >> On Thu, Nov 19, 2020 at 06:09:03AM +0000, Daniel Rosenberg wrote: >> > This shifts the responsibility of setting up dentry operations from >> > fscrypt to the individual filesystems, allowing them to have their own >> > operations while still setting fscrypt's d_revalidate as appropriate. >> > >> > Most filesystems can just use generic_set_encrypted_ci_d_ops, unless >> > they have their own specific dentry operations as well. That operation >> > will set the minimal d_ops required under the circumstances. >> > >> > Since the fscrypt d_ops are set later on, we must set all d_ops there, >> > since we cannot adjust those later on. This should not result in any >> > change in behavior. >> > >> > Signed-off-by: Daniel Rosenberg <drosen@google.com> >> > Acked-by: Eric Biggers <ebiggers@google.com> >> > --- >> >> ... >> >> > extern const struct file_operations ext4_dir_operations; >> > >> > -#ifdef CONFIG_UNICODE >> > -extern const struct dentry_operations ext4_dentry_ops; >> > -#endif >> > - >> > /* file.c */ >> > extern const struct inode_operations ext4_file_inode_operations; >> > extern const struct file_operations ext4_file_operations; >> > diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c >> > index 33509266f5a0..12a417ff5648 100644 >> > --- a/fs/ext4/namei.c >> > +++ b/fs/ext4/namei.c >> > @@ -1614,6 +1614,7 @@ static struct buffer_head *ext4_lookup_entry(struct inode *dir, >> > struct buffer_head *bh; >> > >> > err = ext4_fname_prepare_lookup(dir, dentry, &fname); >> > + generic_set_encrypted_ci_d_ops(dentry); >> >> One thing might be worth noticing is that currently overlayfs might >> not work properly when dentry->d_sb->s_encoding is set even only some >> subdirs are CI-enabled but the others not, see generic_set_encrypted_ci_d_ops(), >> ovl_mount_dir_noesc => ovl_dentry_weird() >> >> For more details, see: >> https://android-review.googlesource.com/c/device/linaro/hikey/+/1483316/2#message-2e1f6ab0010a3e35e7d8effea73f60341f84ee4d >> >> Just found it by chance (and not sure if it's vital for now), and >> a kind reminder about this. >> > > Yes, overlayfs doesn't work on ext4 or f2fs filesystems that have the casefold > feature enabled, regardless of which directories are actually using casefolding. > This is an existing limitation which was previously discussed, e.g. at > https://lkml.kernel.org/linux-ext4/CAOQ4uxgPXBazE-g2v=T_vOvnr_f0ZHyKYZ4wvn7A3ePatZrhnQ@mail.gmail.com/T/#u > and > https://lkml.kernel.org/linux-ext4/20191203051049.44573-1-drosen@google.com/T/#u. > > Gabriel and Daniel, is one of you still looking into fixing this? Eric, overlayfs+CI has been on my todo list for over a year now, as I have a customer who wants to mix them, but I haven't been able to get to it. I'm sure I won't be able to get to it until mid next year, so if anyone wants to tackle it, feel free to do it. > IIUC, the current thinking is that when the casefolding flag is set on > a directory, it's too late to assign dentry_operations at that point. yes > But what if all child dentries (which must be negative) are > invalidated first, I recall I tried this approach when I quickly looked over this last year, but my limited vfs knowledge prevented me from getting something working. But it makes sense. > and also the filesystem forbids setting the casefold flag on encrypted > directories that are accessed via a no-key name (so that > fscrypt_d_revalidate isn't needed -- i.e. the directory would only go > from "no d_ops" to "generic_ci_dentry_ops", not from > "generic_encrypted_dentry_ops" to "generic_encrypted_ci_dentry_ops")?
> > This change has the side-effect of removing the capability of the root > directory from being case-insensitive. It is not a backward > incompatible change because there is no way to make the root directory > CI at the moment (it is never empty). But this restriction seems > artificial. Is there a real reason to prevent the root inode from being > case-insensitive? > I don't have a use case where I need a root directory to be CI. In > fact, when I first implemented CI, I did want to block the root directory > from being made CI, just to prevent people from doing "chattr +F /" and > complaining afterwards when /usr/lib breaks. > > My concern with the curent patch was whether this side-effect was > considered, but I'm happy with either semantics. > > -- > Gabriel Krisman Bertazi That's just from the lost+found directory right? If you remove it you can still change it, and then add the lost+found directory back. Isn't that how it works currently? I definitely didn't intend to change any behavior around non-encrypted casefolding there. I should look at what fsck does if you do that and have a LoSt+fOuNd folder... -Daniel Rosenberg
diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c index 1fbe6c24d705..cb3cfa6329ba 100644 --- a/fs/crypto/fname.c +++ b/fs/crypto/fname.c @@ -570,7 +570,3 @@ int fscrypt_d_revalidate(struct dentry *dentry, unsigned int flags) return valid; } EXPORT_SYMBOL_GPL(fscrypt_d_revalidate); - -const struct dentry_operations fscrypt_d_ops = { - .d_revalidate = fscrypt_d_revalidate, -}; diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h index 4f5806a3b73d..df9c48c1fbf7 100644 --- a/fs/crypto/fscrypt_private.h +++ b/fs/crypto/fscrypt_private.h @@ -294,7 +294,6 @@ int fscrypt_fname_encrypt(const struct inode *inode, const struct qstr *iname, bool fscrypt_fname_encrypted_size(const union fscrypt_policy *policy, u32 orig_len, u32 max_len, u32 *encrypted_len_ret); -extern const struct dentry_operations fscrypt_d_ops; /* hkdf.c */ diff --git a/fs/crypto/hooks.c b/fs/crypto/hooks.c index 20b0df47fe6a..9006fa983335 100644 --- a/fs/crypto/hooks.c +++ b/fs/crypto/hooks.c @@ -117,7 +117,6 @@ int __fscrypt_prepare_lookup(struct inode *dir, struct dentry *dentry, spin_lock(&dentry->d_lock); dentry->d_flags |= DCACHE_NOKEY_NAME; spin_unlock(&dentry->d_lock); - d_set_d_op(dentry, &fscrypt_d_ops); } return err; } diff --git a/fs/ext4/dir.c b/fs/ext4/dir.c index ca50c90adc4c..e757319a4472 100644 --- a/fs/ext4/dir.c +++ b/fs/ext4/dir.c @@ -667,10 +667,3 @@ const struct file_operations ext4_dir_operations = { .open = ext4_dir_open, .release = ext4_release_dir, }; - -#ifdef CONFIG_UNICODE -const struct dentry_operations ext4_dentry_ops = { - .d_hash = generic_ci_d_hash, - .d_compare = generic_ci_d_compare, -}; -#endif diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h index bf9429484462..ad77f01d9e20 100644 --- a/fs/ext4/ext4.h +++ b/fs/ext4/ext4.h @@ -3380,10 +3380,6 @@ static inline void ext4_unlock_group(struct super_block *sb, /* dir.c */ extern const struct file_operations ext4_dir_operations; -#ifdef CONFIG_UNICODE -extern const struct dentry_operations ext4_dentry_ops; -#endif - /* file.c */ extern const struct inode_operations ext4_file_inode_operations; extern const struct file_operations ext4_file_operations; diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c index 33509266f5a0..12a417ff5648 100644 --- a/fs/ext4/namei.c +++ b/fs/ext4/namei.c @@ -1614,6 +1614,7 @@ static struct buffer_head *ext4_lookup_entry(struct inode *dir, struct buffer_head *bh; err = ext4_fname_prepare_lookup(dir, dentry, &fname); + generic_set_encrypted_ci_d_ops(dentry); if (err == -ENOENT) return NULL; if (err) diff --git a/fs/ext4/super.c b/fs/ext4/super.c index 6633b20224d5..0288bedf46e1 100644 --- a/fs/ext4/super.c +++ b/fs/ext4/super.c @@ -4968,11 +4968,6 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent) goto failed_mount4; } -#ifdef CONFIG_UNICODE - if (sb->s_encoding) - sb->s_d_op = &ext4_dentry_ops; -#endif - sb->s_root = d_make_root(root); if (!sb->s_root) { ext4_msg(sb, KERN_ERR, "get root dentry failed"); diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c index 4b9ef8bbfa4a..71fdf5076461 100644 --- a/fs/f2fs/dir.c +++ b/fs/f2fs/dir.c @@ -1099,10 +1099,3 @@ const struct file_operations f2fs_dir_operations = { .compat_ioctl = f2fs_compat_ioctl, #endif }; - -#ifdef CONFIG_UNICODE -const struct dentry_operations f2fs_dentry_ops = { - .d_hash = generic_ci_d_hash, - .d_compare = generic_ci_d_compare, -}; -#endif diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h index cb700d797296..62b4f31d30e2 100644 --- a/fs/f2fs/f2fs.h +++ b/fs/f2fs/f2fs.h @@ -3767,9 +3767,6 @@ static inline void f2fs_update_sit_info(struct f2fs_sb_info *sbi) {} #endif extern const struct file_operations f2fs_dir_operations; -#ifdef CONFIG_UNICODE -extern const struct dentry_operations f2fs_dentry_ops; -#endif extern const struct file_operations f2fs_file_operations; extern const struct inode_operations f2fs_file_inode_operations; extern const struct address_space_operations f2fs_dblock_aops; diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c index 8fa37d1434de..6edb1ab579a1 100644 --- a/fs/f2fs/namei.c +++ b/fs/f2fs/namei.c @@ -497,6 +497,7 @@ static struct dentry *f2fs_lookup(struct inode *dir, struct dentry *dentry, } err = f2fs_prepare_lookup(dir, dentry, &fname); + generic_set_encrypted_ci_d_ops(dentry); if (err == -ENOENT) goto out_splice; if (err) diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c index 00eff2f51807..f51d52591c99 100644 --- a/fs/f2fs/super.c +++ b/fs/f2fs/super.c @@ -3427,7 +3427,6 @@ static int f2fs_setup_casefold(struct f2fs_sb_info *sbi) sbi->sb->s_encoding = encoding; sbi->sb->s_encoding_flags = encoding_flags; - sbi->sb->s_d_op = &f2fs_dentry_ops; } #else if (f2fs_sb_has_casefold(sbi)) { diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c index 155521e51ac5..7a920434d741 100644 --- a/fs/ubifs/dir.c +++ b/fs/ubifs/dir.c @@ -203,6 +203,7 @@ static struct dentry *ubifs_lookup(struct inode *dir, struct dentry *dentry, dbg_gen("'%pd' in dir ino %lu", dentry, dir->i_ino); err = fscrypt_prepare_lookup(dir, dentry, &nm); + generic_set_encrypted_ci_d_ops(dentry); if (err == -ENOENT) return d_splice_alias(NULL, dentry); if (err) diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h index a8f7a43f031b..e72f80482671 100644 --- a/include/linux/fscrypt.h +++ b/include/linux/fscrypt.h @@ -741,8 +741,11 @@ static inline int fscrypt_prepare_rename(struct inode *old_dir, * directory's encryption key is available, then the lookup is assumed to be by * plaintext name; otherwise, it is assumed to be by no-key name. * - * This also installs a custom ->d_revalidate() method which will invalidate the - * dentry if it was created without the key and the key is later added. + * This will set DCACHE_NOKEY_NAME on the dentry if the lookup is by no-key + * name. In this case the filesystem must assign the dentry a dentry_operations + * which contains fscrypt_d_revalidate (or contains a d_revalidate method that + * calls fscrypt_d_revalidate), so that the dentry will be invalidated if the + * directory's encryption key is later added. * * Return: 0 on success; -ENOENT if the directory's key is unavailable but the * filename isn't a valid no-key name, so a negative dentry should be created;