Re: [f2fs-dev] [PATCH v16 3/9] libfs: Introduce case-insensitive string comparison helper
On Fri, Apr 05, 2024 at 03:13:26PM +0300, Eugen Hristev wrote:
> +/**
> + * generic_ci_match() - Match a name (case-insensitively) with a dirent.
> + * This is a filesystem helper for comparison with directory entries.
> + * generic_ci_d_compare should be used in VFS' ->d_compare instead.
> + *
> + * @parent: Inode of the parent of the dirent under comparison
> + * @name: name under lookup.
> + * @folded_name: Optional pre-folded name under lookup
> + * @de_name: Dirent name.
> + * @de_name_len: dirent name length.
> + *
> + * Test whether a case-insensitive directory entry matches the filename
> + * being searched. If @folded_name is provided, it is used instead of
> + * recalculating the casefold of @name.
> + *
> + * Return: > 0 if the directory entry matches, 0 if it doesn't match, or
> + * < 0 on error.
> + */
> +int generic_ci_match(const struct inode *parent,
> + const struct qstr *name,
> + const struct qstr *folded_name,
> + const u8 *de_name, u32 de_name_len)
> +{
> + const struct super_block *sb = parent->i_sb;
> + const struct unicode_map *um = sb->s_encoding;
> + struct fscrypt_str decrypted_name = FSTR_INIT(NULL, de_name_len);
> + struct qstr dirent = QSTR_INIT(de_name, de_name_len);
> + int res = 0;
> +
> + if (IS_ENCRYPTED(parent)) {
> + const struct fscrypt_str encrypted_name =
> + FSTR_INIT((u8 *) de_name, de_name_len);
> +
> + if (WARN_ON_ONCE(!fscrypt_has_encryption_key(parent)))
> + return -EINVAL;
> +
> + decrypted_name.name = kmalloc(de_name_len, GFP_KERNEL);
> + if (!decrypted_name.name)
> + return -ENOMEM;
> + res = fscrypt_fname_disk_to_usr(parent, 0, 0, _name,
> + _name);
> + if (res < 0)
> + goto out;
If fscrypt_fname_disk_to_usr() returns an error and !sb_has_strict_encoding(sb),
then this function returns 0 (indicating no match) instead of the error code
(indicating an error). Is that the correct behavior? I would think that
strict_encoding should only have an effect on the actual name comparison.
> + /*
> + * Attempt a case-sensitive match first. It is cheaper and
> + * should cover most lookups, including all the sane
> + * applications that expect a case-sensitive filesystem.
> + */
> + if (folded_name->name) {
> + if (dirent.len == folded_name->len &&
> + !memcmp(folded_name->name, dirent.name, dirent.len))
> + goto out;
> + res = utf8_strncasecmp_folded(um, folded_name, );
Shouldn't the memcmp be done with the original user-specified name, not the
casefolded name? I would think that the user-specified name is the one that's
more likely to match the on-disk name, because of case preservation. In most
cases users will specify the same case on both file creation and later access.
- Eric
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v16 9/9] f2fs: Move CONFIG_UNICODE defguards into the code flow
On Fri, Apr 05, 2024 at 03:13:32PM +0300, Eugen Hristev wrote: > From: Gabriel Krisman Bertazi > > Instead of a bunch of ifdefs, make the unicode built checks part of the > code flow where possible, as requested by Torvalds. > > Signed-off-by: Gabriel Krisman Bertazi > [eugen.hris...@collabora.com: port to 6.8-rc3] > Signed-off-by: Eugen Hristev > --- > fs/f2fs/namei.c | 10 -- > fs/f2fs/super.c | 8 > 2 files changed, 8 insertions(+), 10 deletions(-) Reviewed-by: Eric Biggers - Eric ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v16 8/9] ext4: Move CONFIG_UNICODE defguards into the code flow
On Fri, Apr 05, 2024 at 03:13:31PM +0300, Eugen Hristev wrote: > From: Gabriel Krisman Bertazi > > Instead of a bunch of ifdefs, make the unicode built checks part of the > code flow where possible, as requested by Torvalds. > > Signed-off-by: Gabriel Krisman Bertazi > [eugen.hris...@collabora.com: port to 6.8-rc3] > Signed-off-by: Eugen Hristev > --- > fs/ext4/crypto.c | 10 ++ > fs/ext4/ext4.h | 33 + > fs/ext4/namei.c | 14 +- > fs/ext4/super.c | 4 +--- > 4 files changed, 29 insertions(+), 32 deletions(-) Reviewed-by: Eric Biggers - Eric ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v16 7/9] f2fs: Log error when lookup of encoded dentry fails
On Fri, Apr 05, 2024 at 03:13:30PM +0300, Eugen Hristev wrote: > If the volume is in strict mode, generi c_ci_compare can report a broken > encoding name. This will not trigger on a bad lookup, which is caught > earlier, only if the actual disk name is bad. > > Suggested-by: Gabriel Krisman Bertazi > Signed-off-by: Eugen Hristev > --- > fs/f2fs/dir.c | 15 ++- > 1 file changed, 10 insertions(+), 5 deletions(-) > Reviewed-by: Eric Biggers But please fix the typo: "generi c_ci_compare" => "generic_ci_d_compare" - Eric ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v16 6/9] ext4: Log error when lookup of encoded dentry fails
On Fri, Apr 05, 2024 at 03:13:29PM +0300, Eugen Hristev wrote: > From: Gabriel Krisman Bertazi > > If the volume is in strict mode, ext4_ci_compare can report a broken > encoding name. This will not trigger on a bad lookup, which is caught > earlier, only if the actual disk name is bad. > > Signed-off-by: Gabriel Krisman Bertazi > Signed-off-by: Eugen Hristev > --- > fs/ext4/namei.c | 3 +++ > 1 file changed, 3 insertions(+) > Reviewed-by: Eric Biggers - Eric ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v16 5/9] f2fs: Reuse generic_ci_match for ci comparisons
On Fri, Apr 05, 2024 at 03:13:28PM +0300, Eugen Hristev wrote: > From: Gabriel Krisman Bertazi > > Now that ci_match is part of libfs, make f2fs reuse it instead of having > a different implementation. > > Signed-off-by: Gabriel Krisman Bertazi > Signed-off-by: Eugen Hristev > --- > fs/f2fs/dir.c | 58 --- > 1 file changed, 4 insertions(+), 54 deletions(-) Reviewed-by: Eric Biggers - Eric ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v16 4/9] ext4: Reuse generic_ci_match for ci comparisons
On Fri, Apr 05, 2024 at 03:13:27PM +0300, Eugen Hristev wrote: > From: Gabriel Krisman Bertazi > > Instead of reimplementing ext4_match_ci, use the new libfs helper. > > It also adds a comment explaining why fname->cf_name.name must be > checked prior to the encryption hash optimization, because that tripped > me before. > > Signed-off-by: Gabriel Krisman Bertazi > Signed-off-by: Eugen Hristev > --- > fs/ext4/namei.c | 91 +++-- > 1 file changed, 27 insertions(+), 64 deletions(-) Reviewed-by: Eric Biggers - Eric ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v16 2/9] f2fs: Simplify the handling of cached insensitive names
On Fri, Apr 05, 2024 at 03:13:25PM +0300, Eugen Hristev wrote: > From: Gabriel Krisman Bertazi > > Keeping it as qstr avoids the unnecessary conversion in f2fs_match > > Signed-off-by: Gabriel Krisman Bertazi > [eugen.hris...@collabora.com: port to 6.8-rc3 and minor changes] > Signed-off-by: Eugen Hristev > --- > fs/f2fs/dir.c | 53 ++ > fs/f2fs/f2fs.h | 16 +- > fs/f2fs/recovery.c | 9 +--- > 3 files changed, 46 insertions(+), 32 deletions(-) Reviewed-by: Eric Biggers (But please change "cached insensitive" to "case-insensitive") - Eric ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v16 1/9] ext4: Simplify the handling of cached insensitive names
On Fri, Apr 05, 2024 at 03:13:24PM +0300, Eugen Hristev wrote: > From: Gabriel Krisman Bertazi > > Keeping it as qstr avoids the unnecessary conversion in ext4_match > > Signed-off-by: Gabriel Krisman Bertazi > [eugen.hris...@collabora.com: port to 6.8-rc3] > Signed-off-by: Eugen Hristev Reviewed-by: Eric Biggers (But please change "cached insensitive" to "case-insensitive") - Eric ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [PATCH 1/6] fscrypt: Convert bh_get_inode_and_lblk_num to use a folio
On Tue, Apr 23, 2024 at 11:55:32PM +0100, Matthew Wilcox (Oracle) wrote:
> Remove uses of page->index, page_mapping() and b_page. Saves a call
> to compound_head().
>
> Signed-off-by: Matthew Wilcox (Oracle)
> ---
> fs/crypto/inline_crypt.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/fs/crypto/inline_crypt.c b/fs/crypto/inline_crypt.c
> index b4002aea7cdb..40de69860dcf 100644
> --- a/fs/crypto/inline_crypt.c
> +++ b/fs/crypto/inline_crypt.c
> @@ -284,7 +284,7 @@ static bool bh_get_inode_and_lblk_num(const struct
> buffer_head *bh,
> const struct inode **inode_ret,
> u64 *lblk_num_ret)
> {
> - struct page *page = bh->b_page;
> + struct folio *folio = bh->b_folio;
> const struct address_space *mapping;
> const struct inode *inode;
>
> @@ -292,13 +292,13 @@ static bool bh_get_inode_and_lblk_num(const struct
> buffer_head *bh,
>* The ext4 journal (jbd2) can submit a buffer_head it directly created
>* for a non-pagecache page. fscrypt doesn't care about these.
>*/
> - mapping = page_mapping(page);
> + mapping = folio_mapping(folio);
> if (!mapping)
> return false;
> inode = mapping->host;
>
> *inode_ret = inode;
> - *lblk_num_ret = ((u64)page->index << (PAGE_SHIFT - inode->i_blkbits)) +
> + *lblk_num_ret = ((u64)folio->index << (PAGE_SHIFT - inode->i_blkbits)) +
> (bh_offset(bh) >> inode->i_blkbits);
> return true;
> }
Reviewed-by: Eric Biggers
- Eric
Re: [f2fs-dev] [PATCH v15 7/9] f2fs: Log error when lookup of encoded dentry fails
On Tue, Apr 02, 2024 at 06:48:40PM +0300, Eugen Hristev via Linux-f2fs-devel
wrote:
> If the volume is in strict mode, generi c_ci_compare can report a broken
> encoding name. This will not trigger on a bad lookup, which is caught
> earlier, only if the actual disk name is bad.
>
> Suggested-by: Gabriel Krisman Bertazi
> Signed-off-by: Eugen Hristev
> ---
> fs/f2fs/dir.c | 15 ++-
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
> index 88b0045d0c4f..64286d80dd30 100644
> --- a/fs/f2fs/dir.c
> +++ b/fs/f2fs/dir.c
> @@ -192,11 +192,16 @@ static inline int f2fs_match_name(const struct inode
> *dir,
> struct fscrypt_name f;
>
> #if IS_ENABLED(CONFIG_UNICODE)
> - if (fname->cf_name.name)
> - return generic_ci_match(dir, fname->usr_fname,
> - >cf_name,
> - de_name, de_name_len);
> -
> + if (fname->cf_name.name) {
> + int ret = generic_ci_match(dir, fname->usr_fname,
> +>cf_name,
> +de_name, de_name_len);
> + if (ret == -EINVAL)
> + f2fs_warn(F2FS_SB(dir->i_sb),
> + "Directory contains filename that is invalid
> UTF-8");
> +
Shouldn't this use f2fs_warn_ratelimited?
- Eric
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v15 6/9] ext4: Log error when lookup of encoded dentry fails
On Tue, Apr 02, 2024 at 06:48:39PM +0300, Eugen Hristev via Linux-f2fs-devel wrote: > From: Gabriel Krisman Bertazi > > If the volume is in strict mode, ext4_ci_compare can report a broken > encoding name. This will not trigger on a bad lookup, which is caught > earlier, only if the actual disk name is bad. > > Signed-off-by: Gabriel Krisman Bertazi > Signed-off-by: Eugen Hristev > --- > fs/ext4/namei.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c > index 2d0ee232fbe7..3268cf45d9db 100644 > --- a/fs/ext4/namei.c > +++ b/fs/ext4/namei.c > @@ -1477,6 +1477,9 @@ static bool ext4_match(struct inode *parent, >* only case where it happens is on a disk >* corruption or ENOMEM. >*/ > + if (ret == -EINVAL) > + EXT4_ERROR_INODE(parent, > + "Directory contains filename that is > invalid UTF-8"); > return false; I'm seeing this error when the volume is *not* in strict mode and a file has a name that is not valid UTF-8. That doesn't seem to be working as intended. mkfs.ext4 -F -O casefold /dev/vdb mount /dev/vdb /mnt mkdir /mnt/dir chattr +F /mnt/dir touch /mnt/dir/$'\xff' [ 1528.691319] EXT4-fs (vdb): Using encoding defined by superblock: utf8-12.1.0 with flags 0x0 [ 1528.707793] EXT4-fs (vdb): mounted filesystem 0be607cc-0dae-4e7f-a40f-4fe8075e8e50 r/w with ordered data mode. Quota mode: none. [ 1528.728583] EXT4-fs error (device vdb): ext4_match:1481: inode #13: comm touch: Directory contains filename that is invalid UTF-8 [ 1528.730700] EXT4-fs error (device vdb): ext4_match:1481: inode #13: comm touch: Directory contains filename that is invalid UTF-8 [ 1528.732976] EXT4-fs error (device vdb): ext4_match:1481: inode #13: comm touch: Directory contains filename that is invalid UTF-8 [ 1528.735536] EXT4-fs error (device vdb): ext4_match:1481: inode #13: comm touch: Directory contains filename that is invalid UTF-8 ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v15 4/9] ext4: Reuse generic_ci_match for ci comparisons
On Tue, Apr 02, 2024 at 08:43:28PM -0700, Eric Biggers wrote:
> On Tue, Apr 02, 2024 at 06:48:37PM +0300, Eugen Hristev wrote:
> > + ret = generic_ci_match(parent, fname->usr_fname,
> > + >cf_name, de->name,
> > + de->name_len);
> > + if (ret < 0) {
> > + /*
> > +* Treat comparison errors as not a match. The
> > +* only case where it happens is on a disk
> > +* corruption or ENOMEM.
> > +*/
> > + return false;
> > }
> > - return !ext4_ci_compare(parent, fname->usr_fname, de->name,
> > - de->name_len, false);
> > + return ret;
>
> Maybe write this as simply 'return ret > 0;'?
Ah, I see that patch 6 adds a check for -EINVAL here, in which case the
'if (ret < 0)' makes sense.
- Eric
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v15 4/9] ext4: Reuse generic_ci_match for ci comparisons
On Tue, Apr 02, 2024 at 06:48:37PM +0300, Eugen Hristev wrote:
> + ret = generic_ci_match(parent, fname->usr_fname,
> +>cf_name, de->name,
> +de->name_len);
> + if (ret < 0) {
> + /*
> + * Treat comparison errors as not a match. The
> + * only case where it happens is on a disk
> + * corruption or ENOMEM.
> + */
> + return false;
> }
> - return !ext4_ci_compare(parent, fname->usr_fname, de->name,
> - de->name_len, false);
> + return ret;
Maybe write this as simply 'return ret > 0;'?
- Eric
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v15 3/9] libfs: Introduce case-insensitive string comparison helper
On Tue, Apr 02, 2024 at 06:48:36PM +0300, Eugen Hristev wrote:
> +int generic_ci_match(const struct inode *parent,
> + const struct qstr *name,
> + const struct qstr *folded_name,
> + const u8 *de_name, u32 de_name_len)
> +{
> + const struct super_block *sb = parent->i_sb;
> + const struct unicode_map *um = sb->s_encoding;
> + struct fscrypt_str decrypted_name = FSTR_INIT(NULL, de_name_len);
> + struct qstr dirent = QSTR_INIT(de_name, de_name_len);
> + int res = 0, match = 0;
> +
> + if (IS_ENCRYPTED(parent)) {
> + const struct fscrypt_str encrypted_name =
> + FSTR_INIT((u8 *) de_name, de_name_len);
> +
> + if (WARN_ON_ONCE(!fscrypt_has_encryption_key(parent)))
> + return -EINVAL;
> +
> + decrypted_name.name = kmalloc(de_name_len, GFP_KERNEL);
> + if (!decrypted_name.name)
> + return -ENOMEM;
> + res = fscrypt_fname_disk_to_usr(parent, 0, 0, _name,
> + _name);
> + if (res < 0)
> + goto out;
> + dirent.name = decrypted_name.name;
> + dirent.len = decrypted_name.len;
> + }
> +
> + /*
> + * Attempt a case-sensitive match first. It is cheaper and
> + * should cover most lookups, including all the sane
> + * applications that expect a case-sensitive filesystem.
> + */
> + if (folded_name->name) {
> + if (dirent.len == folded_name->len &&
> + !memcmp(folded_name->name, dirent.name, dirent.len)) {
> + match = 1;
> + goto out;
> + }
> + res = utf8_strncasecmp_folded(um, folded_name, );
> + } else {
> + if (dirent.len == name->len &&
> + !memcmp(name->name, dirent.name, dirent.len) &&
> + (!sb_has_strict_encoding(sb) || !utf8_validate(um, name))) {
> + match = 1;
> + goto out;
> + }
> + res = utf8_strncasecmp(um, name, );
> + }
The 'match' variable is unnecessary because setting res=0 achieves the same
effect.
- Eric
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v15 2/9] f2fs: Simplify the handling of cached insensitive names
On Tue, Apr 02, 2024 at 06:48:35PM +0300, Eugen Hristev wrote:
> diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
> index d0f24ccbd1ac..7e53abf6d216 100644
> --- a/fs/f2fs/recovery.c
> +++ b/fs/f2fs/recovery.c
> @@ -153,11 +153,8 @@ static int init_recovered_filename(const struct inode
> *dir,
> if (err)
> return err;
> f2fs_hash_filename(dir, fname);
> -#if IS_ENABLED(CONFIG_UNICODE)
> /* Case-sensitive match is fine for recovery */
> - kmem_cache_free(f2fs_cf_name_slab, fname->cf_name.name);
> - fname->cf_name.name = NULL;
> -#endif
> + f2fs_free_casefolded_name(fname);
> } else {
> f2fs_hash_filename(dir, fname);
> }
This change makes the declaration of f2fs_cf_name_slab in recovery.c
unnecessary. It can be removed.
- Eric
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v7 00/10] Set casefold/fscrypt dentry operations through sb->s_d_op
On Wed, Feb 21, 2024 at 12:14:02PM -0500, Gabriel Krisman Bertazi wrote: > > 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 (10): > ovl: Always reject mounting over case-insensitive directories > fscrypt: Factor out a helper to configure the lookup dentry > fscrypt: Drop d_revalidate for valid dentries during lookup > fscrypt: Drop d_revalidate once the key is added > libfs: Merge encrypted_ci_dentry_ops and ci_dentry_ops > libfs: Add helper to choose dentry operations at mount-time > ext4: Configure dentry operations at dentry-creation time > f2fs: Configure dentry operations at dentry-creation time > ubifs: Configure dentry operations at dentry-creation time > libfs: Drop generic_set_encrypted_ci_d_ops > > fs/crypto/hooks.c | 15 -- > fs/ext4/namei.c | 1 - > fs/ext4/super.c | 1 + > fs/f2fs/namei.c | 1 - > fs/f2fs/super.c | 1 + > fs/libfs.c | 62 +++--- > fs/overlayfs/params.c | 14 +++-- > fs/ubifs/dir.c | 1 - > fs/ubifs/super.c| 1 + > include/linux/fs.h | 11 ++- > include/linux/fscrypt.h | 66 - > 11 files changed, 105 insertions(+), 69 deletions(-) > Looks good, Reviewed-by: Eric Biggers - Eric ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v6 04/10] fscrypt: Drop d_revalidate once the key is added
On Wed, Feb 14, 2024 at 04:16:31PM -0800, Eric Biggers wrote: > On Mon, Feb 12, 2024 at 09:13:15PM -0500, Gabriel Krisman Bertazi wrote: > > From fscrypt perspective, once the key is available, the dentry will > > remain valid until evicted for other reasons, since keyed dentries don't > > require revalidation and, if the key is removed, the dentry is > > forcefully evicted. Therefore, we don't need to keep revalidating them > > repeatedly. > > > > Obviously, we can only do this if fscrypt is the only thing requiring > > revalidation for a dentry. For this reason, we only disable > > d_revalidate if the .d_revalidate hook is fscrypt_d_revalidate itself. > > > > It is safe to do it here because when moving the dentry to the > > plain-text version, we are holding the d_lock. We might race with a > > concurrent RCU lookup but this is harmless because, at worst, we will > > get an extra d_revalidate on the keyed dentry, which is will find the > > dentry is valid. > > > > Finally, now that we do more than just clear the DCACHE_NOKEY_NAME in > > fscrypt_handle_d_move, skip it entirely for plaintext dentries, to avoid > > extra costs. > > > > Signed-off-by: Gabriel Krisman Bertazi > > I think this explanation misses an important point, which is that it's only > *directories* where a no-key dentry can become the regular dentry. The VFS > does > the move because it only allows one dentry to exist per directory. > > For nondirectories, the dentries don't get reused and this patch is > irrelevant. > > (Of course, there's no point in making fscrypt_handle_d_move() check whether > the > dentry is a directory, since checking DCACHE_NOKEY_NAME is sufficient.) > > The diff itself looks good -- thanks. > Also, do I understand correctly that this patch is a performance optimization, not preventing a performance regression? The similar patch that precedes this one, "fscrypt: Drop d_revalidate for valid dentries during lookup", is about preventing a performance regression on dentries that aren't no-key. This patch looks deceptively similar, but it only affects no-key directory dentries, which we were already doing the fscrypt_d_revalidate for, even after the move to the plaintext name. It's probably still a worthwhile optimization to stop doing the fscrypt_d_revalidate when a directory dentry gets moved like that. But I want to make sure I'm correctly understanding each patch. - Eric ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v6 00/10] Set casefold/fscrypt dentry operations through sb->s_d_op
On Mon, Feb 12, 2024 at 09:13:11PM -0500, Gabriel Krisman Bertazi wrote: > Hi, > > v6 of this patchset applying the comments from Eric and the suggestion from > Christian. Thank you for your feedback. > > Eric, since this is getting close to merging, how do you want to handle > it? I will take patch 1 through my tree already, are you fine if I merge > this through unicode or do you want to take it through fscrypt? > > As usual, this survived fstests on ext4 and f2fs. I think you should just take the whole series through the unicode tree. If I understand correctly, this series is really about fixing things for casefolding support, not fscrypt support, right? There is a lot of interaction between the two, but ultimately it's casefold that gets fixed. - Eric ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v6 04/10] fscrypt: Drop d_revalidate once the key is added
On Mon, Feb 12, 2024 at 09:13:15PM -0500, Gabriel Krisman Bertazi wrote: > From fscrypt perspective, once the key is available, the dentry will > remain valid until evicted for other reasons, since keyed dentries don't > require revalidation and, if the key is removed, the dentry is > forcefully evicted. Therefore, we don't need to keep revalidating them > repeatedly. > > Obviously, we can only do this if fscrypt is the only thing requiring > revalidation for a dentry. For this reason, we only disable > d_revalidate if the .d_revalidate hook is fscrypt_d_revalidate itself. > > It is safe to do it here because when moving the dentry to the > plain-text version, we are holding the d_lock. We might race with a > concurrent RCU lookup but this is harmless because, at worst, we will > get an extra d_revalidate on the keyed dentry, which is will find the > dentry is valid. > > Finally, now that we do more than just clear the DCACHE_NOKEY_NAME in > fscrypt_handle_d_move, skip it entirely for plaintext dentries, to avoid > extra costs. > > Signed-off-by: Gabriel Krisman Bertazi I think this explanation misses an important point, which is that it's only *directories* where a no-key dentry can become the regular dentry. The VFS does the move because it only allows one dentry to exist per directory. For nondirectories, the dentries don't get reused and this patch is irrelevant. (Of course, there's no point in making fscrypt_handle_d_move() check whether the dentry is a directory, since checking DCACHE_NOKEY_NAME is sufficient.) The diff itself looks good -- thanks. - Eric ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v6 03/10] fscrypt: Drop d_revalidate for valid dentries during lookup
On Mon, Feb 12, 2024 at 09:13:14PM -0500, Gabriel Krisman Bertazi wrote:
> Finally, we need to clean the dentry->flags even for unencrypted
> dentries, so the ->d_lock might be acquired even for them. In order to
might => must?
> diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
> index 47567a6a4f9d..d1f17b90c30f 100644
> --- a/include/linux/fscrypt.h
> +++ b/include/linux/fscrypt.h
> @@ -951,10 +951,29 @@ static inline int fscrypt_prepare_rename(struct inode
> *old_dir,
> static inline void fscrypt_prepare_dentry(struct dentry *dentry,
> bool is_nokey_name)
> {
> + /*
> + * This code tries to only take ->d_lock when necessary to write
> + * to ->d_flags. We shouldn't be peeking on d_flags for
> + * DCACHE_OP_REVALIDATE unlocked, but in the unlikely case
> + * there is a race, the worst it can happen is that we fail to
> + * unset DCACHE_OP_REVALIDATE and pay the cost of an extra
> + * d_revalidate.
> + */
> if (is_nokey_name) {
> spin_lock(>d_lock);
> dentry->d_flags |= DCACHE_NOKEY_NAME;
> spin_unlock(>d_lock);
> + } else if (dentry->d_flags & DCACHE_OP_REVALIDATE &&
> +dentry->d_op->d_revalidate == fscrypt_d_revalidate) {
> + /*
> + * Unencrypted dentries and encrypted dentries where the
> + * key is available are always valid from fscrypt
> + * perspective. Avoid the cost of calling
> + * fscrypt_d_revalidate unnecessarily.
> + */
> + spin_lock(>d_lock);
> + dentry->d_flags &= ~DCACHE_OP_REVALIDATE;
> + spin_unlock(>d_lock);
> }
> }
Does this all get optimized out when !CONFIG_FS_ENCRYPTION?
As-is, I don't think the d_revalidate part will be optimized out.
You may need to create a !CONFIG_FS_ENCRYPTION stub explicitly.
- Eric
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v6 02/10] fscrypt: Factor out a helper to configure the lookup dentry
On Mon, Feb 12, 2024 at 09:13:13PM -0500, Gabriel Krisman Bertazi wrote: > Both fscrypt_prepare_lookup_dentry_partial and > fscrypt_prepare_lookup_dentry Neither of these functions exist. - Eric ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v5 04/12] fscrypt: Drop d_revalidate for valid dentries during lookup
On Wed, Jan 31, 2024 at 03:35:40PM -0300, Gabriel Krisman Bertazi wrote:
> Eric Biggers writes:
>
> > On Mon, Jan 29, 2024 at 05:43:22PM -0300, Gabriel Krisman Bertazi wrote:
> >> Unencrypted and encrypted-dentries where the key is available don't need
> >> to be revalidated with regards to fscrypt, since they don't go stale
> >> from under VFS and the key cannot be removed for the encrypted case
> >> without evicting the dentry. Mark them with d_set_always_valid, to
> >
> > "d_set_always_valid" doesn't appear in the diff itself.
> >
> >> diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
> >> index 4aaf847955c0..a22997b9f35c 100644
> >> --- a/include/linux/fscrypt.h
> >> +++ b/include/linux/fscrypt.h
> >> @@ -942,11 +942,22 @@ static inline int fscrypt_prepare_rename(struct
> >> inode *old_dir,
> >> static inline void fscrypt_prepare_lookup_dentry(struct dentry *dentry,
> >> bool is_nokey_name)
> >> {
> >> - if (is_nokey_name) {
> >> - spin_lock(>d_lock);
> >> + spin_lock(>d_lock);
> >> +
> >> + if (is_nokey_name)
> >>dentry->d_flags |= DCACHE_NOKEY_NAME;
> >> - spin_unlock(>d_lock);
> >> + else if (dentry->d_flags & DCACHE_OP_REVALIDATE &&
> >> + dentry->d_op->d_revalidate == fscrypt_d_revalidate) {
> >> + /*
> >> + * Unencrypted dentries and encrypted dentries where the
> >> + * key is available are always valid from fscrypt
> >> + * perspective. Avoid the cost of calling
> >> + * fscrypt_d_revalidate unnecessarily.
> >> + */
> >> + dentry->d_flags &= ~DCACHE_OP_REVALIDATE;
> >>}
> >> +
> >> + spin_unlock(>d_lock);
> >
> > This makes lookups in unencrypted directories start doing the
> > spin_lock/spin_unlock pair. Is that really necessary?
> >
> > These changes also make the inline function fscrypt_prepare_lookup() very
> > long
> > (when including the fscrypt_prepare_lookup_dentry() that's inlined into it).
> > The rule that I'm trying to follow is that to the extent that the fscrypt
> > helper
> > functions are inlined, the inline part should be a fast path for unencrypted
> > directories. Encrypted directories should be handled out-of-line.
> >
> > So looking at the original fscrypt_prepare_lookup():
> >
> > static inline int fscrypt_prepare_lookup(struct inode *dir,
> > struct dentry *dentry,
> > struct fscrypt_name *fname)
> > {
> > if (IS_ENCRYPTED(dir))
> > return __fscrypt_prepare_lookup(dir, dentry, fname);
> >
> > memset(fname, 0, sizeof(*fname));
> > fname->usr_fname = >d_name;
> > fname->disk_name.name = (unsigned char *)dentry->d_name.name;
> > fname->disk_name.len = dentry->d_name.len;
> > return 0;
> > }
> >
> > If you could just add the DCACHE_OP_REVALIDATE clearing for dentries in
> > unencrypted directories just before the "return 0;", hopefully without the
> > spinlock, that would be good. Yes, that does mean that
> > __fscrypt_prepare_lookup() will have to handle it too, for the case of
> > dentries
> > in encrypted directories, but that seems okay.
>
> ok, will do. IIUC, we might be able to do without the d_lock
> provided there is no store tearing.
>
> But what was the reason you need the d_lock to set DCACHE_NOKEY_NAME
> during lookup? Is there a race with parallel lookup setting d_flag that
> I couldn't find? Or is it another reason?
d_flags is documented to be protected by d_lock. So for setting
DCACHE_NOKEY_NAME, fs/crypto/ just does the safe thing of taking d_lock. I
never really looked into whether the lock can be skipped there (i.e., whether
anything else can change d_flags while ->lookup is running), since this code
only ran for no-key names, for which performance isn't really important.
This patch would extend that locking to a new context in which it would be
executed several orders of magnitude more often. So, making sure it's properly
optimized becomes more important. It looks like it *might* be the case that
->lookup has exclusive access to d_flags, by virtue of having allocated the
dentry, so I'm just wondering if we can take advantage of that (or whether in
classic VFS fashion there's some edge case where that assumption is wrong).
- Eric
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v5 07/12] libfs: Merge encrypted_ci_dentry_ops and ci_dentry_ops
On Mon, Jan 29, 2024 at 05:43:25PM -0300, Gabriel Krisman Bertazi wrote: > In preparation to get case-insensitive dentry operations from > sb->s_d_op again, use the same structure for case-insensitive > filesystems with and without fscrypt. > > This means that on a casefolded filesystem without fscrypt, we end up Again, we shouldn't use the term "case-insensitive filesystem" (or casefolded filesystem) when we actually mean "case-insensitive capable filesystem", or more precisely "filesystem that supports case-insensitive directories". Real case-insensitive filesystems like FAT are not relevant here. > Also, since the first fscrypt_d_revalidate will call d_set_always_valid for us This is outdated. - Eric ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v5 06/12] fscrypt: Ignore plaintext dentries during d_move
On Mon, Jan 29, 2024 at 05:43:24PM -0300, Gabriel Krisman Bertazi wrote:
> Now that we do more than just clear the DCACHE_NOKEY_NAME in
> fscrypt_handle_d_move, skip it entirely for plaintext dentries, to avoid
> extra costs.
>
> Note that VFS will call this function for any dentry, whether the volume
> has fscrypt on not. But, since we only care about DCACHE_NOKEY_NAME, we
> can check for that, to avoid touching the superblock for other fields
> that identify a fscrypt volume.
>
> Note also that fscrypt_handle_d_move is hopefully inlined back into
> __d_move, so the call cost is not significant. Considering that
> DCACHE_NOKEY_NAME is a fscrypt-specific flag, we do the check in fscrypt
> code instead of the caller.
>
> Signed-off-by: Gabriel Krisman Bertazi
>
> ---
> Changes since v4:
> - Check based on the dentry itself (eric)
> ---
> include/linux/fscrypt.h | 9 +
> 1 file changed, 9 insertions(+)
>
> diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
> index c1e285053b3e..ab668760d63e 100644
> --- a/include/linux/fscrypt.h
> +++ b/include/linux/fscrypt.h
> @@ -232,6 +232,15 @@ static inline bool
> fscrypt_needs_contents_encryption(const struct inode *inode)
> */
> static inline void fscrypt_handle_d_move(struct dentry *dentry)
> {
> + /*
> + * VFS calls fscrypt_handle_d_move even for non-fscrypt
> + * filesystems. Since we only care about DCACHE_NOKEY_NAME
> + * dentries here, check that to bail out quickly, if possible.
> + */
> + if (!(dentry->d_flags & DCACHE_NOKEY_NAME))
> + return;
I think you're over-complicating this a bit. This should be merged with patch
5, since this is basically fixing patch 5, and the end result should look like:
/*
* When d_splice_alias() moves a directory's no-key alias to its plaintext alias
* as a result of the encryption key being added, DCACHE_NOKEY_NAME must be
* cleared and there might be an opportunity to disable d_revalidate. Note that
* we don't have to support the inverse operation because fscrypt doesn't allow
* no-key names to be the source or target of a rename().
*/
static inline void fscrypt_handle_d_move(struct dentry *dentry)
{
if (dentry->d_flags & DCACHE_NOKEY_NAME) {
dentry->d_flags &= ~DCACHE_NOKEY_NAME;
if (dentry->d_op->d_revalidate == fscrypt_d_revalidate)
dentry->d_flags &= ~DCACHE_OP_REVALIDATE;
}
}
Note that checking for NULL dentry->d_op is not necessary, since it's already
been verified that DCACHE_NOKEY_NAME is set, which means fscrypt is in use,
which means that there are dentry_operations.
- Eric
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v5 04/12] fscrypt: Drop d_revalidate for valid dentries during lookup
On Mon, Jan 29, 2024 at 05:43:22PM -0300, Gabriel Krisman Bertazi wrote:
> Unencrypted and encrypted-dentries where the key is available don't need
> to be revalidated with regards to fscrypt, since they don't go stale
> from under VFS and the key cannot be removed for the encrypted case
> without evicting the dentry. Mark them with d_set_always_valid, to
"d_set_always_valid" doesn't appear in the diff itself.
> diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
> index 4aaf847955c0..a22997b9f35c 100644
> --- a/include/linux/fscrypt.h
> +++ b/include/linux/fscrypt.h
> @@ -942,11 +942,22 @@ static inline int fscrypt_prepare_rename(struct inode
> *old_dir,
> static inline void fscrypt_prepare_lookup_dentry(struct dentry *dentry,
>bool is_nokey_name)
> {
> - if (is_nokey_name) {
> - spin_lock(>d_lock);
> + spin_lock(>d_lock);
> +
> + if (is_nokey_name)
> dentry->d_flags |= DCACHE_NOKEY_NAME;
> - spin_unlock(>d_lock);
> + else if (dentry->d_flags & DCACHE_OP_REVALIDATE &&
> + dentry->d_op->d_revalidate == fscrypt_d_revalidate) {
> + /*
> + * Unencrypted dentries and encrypted dentries where the
> + * key is available are always valid from fscrypt
> + * perspective. Avoid the cost of calling
> + * fscrypt_d_revalidate unnecessarily.
> + */
> + dentry->d_flags &= ~DCACHE_OP_REVALIDATE;
> }
> +
> + spin_unlock(>d_lock);
This makes lookups in unencrypted directories start doing the
spin_lock/spin_unlock pair. Is that really necessary?
These changes also make the inline function fscrypt_prepare_lookup() very long
(when including the fscrypt_prepare_lookup_dentry() that's inlined into it).
The rule that I'm trying to follow is that to the extent that the fscrypt helper
functions are inlined, the inline part should be a fast path for unencrypted
directories. Encrypted directories should be handled out-of-line.
So looking at the original fscrypt_prepare_lookup():
static inline int fscrypt_prepare_lookup(struct inode *dir,
struct dentry *dentry,
struct fscrypt_name *fname)
{
if (IS_ENCRYPTED(dir))
return __fscrypt_prepare_lookup(dir, dentry, fname);
memset(fname, 0, sizeof(*fname));
fname->usr_fname = >d_name;
fname->disk_name.name = (unsigned char *)dentry->d_name.name;
fname->disk_name.len = dentry->d_name.len;
return 0;
}
If you could just add the DCACHE_OP_REVALIDATE clearing for dentries in
unencrypted directories just before the "return 0;", hopefully without the
spinlock, that would be good. Yes, that does mean that
__fscrypt_prepare_lookup() will have to handle it too, for the case of dentries
in encrypted directories, but that seems okay.
- Eric
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v5 02/12] fscrypt: Factor out a helper to configure the lookup dentry
On Mon, Jan 29, 2024 at 05:43:20PM -0300, Gabriel Krisman Bertazi wrote:
> Both fscrypt_prepare_lookup_dentry_partial and
> fscrypt_prepare_lookup_dentry will set DCACHE_NOKEY_NAME for dentries
> when the key is not available.
Shouldn't this say: "Both fscrypt_prepare_lookup() and
fscrypt_prepare_lookup_partial() set DCACHE_NOKEY_NAME for dentries when the key
is not available."
> @@ -131,12 +128,13 @@ EXPORT_SYMBOL_GPL(__fscrypt_prepare_lookup);
> int fscrypt_prepare_lookup_partial(struct inode *dir, struct dentry *dentry)
> {
> int err = fscrypt_get_encryption_info(dir, true);
> + bool is_nokey_name = false;
> +
> + if (!err && !fscrypt_has_encryption_key(dir))
> + is_nokey_name = true;
bool is_nokey_name = (err == 0 && !fscrypt_has_encryption_key(dir));
> diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
> index 12f9e455d569..68ca8706483a 100644
> --- a/include/linux/fscrypt.h
> +++ b/include/linux/fscrypt.h
> @@ -948,6 +948,16 @@ static inline int fscrypt_prepare_rename(struct inode
> *old_dir,
> return 0;
> }
>
> +static inline void fscrypt_prepare_lookup_dentry(struct dentry *dentry,
> + bool is_nokey_name)
Maybe just fscrypt_prepare_dentry()?
- Eric
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v5 01/12] ovl: Reject mounting over case-insensitive directories
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
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v3 04/10] fscrypt: Drop d_revalidate once the key is added
On Thu, Jan 25, 2024 at 05:20:56PM -0300, Gabriel Krisman Bertazi wrote:
> Eric Biggers writes:
>
> > On Fri, Jan 19, 2024 at 03:47:36PM -0300, Gabriel Krisman Bertazi wrote:
> >> /*
> >> * When d_splice_alias() moves a directory's no-key alias to its plaintext
> >> alias
> >> * as a result of the encryption key being added, DCACHE_NOKEY_NAME must be
> >> * cleared. Note that we don't have to support arbitrary moves of this
> >> flag
> >> * because fscrypt doesn't allow no-key names to be the source or target
> >> of a
> >> * rename().
> >> */
> >> static inline void fscrypt_handle_d_move(struct dentry *dentry)
> >> {
> >>dentry->d_flags &= ~DCACHE_NOKEY_NAME;
> >> +
> >> + /*
> >> + * Save the d_revalidate call cost during VFS operations. We
> >> + * can do it because, when the key is available, the dentry
> >> + * can't go stale and the key won't go away without eviction.
> >> + */
> >> + if (dentry->d_op && dentry->d_op->d_revalidate == fscrypt_d_revalidate)
> >> + dentry->d_flags &= ~DCACHE_OP_REVALIDATE;
> >> }
> >
> > Is there any way to optimize this further for the case where fscrypt is not
> > being used? This is called unconditionally from d_move(). I've generally
> > been
> > trying to avoid things like this where the fscrypt support slows things
> > down for
> > everyone even when they're not using the feature.
>
> The problem would be figuring out whether the filesystem has fscrypt
> enabled. I think we can rely on sb->s_cop always being set:
>
> if (sb->s_cop)
>fscrypt_handle_d_move(dentry);
>
> What do you think?
That's better, I just wonder if there's an even better way. Do you need to do
this for dentries that don't have DCACHE_NOKEY_NAME set? If not, it would be
more efficient to test for DCACHE_NOKEY_NAME before clearing the flags.
- Eric
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v3 01/10] ovl: Reject mounting case-insensitive filesystems
On Thu, Jan 25, 2024 at 01:55:00PM -0300, Gabriel Krisman Bertazi wrote: > I'm not sure how to change and still make it readable by users. How about: > > return invalfc(fc, "case-insensitive capable filesystem on %s not > supported", name); > > what do you think? "case-insensitive capable" sounds good. - Eric ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH] f2fs: Support enhanced hot/cold data separation for f2fs
On Fri, Jan 26, 2024 at 01:32:05PM -0800, Luis Chamberlain wrote:
> On Fri, Jan 26, 2024 at 09:01:06PM +, Matthew Wilcox wrote:
> > On Thu, Jan 25, 2024 at 12:54:47PM -0800, Luis Chamberlain wrote:
> > > On Thu, Jan 25, 2024 at 08:47:39PM +, Matthew Wilcox wrote:
> > > > On Wed, Dec 07, 2022 at 12:51:13PM -0800, Luis Chamberlain wrote:
> > > > > Me and Pankaj are very interested in helping on this front. And so
> > > > > we'll
> > > > > start to organize and talk every week about this to see what is
> > > > > missing.
> > > > > First order of business however will be testing so we'll have to
> > > > > establish a public baseline to ensure we don't regress. For this we
> > > > > intend
> > > > > on using kdevops so that'll be done first.
> > > > >
> > > > > If folks have patches they want to test in consideration for folio /
> > > > > iomap enhancements feel free to Cc us :)
> > > > >
> > > > > After we establish a baseline we can move forward with taking on tasks
> > > > > which will help with this conversion.
> > > >
> > > > So ... it's been a year. How is this project coming along? There
> > > > weren't a lot of commits to f2fs in 2023 that were folio related.
> > >
> > > The review at LSFMM revealed iomap based filesystems were the priority
> > > and so that has been the priority. Once we tackle that and get XFS
> > > support we can revisit which next fs to help out with. Testing has been
> > > a *huge* part of our endeavor, and naturally getting XFS patches up to
> > > what is required has just taken a bit more time. But you can expect
> > > patches for that within a month or so.
> >
> > Is anyone working on the iomap conversion for f2fs?
>
> It already has been done for direct IO by Eric as per commit a1e09b03e6f5
> ("f2fs: use iomap for direct I/O"), not clear to me if anyone is working
> on buffered-io. Then f2fs_commit_super() seems to be the last buffer-head
> user, and its not clear what the replacement could be yet.
>
> Jaegeuk, Eric, have you guys considered this?
>
Sure, I've *considered* that, along with other requested filesystem
modernization projects such as converting f2fs to use the new mount API and
finishing ext4's conversion to iomap. But, I haven't had time to work on these
projects, nor to get very involved in f2fs beyond what's needed to maintain the
fscrypt and fsverity support. I'm not anywhere close to a full-time filesystem
developer. I did implement the f2fs iomap direct I/O support two years ago
because it made the fscrypt direct I/O support easier. Note that these types of
changes are fairly disruptive, and there were bugs that resulted from my
patches, despite my best efforts. It's necessary for someone to get deeply
involved in these types of changes and follow them all the way through.
- Eric
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v3 06/10] libfs: Add helper to choose dentry operations at mount
On Fri, Jan 19, 2024 at 03:47:38PM -0300, Gabriel Krisman Bertazi wrote: > +/** > + * generic_set_sb_d_ops - helper for choosing the set of > + * filesystem-wide dentry operations for the enabled features > + * @sb: superblock to be configured > + * > + * Filesystems supporting casefolding and/or fscrypt can call this > + * helper at mount-time to configure sb->s_d_root to best set of dentry s_d_op, not s_d_root. - Eric ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v3 04/10] fscrypt: Drop d_revalidate once the key is added
On Fri, Jan 19, 2024 at 03:47:36PM -0300, Gabriel Krisman Bertazi wrote:
> /*
> * When d_splice_alias() moves a directory's no-key alias to its plaintext
> alias
> * as a result of the encryption key being added, DCACHE_NOKEY_NAME must be
> * cleared. Note that we don't have to support arbitrary moves of this flag
> * because fscrypt doesn't allow no-key names to be the source or target of a
> * rename().
> */
> static inline void fscrypt_handle_d_move(struct dentry *dentry)
> {
> dentry->d_flags &= ~DCACHE_NOKEY_NAME;
> +
> + /*
> + * Save the d_revalidate call cost during VFS operations. We
> + * can do it because, when the key is available, the dentry
> + * can't go stale and the key won't go away without eviction.
> + */
> + if (dentry->d_op && dentry->d_op->d_revalidate == fscrypt_d_revalidate)
> + dentry->d_flags &= ~DCACHE_OP_REVALIDATE;
> }
Is there any way to optimize this further for the case where fscrypt is not
being used? This is called unconditionally from d_move(). I've generally been
trying to avoid things like this where the fscrypt support slows things down for
everyone even when they're not using the feature.
In any case, as always please don't let function comments get outdated. Since
it now seems to just describe the DCACHE_NOKEY_NAME clearing and not the whole
function, maybe it should be moved to above that line.
- Eric
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v3 02/10] fscrypt: Share code between functions that prepare lookup
On Fri, Jan 19, 2024 at 03:47:34PM -0300, Gabriel Krisman Bertazi wrote:
> To make the patch simpler, we now call fscrypt_get_encryption_info twice
> for fscrypt_prepare_lookup, once inside fscrypt_setup_filename and once
> inside fscrypt_prepare_lookup_dentry. It seems safe to do, and
> considering it will bail early in the second lookup and most lookups
> should go to the dcache anyway, it doesn't seem problematic for
> performance. In addition, we add a function call for the unencrypted
> case, also during lookup.
Unfortunately I don't think it's correct. This is basically undoing my fix
b01531db6cec ("fscrypt: fix race where ->lookup() marks plaintext dentry as
ciphertext") from several years ago.
When a lookup is done, the filesystem needs to either treat the name being
looked up as a no-key name *or* as a regular name, depending on whether the
directory's key is present. We shouldn't enable race conditions where, due to
the key being concurrently added, the name is treated as a no-key name for
filename matching purposes but a regular name for dentry validation purposes.
That can result in an anomaly where a file that exists ends up with a negative
dentry that doesn't get invalidated.
Basically, the boolean fscrypt_name::is_nokey_name that's produced by
fscrypt_setup_filename() should continue to be propagated to DCACHE_NOKEY_NAME.
- Eric
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v3 01/10] ovl: Reject mounting case-insensitive filesystems
On Fri, Jan 19, 2024 at 03:47:33PM -0300, Gabriel Krisman Bertazi wrote: > ovl: Reject mounting case-insensitive filesystems Overlayfs doesn't mount filesystems. I think you might mean something like reject case-insensitive lowerdirs? > + /* > + * Root dentries of case-insensitive 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 filesystem on %s not > supported", name); sb_has_encoding() doesn't mean that the filesystem is case-insensitive. It means that the filesystem supports individual case-insensitive directories. With that in mind, is this code still working as intended? If so, can you update the comment and error message accordingly? - Eric ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [syzbot] [f2fs?] KASAN: slab-use-after-free Read in destroy_device_list
On Sat, Jan 13, 2024 at 08:59:04PM -0800, syzbot wrote:
> syzbot has bisected this issue to:
>
> commit 275dca4630c165edea9abe27113766bc1173f878
> Author: Eric Biggers
> Date: Wed Dec 27 17:14:28 2023 +
>
> f2fs: move release of block devices to after kill_block_super()
>
> bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=10639913e8
> start commit: 052d534373b7 Merge tag 'exfat-for-6.8-rc1' of git://git.ke..
> git tree: upstream
> final oops: https://syzkaller.appspot.com/x/report.txt?x=12639913e8
> console output: https://syzkaller.appspot.com/x/log.txt?x=14639913e8
> kernel config: https://syzkaller.appspot.com/x/.config?x=878a2a4af11180a7
> dashboard link: https://syzkaller.appspot.com/bug?extid=a5e651ca75fa0260acd5
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=167b0f47e8
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=11255313e8
>
> Reported-by: syzbot+a5e651ca75fa0260a...@syzkaller.appspotmail.com
> Fixes: 275dca4630c1 ("f2fs: move release of block devices to after
> kill_block_super()")
>
> For information about bisection process see: https://goo.gl/tpsmEJ#bisection
>
#syz fix: f2fs: fix double free of f2fs_sb_info
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [syzbot] [f2fs?] KASAN: slab-use-after-free Read in kill_f2fs_super
On Fri, Jan 12, 2024 at 10:38:04PM -0800, syzbot wrote:
> syzbot has bisected this issue to:
>
> commit 275dca4630c165edea9abe27113766bc1173f878
> Author: Eric Biggers
> Date: Wed Dec 27 17:14:28 2023 +
>
> f2fs: move release of block devices to after kill_block_super()
>
> bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=16071613e8
> start commit: 70d201a40823 Merge tag 'f2fs-for-6.8-rc1' of git://git.ker..
> git tree: upstream
> final oops: https://syzkaller.appspot.com/x/report.txt?x=15071613e8
> console output: https://syzkaller.appspot.com/x/log.txt?x=11071613e8
> kernel config: https://syzkaller.appspot.com/x/.config?x=4607bc15d1c4bb90
> dashboard link: https://syzkaller.appspot.com/bug?extid=8f477ac014ff5b32d81f
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=112b660be8
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=14c1df5de8
>
> Reported-by: syzbot+8f477ac014ff5b32d...@syzkaller.appspotmail.com
> Fixes: 275dca4630c1 ("f2fs: move release of block devices to after
> kill_block_super()")
>
> For information about bisection process see: https://goo.gl/tpsmEJ#bisection
#syz fix: f2fs: fix double free of f2fs_sb_info
[GIT PULL] fscrypt fix for 6.8-rc1
The following changes since commit 38814330fedd778edffcabe0c8cb462ee365782e: Merge tag 'devicetree-for-6.8' of git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux (2024-01-12 15:05:30 -0800) are available in the Git repository at: https://git.kernel.org/pub/scm/fs/fscrypt/linux.git tags/fscrypt-for-linus for you to fetch changes up to c919330dd57835970b37676d377de3eaaea2c1e9: f2fs: fix double free of f2fs_sb_info (2024-01-12 18:55:09 -0800) Fix a bug in my change to how f2fs frees its superblock info (which was part of changing the timing of fscrypt keyring destruction). Eric Biggers (1): f2fs: fix double free of f2fs_sb_info fs/f2fs/super.c | 1 + 1 file changed, 1 insertion(+)
Re: [PATCH] f2fs: fix double free of f2fs_sb_info
On Fri, Jan 12, 2024 at 05:28:31PM -0800, Jaegeuk Kim wrote:
> On 01/12, Eric Biggers wrote:
> > On Fri, Jan 12, 2024 at 04:57:47PM -0800, Eric Biggers wrote:
> > > From: Eric Biggers
> > >
> > > kill_f2fs_super() is called even if f2fs_fill_super() fails.
> > > f2fs_fill_super() frees the struct f2fs_sb_info, so it must set
> > > sb->s_fs_info to NULL to prevent it from being freed again.
> > >
> > > Fixes: 275dca4630c1 ("f2fs: move release of block devices to after
> > > kill_block_super()")
> > > Reported-by: syzbot+8f477ac014ff5b32d...@syzkaller.appspotmail.com
> > > Closes: https://lore.kernel.org/r/6cb174060ec34...@google.com
> > > Signed-off-by: Eric Biggers
> >
> > Jaegeuk, I'd be glad to take this through the fscrypt tree since that's
> > where my
>
> Ok, are you heading to push this in -rc1?
>
> > broken commit came from. But let me know if you want to just take this
> > through
> > the f2fs tree.
> >
Yes, we should get this into -rc1.
- Eric
Re: [PATCH] f2fs: fix double free of f2fs_sb_info
On Fri, Jan 12, 2024 at 04:57:47PM -0800, Eric Biggers wrote:
> From: Eric Biggers
>
> kill_f2fs_super() is called even if f2fs_fill_super() fails.
> f2fs_fill_super() frees the struct f2fs_sb_info, so it must set
> sb->s_fs_info to NULL to prevent it from being freed again.
>
> Fixes: 275dca4630c1 ("f2fs: move release of block devices to after
> kill_block_super()")
> Reported-by: syzbot+8f477ac014ff5b32d...@syzkaller.appspotmail.com
> Closes: https://lore.kernel.org/r/6cb174060ec34...@google.com
> Signed-off-by: Eric Biggers
Jaegeuk, I'd be glad to take this through the fscrypt tree since that's where my
broken commit came from. But let me know if you want to just take this through
the f2fs tree.
- Eric
[PATCH] f2fs: fix double free of f2fs_sb_info
From: Eric Biggers
kill_f2fs_super() is called even if f2fs_fill_super() fails.
f2fs_fill_super() frees the struct f2fs_sb_info, so it must set
sb->s_fs_info to NULL to prevent it from being freed again.
Fixes: 275dca4630c1 ("f2fs: move release of block devices to after
kill_block_super()")
Reported-by: syzbot+8f477ac014ff5b32d...@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/r/6cb174060ec34...@google.com
Signed-off-by: Eric Biggers
---
fs/f2fs/super.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index d00d21a8b53ad..d45ab0992ae59 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -4873,20 +4873,21 @@ static int f2fs_fill_super(struct super_block *sb, void
*data, int silent)
kfree(F2FS_OPTION(sbi).s_qf_names[i]);
#endif
fscrypt_free_dummy_policy(_OPTION(sbi).dummy_enc_policy);
kvfree(options);
free_sb_buf:
kfree(raw_super);
free_sbi:
if (sbi->s_chksum_driver)
crypto_free_shash(sbi->s_chksum_driver);
kfree(sbi);
+ sb->s_fs_info = NULL;
/* give only one another chance */
if (retry_cnt > 0 && skip_recovery) {
retry_cnt--;
shrink_dcache_sb(sb);
goto try_onemore;
}
return err;
}
base-commit: 38814330fedd778edffcabe0c8cb462ee365782e
--
2.43.0
Re: [f2fs-dev] [syzbot] [f2fs?] KASAN: slab-use-after-free Read in kill_f2fs_super
On Fri, Jan 12, 2024 at 04:32:21PM -0800, syzbot wrote: > loop0: detected capacity change from 0 to 63271 > F2FS-fs (loop0): Mismatch start address, segment0(512) cp_blkaddr(605) > F2FS-fs (loop0): Can't find valid F2FS filesystem in 1th superblock > F2FS-fs (loop0): invalid crc value > F2FS-fs (loop0): SIT is corrupted node# 0 vs 1 > F2FS-fs (loop0): Failed to initialize F2FS segment manager (-117) > == > BUG: KASAN: slab-use-after-free in destroy_device_list fs/f2fs/super.c:1606 > [inline] > BUG: KASAN: slab-use-after-free in kill_f2fs_super+0x618/0x690 > fs/f2fs/super.c:4932 > Read of size 4 at addr 888023bdd77c by task syz-executor275/5046 Sorry, this is my fault. I'll fix this. - Eric ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [PATCH v2 0/4] xfstests: test custom crypto data unit size
On Tue, Nov 21, 2023 at 02:39:05PM -0800, Eric Biggers wrote:
> This series adds a test that verifies the on-disk format of encrypted
> files that use a crypto data unit size that differs from the filesystem
> block size. This tests the functionality that was introduced in Linux
> 6.7 by kernel commit 5b1188847180 ("fscrypt: support crypto data unit
> size less than filesystem block size").
Hi Zorro, can you consider applying this series? It's been out for review for
3 months, so I don't think reviews are going to come in. The prerequisite
xfsprogs patch is already present on the for-next branch of xfsprogs.
Thanks!
- Eric
[f2fs-dev] [GIT PULL] fscrypt updates for 6.8
The following changes since commit 33cc938e65a98f1d29d0a18403dbbee050dcad9a: Linux 6.7-rc4 (2023-12-03 18:52:56 +0900) are available in the Git repository at: https://git.kernel.org/pub/scm/fs/fscrypt/linux.git tags/fscrypt-for-linus for you to fetch changes up to 2a0e85719892a1d63f8f287563e2c1778a77879e: fs: move fscrypt keyring destruction to after ->put_super (2023-12-27 21:56:01 -0600) Adjust the timing of the fscrypt keyring destruction, to prepare for btrfs's fscrypt support. Also document that CephFS supports fscrypt now. ---- Eric Biggers (4): fscrypt.rst: update definition of struct fscrypt_context_v2 fscrypt: update comment for do_remove_key() fscrypt: document that CephFS supports fscrypt now f2fs: move release of block devices to after kill_block_super() Josef Bacik (1): fs: move fscrypt keyring destruction to after ->put_super Documentation/filesystems/fscrypt.rst | 21 +++-- fs/crypto/Kconfig | 2 +- fs/crypto/keyring.c | 6 +++--- fs/f2fs/super.c | 13 - fs/super.c| 12 ++-- 5 files changed, 29 insertions(+), 25 deletions(-) ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
[PATCH v2 2/2] fs: move fscrypt keyring destruction to after ->put_super
From: Josef Bacik
btrfs has a variety of asynchronous things we do with inodes that can
potentially last until ->put_super, when we shut everything down and
clean up all of our async work. Due to this we need to move
fscrypt_destroy_keyring() to after ->put_super, otherwise we get
warnings about still having active references on the master key.
Signed-off-by: Josef Bacik
Reviewed-by: Christoph Hellwig
Reviewed-by: Neal Gompa
Signed-off-by: Eric Biggers
---
fs/super.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/fs/super.c b/fs/super.c
index 076392396e724..faf7d248145d2 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -674,34 +674,34 @@ void generic_shutdown_super(struct super_block *sb)
/* Evict all inodes with zero refcount. */
evict_inodes(sb);
/*
* Clean up and evict any inodes that still have references due
* to fsnotify or the security policy.
*/
fsnotify_sb_delete(sb);
security_sb_delete(sb);
- /*
-* Now that all potentially-encrypted inodes have been evicted,
-* the fscrypt keyring can be destroyed.
-*/
- fscrypt_destroy_keyring(sb);
-
if (sb->s_dio_done_wq) {
destroy_workqueue(sb->s_dio_done_wq);
sb->s_dio_done_wq = NULL;
}
if (sop->put_super)
sop->put_super(sb);
+ /*
+* Now that all potentially-encrypted inodes have been evicted,
+* the fscrypt keyring can be destroyed.
+*/
+ fscrypt_destroy_keyring(sb);
+
if (CHECK_DATA_CORRUPTION(!list_empty(>s_inodes),
"VFS: Busy inodes after unmount of %s (%s)",
sb->s_id, sb->s_type->name)) {
/*
* Adding a proper bailout path here would be hard, but
* we can at least make it more likely that a later
* iput_final() or such crashes cleanly.
*/
struct inode *inode;
--
2.43.0
[PATCH v2 1/2] f2fs: move release of block devices to after kill_block_super()
From: Eric Biggers
Call destroy_device_list() and free the f2fs_sb_info from
kill_f2fs_super(), after the call to kill_block_super(). This is
necessary to order it after the call to fscrypt_destroy_keyring() once
generic_shutdown_super() starts calling fscrypt_destroy_keyring() just
after calling ->put_super. This is because fscrypt_destroy_keyring()
may call into f2fs_get_devices() via the fscrypt_operations.
Signed-off-by: Eric Biggers
---
fs/f2fs/super.c | 13 -
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 033af907c3b1d..d66e0692ac02e 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -1710,42 +1710,39 @@ static void f2fs_put_super(struct super_block *sb)
f2fs_destroy_node_manager(sbi);
f2fs_destroy_segment_manager(sbi);
/* flush s_error_work before sbi destroy */
flush_work(>s_error_work);
f2fs_destroy_post_read_wq(sbi);
kvfree(sbi->ckpt);
- sb->s_fs_info = NULL;
if (sbi->s_chksum_driver)
crypto_free_shash(sbi->s_chksum_driver);
kfree(sbi->raw_super);
- destroy_device_list(sbi);
f2fs_destroy_page_array_cache(sbi);
f2fs_destroy_xattr_caches(sbi);
mempool_destroy(sbi->write_io_dummy);
#ifdef CONFIG_QUOTA
for (i = 0; i < MAXQUOTAS; i++)
kfree(F2FS_OPTION(sbi).s_qf_names[i]);
#endif
fscrypt_free_dummy_policy(_OPTION(sbi).dummy_enc_policy);
destroy_percpu_info(sbi);
f2fs_destroy_iostat(sbi);
for (i = 0; i < NR_PAGE_TYPE; i++)
kvfree(sbi->write_io[i]);
#if IS_ENABLED(CONFIG_UNICODE)
utf8_unload(sb->s_encoding);
#endif
- kfree(sbi);
}
int f2fs_sync_fs(struct super_block *sb, int sync)
{
struct f2fs_sb_info *sbi = F2FS_SB(sb);
int err = 0;
if (unlikely(f2fs_cp_error(sbi)))
return 0;
if (unlikely(is_sbi_flag_set(sbi, SBI_CP_DISABLED)))
@@ -4895,23 +4892,23 @@ static int f2fs_fill_super(struct super_block *sb, void
*data, int silent)
}
static struct dentry *f2fs_mount(struct file_system_type *fs_type, int flags,
const char *dev_name, void *data)
{
return mount_bdev(fs_type, flags, dev_name, data, f2fs_fill_super);
}
static void kill_f2fs_super(struct super_block *sb)
{
- if (sb->s_root) {
- struct f2fs_sb_info *sbi = F2FS_SB(sb);
+ struct f2fs_sb_info *sbi = F2FS_SB(sb);
+ if (sb->s_root) {
set_sbi_flag(sbi, SBI_IS_CLOSE);
f2fs_stop_gc_thread(sbi);
f2fs_stop_discard_thread(sbi);
#ifdef CONFIG_F2FS_FS_COMPRESSION
/*
* latter evict_inode() can bypass checking and invalidating
* compress inode cache.
*/
if (test_opt(sbi, COMPRESS_CACHE))
@@ -4924,20 +4921,26 @@ static void kill_f2fs_super(struct super_block *sb)
.reason = CP_UMOUNT,
};
stat_inc_cp_call_count(sbi, TOTAL_CALL);
f2fs_write_checkpoint(sbi, );
}
if (is_sbi_flag_set(sbi, SBI_IS_RECOVERED) && f2fs_readonly(sb))
sb->s_flags &= ~SB_RDONLY;
}
kill_block_super(sb);
+ /* Release block devices last, after fscrypt_destroy_keyring(). */
+ if (sbi) {
+ destroy_device_list(sbi);
+ kfree(sbi);
+ sb->s_fs_info = NULL;
+ }
}
static struct file_system_type f2fs_fs_type = {
.owner = THIS_MODULE,
.name = "f2fs",
.mount = f2fs_mount,
.kill_sb= kill_f2fs_super,
.fs_flags = FS_REQUIRES_DEV | FS_ALLOW_IDMAP,
};
MODULE_ALIAS_FS("f2fs");
--
2.43.0
[PATCH v2 0/2] Move fscrypt keyring destruction to after ->put_super
This series moves the fscrypt keyring destruction to after ->put_super, as this will be needed by the btrfs fscrypt support. To make this possible, it also changes f2fs to release its block devices after generic_shutdown_super() rather than before. This supersedes "[PATCH] fscrypt: move the call to fscrypt_destroy_keyring() into ->put_super()" (https://lore.kernel.org/linux-fscrypt/20231206001325.13676-1-ebigg...@kernel.org/T/#u) Changed in v2: - Added a comment to f2fs patch. - Dropped btrfs patch from series; it will go in separately. - Added some Reviewed-bys. Eric Biggers (1): f2fs: move release of block devices to after kill_block_super() Josef Bacik (1): fs: move fscrypt keyring destruction to after ->put_super fs/f2fs/super.c | 13 - fs/super.c | 12 ++-- 2 files changed, 14 insertions(+), 11 deletions(-) base-commit: fbafc3e621c3f4ded43720fdb1d6ce1728ec664e -- 2.43.0
Re: [f2fs-dev] [PATCH 2/3] f2fs: move release of block devices to after kill_block_super()
On Tue, Dec 12, 2023 at 08:00:17PM -0800, Eric Biggers wrote: > From: Eric Biggers > > Call destroy_device_list() and free the f2fs_sb_info from > kill_f2fs_super(), after the call to kill_block_super(). This is > necessary to order it after the call to fscrypt_destroy_keyring() once > generic_shutdown_super() starts calling fscrypt_destroy_keyring() just > after calling ->put_super. This is because fscrypt_destroy_keyring() > may call into f2fs_get_devices() via the fscrypt_operations. > > Signed-off-by: Eric Biggers > --- > fs/f2fs/super.c | 12 +++- > 1 file changed, 7 insertions(+), 5 deletions(-) Jaegeuk and Chao, when you have a chance can you review or ack this? I'm thinking of taking patches 2-3 of this series through the fscrypt tree for 6.8. - Eric ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v2 8/8] fscrypt: Move d_revalidate configuration back into fscrypt
On Thu, Dec 21, 2023 at 07:39:40AM +, Al Viro wrote: > Hmm... Could we simply set ->s_d_op to _dentry_ops in non-ci case > *AND* have __fscrypt_prepare_lookup() clear DCACHE_OP_REVALIDATE in case > when it's not setting DCACHE_NOKEY_NAME and ->d_op->d_revalidate is > equal to fscrypt_d_revalidate? I mean, > > spin_lock(>d_lock); > if (fname->is_nokey_name) > dentry->d_flags |= DCACHE_NOKEY_NAME; > else if (dentry->d_flags & DCACHE_OP_REVALIDATE && >dentry->d_op->d_revalidate == fscrypt_d_revalidate) > dentry->d_flags &= ~DCACHE_OP_REVALIDATE; > spin_unlock(>d_lock); > > here + always set ->s_d_op for ext4 and friends (conditional upon > the CONFIG_UNICODE). > > No encryption - fine, you get ->is_nokey_name false from the very > beginning, DCACHE_OP_REVALIDATE is cleared and VFS won't ever call > ->d_revalidate(); not even the first time. > > Yes, you pay minimal price in dentry_unlink_inode() when we hit > if (dentry->d_op && dentry->d_op->d_iput) > and bugger off after the second fetch instead of the first one. > I would be quite surprised if it turns out to be measurable, > but if it is, we can always add DCACHE_OP_IPUT to flags. > Similar for ->d_op->d_release (called in the end of > __dentry_kill()). Again, that only makes sense if we get > a measurable overhead from that. fscrypt_prepare_lookup() handles unencrypted directories inline, without calling __fscrypt_prepare_lookup() which is only for encrypted directories. So the logic to clear DCACHE_OP_REVALIDATE would need to be there too. - Eric ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v2 0/8] Revert setting casefolding dentry operations through s_d_op
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). - Eric ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v2 8/8] fscrypt: Move d_revalidate configuration back into fscrypt
On Fri, Dec 15, 2023 at 04:16:08PM -0500, Gabriel Krisman Bertazi wrote:
> int __fscrypt_prepare_lookup(struct inode *dir, struct dentry *dentry,
>struct fscrypt_name *fname)
> {
> @@ -106,6 +110,10 @@ int __fscrypt_prepare_lookup(struct inode *dir, struct
> dentry *dentry,
> spin_lock(>d_lock);
> dentry->d_flags |= DCACHE_NOKEY_NAME;
> spin_unlock(>d_lock);
> +
> + /* Give preference to the filesystem hooks, if any. */
> + if (!dentry->d_op)
> + d_set_d_op(dentry, _dentry_ops);
I think that fscrypt_prepare_lookup_partial() should get this too, since it sets
DCACHE_NOKEY_NAME too (though currently the only filesystem that calls it has
its own d_revalidate anyway).
- Eric
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v2 2/8] fscrypt: Drop d_revalidate if key is available
On Fri, Dec 15, 2023 at 04:16:02PM -0500, Gabriel Krisman Bertazi wrote:
> fscrypt dentries are always valid once the key is available. Since the
> key cannot be removed without evicting the dentry, we don't need to keep
> retrying to revalidate it.
>
> Signed-off-by: Gabriel Krisman Bertazi
IIUC, this patch minimizes the overhead of fscrypt_d_revalidate() both for
encrypted and unencrypted dentries. That's what's needed (seeing as this series
makes fscrypt_d_revalidate be installed on unencrypted dentries), but the commit
message only mentions the encrypted case. It would be helpful to mention both.
> ---
> fs/crypto/fname.c | 9 -
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c
> index 7b3fc189593a..0457ba2d7d76 100644
> --- a/fs/crypto/fname.c
> +++ b/fs/crypto/fname.c
> @@ -591,8 +591,15 @@ int fscrypt_d_revalidate(struct dentry *dentry, unsigned
> int flags)
>* reverting to no-key names without evicting the directory's inode
>* -- which implies eviction of the dentries in the directory.
>*/
> - if (!(dentry->d_flags & DCACHE_NOKEY_NAME))
> + if (!(dentry->d_flags & DCACHE_NOKEY_NAME)) {
> + /*
> + * If fscrypt is the only feature requiring
> + * revalidation for this dentry, we can just disable it.
> + */
> + if (dentry->d_op->d_revalidate == _d_revalidate)
No need for the & in _d_revalidate.
- Eric
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v2 4/8] libfs: Expose generic_ci_dentry_ops outside of libfs
On Fri, Dec 15, 2023 at 04:16:04PM -0500, Gabriel Krisman Bertazi wrote:
> In preparation to allow filesystems to set this through sb->s_d_op,
> expose the symbol directly to the filesystems.
>
> Signed-off-by: Gabriel Krisman Bertazi
> ---
> fs/libfs.c | 2 +-
> include/linux/fs.h | 1 +
> 2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/fs/libfs.c b/fs/libfs.c
> index 52c944173e57..b8ecada3a5b2 100644
> --- a/fs/libfs.c
> +++ b/fs/libfs.c
> @@ -1765,7 +1765,7 @@ static int generic_ci_d_hash(const struct dentry
> *dentry, struct qstr *str)
> return 0;
> }
>
> -static const struct dentry_operations generic_ci_dentry_ops = {
> +const struct dentry_operations generic_ci_dentry_ops = {
> .d_hash = generic_ci_d_hash,
> .d_compare = generic_ci_d_compare,
This needs an EXPORT_SYMBOL_GPL(), since the filesystems that will use this can
be loadable modules.
- Eric
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v2 3/8] libfs: Merge encrypted_ci_dentry_ops and ci_dentry_ops
On Fri, Dec 15, 2023 at 04:16:03PM -0500, Gabriel Krisman Bertazi wrote: > +#if defined(CONFIG_FS_ENCRYPTION) > + .d_revalidate = fscrypt_d_revalidate, > +#endif #ifdef CONFIG_FS_ENCRYPTION, since it's a bool. - Eric ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
[f2fs-dev] [PATCH 3/3] fs: move fscrypt keyring destruction to after ->put_super
From: Josef Bacik
btrfs has a variety of asynchronous things we do with inodes that can
potentially last until ->put_super, when we shut everything down and
clean up all of our async work. Due to this we need to move
fscrypt_destroy_keyring() to after ->put_super, otherwise we get
warnings about still having active references on the master key.
Signed-off-by: Josef Bacik
Signed-off-by: Eric Biggers
---
fs/super.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/fs/super.c b/fs/super.c
index 076392396e724..faf7d248145d2 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -674,34 +674,34 @@ void generic_shutdown_super(struct super_block *sb)
/* Evict all inodes with zero refcount. */
evict_inodes(sb);
/*
* Clean up and evict any inodes that still have references due
* to fsnotify or the security policy.
*/
fsnotify_sb_delete(sb);
security_sb_delete(sb);
- /*
-* Now that all potentially-encrypted inodes have been evicted,
-* the fscrypt keyring can be destroyed.
-*/
- fscrypt_destroy_keyring(sb);
-
if (sb->s_dio_done_wq) {
destroy_workqueue(sb->s_dio_done_wq);
sb->s_dio_done_wq = NULL;
}
if (sop->put_super)
sop->put_super(sb);
+ /*
+* Now that all potentially-encrypted inodes have been evicted,
+* the fscrypt keyring can be destroyed.
+*/
+ fscrypt_destroy_keyring(sb);
+
if (CHECK_DATA_CORRUPTION(!list_empty(>s_inodes),
"VFS: Busy inodes after unmount of %s (%s)",
sb->s_id, sb->s_type->name)) {
/*
* Adding a proper bailout path here would be hard, but
* we can at least make it more likely that a later
* iput_final() or such crashes cleanly.
*/
struct inode *inode;
--
2.43.0
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
[f2fs-dev] [PATCH 0/3] Move fscrypt keyring destruction to after ->put_super
This series moves the fscrypt keyring destruction to after ->put_super, as this will be needed by the btrfs fscrypt support. To make this possible, it also changes btrfs and f2fs to release their block devices after generic_shutdown_super() rather than before. This supersedes "[PATCH] fscrypt: move the call to fscrypt_destroy_keyring() into ->put_super()" (https://lore.kernel.org/linux-fscrypt/20231206001325.13676-1-ebigg...@kernel.org/T/#u) This applies to v6.7-rc5. Christoph Hellwig (1): btrfs: call btrfs_close_devices from ->kill_sb Eric Biggers (1): f2fs: move release of block devices to after kill_block_super() Josef Bacik (1): fs: move fscrypt keyring destruction to after ->put_super fs/btrfs/disk-io.c | 4 ++-- fs/btrfs/super.c | 7 ++- fs/f2fs/super.c| 12 +++- fs/super.c | 12 ++-- 4 files changed, 17 insertions(+), 18 deletions(-) base-commit: a39b6ac3781d46ba18193c9dbb2110f31e9bffe9 -- 2.43.0 ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
[f2fs-dev] [PATCH 1/3] btrfs: call btrfs_close_devices from ->kill_sb
From: Christoph Hellwig
blkdev_put must not be called under sb->s_umount to avoid a lock order
reversal with disk->open_mutex once call backs from block devices to
the file system using the holder ops are supported. Move the call
to btrfs_close_devices into btrfs_free_fs_info so that it is closed
from ->kill_sb (which is also called from the mount failure handling
path unlike ->put_super) as well as when an fs_info is freed because
an existing superblock already exists.
Signed-off-by: Christoph Hellwig
Reviewed-by: Christian Brauner
Signed-off-by: Eric Biggers
---
fs/btrfs/disk-io.c | 4 ++--
fs/btrfs/super.c | 7 ++-
2 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index bbcc3df776461..fe98e6b1d9c61 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1237,20 +1237,22 @@ static void free_global_roots(struct btrfs_fs_info
*fs_info)
while ((node = rb_first_postorder(_info->global_root_tree)) != NULL)
{
root = rb_entry(node, struct btrfs_root, rb_node);
rb_erase(>rb_node, _info->global_root_tree);
btrfs_put_root(root);
}
}
void btrfs_free_fs_info(struct btrfs_fs_info *fs_info)
{
+ if (fs_info->fs_devices)
+ btrfs_close_devices(fs_info->fs_devices);
percpu_counter_destroy(_info->dirty_metadata_bytes);
percpu_counter_destroy(_info->delalloc_bytes);
percpu_counter_destroy(_info->ordered_bytes);
percpu_counter_destroy(_info->dev_replace.bio_counter);
btrfs_free_csum_hash(fs_info);
btrfs_free_stripe_hash_table(fs_info);
btrfs_free_ref_cache(fs_info);
kfree(fs_info->balance_ctl);
kfree(fs_info->delayed_root);
free_global_roots(fs_info);
@@ -3605,21 +3607,20 @@ int __cold open_ctree(struct super_block *sb, struct
btrfs_fs_devices *fs_device
invalidate_inode_pages2(fs_info->btree_inode->i_mapping);
fail_sb_buffer:
btrfs_stop_all_workers(fs_info);
btrfs_free_block_groups(fs_info);
fail_alloc:
btrfs_mapping_tree_free(_info->mapping_tree);
iput(fs_info->btree_inode);
fail:
- btrfs_close_devices(fs_info->fs_devices);
ASSERT(ret < 0);
return ret;
}
ALLOW_ERROR_INJECTION(open_ctree, ERRNO);
static void btrfs_end_super_write(struct bio *bio)
{
struct btrfs_device *device = bio->bi_private;
struct bio_vec *bvec;
struct bvec_iter_all iter_all;
@@ -4385,21 +4386,20 @@ void __cold close_ctree(struct btrfs_fs_info *fs_info)
* have had an IO error and have left over tree log blocks that aren't
* cleaned up until the fs roots are freed. This makes the block group
* accounting appear to be wrong because there's pending reserved bytes,
* so make sure we do the block group cleanup afterwards.
*/
btrfs_free_block_groups(fs_info);
iput(fs_info->btree_inode);
btrfs_mapping_tree_free(_info->mapping_tree);
- btrfs_close_devices(fs_info->fs_devices);
}
void btrfs_mark_buffer_dirty(struct btrfs_trans_handle *trans,
struct extent_buffer *buf)
{
struct btrfs_fs_info *fs_info = buf->fs_info;
u64 transid = btrfs_header_generation(buf);
#ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
/*
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index ef256b944c72a..9616ce63c5630 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -1450,55 +1450,52 @@ static struct dentry *btrfs_mount_root(struct
file_system_type *fs_type,
fs_devices = device->fs_devices;
fs_info->fs_devices = fs_devices;
error = btrfs_open_devices(fs_devices, mode, fs_type);
mutex_unlock(_mutex);
if (error)
goto error_fs_info;
if (!(flags & SB_RDONLY) && fs_devices->rw_devices == 0) {
error = -EACCES;
- goto error_close_devices;
+ goto error_fs_info;
}
bdev = fs_devices->latest_dev->bdev;
s = sget(fs_type, btrfs_test_super, btrfs_set_super, flags | SB_NOSEC,
fs_info);
if (IS_ERR(s)) {
error = PTR_ERR(s);
- goto error_close_devices;
+ goto error_fs_info;
}
if (s->s_root) {
- btrfs_close_devices(fs_devices);
btrfs_free_fs_info(fs_info);
if ((flags ^ s->s_flags) & SB_RDONLY)
error = -EBUSY;
} else {
snprintf(s->s_id, sizeof(s->s_id), "%pg", bdev);
shrinker_debugfs_rename(s->s_shrink, "sb-%s:%s", fs_type->name,
s->s_id);
btrfs_sb(s)->bdev_holder = fs_type;
[f2fs-dev] [PATCH 2/3] f2fs: move release of block devices to after kill_block_super()
From: Eric Biggers
Call destroy_device_list() and free the f2fs_sb_info from
kill_f2fs_super(), after the call to kill_block_super(). This is
necessary to order it after the call to fscrypt_destroy_keyring() once
generic_shutdown_super() starts calling fscrypt_destroy_keyring() just
after calling ->put_super. This is because fscrypt_destroy_keyring()
may call into f2fs_get_devices() via the fscrypt_operations.
Signed-off-by: Eric Biggers
---
fs/f2fs/super.c | 12 +++-
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 033af907c3b1d..ba95a341a9a36 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -1710,42 +1710,39 @@ static void f2fs_put_super(struct super_block *sb)
f2fs_destroy_node_manager(sbi);
f2fs_destroy_segment_manager(sbi);
/* flush s_error_work before sbi destroy */
flush_work(>s_error_work);
f2fs_destroy_post_read_wq(sbi);
kvfree(sbi->ckpt);
- sb->s_fs_info = NULL;
if (sbi->s_chksum_driver)
crypto_free_shash(sbi->s_chksum_driver);
kfree(sbi->raw_super);
- destroy_device_list(sbi);
f2fs_destroy_page_array_cache(sbi);
f2fs_destroy_xattr_caches(sbi);
mempool_destroy(sbi->write_io_dummy);
#ifdef CONFIG_QUOTA
for (i = 0; i < MAXQUOTAS; i++)
kfree(F2FS_OPTION(sbi).s_qf_names[i]);
#endif
fscrypt_free_dummy_policy(_OPTION(sbi).dummy_enc_policy);
destroy_percpu_info(sbi);
f2fs_destroy_iostat(sbi);
for (i = 0; i < NR_PAGE_TYPE; i++)
kvfree(sbi->write_io[i]);
#if IS_ENABLED(CONFIG_UNICODE)
utf8_unload(sb->s_encoding);
#endif
- kfree(sbi);
}
int f2fs_sync_fs(struct super_block *sb, int sync)
{
struct f2fs_sb_info *sbi = F2FS_SB(sb);
int err = 0;
if (unlikely(f2fs_cp_error(sbi)))
return 0;
if (unlikely(is_sbi_flag_set(sbi, SBI_CP_DISABLED)))
@@ -4895,23 +4892,23 @@ static int f2fs_fill_super(struct super_block *sb, void
*data, int silent)
}
static struct dentry *f2fs_mount(struct file_system_type *fs_type, int flags,
const char *dev_name, void *data)
{
return mount_bdev(fs_type, flags, dev_name, data, f2fs_fill_super);
}
static void kill_f2fs_super(struct super_block *sb)
{
- if (sb->s_root) {
- struct f2fs_sb_info *sbi = F2FS_SB(sb);
+ struct f2fs_sb_info *sbi = F2FS_SB(sb);
+ if (sb->s_root) {
set_sbi_flag(sbi, SBI_IS_CLOSE);
f2fs_stop_gc_thread(sbi);
f2fs_stop_discard_thread(sbi);
#ifdef CONFIG_F2FS_FS_COMPRESSION
/*
* latter evict_inode() can bypass checking and invalidating
* compress inode cache.
*/
if (test_opt(sbi, COMPRESS_CACHE))
@@ -4924,20 +4921,25 @@ static void kill_f2fs_super(struct super_block *sb)
.reason = CP_UMOUNT,
};
stat_inc_cp_call_count(sbi, TOTAL_CALL);
f2fs_write_checkpoint(sbi, );
}
if (is_sbi_flag_set(sbi, SBI_IS_RECOVERED) && f2fs_readonly(sb))
sb->s_flags &= ~SB_RDONLY;
}
kill_block_super(sb);
+ if (sbi) {
+ destroy_device_list(sbi);
+ kfree(sbi);
+ sb->s_fs_info = NULL;
+ }
}
static struct file_system_type f2fs_fs_type = {
.owner = THIS_MODULE,
.name = "f2fs",
.mount = f2fs_mount,
.kill_sb= kill_f2fs_super,
.fs_flags = FS_REQUIRES_DEV | FS_ALLOW_IDMAP,
};
MODULE_ALIAS_FS("f2fs");
--
2.43.0
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v2] f2fs: Restrict max filesize for 16K f2fs
On Mon, Dec 04, 2023 at 03:46:15PM -0800, Daniel Rosenberg via Linux-f2fs-devel
wrote:
> Blocks are tracked by u32, so the max permitted filesize is
> U32_MAX * BLOCK_SIZE. Additionally, in order to support crypto data unit
> sizes of 4K with a 16K block size with IV_INO_LBLK_{32,63}, we must
{32,63} should be {32,64}
> + /*
> + * For compatibility with FSCRYPT_POLICY_IV_INO_LBLK_{64,32} with a
> + * 4K crypto data unit, we must restrict the max filesize to what can
> + * fit within U32_MAX data units.
FSCRYPT_POLICY_IV_INO_LBLK_{64,32} should be
FSCRYPT_POLICY_FLAG_IV_INO_LBLK_{64,32}
> + *
> + * Since the blocksize must currently be equal to the page size,
> + * we can use a constant for that. Note if this is not the case
> + * in the future that inode is NULL while setting up the superblock.
I'm not sure what the last sentence is trying to say.
> + */
> +
> + result = min(result, ((loff_t) U32_MAX * 4096) >> F2FS_BLKSIZE_BITS);
Is it intentional that this is off by 1? If indices can be up to U32_MAX, then
the maximum size is U32_MAX + 1. It's not a bad idea to go with the lower size,
so that max_index + 1 does not overflow. But that's not what the explanation
says, so this seems to be accidental.
- Eric
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH] f2fs: Restrict max filesize for 16K f2fs
On Fri, Dec 01, 2023 at 07:03:47PM -0800, Daniel Rosenberg via Linux-f2fs-devel
wrote:
> + /* For compatibility with 4K crypto unit size, we must restrict
> + * the max filesize to what can fit within U32_MAX 4K units.
> + * Since the blocksize must currently be equal to the page size,
> + * we can grab that from there. inode is NULL when setting up
> + * the superblock.
> + */
> +
> + result = min(result, ((loff_t) U32_MAX * 4096) >> PAGE_SHIFT);
This should be formatted like:
/*
* For compatibility with ...
*/
result = ...
Also, the comment should mention that this is for the IV_INO_LBLK_64 and
IV_INO_LBLK_32 encryption settings. There is no issue with the default
encryption setting.
Also, use F2FS_BLKSIZE_BITS instead of PAGE_SHIFT?
Also, the commit message could use:
Fixes: d7e9a9037de2 ("f2fs: Support Block Size == Page Size")
- Eric
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
[PATCH v2 4/4] generic: add test for custom crypto data unit size
From: Eric Biggers
Add a test that verifies the on-disk format of encrypted files that use
a crypto data unit size that differs from the filesystem block size.
This tests the functionality that was introduced in Linux 6.7 by kernel
commit 5b1188847180 ("fscrypt: support crypto data unit size less than
filesystem block size").
This depends on the xfsprogs patch
"xfs_io/encrypt: support specifying crypto data unit size"
(https://lore.kernel.org/r/20231013062639.141468-1-ebigg...@kernel.org)
which adds the '-s' option to the set_encpolicy command of xfs_io.
As usual, the test skips itself when any prerequisite isn't met.
Signed-off-by: Eric Biggers
---
tests/generic/900 | 29 +
tests/generic/900.out | 11 +++
2 files changed, 40 insertions(+)
create mode 100755 tests/generic/900
create mode 100644 tests/generic/900.out
diff --git a/tests/generic/900 b/tests/generic/900
new file mode 100755
index ..8d1b5766
--- /dev/null
+++ b/tests/generic/900
@@ -0,0 +1,29 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright 2023 Google LLC
+#
+# FS QA Test No. generic/900
+#
+# Verify the on-disk format of encrypted files that use a crypto data unit size
+# that differs from the filesystem block size. This tests the functionality
+# that was introduced in Linux 6.7 by kernel commit 5b1188847180
+# ("fscrypt: support crypto data unit size less than filesystem block size").
+#
+. ./common/preamble
+_begin_fstest auto quick encrypt
+
+. ./common/filter
+. ./common/encrypt
+
+_supported_fs generic
+
+# For now, just test 512-byte and 1024-byte data units. Filesystems accept
+# power-of-2 sizes between 512 and the filesystem block size, inclusively.
+# Testing 512 and 1024 ensures this test will run for any FS block size >= 1024
+# (provided that the filesystem supports sub-block data units at all).
+_verify_ciphertext_for_encryption_policy AES-256-XTS AES-256-CTS-CBC v2
log2_dusize=9
+_verify_ciphertext_for_encryption_policy AES-256-XTS AES-256-CTS-CBC v2
log2_dusize=10
+
+# success, all done
+status=0
+exit
diff --git a/tests/generic/900.out b/tests/generic/900.out
new file mode 100644
index ..3259f08c
--- /dev/null
+++ b/tests/generic/900.out
@@ -0,0 +1,11 @@
+QA output created by 900
+
+Verifying ciphertext with parameters:
+ contents_encryption_mode: AES-256-XTS
+ filenames_encryption_mode: AES-256-CTS-CBC
+ options: v2 log2_dusize=9
+
+Verifying ciphertext with parameters:
+ contents_encryption_mode: AES-256-XTS
+ filenames_encryption_mode: AES-256-CTS-CBC
+ options: v2 log2_dusize=10
--
2.42.1
[PATCH v2 3/4] common/encrypt: support custom data unit size
From: Eric Biggers
Make _require_scratch_encryption() and
_require_encryption_policy_support() support the new '-s' option to
set_encpolicy to specify a custom value of log2_data_unit_size.
Likewise, make _verify_ciphertext_for_encryption_policy() accept an
argument "log2_dusize=*" to cause it to use the specified data unit size
for the test and verify that the file contents are encrypted as expected
for that data unit size.
Signed-off-by: Eric Biggers
---
common/encrypt | 38 ++
1 file changed, 30 insertions(+), 8 deletions(-)
diff --git a/common/encrypt b/common/encrypt
index 5688745c..d90a566a 100644
--- a/common/encrypt
+++ b/common/encrypt
@@ -1,32 +1,41 @@
##/bin/bash
# SPDX-License-Identifier: GPL-2.0
# Copyright (c) 2016 Google, Inc. All Rights Reserved.
#
# Functions for setting up and testing file encryption
#
# _require_scratch_encryption [-c CONTENTS_MODE] [-n FILENAMES_MODE]
#[-f POLICY_FLAGS] [-v POLICY_VERSION]
+#[-s LOG2_DUSIZE]
#
# Require encryption support on the scratch device.
#
# This checks for support for the default type of encryption policy (v1 with
# AES-256-XTS and AES-256-CTS). Options can be specified to also require
# support for a different type of encryption policy.
#
_require_scratch_encryption()
{
- _require_scratch
+ local arg
+ _require_scratch
_require_xfs_io_command "set_encpolicy"
+ for arg; do
+ if [ "$arg" = "-s" ]; then
+ # -s option was added later. Make sure it's available.
+ _require_xfs_io_command "set_encpolicy" "-s"
+ fi
+ done
+
# The 'test_dummy_encryption' mount option interferes with trying to use
# encryption for real, even if we are just trying to get/set policies
# and never put any keys in the keyring. So skip the real encryption
# tests if the 'test_dummy_encryption' mount option was specified.
_exclude_scratch_mount_option "test_dummy_encryption"
# Make a filesystem on the scratch device with the encryption feature
# enabled. If this fails then probably the userspace tools (e.g.
# e2fsprogs or f2fs-tools) are too old to understand encryption.
if ! _scratch_mkfs_encrypted &>>$seqres.full; then
@@ -67,35 +76,35 @@ _require_scratch_encryption()
_require_encryption_policy_support()
{
local mnt=$1
local dir=$mnt/tmpdir
local set_encpolicy_args=""
local policy_flags=0
local policy_version=1
local c
OPTIND=2
- while getopts "c:n:f:v:" c; do
+ while getopts "c:n:f:s:v:" c; do
case $c in
- c|n)
+ c|n|s)
set_encpolicy_args+=" -$c $OPTARG"
;;
f)
set_encpolicy_args+=" -$c $OPTARG"
policy_flags=$OPTARG
;;
v)
set_encpolicy_args+=" -$c $OPTARG"
policy_version=$OPTARG
;;
*)
- _fail "Unrecognized option '$c'"
+ _fail "${FUNCNAME[0]}: unrecognized option '$c'"
;;
esac
done
set_encpolicy_args=${set_encpolicy_args# }
echo "Checking whether kernel supports encryption policy:
$set_encpolicy_args" \
>> $seqres.full
if (( policy_flags & (FSCRYPT_POLICY_FLAG_IV_INO_LBLK_64 |
FSCRYPT_POLICY_FLAG_IV_INO_LBLK_32) )); then
@@ -756,28 +765,27 @@ _do_verify_ciphertext_for_encryption_policy()
# Now unmount the filesystem and verify the ciphertext we just wrote.
_scratch_unmount
echo "Verifying encrypted file contents" >> $seqres.full
for f in "${test_contents_files[@]}"; do
read -r src inode blocklist <<< "$f"
nonce=$(_get_encryption_nonce $SCRATCH_DEV $inode)
_dump_ciphertext_blocks $SCRATCH_DEV $blocklist >
$tmp.actual_contents
$crypt_contents_cmd $contents_encryption_mode $raw_key_hex \
- --file-nonce=$nonce --data-unit-size=$blocksize \
- --inode-number=$inode < $src > $tmp.expected_contents
+ --file-nonce=$nonce --inode-number=$inode \
+< $src > $tmp.expected_contents
if ! cmp $tmp.expected_contents $tmp.actual_contents; then
_fail "Expected encrypted contents != actual encrypted
co
[PATCH v2 0/4] xfstests: test custom crypto data unit size
This series adds a test that verifies the on-disk format of encrypted
files that use a crypto data unit size that differs from the filesystem
block size. This tests the functionality that was introduced in Linux
6.7 by kernel commit 5b1188847180 ("fscrypt: support crypto data unit
size less than filesystem block size").
This depends on the xfsprogs patch
"xfs_io/encrypt: support specifying crypto data unit size"
(https://lore.kernel.org/r/20231013062639.141468-1-ebigg...@kernel.org)
which adds the '-s' option to the set_encpolicy command of xfs_io.
As usual, the test skips itself when any prerequisite isn't met.
I've tested the new test on both ext4 and f2fs.
Changed in v2:
- Updated the cover letter, commit message, and a comment to reflect
that the kernel commit that added this feature was merged in 6.7.
- Rebased onto latest for-next branch of xfstests.
Eric Biggers (4):
fscrypt-crypt-util: rename block to data unit
common/rc: fix _require_xfs_io_command with digits in argument
common/encrypt: support custom data unit size
generic: add test for custom crypto data unit size
common/encrypt | 42 +-
common/rc| 2 +-
src/fscrypt-crypt-util.c | 93
tests/f2fs/002 | 6 +--
tests/generic/900| 29 +
tests/generic/900.out| 11 +
6 files changed, 123 insertions(+), 60 deletions(-)
create mode 100755 tests/generic/900
create mode 100644 tests/generic/900.out
base-commit: b9e1a88f8198ac02f3b82fe3b127d4e14f4a97b7
--
2.42.1
[PATCH v2 2/4] common/rc: fix _require_xfs_io_command with digits in argument
From: Eric Biggers '_require_xfs_io_command set_encpolicy -s' does not work as expected because the following in the output of 'xfs_io -c "help set_encpolicy"': -s LOG2_DUSIZE -- log2 of data unit size ... does not match the regex: "^ -s ([a-zA-Z_]+ )?--" ... because the 2 in the argument name LOG2_DUSIZE is not matched. Fix the regex to support digits in the argument name. Signed-off-by: Eric Biggers --- common/rc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/rc b/common/rc index cc92fe06..dab672d8 100644 --- a/common/rc +++ b/common/rc @@ -2719,21 +2719,21 @@ _require_xfs_io_command() echo $testio | grep -q "foreign file active" && \ _notrun "xfs_io $command $param_checked not supported on $FSTYP" echo $testio | grep -q "Function not implemented" && \ _notrun "xfs_io $command $param_checked support is missing (missing syscall?)" echo $testio | grep -q "unknown flag" && \ _notrun "xfs_io $command $param_checked support is missing (unknown flag)" [ -n "$param" ] || return if [ -z "$param_checked" ]; then - $XFS_IO_PROG -c "help $command" | grep -E -q "^ $param ([a-zA-Z_]+ )?--" || \ + $XFS_IO_PROG -c "help $command" | grep -E -q "^ $param ([a-zA-Z0-9_]+ )?--" || \ _notrun "xfs_io $command doesn't support $param" else # xfs_io could result in "command %c not supported" if it was # built on kernels not supporting pwritev2() calls echo $testio | grep -q "\(invalid option\|not supported\)" && \ _notrun "xfs_io $command doesn't support $param" fi # On XFS, ioctl(FSSETXATTR)(called by xfs_io -c "chattr") maskes off unsupported # or invalid flags silently so need to check these flags by extra ioctl(FSGETXATTR) -- 2.42.1
[PATCH v2 1/4] fscrypt-crypt-util: rename block to data unit
From: Eric Biggers
Rename the --block-size option to --data-unit-size, and rename the
--block-number option to --data-unit-index.
This does not change any functionality, but this avoids confusion now
that the kernel supports the case where the crypto data unit size is not
the same as the filesystem block size. fscrypt-crypt-util cares about
the crypto data unit size, not the filesystem block size.
Signed-off-by: Eric Biggers
---
common/encrypt | 10 ++---
src/fscrypt-crypt-util.c | 93
tests/f2fs/002 | 6 +--
3 files changed, 55 insertions(+), 54 deletions(-)
diff --git a/common/encrypt b/common/encrypt
index 1a77e23b..5688745c 100644
--- a/common/encrypt
+++ b/common/encrypt
@@ -756,51 +756,51 @@ _do_verify_ciphertext_for_encryption_policy()
# Now unmount the filesystem and verify the ciphertext we just wrote.
_scratch_unmount
echo "Verifying encrypted file contents" >> $seqres.full
for f in "${test_contents_files[@]}"; do
read -r src inode blocklist <<< "$f"
nonce=$(_get_encryption_nonce $SCRATCH_DEV $inode)
_dump_ciphertext_blocks $SCRATCH_DEV $blocklist >
$tmp.actual_contents
$crypt_contents_cmd $contents_encryption_mode $raw_key_hex \
- --file-nonce=$nonce --block-size=$blocksize \
+ --file-nonce=$nonce --data-unit-size=$blocksize \
--inode-number=$inode < $src > $tmp.expected_contents
if ! cmp $tmp.expected_contents $tmp.actual_contents; then
_fail "Expected encrypted contents != actual encrypted
contents. File: $f"
fi
$crypt_contents_cmd $contents_encryption_mode $raw_key_hex \
- --decrypt --file-nonce=$nonce --block-size=$blocksize \
- --inode-number=$inode \
+ --decrypt --file-nonce=$nonce \
+--data-unit-size=$blocksize --inode-number=$inode \
< $tmp.actual_contents > $tmp.decrypted_contents
if ! cmp $src $tmp.decrypted_contents; then
_fail "Contents decryption sanity check failed. File:
$f"
fi
done
echo "Verifying encrypted file names" >> $seqres.full
for f in "${test_filenames_files[@]}"; do
read -r name inode dir_inode padding <<< "$f"
nonce=$(_get_encryption_nonce $SCRATCH_DEV $dir_inode)
_get_ciphertext_filename $SCRATCH_DEV $inode $dir_inode \
> $tmp.actual_name
echo -n "$name" | \
$crypt_filename_cmd $filenames_encryption_mode \
$raw_key_hex --file-nonce=$nonce --padding=$padding \
- --block-size=255 --inode-number=$dir_inode \
+ --data-unit-size=255 --inode-number=$dir_inode \
> $tmp.expected_name
if ! cmp $tmp.expected_name $tmp.actual_name; then
_fail "Expected encrypted filename != actual encrypted
filename. File: $f"
fi
$crypt_filename_cmd $filenames_encryption_mode $raw_key_hex \
--decrypt --file-nonce=$nonce --padding=$padding \
- --block-size=255 --inode-number=$dir_inode \
+ --data-unit-size=255 --inode-number=$dir_inode \
< $tmp.actual_name > $tmp.decrypted_name
decrypted_name=$(tr -d '\0' < $tmp.decrypted_name)
if [ "$name" != "$decrypted_name" ]; then
_fail "Filename decryption sanity check failed ($name
!= $decrypted_name). File: $f"
fi
done
}
# fscrypt UAPI constants (see )
diff --git a/src/fscrypt-crypt-util.c b/src/fscrypt-crypt-util.c
index 96f04799..a1b5005d 100644
--- a/src/fscrypt-crypt-util.c
+++ b/src/fscrypt-crypt-util.c
@@ -54,23 +54,23 @@ static void usage(FILE *fp)
"MASTER_KEY (or a key derived from it, if a KDF is specified), and writes
the\n"
"resulting ciphertext (or plaintext) to stdout.\n"
"\n"
"CIPHER can be AES-256-XTS, AES-256-CTS-CBC, AES-128-CBC-ESSIV,
AES-128-CTS-CBC,\n"
"Adiantum, or AES-256-HCTR2. MASTER_KEY must be a hex string long enough
for\n"
"the cipher.\n"
"\n"
"WARNING: this program is only meant for testing, not for \"real\" use!\n"
"\n"
"Options:\n"
-" --block-number=BNUM Starting block number for IV generation.\n"
+" --data-unit-index=DUIDX Starti
[f2fs-dev] [PATCH] f2fs: explicitly null-terminate the xattr list
From: Eric Biggers When setting an xattr, explicitly null-terminate the xattr list. This eliminates the fragile assumption that the unused xattr space is always zeroed. Signed-off-by: Eric Biggers --- fs/f2fs/xattr.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c index 47e88b4d4e7d0..a8fc2cac68799 100644 --- a/fs/f2fs/xattr.c +++ b/fs/f2fs/xattr.c @@ -747,20 +747,26 @@ static int __f2fs_setxattr(struct inode *inode, int index, * Before we come here, old entry is removed. * We just write new entry. */ last->e_name_index = index; last->e_name_len = len; memcpy(last->e_name, name, len); pval = last->e_name + len; memcpy(pval, value, size); last->e_value_size = cpu_to_le16(size); new_hsize += newsize; + /* +* Explicitly add the null terminator. The unused xattr space +* is supposed to always be zeroed, which would make this +* unnecessary, but don't depend on that. +*/ + *(u32 *)((u8 *)last + newsize) = 0; } error = write_all_xattrs(inode, new_hsize, base_addr, ipage); if (error) goto exit; if (index == F2FS_XATTR_INDEX_ENCRYPTION && !strcmp(name, F2FS_XATTR_NAME_ENCRYPTION_CONTEXT)) f2fs_set_encrypted_inode(inode); if (S_ISDIR(inode->i_mode)) base-commit: be3ca57cfb777ad820c6659d52e60bbdd36bf5ff -- 2.42.1 ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
[f2fs-dev] [GIT PULL] fscrypt updates for 6.7
The following changes since commit 6465e260f48790807eef06b583b38ca9789b6072: Linux 6.6-rc3 (2023-09-24 14:31:13 -0700) are available in the Git repository at: https://git.kernel.org/pub/scm/fs/fscrypt/linux.git tags/fscrypt-for-linus for you to fetch changes up to 15baf55481de700f8c4494cddb80ec4f4575548b: fscrypt: track master key presence separately from secret (2023-10-16 21:23:45 -0700) This update adds support for configuring the crypto data unit size (i.e. the granularity of file contents encryption) to be less than the filesystem block size. This can allow users to use inline encryption hardware in some cases when it wouldn't otherwise be possible. In addition, there are two commits that are prerequisites for the extent-based encryption support that the btrfs folks are working on. Eric Biggers (6): fscrypt: make it clearer that key_prefix is deprecated fscrypt: make the bounce page pool opt-in instead of opt-out fscrypt: compute max_lblk_bits from s_maxbytes and block size fscrypt: replace get_ino_and_lblk_bits with just has_32bit_inodes fscrypt: support crypto data unit size less than filesystem block size fscrypt: track master key presence separately from secret Josef Bacik (1): fscrypt: rename fscrypt_info => fscrypt_inode_info Documentation/filesystems/fscrypt.rst | 121 ++--- fs/ceph/crypto.c | 1 + fs/crypto/bio.c | 39 fs/crypto/crypto.c| 163 ++--- fs/crypto/fname.c | 6 +- fs/crypto/fscrypt_private.h | 164 ++ fs/crypto/hooks.c | 4 +- fs/crypto/inline_crypt.c | 32 +++ fs/crypto/keyring.c | 82 ++--- fs/crypto/keysetup.c | 62 +++-- fs/crypto/keysetup_v1.c | 20 +++-- fs/crypto/policy.c| 83 +++-- fs/ext4/crypto.c | 13 +-- fs/f2fs/super.c | 13 +-- fs/ubifs/crypto.c | 3 +- include/linux/fs.h| 4 +- include/linux/fscrypt.h | 82 ++--- include/uapi/linux/fscrypt.h | 3 +- 18 files changed, 546 insertions(+), 349 deletions(-) ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
[f2fs-dev] [PATCH] f2fs-tools: ensure that unused xattr space is zeroized
From: Eric Biggers
Make fsck.f2fs zeroize the unused xattr space, i.e. the space after the
end of the zero-terminated xattr list, if it isn't already zeroized.
This is important because the kernel currently does not explicitly
zero-terminate the list when writing xattrs. So, the kernel relies on
the unused space containing zeroes.
Also, add a missing free() to fix a memory leak.
Signed-off-by: Eric Biggers
---
fsck/fsck.c | 36 +++-
1 file changed, 27 insertions(+), 9 deletions(-)
diff --git a/fsck/fsck.c b/fsck/fsck.c
index 72daa71..51e46e4 100644
--- a/fsck/fsck.c
+++ b/fsck/fsck.c
@@ -827,54 +827,72 @@ void fsck_reada_all_direct_node_blocks(struct
f2fs_sb_info *sbi,
{
int i;
for (i = 0; i < NIDS_PER_BLOCK; i++) {
u32 nid = le32_to_cpu(node_blk->in.nid[i]);
fsck_reada_node_block(sbi, nid);
}
}
+static bool is_zeroed(const u8 *p, size_t size)
+{
+ size_t i;
+
+ for (i = 0; i < size; i++) {
+ if (p[i])
+ return false;
+ }
+ return true;
+}
+
int chk_extended_attributes(struct f2fs_sb_info *sbi, u32 nid,
struct f2fs_node *inode)
{
void *xattr;
void *last_base_addr;
struct f2fs_xattr_entry *ent;
__u32 xattr_size = XATTR_SIZE(>i);
+ bool need_fix = false;
if (xattr_size == 0)
return 0;
xattr = read_all_xattrs(sbi, inode, false);
ASSERT(xattr);
last_base_addr = (void *)xattr + xattr_size;
list_for_each_xattr(ent, xattr) {
if ((void *)(ent) + sizeof(__u32) > last_base_addr ||
(void *)XATTR_NEXT_ENTRY(ent) > last_base_addr) {
ASSERT_MSG("[0x%x] last xattr entry (offset: %lx) "
"crosses the boundary",
nid, (long int)((void *)ent - xattr));
- if (c.fix_on) {
- memset(ent, 0,
- (char *)last_base_addr - (char *)ent);
- write_all_xattrs(sbi, inode, xattr_size, xattr);
- FIX_MSG("[0x%x] nullify wrong xattr entries",
- nid);
- return 1;
- }
+ need_fix = true;
break;
}
}
-
+ if (!need_fix &&
+ !is_zeroed((u8 *)ent, (u8 *)last_base_addr - (u8 *)ent)) {
+ ASSERT_MSG("[0x%x] nonzero bytes in xattr space after "
+ "end of list", nid);
+ need_fix = true;
+ }
+ if (need_fix && c.fix_on) {
+ memset(ent, 0, (u8 *)last_base_addr - (u8 *)ent);
+ write_all_xattrs(sbi, inode, xattr_size, xattr);
+ FIX_MSG("[0x%x] nullify wrong xattr entries", nid);
+ free(xattr);
+ return 1;
+ }
+ free(xattr);
return 0;
}
/* start with valid nid and blkaddr */
void fsck_chk_inode_blk(struct f2fs_sb_info *sbi, u32 nid,
enum FILE_TYPE ftype, struct f2fs_node *node_blk,
u32 *blk_cnt, struct f2fs_compr_blk_cnt *cbc,
struct node_info *ni, struct child_info *child_d)
{
struct f2fs_fsck *fsck = F2FS_FSCK(sbi);
base-commit: 104b6b83206a9919d4b10f310981cc99fbbc8ed1
--
2.42.0.820.g83a721a137-goog
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
[f2fs-dev] [PATCH v3 1/5] fscrypt: make it clearer that key_prefix is deprecated
From: Eric Biggers
fscrypt_operations::key_prefix should not be set by any filesystems that
aren't setting it already. This is already documented, but apparently
it's not sufficiently clear, as both ceph and btrfs have tried to set
it. Rename the field to legacy_key_prefix and improve the documentation
to hopefully make it clearer.
Signed-off-by: Eric Biggers
---
fs/crypto/keysetup_v1.c | 5 +++--
fs/ext4/crypto.c| 2 +-
fs/f2fs/super.c | 2 +-
fs/ubifs/crypto.c | 2 +-
include/linux/fscrypt.h | 14 +-
5 files changed, 15 insertions(+), 10 deletions(-)
diff --git a/fs/crypto/keysetup_v1.c b/fs/crypto/keysetup_v1.c
index 75dabd9b27f9b..86b48a2b47d1b 100644
--- a/fs/crypto/keysetup_v1.c
+++ b/fs/crypto/keysetup_v1.c
@@ -292,29 +292,30 @@ static int setup_v1_file_key_derived(struct fscrypt_info
*ci,
int fscrypt_setup_v1_file_key(struct fscrypt_info *ci, const u8
*raw_master_key)
{
if (ci->ci_policy.v1.flags & FSCRYPT_POLICY_FLAG_DIRECT_KEY)
return setup_v1_file_key_direct(ci, raw_master_key);
else
return setup_v1_file_key_derived(ci, raw_master_key);
}
int fscrypt_setup_v1_file_key_via_subscribed_keyrings(struct fscrypt_info *ci)
{
+ const struct super_block *sb = ci->ci_inode->i_sb;
struct key *key;
const struct fscrypt_key *payload;
int err;
key = find_and_lock_process_key(FSCRYPT_KEY_DESC_PREFIX,
ci->ci_policy.v1.master_key_descriptor,
ci->ci_mode->keysize, );
- if (key == ERR_PTR(-ENOKEY) && ci->ci_inode->i_sb->s_cop->key_prefix) {
- key =
find_and_lock_process_key(ci->ci_inode->i_sb->s_cop->key_prefix,
+ if (key == ERR_PTR(-ENOKEY) && sb->s_cop->legacy_key_prefix) {
+ key = find_and_lock_process_key(sb->s_cop->legacy_key_prefix,
ci->ci_policy.v1.master_key_descriptor,
ci->ci_mode->keysize, );
}
if (IS_ERR(key))
return PTR_ERR(key);
err = fscrypt_setup_v1_file_key(ci, payload->raw);
up_read(>sem);
key_put(key);
return err;
diff --git a/fs/ext4/crypto.c b/fs/ext4/crypto.c
index 453d4da5de520..99a4769a53f63 100644
--- a/fs/ext4/crypto.c
+++ b/fs/ext4/crypto.c
@@ -233,18 +233,18 @@ static bool ext4_has_stable_inodes(struct super_block *sb)
}
static void ext4_get_ino_and_lblk_bits(struct super_block *sb,
int *ino_bits_ret, int *lblk_bits_ret)
{
*ino_bits_ret = 8 * sizeof(EXT4_SB(sb)->s_es->s_inodes_count);
*lblk_bits_ret = 8 * sizeof(ext4_lblk_t);
}
const struct fscrypt_operations ext4_cryptops = {
- .key_prefix = "ext4:",
+ .legacy_key_prefix = "ext4:",
.get_context= ext4_get_context,
.set_context= ext4_set_context,
.get_dummy_policy = ext4_get_dummy_policy,
.empty_dir = ext4_empty_dir,
.has_stable_inodes = ext4_has_stable_inodes,
.get_ino_and_lblk_bits = ext4_get_ino_and_lblk_bits,
};
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index a8c8232852bb1..f60062b558fd1 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -3224,21 +3224,21 @@ static struct block_device **f2fs_get_devices(struct
super_block *sb,
if (!devs)
return ERR_PTR(-ENOMEM);
for (i = 0; i < sbi->s_ndevs; i++)
devs[i] = FDEV(i).bdev;
*num_devs = sbi->s_ndevs;
return devs;
}
static const struct fscrypt_operations f2fs_cryptops = {
- .key_prefix = "f2fs:",
+ .legacy_key_prefix = "f2fs:",
.get_context= f2fs_get_context,
.set_context= f2fs_set_context,
.get_dummy_policy = f2fs_get_dummy_policy,
.empty_dir = f2fs_empty_dir,
.has_stable_inodes = f2fs_has_stable_inodes,
.get_ino_and_lblk_bits = f2fs_get_ino_and_lblk_bits,
.get_devices= f2fs_get_devices,
};
#endif
diff --git a/fs/ubifs/crypto.c b/fs/ubifs/crypto.c
index 3125e76376ee6..1be3e11da3b3e 100644
--- a/fs/ubifs/crypto.c
+++ b/fs/ubifs/crypto.c
@@ -82,15 +82,15 @@ int ubifs_decrypt(const struct inode *inode, struct
ubifs_data_node *dn,
ubifs_err(c, "fscrypt_decrypt_block_inplace() failed: %d", err);
return err;
}
*out_len = clen;
return 0;
}
const struct fscrypt_operations ubifs_crypt_operations = {
.flags = FS_CFLG_OWN_PAGES,
- .key_prefix = "ubifs:",
+ .legacy_
[f2fs-dev] [PATCH v3 4/5] fscrypt: replace get_ino_and_lblk_bits with just has_32bit_inodes
From: Eric Biggers
Now that fs/crypto/ computes the filesystem's lblk_bits from its maximum
file size, it is no longer necessary for filesystems to provide
lblk_bits via fscrypt_operations::get_ino_and_lblk_bits.
It is still necessary for fs/crypto/ to retrieve ino_bits from the
filesystem. However, this is used only to decide whether inode numbers
fit in 32 bits. Also, ino_bits is static for all relevant filesystems,
i.e. it doesn't depend on the filesystem instance.
Therefore, in the interest of keeping things as simple as possible,
replace 'get_ino_and_lblk_bits' with a flag 'has_32bit_inodes'. This
can always be changed back to a function if a filesystem needs it to be
dynamic, but for now a static flag is all that's needed.
Signed-off-by: Eric Biggers
---
fs/crypto/policy.c | 33 +++--
fs/ext4/crypto.c| 9 +
fs/f2fs/super.c | 9 +
include/linux/fscrypt.h | 26 +++---
4 files changed, 28 insertions(+), 49 deletions(-)
diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
index 7b34949e49de6..32709dad9762b 100644
--- a/fs/crypto/policy.c
+++ b/fs/crypto/policy.c
@@ -111,25 +111,25 @@ static bool supported_direct_key_modes(const struct inode
*inode,
if (mode->ivsize < offsetofend(union fscrypt_iv, nonce)) {
fscrypt_warn(inode, "Direct key flag not allowed with %s",
mode->friendly_name);
return false;
}
return true;
}
static bool supported_iv_ino_lblk_policy(const struct fscrypt_policy_v2
*policy,
-const struct inode *inode,
-const char *type, int max_ino_bits)
+const struct inode *inode)
{
+ const char *type = (policy->flags & FSCRYPT_POLICY_FLAG_IV_INO_LBLK_64)
+ ? "IV_INO_LBLK_64" : "IV_INO_LBLK_32";
struct super_block *sb = inode->i_sb;
- int ino_bits = 64, lblk_bits = 64;
/*
* IV_INO_LBLK_* exist only because of hardware limitations, and
* currently the only known use case for them involves AES-256-XTS.
* That's also all we test currently. For these reasons, for now only
* allow AES-256-XTS here. This can be relaxed later if a use case for
* IV_INO_LBLK_* with other encryption modes arises.
*/
if (policy->contents_encryption_mode != FSCRYPT_MODE_AES_256_XTS) {
fscrypt_warn(inode,
@@ -142,23 +142,29 @@ static bool supported_iv_ino_lblk_policy(const struct
fscrypt_policy_v2 *policy,
* It's unsafe to include inode numbers in the IVs if the filesystem can
* potentially renumber inodes, e.g. via filesystem shrinking.
*/
if (!sb->s_cop->has_stable_inodes ||
!sb->s_cop->has_stable_inodes(sb)) {
fscrypt_warn(inode,
"Can't use %s policy on filesystem '%s' because it
doesn't have stable inode numbers",
type, sb->s_id);
return false;
}
- if (sb->s_cop->get_ino_and_lblk_bits)
- sb->s_cop->get_ino_and_lblk_bits(sb, _bits, _bits);
- if (ino_bits > max_ino_bits) {
+
+ /*
+* IV_INO_LBLK_64 and IV_INO_LBLK_32 both require that inode numbers fit
+* in 32 bits. In principle, IV_INO_LBLK_32 could support longer inode
+* numbers because it hashes the inode number; however, currently the
+* inode number is gotten from inode::i_ino which is 'unsigned long'.
+* So for now the implementation limit is 32 bits.
+*/
+ if (!sb->s_cop->has_32bit_inodes) {
fscrypt_warn(inode,
"Can't use %s policy on filesystem '%s' because
its inode numbers are too long",
type, sb->s_id);
return false;
}
/*
* IV_INO_LBLK_64 and IV_INO_LBLK_32 both require that file logical
* block numbers fit in 32 bits.
*/
@@ -235,32 +241,23 @@ static bool fscrypt_supported_v2_policy(const struct
fscrypt_policy_v2 *policy,
fscrypt_warn(inode, "Mutually exclusive encryption flags
(0x%02x)",
policy->flags);
return false;
}
if ((policy->flags & FSCRYPT_POLICY_FLAG_DIRECT_KEY) &&
!supported_direct_key_modes(inode, policy->contents_encryption_mode,
policy->filenames_encryption_mode))
return false;
- if ((policy->flags & FSCRYPT_POLICY_FLAG_IV_INO_LBLK_64) &&
- !supported_iv_ino_lblk_policy(policy, inode, "IV_INO_LBLK_64", 32))
[f2fs-dev] [PATCH v3 3/5] fscrypt: compute max_lblk_bits from s_maxbytes and block size
From: Eric Biggers
For a given filesystem, the number of bits used by the maximum file
logical block number is computable from the maximum file size and block
size. These values are always present in struct super_block.
Therefore, compute it this way instead of using the value from
fscrypt_operations::get_ino_and_lblk_bits. Since filesystems always
have to set the super_block fields anyway, this avoids having to provide
this information redundantly via fscrypt_operations.
This change is in preparation for adding support for sub-block data
units. For that, the value that is needed will become "the maximum file
data unit index". A hardcoded value won't suffice for that; it will
need to be computed anyway.
Signed-off-by: Eric Biggers
---
fs/crypto/fscrypt_private.h | 10 ++
fs/crypto/inline_crypt.c| 7 ++-
fs/crypto/policy.c | 18 ++
3 files changed, 22 insertions(+), 13 deletions(-)
diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
index 2d63da48635ab..4b113214b53af 100644
--- a/fs/crypto/fscrypt_private.h
+++ b/fs/crypto/fscrypt_private.h
@@ -289,20 +289,30 @@ union fscrypt_iv {
/* per-file nonce; only set in DIRECT_KEY mode */
u8 nonce[FSCRYPT_FILE_NONCE_SIZE];
};
u8 raw[FSCRYPT_MAX_IV_SIZE];
__le64 dun[FSCRYPT_MAX_IV_SIZE / sizeof(__le64)];
};
void fscrypt_generate_iv(union fscrypt_iv *iv, u64 lblk_num,
const struct fscrypt_info *ci);
+/*
+ * Return the number of bits used by the maximum file logical block number that
+ * is possible on the given filesystem.
+ */
+static inline int
+fscrypt_max_file_lblk_bits(const struct super_block *sb)
+{
+ return fls64(sb->s_maxbytes - 1) - sb->s_blocksize_bits;
+}
+
/* fname.c */
bool __fscrypt_fname_encrypted_size(const union fscrypt_policy *policy,
u32 orig_len, u32 max_len,
u32 *encrypted_len_ret);
/* hkdf.c */
struct fscrypt_hkdf {
struct crypto_shash *hmac_tfm;
};
diff --git a/fs/crypto/inline_crypt.c b/fs/crypto/inline_crypt.c
index 8bfb3ce864766..7d9f6c167de58 100644
--- a/fs/crypto/inline_crypt.c
+++ b/fs/crypto/inline_crypt.c
@@ -34,37 +34,34 @@ static struct block_device **fscrypt_get_devices(struct
super_block *sb,
devs = kmalloc(sizeof(*devs), GFP_KERNEL);
if (!devs)
return ERR_PTR(-ENOMEM);
devs[0] = sb->s_bdev;
*num_devs = 1;
return devs;
}
static unsigned int fscrypt_get_dun_bytes(const struct fscrypt_info *ci)
{
- struct super_block *sb = ci->ci_inode->i_sb;
+ const struct super_block *sb = ci->ci_inode->i_sb;
unsigned int flags = fscrypt_policy_flags(>ci_policy);
- int ino_bits = 64, lblk_bits = 64;
if (flags & FSCRYPT_POLICY_FLAG_DIRECT_KEY)
return offsetofend(union fscrypt_iv, nonce);
if (flags & FSCRYPT_POLICY_FLAG_IV_INO_LBLK_64)
return sizeof(__le64);
if (flags & FSCRYPT_POLICY_FLAG_IV_INO_LBLK_32)
return sizeof(__le32);
/* Default case: IVs are just the file logical block number */
- if (sb->s_cop->get_ino_and_lblk_bits)
- sb->s_cop->get_ino_and_lblk_bits(sb, _bits, _bits);
- return DIV_ROUND_UP(lblk_bits, 8);
+ return DIV_ROUND_UP(fscrypt_max_file_lblk_bits(sb), 8);
}
/*
* Log a message when starting to use blk-crypto (native) or
blk-crypto-fallback
* for an encryption mode for the first time. This is the blk-crypto
* counterpart to the message logged when starting to use the crypto API for
the
* first time. A limitation is that these messages don't convey which specific
* filesystems or files are using each implementation. However, *usually*
* systems use just one implementation per mode, which makes these messages
* helpful for debugging problems where the "wrong" implementation is used.
diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
index f4456ecb3f877..7b34949e49de6 100644
--- a/fs/crypto/policy.c
+++ b/fs/crypto/policy.c
@@ -112,22 +112,21 @@ static bool supported_direct_key_modes(const struct inode
*inode,
if (mode->ivsize < offsetofend(union fscrypt_iv, nonce)) {
fscrypt_warn(inode, "Direct key flag not allowed with %s",
mode->friendly_name);
return false;
}
return true;
}
static bool supported_iv_ino_lblk_policy(const struct fscrypt_policy_v2
*policy,
const struct inode *inode,
-const char *type,
-int max_ino_bits, int max_lblk_bits)
+const char *type, int max_ino_bits)
{
struct super_block *sb = inode->i_
[f2fs-dev] [PATCH v3 5/5] fscrypt: support crypto data unit size less than filesystem block size
From: Eric Biggers Until now, fscrypt has always used the filesystem block size as the granularity of file contents encryption. Two scenarios have come up where a sub-block granularity of contents encryption would be useful: 1. Inline crypto hardware that only supports a crypto data unit size that is less than the filesystem block size. 2. Support for direct I/O at a granularity less than the filesystem block size, for example at the block device's logical block size in order to match the traditional direct I/O alignment requirement. (1) first came up with older eMMC inline crypto hardware that only supports a crypto data unit size of 512 bytes. That specific case ultimately went away because all systems with that hardware continued using out of tree code and never actually upgraded to the upstream inline crypto framework. But, now it's coming back in a new way: some current UFS controllers only support a data unit size of 4096 bytes, and there is a proposal to increase the filesystem block size to 16K. (2) was discussed as a "nice to have" feature, though not essential, when support for direct I/O on encrypted files was being upstreamed. Still, the fact that this feature has come up several times does suggest it would be wise to have available. Therefore, this patch implements it by using one of the reserved bytes in fscrypt_policy_v2 to allow users to select a sub-block data unit size. Supported data unit sizes are powers of 2 between 512 and the filesystem block size, inclusively. Support is implemented for both the FS-layer and inline crypto cases. This patch focuses on the basic support for sub-block data units. Some things are out of scope for this patch but may be addressed later: - Supporting sub-block data units in combination with FSCRYPT_POLICY_FLAG_IV_INO_LBLK_64, in most cases. Unfortunately this combination usually causes data unit indices to exceed 32 bits, and thus fscrypt_supported_policy() correctly disallows it. The users who potentially need this combination are using f2fs. To support it, f2fs would need to provide an option to slightly reduce its max file size. - Supporting sub-block data units in combination with FSCRYPT_POLICY_FLAG_IV_INO_LBLK_32. This has the same problem described above, but also it will need special code to make DUN wraparound still happen on a FS block boundary. - Supporting use case (2) mentioned above. The encrypted direct I/O code will need to stop requiring and assuming FS block alignment. This won't be hard, but it belongs in a separate patch. - Supporting this feature on filesystems other than ext4 and f2fs. (Filesystems declare support for it via their fscrypt_operations.) On UBIFS, sub-block data units don't make sense because UBIFS encrypts variable-length blocks as a result of compression. CephFS could support it, but a bit more work would be needed to make the fscrypt_*_block_inplace functions play nicely with sub-block data units. I don't think there's a use case for this on CephFS anyway. Signed-off-by: Eric Biggers --- Documentation/filesystems/fscrypt.rst | 117 -- fs/crypto/bio.c | 39 fs/crypto/crypto.c| 139 ++ fs/crypto/fscrypt_private.h | 56 --- fs/crypto/inline_crypt.c | 14 ++- fs/crypto/keysetup.c | 5 + fs/crypto/policy.c| 34 ++- fs/ext4/crypto.c | 1 + fs/f2fs/super.c | 1 + include/linux/fscrypt.h | 12 +++ include/uapi/linux/fscrypt.h | 3 +- 11 files changed, 288 insertions(+), 133 deletions(-) diff --git a/Documentation/filesystems/fscrypt.rst b/Documentation/filesystems/fscrypt.rst index a624e92f2687f..28700fb41a00b 100644 --- a/Documentation/filesystems/fscrypt.rst +++ b/Documentation/filesystems/fscrypt.rst @@ -254,23 +254,23 @@ significant advantages to key wrapping. In particular, currently there is no requirement to support unlocking a file with multiple alternative master keys or to support rotating master keys. Instead, the master keys may be wrapped in userspace, e.g. as is done by the `fscrypt <https://github.com/google/fscrypt>`_ tool. DIRECT_KEY policies --- The Adiantum encryption mode (see `Encryption modes and usage`_) is suitable for both contents and filenames encryption, and it accepts -long IVs --- long enough to hold both an 8-byte logical block number -and a 16-byte per-file nonce. Also, the overhead of each Adiantum key -is greater than that of an AES-256-XTS key. +long IVs --- long enough to hold both an 8-byte data unit index and a +16-byte per-file nonce. Also, the overhead of each Adiantum key is +greater than that of an AES-256-XTS key. Therefore, to improve performance and save memory, for Adiantum a "direct key" configuration is
[f2fs-dev] [PATCH v3 0/5] fscrypt: add support for data_unit_size < fs_block_size
This patchset adds support for configuring the granularity of file contents encryption (a.k.a. the "crypto data unit size") to be less than the filesystem block size on ext4 and f2fs. The main use case for this is to support inline crypto hardware that only supports a data unit size that is less than the FS block size being used. Another possible use case is to support direct I/O on encrypted files without the FS block alignment restriction. Note that decreasing the crypto data unit size decreases efficiency, so this feature should only be used when needed. For full details, see patch 5 which adds the actual feature. Patches 1-4 are preparatory patches. I've written an xfstest that verifies that when a sub-block data unit size is selected, the data on-disk is encrypted correctly with that data unit size. I'll be sending that out separately. Other testing of this patchset with xfstests has gone well, though it turns out that some additional changes will be needed for a sub-block data unit size to work with the IV_INO_LBLK_* encryption settings. See patch 5 for details. This patchset focuses on basic sub-block data unit size support first. This patchset will cause some conflicts in the extent-based encryption patches that the btrfs folks are working on, as both are touching file contents encryption, but logically they are orthogonal features. This patchset is based on v6.6-rc3. Changed in v3: - Shortened 'legacy_key_prefix_for_backcompat' to 'legacy_key_prefix' - Other miscellaneous cleanups - Rebased onto v6.6-rc3 Changed in v2: - Rebased onto v6.6-rc1 and took into account CephFS's recent addition of support for fscrypt - Narrowed the focus somewhat by dropping the attempted support for IV_INO_LBLK_32 and clearly documenting what is considered out of scope for now - Other cleanups Eric Biggers (5): fscrypt: make it clearer that key_prefix is deprecated fscrypt: make the bounce page pool opt-in instead of opt-out fscrypt: compute max_lblk_bits from s_maxbytes and block size fscrypt: replace get_ino_and_lblk_bits with just has_32bit_inodes fscrypt: support crypto data unit size less than filesystem block size Documentation/filesystems/fscrypt.rst | 117 ++-- fs/ceph/crypto.c | 1 + fs/crypto/bio.c | 39 --- fs/crypto/crypto.c| 148 +++--- fs/crypto/fscrypt_private.h | 58 -- fs/crypto/inline_crypt.c | 19 ++-- fs/crypto/keysetup.c | 5 + fs/crypto/keysetup_v1.c | 5 +- fs/crypto/policy.c| 73 + fs/ext4/crypto.c | 13 +-- fs/f2fs/super.c | 13 +-- fs/ubifs/crypto.c | 3 +- include/linux/fscrypt.h | 72 - include/uapi/linux/fscrypt.h | 3 +- 14 files changed, 364 insertions(+), 205 deletions(-) base-commit: 6465e260f48790807eef06b583b38ca9789b6072 -- 2.42.0 ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
[f2fs-dev] [PATCH v3 2/5] fscrypt: make the bounce page pool opt-in instead of opt-out
From: Eric Biggers
Replace FS_CFLG_OWN_PAGES with a bit flag 'needs_bounce_pages' which has
the opposite meaning. I.e., filesystems now opt into the bounce page
pool instead of opt out. Make fscrypt_alloc_bounce_page() check that
the bounce page pool has been initialized.
I believe the opt-in makes more sense, since nothing else in
fscrypt_operations is opt-out, and these days filesystems can choose to
use blk-crypto which doesn't need the fscrypt bounce page pool. Also, I
happen to be planning to add two more flags, and I wanted to fix the
"FS_CFLG_" name anyway as it wasn't prefixed with "FSCRYPT_".
Signed-off-by: Eric Biggers
---
fs/ceph/crypto.c| 1 +
fs/crypto/crypto.c | 9 -
fs/ext4/crypto.c| 1 +
fs/f2fs/super.c | 1 +
fs/ubifs/crypto.c | 1 -
include/linux/fscrypt.h | 20 +++-
6 files changed, 22 insertions(+), 11 deletions(-)
diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
index e4d5cd56a80b9..cc63f1e6fdef6 100644
--- a/fs/ceph/crypto.c
+++ b/fs/ceph/crypto.c
@@ -126,20 +126,21 @@ static bool ceph_crypt_empty_dir(struct inode *inode)
return ci->i_rsubdirs + ci->i_rfiles == 1;
}
static const union fscrypt_policy *ceph_get_dummy_policy(struct super_block
*sb)
{
return ceph_sb_to_client(sb)->fsc_dummy_enc_policy.policy;
}
static struct fscrypt_operations ceph_fscrypt_ops = {
+ .needs_bounce_pages = 1,
.get_context= ceph_crypt_get_context,
.set_context= ceph_crypt_set_context,
.get_dummy_policy = ceph_get_dummy_policy,
.empty_dir = ceph_crypt_empty_dir,
};
void ceph_fscrypt_set_ops(struct super_block *sb)
{
fscrypt_set_ops(sb, _fscrypt_ops);
}
diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
index 6a837e4b80dcb..aed0c5ea75781 100644
--- a/fs/crypto/crypto.c
+++ b/fs/crypto/crypto.c
@@ -42,20 +42,27 @@ static DEFINE_MUTEX(fscrypt_init_mutex);
struct kmem_cache *fscrypt_info_cachep;
void fscrypt_enqueue_decrypt_work(struct work_struct *work)
{
queue_work(fscrypt_read_workqueue, work);
}
EXPORT_SYMBOL(fscrypt_enqueue_decrypt_work);
struct page *fscrypt_alloc_bounce_page(gfp_t gfp_flags)
{
+ if (WARN_ON_ONCE(!fscrypt_bounce_page_pool)) {
+ /*
+* Oops, the filesystem called a function that uses the bounce
+* page pool, but it didn't set needs_bounce_pages.
+*/
+ return NULL;
+ }
return mempool_alloc(fscrypt_bounce_page_pool, gfp_flags);
}
/**
* fscrypt_free_bounce_page() - free a ciphertext bounce page
* @bounce_page: the bounce page to free, or NULL
*
* Free a bounce page that was allocated by fscrypt_encrypt_pagecache_blocks(),
* or by fscrypt_alloc_bounce_page() directly.
*/
@@ -318,21 +325,21 @@ EXPORT_SYMBOL(fscrypt_decrypt_block_inplace);
int fscrypt_initialize(struct super_block *sb)
{
int err = 0;
mempool_t *pool;
/* pairs with smp_store_release() below */
if (likely(smp_load_acquire(_bounce_page_pool)))
return 0;
/* No need to allocate a bounce page pool if this FS won't use it. */
- if (sb->s_cop->flags & FS_CFLG_OWN_PAGES)
+ if (!sb->s_cop->needs_bounce_pages)
return 0;
mutex_lock(_init_mutex);
if (fscrypt_bounce_page_pool)
goto out_unlock;
err = -ENOMEM;
pool = mempool_create_page_pool(num_prealloc_crypto_pages, 0);
if (!pool)
goto out_unlock;
diff --git a/fs/ext4/crypto.c b/fs/ext4/crypto.c
index 99a4769a53f63..5cd7bcfae46b2 100644
--- a/fs/ext4/crypto.c
+++ b/fs/ext4/crypto.c
@@ -233,18 +233,19 @@ static bool ext4_has_stable_inodes(struct super_block *sb)
}
static void ext4_get_ino_and_lblk_bits(struct super_block *sb,
int *ino_bits_ret, int *lblk_bits_ret)
{
*ino_bits_ret = 8 * sizeof(EXT4_SB(sb)->s_es->s_inodes_count);
*lblk_bits_ret = 8 * sizeof(ext4_lblk_t);
}
const struct fscrypt_operations ext4_cryptops = {
+ .needs_bounce_pages = 1,
.legacy_key_prefix = "ext4:",
.get_context= ext4_get_context,
.set_context= ext4_set_context,
.get_dummy_policy = ext4_get_dummy_policy,
.empty_dir = ext4_empty_dir,
.has_stable_inodes = ext4_has_stable_inodes,
.get_ino_and_lblk_bits = ext4_get_ino_and_lblk_bits,
};
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index f60062b558fd1..55aa0ed531f22 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -3224,20 +3224,21 @@ static struct block_device **f2fs_get_devices(struct
super_block *sb,
if (!devs)
return ERR_PTR(-ENOMEM);
for (i = 0; i < sbi->s_ndevs; i++)
devs[
[f2fs-dev] [PATCH v2 4/5] fscrypt: replace get_ino_and_lblk_bits with just has_32bit_inodes
From: Eric Biggers
Now that fs/crypto/ computes the filesystem's lblk_bits from its maximum
file size, it is no longer necessary for filesystems to provide
lblk_bits via fscrypt_operations::get_ino_and_lblk_bits.
It is still necessary for fs/crypto/ to retrieve ino_bits from the
filesystem. However, this is used only to decide whether inode numbers
fit in 32 bits. Also, ino_bits is static for all relevant filesystems,
i.e. it doesn't depend on the filesystem instance.
Therefore, in the interest of keeping things as simple as possible,
replace 'get_ino_and_lblk_bits' with a flag 'has_32bit_inodes'. This
can always be changed back to a function if a filesystem needs it to be
dynamic, but for now a static flag is all that's needed.
Signed-off-by: Eric Biggers
---
fs/crypto/policy.c | 33 +++--
fs/ext4/crypto.c| 9 +
fs/f2fs/super.c | 9 +
include/linux/fscrypt.h | 26 +++---
4 files changed, 28 insertions(+), 49 deletions(-)
diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
index 36bffc4d6228d..c8072a634af8f 100644
--- a/fs/crypto/policy.c
+++ b/fs/crypto/policy.c
@@ -118,11 +118,11 @@ static bool supported_direct_key_modes(const struct inode
*inode,
}
static bool supported_iv_ino_lblk_policy(const struct fscrypt_policy_v2
*policy,
-const struct inode *inode,
-const char *type, int max_ino_bits)
+const struct inode *inode)
{
+ const char *type = (policy->flags & FSCRYPT_POLICY_FLAG_IV_INO_LBLK_64)
+ ? "IV_INO_LBLK_64" : "IV_INO_LBLK_32";
struct super_block *sb = inode->i_sb;
- int ino_bits = 64, lblk_bits = 64;
/*
* IV_INO_LBLK_* exist only because of hardware limitations, and
@@ -149,9 +149,15 @@ static bool supported_iv_ino_lblk_policy(const struct
fscrypt_policy_v2 *policy,
type, sb->s_id);
return false;
}
- if (sb->s_cop->get_ino_and_lblk_bits)
- sb->s_cop->get_ino_and_lblk_bits(sb, _bits, _bits);
- if (ino_bits > max_ino_bits) {
+
+ /*
+* IV_INO_LBLK_64 requires that inode numbers fit in 32 bits.
+* IV_INO_LBLK_32 hashes the inode number, so in principle it can
+* support any length; however, currently the inode number is gotten
+* from inode::i_ino which is 'unsigned long'. So for now the
+* implementation limit is 32 bits, the same as IV_INO_LBLK_64.
+*/
+ if (!sb->s_cop->has_32bit_inodes) {
fscrypt_warn(inode,
"Can't use %s policy on filesystem '%s' because
its maximum inode number is too large",
type, sb->s_id);
@@ -242,18 +248,9 @@ static bool fscrypt_supported_v2_policy(const struct
fscrypt_policy_v2 *policy,
policy->filenames_encryption_mode))
return false;
- if ((policy->flags & FSCRYPT_POLICY_FLAG_IV_INO_LBLK_64) &&
- !supported_iv_ino_lblk_policy(policy, inode, "IV_INO_LBLK_64", 32))
- return false;
-
- /*
-* IV_INO_LBLK_32 hashes the inode number, so in principle it can
-* support any ino_bits. However, currently the inode number is gotten
-* from inode::i_ino which is 'unsigned long'. So for now the
-* implementation limit is 32 bits.
-*/
- if ((policy->flags & FSCRYPT_POLICY_FLAG_IV_INO_LBLK_32) &&
- !supported_iv_ino_lblk_policy(policy, inode, "IV_INO_LBLK_32", 32))
+ if ((policy->flags & (FSCRYPT_POLICY_FLAG_IV_INO_LBLK_64 |
+ FSCRYPT_POLICY_FLAG_IV_INO_LBLK_32)) &&
+ !supported_iv_ino_lblk_policy(policy, inode))
return false;
if (memchr_inv(policy->__reserved, 0, sizeof(policy->__reserved))) {
diff --git a/fs/ext4/crypto.c b/fs/ext4/crypto.c
index a9221be67f2a7..2859d9569aa74 100644
--- a/fs/ext4/crypto.c
+++ b/fs/ext4/crypto.c
@@ -232,20 +232,13 @@ static bool ext4_has_stable_inodes(struct super_block *sb)
return ext4_has_feature_stable_inodes(sb);
}
-static void ext4_get_ino_and_lblk_bits(struct super_block *sb,
- int *ino_bits_ret, int *lblk_bits_ret)
-{
- *ino_bits_ret = 8 * sizeof(EXT4_SB(sb)->s_es->s_inodes_count);
- *lblk_bits_ret = 8 * sizeof(ext4_lblk_t);
-}
-
const struct fscrypt_operations ext4_cryptops = {
.needs_bounce_pages = 1,
+ .has_32bit_inodes = 1,
.legacy_key_prefix_for_backcompat = "ext4:",
.get_context= ext4_get_context,
.set_context= ext4_set_context,
[f2fs-dev] [PATCH v2 5/5] fscrypt: support crypto data unit size less than filesystem block size
From: Eric Biggers Until now, fscrypt has always used the filesystem block size as the granularity of file contents encryption. Two scenarios have come up where a sub-block granularity of contents encryption would be useful: 1. Inline encryption hardware that only supports a crypto data unit size that is less than the filesystem block size. 2. Support for direct I/O at a granularity less than the filesystem block size, for example at the block device's logical block size in order to match the traditional direct I/O alignment requirement. (1) first came up with older eMMC inline crypto hardware that only supports a crypto data unit size of 512 bytes. That specific case ultimately went away because all systems with that hardware continued using out of tree code and never actually upgraded to the upstream inline crypto framework. But, now it's coming back in a new way: some current UFS controllers only support a data unit size of 4096 bytes, and there is a proposal to increase the filesystem block size to 16K. (2) was discussed as a "nice to have" feature, though not essential, when support for direct I/O on encrypted files was being upstreamed. Still, the fact that this feature has come up several times does suggest it would be wise to have available. Therefore, this patch implements it by using one of the reserved bytes in fscrypt_policy_v2 to allow users to select a sub-block data unit size. Supported data unit sizes are powers of 2 between 512 and the filesystem block size, inclusively. Support is implemented for both the FS-layer and inline crypto cases. This patch focuses on the basic support for sub-block data units. Some things are out of scope of this patch but may be addressed later: - Supporting sub-block data units in combination with FSCRYPT_POLICY_FLAG_IV_INO_LBLK_64, in most cases. Unfortunately this combination usually causes data unit indices to exceed 32 bits, and thus fscrypt_supported_policy() correctly disallows it. The users who potentially need this combination are using f2fs. To support it, f2fs would need to provide an option to slightly reduce its max file size. - Supporting sub-block data units in combination with FSCRYPT_POLICY_FLAG_IV_INO_LBLK_32. This has the same problem described above, but also it will need special code to make DUN wraparound still happen on a FS block boundary. - Supporting use case (2) mentioned above. The encrypted direct I/O code will need to stop requiring and assuming FS block alignment. This won't be hard, but it belongs in a separate patch. - Supporting this feature on filesystems other than ext4 and f2fs. (Filesystems declare support for it via their fscrypt_operations.) On UBIFS, sub-block data units don't make sense because UBIFS encrypts variable-length blocks as a result of compression. CephFS could support it, but a bit more work would be needed to make the fscrypt_*_block_inplace functions play nicely with sub-block data units. I don't think there's a use case for this on CephFS anyway. Signed-off-by: Eric Biggers --- Documentation/filesystems/fscrypt.rst | 116 +++-- fs/crypto/bio.c | 39 fs/crypto/crypto.c| 139 ++ fs/crypto/fscrypt_private.h | 53 +++--- fs/crypto/inline_crypt.c | 20 +++- fs/crypto/keysetup.c | 3 + fs/crypto/policy.c| 34 ++- fs/ext4/crypto.c | 1 + fs/f2fs/super.c | 1 + include/linux/fscrypt.h | 12 +++ include/uapi/linux/fscrypt.h | 3 +- 11 files changed, 288 insertions(+), 133 deletions(-) diff --git a/Documentation/filesystems/fscrypt.rst b/Documentation/filesystems/fscrypt.rst index a624e92f2687f..42898f187246d 100644 --- a/Documentation/filesystems/fscrypt.rst +++ b/Documentation/filesystems/fscrypt.rst @@ -261,9 +261,9 @@ DIRECT_KEY policies The Adiantum encryption mode (see `Encryption modes and usage`_) is suitable for both contents and filenames encryption, and it accepts -long IVs --- long enough to hold both an 8-byte logical block number -and a 16-byte per-file nonce. Also, the overhead of each Adiantum key -is greater than that of an AES-256-XTS key. +long IVs --- long enough to hold both an 8-byte data unit index and a +16-byte per-file nonce. Also, the overhead of each Adiantum key is +greater than that of an AES-256-XTS key. Therefore, to improve performance and save memory, for Adiantum a "direct key" configuration is supported. When the user has enabled @@ -300,8 +300,8 @@ IV_INO_LBLK_32 policies IV_INO_LBLK_32 policies work like IV_INO_LBLK_64, except that for IV_INO_LBLK_32, the inode number is hashed with SipHash-2-4 (where the -SipHash key is derived from the master key) and added to the file -logical block number mod 2^32 to produce a 32-bit IV. +SipHash
[f2fs-dev] [PATCH v2 1/5] fscrypt: make it extra clear that key_prefix is deprecated
From: Eric Biggers
fscrypt_operations::key_prefix should not be set by any filesystems that
aren't setting it already. This is already documented, but apparently
it's not sufficiently clear, as both ceph and btrfs have tried to set
it. Rename the field to 'legacy_key_prefix_for_backcompat' and improve
the documentation to hopefully make it clearer.
Signed-off-by: Eric Biggers
---
fs/crypto/keysetup_v1.c | 5 +++--
fs/ext4/crypto.c| 2 +-
fs/f2fs/super.c | 2 +-
fs/ubifs/crypto.c | 2 +-
include/linux/fscrypt.h | 14 +-
5 files changed, 15 insertions(+), 10 deletions(-)
diff --git a/fs/crypto/keysetup_v1.c b/fs/crypto/keysetup_v1.c
index 75dabd9b27f9b..df44d0d2d44ea 100644
--- a/fs/crypto/keysetup_v1.c
+++ b/fs/crypto/keysetup_v1.c
@@ -299,6 +299,7 @@ int fscrypt_setup_v1_file_key(struct fscrypt_info *ci,
const u8 *raw_master_key)
int fscrypt_setup_v1_file_key_via_subscribed_keyrings(struct fscrypt_info *ci)
{
+ const struct super_block *sb = ci->ci_inode->i_sb;
struct key *key;
const struct fscrypt_key *payload;
int err;
@@ -306,8 +307,8 @@ int
fscrypt_setup_v1_file_key_via_subscribed_keyrings(struct fscrypt_info *ci)
key = find_and_lock_process_key(FSCRYPT_KEY_DESC_PREFIX,
ci->ci_policy.v1.master_key_descriptor,
ci->ci_mode->keysize, );
- if (key == ERR_PTR(-ENOKEY) && ci->ci_inode->i_sb->s_cop->key_prefix) {
- key =
find_and_lock_process_key(ci->ci_inode->i_sb->s_cop->key_prefix,
+ if (key == ERR_PTR(-ENOKEY) &&
sb->s_cop->legacy_key_prefix_for_backcompat) {
+ key =
find_and_lock_process_key(sb->s_cop->legacy_key_prefix_for_backcompat,
ci->ci_policy.v1.master_key_descriptor,
ci->ci_mode->keysize, );
}
diff --git a/fs/ext4/crypto.c b/fs/ext4/crypto.c
index 453d4da5de520..8cdb7bbc655b0 100644
--- a/fs/ext4/crypto.c
+++ b/fs/ext4/crypto.c
@@ -240,7 +240,7 @@ static void ext4_get_ino_and_lblk_bits(struct super_block
*sb,
}
const struct fscrypt_operations ext4_cryptops = {
- .key_prefix = "ext4:",
+ .legacy_key_prefix_for_backcompat = "ext4:",
.get_context= ext4_get_context,
.set_context= ext4_set_context,
.get_dummy_policy = ext4_get_dummy_policy,
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index a8c8232852bb1..8de799a8bad04 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -3231,7 +3231,7 @@ static struct block_device **f2fs_get_devices(struct
super_block *sb,
}
static const struct fscrypt_operations f2fs_cryptops = {
- .key_prefix = "f2fs:",
+ .legacy_key_prefix_for_backcompat = "f2fs:",
.get_context= f2fs_get_context,
.set_context= f2fs_set_context,
.get_dummy_policy = f2fs_get_dummy_policy,
diff --git a/fs/ubifs/crypto.c b/fs/ubifs/crypto.c
index 3125e76376ee6..fab90f9a8eaff 100644
--- a/fs/ubifs/crypto.c
+++ b/fs/ubifs/crypto.c
@@ -89,7 +89,7 @@ int ubifs_decrypt(const struct inode *inode, struct
ubifs_data_node *dn,
const struct fscrypt_operations ubifs_crypt_operations = {
.flags = FS_CFLG_OWN_PAGES,
- .key_prefix = "ubifs:",
+ .legacy_key_prefix_for_backcompat = "ubifs:",
.get_context= ubifs_crypt_get_context,
.set_context= ubifs_crypt_set_context,
.empty_dir = ubifs_crypt_empty_dir,
diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
index c895b12737a19..70e0d4917dd59 100644
--- a/include/linux/fscrypt.h
+++ b/include/linux/fscrypt.h
@@ -73,12 +73,16 @@ struct fscrypt_operations {
unsigned int flags;
/*
-* If set, this is a filesystem-specific key description prefix that
-* will be accepted for "logon" keys for v1 fscrypt policies, in
-* addition to the generic prefix "fscrypt:". This functionality is
-* deprecated, so new filesystems shouldn't set this field.
+* This field exists only for backwards compatibility reasons and should
+* only be set by the filesystems that are setting it already. It
+* contains the filesystem-specific key description prefix that is
+* accepted for "logon" keys for v1 fscrypt policies. This
+* functionality is deprecated in favor of the generic prefix
+* "fscrypt:", which itself is deprecated in favor of the filesystem
+* keyring ioctls such as FS_IOC_ADD_ENCRYPTION_KEY. Filesystems that
+* are newly adding fscrypt support should not set this field.
[f2fs-dev] [PATCH v2 3/5] fscrypt: use s_maxbytes instead of filesystem lblk_bits
From: Eric Biggers
For a given filesystem, the number of bits used by the maximum file
logical block number is computable from the maximum file size and block
size. These values are always present in struct super_block.
Therefore, compute it this way instead of using the value from
fscrypt_operations::get_ino_and_lblk_bits. Since filesystems always
have to set the super_block fields anyway, this avoids having to provide
this information redundantly via fscrypt_operations.
This change is in preparation for adding support for sub-block data
units. For that, the value that is needed will become "the maximum file
data unit index". A hardcoded value won't suffice for that; it will
need to be computed anyway.
Signed-off-by: Eric Biggers
---
fs/crypto/fscrypt_private.h | 10 ++
fs/crypto/inline_crypt.c| 7 ++-
fs/crypto/policy.c | 20 +++-
3 files changed, 23 insertions(+), 14 deletions(-)
diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
index 2d63da48635ab..4b113214b53af 100644
--- a/fs/crypto/fscrypt_private.h
+++ b/fs/crypto/fscrypt_private.h
@@ -296,6 +296,16 @@ union fscrypt_iv {
void fscrypt_generate_iv(union fscrypt_iv *iv, u64 lblk_num,
const struct fscrypt_info *ci);
+/*
+ * Return the number of bits used by the maximum file logical block number that
+ * is possible on the given filesystem.
+ */
+static inline int
+fscrypt_max_file_lblk_bits(const struct super_block *sb)
+{
+ return fls64(sb->s_maxbytes - 1) - sb->s_blocksize_bits;
+}
+
/* fname.c */
bool __fscrypt_fname_encrypted_size(const union fscrypt_policy *policy,
u32 orig_len, u32 max_len,
diff --git a/fs/crypto/inline_crypt.c b/fs/crypto/inline_crypt.c
index 8bfb3ce864766..7d9f6c167de58 100644
--- a/fs/crypto/inline_crypt.c
+++ b/fs/crypto/inline_crypt.c
@@ -41,9 +41,8 @@ static struct block_device **fscrypt_get_devices(struct
super_block *sb,
static unsigned int fscrypt_get_dun_bytes(const struct fscrypt_info *ci)
{
- struct super_block *sb = ci->ci_inode->i_sb;
+ const struct super_block *sb = ci->ci_inode->i_sb;
unsigned int flags = fscrypt_policy_flags(>ci_policy);
- int ino_bits = 64, lblk_bits = 64;
if (flags & FSCRYPT_POLICY_FLAG_DIRECT_KEY)
return offsetofend(union fscrypt_iv, nonce);
@@ -55,9 +54,7 @@ static unsigned int fscrypt_get_dun_bytes(const struct
fscrypt_info *ci)
return sizeof(__le32);
/* Default case: IVs are just the file logical block number */
- if (sb->s_cop->get_ino_and_lblk_bits)
- sb->s_cop->get_ino_and_lblk_bits(sb, _bits, _bits);
- return DIV_ROUND_UP(lblk_bits, 8);
+ return DIV_ROUND_UP(fscrypt_max_file_lblk_bits(sb), 8);
}
/*
diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
index f4456ecb3f877..36bffc4d6228d 100644
--- a/fs/crypto/policy.c
+++ b/fs/crypto/policy.c
@@ -119,8 +119,7 @@ static bool supported_direct_key_modes(const struct inode
*inode,
static bool supported_iv_ino_lblk_policy(const struct fscrypt_policy_v2
*policy,
const struct inode *inode,
-const char *type,
-int max_ino_bits, int max_lblk_bits)
+const char *type, int max_ino_bits)
{
struct super_block *sb = inode->i_sb;
int ino_bits = 64, lblk_bits = 64;
@@ -154,13 +153,18 @@ static bool supported_iv_ino_lblk_policy(const struct
fscrypt_policy_v2 *policy,
sb->s_cop->get_ino_and_lblk_bits(sb, _bits, _bits);
if (ino_bits > max_ino_bits) {
fscrypt_warn(inode,
-"Can't use %s policy on filesystem '%s' because
its inode numbers are too long",
+"Can't use %s policy on filesystem '%s' because
its maximum inode number is too large",
type, sb->s_id);
return false;
}
- if (lblk_bits > max_lblk_bits) {
+
+ /*
+* IV_INO_LBLK_64 and IV_INO_LBLK_32 both require that file logical
+* block numbers fit in 32 bits.
+*/
+ if (fscrypt_max_file_lblk_bits(sb) > 32) {
fscrypt_warn(inode,
-"Can't use %s policy on filesystem '%s' because
its block numbers are too long",
+"Can't use %s policy on filesystem '%s' because
its maximum file size is too large",
type, sb->s_id);
return false;
}
@@ -239,8 +243,7 @@ static bool fscrypt_supported_v2_policy(const struct
fscrypt_policy_v2 *policy,
return false;
if ((policy->flags & FSCRYPT_POLICY_FLAG_IV_INO_LBLK_64) &&am
[f2fs-dev] [PATCH v2 2/5] fscrypt: make the bounce page pool opt-in instead of opt-out
From: Eric Biggers
Replace FS_CFLG_OWN_PAGES with a bit flag 'needs_bounce_pages' which has
the opposite meaning. I.e., filesystems now opt into the bounce page
pool instead of opt out. Make fscrypt_alloc_bounce_page() check that
the bounce page pool has been initialized.
I believe the opt in makes more sense, since nothing else in
fscrypt_operations is opt out, and these days filesystems can choose to
use blk-crypto which doesn't need the fscrypt bounce page pool. Also, I
happen to be planning to add two more flags, and I wanted to fix the
"FS_CFLG_" name anyway as it wasn't prefixed with "FSCRYPT_".
Signed-off-by: Eric Biggers
---
fs/ceph/crypto.c| 1 +
fs/crypto/crypto.c | 9 -
fs/ext4/crypto.c| 1 +
fs/f2fs/super.c | 1 +
fs/ubifs/crypto.c | 1 -
include/linux/fscrypt.h | 19 ++-
6 files changed, 21 insertions(+), 11 deletions(-)
diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
index e4d5cd56a80b9..cc63f1e6fdef6 100644
--- a/fs/ceph/crypto.c
+++ b/fs/ceph/crypto.c
@@ -133,6 +133,7 @@ static const union fscrypt_policy
*ceph_get_dummy_policy(struct super_block *sb)
}
static struct fscrypt_operations ceph_fscrypt_ops = {
+ .needs_bounce_pages = 1,
.get_context= ceph_crypt_get_context,
.set_context= ceph_crypt_set_context,
.get_dummy_policy = ceph_get_dummy_policy,
diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
index 6a837e4b80dcb..803347a5d0a6d 100644
--- a/fs/crypto/crypto.c
+++ b/fs/crypto/crypto.c
@@ -49,6 +49,13 @@ EXPORT_SYMBOL(fscrypt_enqueue_decrypt_work);
struct page *fscrypt_alloc_bounce_page(gfp_t gfp_flags)
{
+ if (WARN_ON_ONCE(!fscrypt_bounce_page_pool)) {
+ /*
+* Oops, the filesystem called a function that uses the bounce
+* page pool, but it forgot to set needs_bounce_pages.
+*/
+ return NULL;
+ }
return mempool_alloc(fscrypt_bounce_page_pool, gfp_flags);
}
@@ -325,7 +332,7 @@ int fscrypt_initialize(struct super_block *sb)
return 0;
/* No need to allocate a bounce page pool if this FS won't use it. */
- if (sb->s_cop->flags & FS_CFLG_OWN_PAGES)
+ if (!sb->s_cop->needs_bounce_pages)
return 0;
mutex_lock(_init_mutex);
diff --git a/fs/ext4/crypto.c b/fs/ext4/crypto.c
index 8cdb7bbc655b0..a9221be67f2a7 100644
--- a/fs/ext4/crypto.c
+++ b/fs/ext4/crypto.c
@@ -240,6 +240,7 @@ static void ext4_get_ino_and_lblk_bits(struct super_block
*sb,
}
const struct fscrypt_operations ext4_cryptops = {
+ .needs_bounce_pages = 1,
.legacy_key_prefix_for_backcompat = "ext4:",
.get_context= ext4_get_context,
.set_context= ext4_set_context,
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 8de799a8bad04..276535af5bf3c 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -3231,6 +3231,7 @@ static struct block_device **f2fs_get_devices(struct
super_block *sb,
}
static const struct fscrypt_operations f2fs_cryptops = {
+ .needs_bounce_pages = 1,
.legacy_key_prefix_for_backcompat = "f2fs:",
.get_context= f2fs_get_context,
.set_context= f2fs_set_context,
diff --git a/fs/ubifs/crypto.c b/fs/ubifs/crypto.c
index fab90f9a8eaff..f0ca403777d9a 100644
--- a/fs/ubifs/crypto.c
+++ b/fs/ubifs/crypto.c
@@ -88,7 +88,6 @@ int ubifs_decrypt(const struct inode *inode, struct
ubifs_data_node *dn,
}
const struct fscrypt_operations ubifs_crypt_operations = {
- .flags = FS_CFLG_OWN_PAGES,
.legacy_key_prefix_for_backcompat = "ubifs:",
.get_context= ubifs_crypt_get_context,
.set_context= ubifs_crypt_set_context,
diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
index 70e0d4917dd59..32290e5fa9abb 100644
--- a/include/linux/fscrypt.h
+++ b/include/linux/fscrypt.h
@@ -59,18 +59,19 @@ struct fscrypt_name {
#ifdef CONFIG_FS_ENCRYPTION
-/*
- * If set, the fscrypt bounce page pool won't be allocated (unless another
- * filesystem needs it). Set this if the filesystem always uses its own bounce
- * pages for writes and therefore won't need the fscrypt bounce page pool.
- */
-#define FS_CFLG_OWN_PAGES (1U << 1)
-
/* Crypto operations for filesystems */
struct fscrypt_operations {
- /* Set of optional flags; see above for allowed flags */
- unsigned int flags;
+ /*
+* If set, then fs/crypto/ will allocate a global bounce page pool. The
+* bounce page pool is required by the following functions:
+*
+* - fscrypt_encrypt_pagecache_blocks()
+* - fscrypt_zeroout_range() for files not using inline crypto
+*
+* If the filesystem doesn't use those, it doesn't n
[f2fs-dev] [PATCH v2 0/5] fscrypt: add support for data_unit_size < fs_block_size
This patchset adds support for configuring the granularity of file contents encryption (a.k.a. the "crypto data unit size") to be less than the filesystem block size. The main use case for this is to support inline crypto hardware that only supports a data unit size that is less than the FS block size being used. Another possible use case is to support direct I/O on encrypted files without the FS block alignment restriction. Note that decreasing the crypto data unit size decreases efficiency, so this feature should only be used when necessary. For full details, see patch 5 which adds the actual feature. Patches 1-4 are preparatory patches. I've written an xfstest that verifies that when a sub-block data unit size is selected, the data on-disk is encrypted correctly with that data unit size. I'll be sending that out separately. Other testing of this patchset with xfstests has gone well, though it turns out a sub-block data unit size doesn't really work with IV_INO_LBLK_* yet (see patch 5). This patchset will cause some conflicts in the extent-based encryption patches that the btrfs folks are working on, as both are touching file contents encryption, but logically they are orthogonal features. This patchset is based on v6.6-rc1. Changed in v2: - Rebased onto v6.6-rc1 and took into account CephFS's recent addition of support for fscrypt - Narrowed the focus somewhat by dropping the attempted support for IV_INO_LBLK_32 and clearly documenting what is considered out of scope for now - Other cleanups Eric Biggers (5): fscrypt: make it extra clear that key_prefix is deprecated fscrypt: make the bounce page pool opt-in instead of opt-out fscrypt: use s_maxbytes instead of filesystem lblk_bits fscrypt: replace get_ino_and_lblk_bits with just has_32bit_inodes fscrypt: support crypto data unit size less than filesystem block size Documentation/filesystems/fscrypt.rst | 116 ++-- fs/ceph/crypto.c | 1 + fs/crypto/bio.c | 39 --- fs/crypto/crypto.c| 148 +++--- fs/crypto/fscrypt_private.h | 55 -- fs/crypto/inline_crypt.c | 25 +++-- fs/crypto/keysetup.c | 3 + fs/crypto/keysetup_v1.c | 5 +- fs/crypto/policy.c| 75 - fs/ext4/crypto.c | 13 +-- fs/f2fs/super.c | 13 +-- fs/ubifs/crypto.c | 3 +- include/linux/fscrypt.h | 71 +++- include/uapi/linux/fscrypt.h | 3 +- 14 files changed, 364 insertions(+), 206 deletions(-) base-commit: 0bb80ecc33a8fb5a682236443c1e740d5c917d1d -- 2.42.0 ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH 2/2] fsverity: move sysctl registration out of signature.c
On Wed, Sep 06, 2023 at 03:49:04PM +0200, Joel Granados wrote:
> On Wed, Jul 05, 2023 at 02:27:43PM -0700, Eric Biggers wrote:
> > From: Eric Biggers
> >
> > Currently the registration of the fsverity sysctls happens in
> > signature.c, which couples it to CONFIG_FS_VERITY_BUILTIN_SIGNATURES.
> >
> > This makes it hard to add new sysctls unrelated to builtin signatures.
> >
> > Also, some users have started checking whether the directory
> > /proc/sys/fs/verity exists as a way to tell whether fsverity is
> > supported. This isn't the intended method; instead, the existence of
> > /sys/fs/$fstype/features/verity should be checked, or users should just
> > try to use the fsverity ioctls. Regardlesss, it should be made to work
> > as expected without a dependency on CONFIG_FS_VERITY_BUILTIN_SIGNATURES.
> >
> > Therefore, move the sysctl registration into init.c. With
> > CONFIG_FS_VERITY_BUILTIN_SIGNATURES, nothing changes. Without it, but
> > with CONFIG_FS_VERITY, an empty list of sysctls is now registered.
> >
> > Signed-off-by: Eric Biggers
> > ---
> > fs/verity/fsverity_private.h | 1 +
> > fs/verity/init.c | 32
> > fs/verity/signature.c| 33 +
> > 3 files changed, 34 insertions(+), 32 deletions(-)
> >
> > diff --git a/fs/verity/fsverity_private.h b/fs/verity/fsverity_private.h
> > index c5ab9023dd2d3..d071a6e32581e 100644
> > --- a/fs/verity/fsverity_private.h
> > +++ b/fs/verity/fsverity_private.h
> > @@ -123,6 +123,7 @@ void __init fsverity_init_info_cache(void);
> > /* signature.c */
> >
> > #ifdef CONFIG_FS_VERITY_BUILTIN_SIGNATURES
> > +extern int fsverity_require_signatures;
> > int fsverity_verify_signature(const struct fsverity_info *vi,
> > const u8 *signature, size_t sig_size);
> >
> > diff --git a/fs/verity/init.c b/fs/verity/init.c
> > index bcd11d63eb1ca..a29f062f6047b 100644
> > --- a/fs/verity/init.c
> > +++ b/fs/verity/init.c
> > @@ -9,6 +9,37 @@
> >
> > #include
> >
> > +#ifdef CONFIG_SYSCTL
> > +static struct ctl_table_header *fsverity_sysctl_header;
> > +
> > +static struct ctl_table fsverity_sysctl_table[] = {
> > +#ifdef CONFIG_FS_VERITY_BUILTIN_SIGNATURES
> > + {
> > + .procname = "require_signatures",
> > + .data = _require_signatures,
> > + .maxlen = sizeof(int),
> > + .mode = 0644,
> > + .proc_handler = proc_dointvec_minmax,
> > + .extra1 = SYSCTL_ZERO,
> > + .extra2 = SYSCTL_ONE,
> > + },
> > +#endif
> > + { }
> > +};
> > +
> Just to double check: When CONFIG_FS_VERITY_BUILTIN_SIGNATURES is not
> defined then you expect an empty directory under "fs/verity". right?
Yes, that's correct.
- Eric
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
[f2fs-dev] [PATCH 1/5] fscrypt: make it extra clear that key_prefix is deprecated
From: Eric Biggers
fscrypt_operations::key_prefix should not be set by any filesystems that
aren't setting it already. This is already documented, but apparently
it's not sufficiently clear, as both ceph and btrfs have tried to set
it. Rename the field to 'legacy_key_prefix_for_backcompat' and improve
the documentation to hopefully make it clearer.
Signed-off-by: Eric Biggers
---
fs/crypto/keysetup_v1.c | 5 +++--
fs/ext4/crypto.c| 2 +-
fs/f2fs/super.c | 2 +-
fs/ubifs/crypto.c | 2 +-
include/linux/fscrypt.h | 14 +-
5 files changed, 15 insertions(+), 10 deletions(-)
diff --git a/fs/crypto/keysetup_v1.c b/fs/crypto/keysetup_v1.c
index 75dabd9b27f9b..df44d0d2d44ea 100644
--- a/fs/crypto/keysetup_v1.c
+++ b/fs/crypto/keysetup_v1.c
@@ -299,6 +299,7 @@ int fscrypt_setup_v1_file_key(struct fscrypt_info *ci,
const u8 *raw_master_key)
int fscrypt_setup_v1_file_key_via_subscribed_keyrings(struct fscrypt_info *ci)
{
+ const struct super_block *sb = ci->ci_inode->i_sb;
struct key *key;
const struct fscrypt_key *payload;
int err;
@@ -306,8 +307,8 @@ int
fscrypt_setup_v1_file_key_via_subscribed_keyrings(struct fscrypt_info *ci)
key = find_and_lock_process_key(FSCRYPT_KEY_DESC_PREFIX,
ci->ci_policy.v1.master_key_descriptor,
ci->ci_mode->keysize, );
- if (key == ERR_PTR(-ENOKEY) && ci->ci_inode->i_sb->s_cop->key_prefix) {
- key =
find_and_lock_process_key(ci->ci_inode->i_sb->s_cop->key_prefix,
+ if (key == ERR_PTR(-ENOKEY) &&
sb->s_cop->legacy_key_prefix_for_backcompat) {
+ key =
find_and_lock_process_key(sb->s_cop->legacy_key_prefix_for_backcompat,
ci->ci_policy.v1.master_key_descriptor,
ci->ci_mode->keysize, );
}
diff --git a/fs/ext4/crypto.c b/fs/ext4/crypto.c
index 453d4da5de520..8cdb7bbc655b0 100644
--- a/fs/ext4/crypto.c
+++ b/fs/ext4/crypto.c
@@ -240,7 +240,7 @@ static void ext4_get_ino_and_lblk_bits(struct super_block
*sb,
}
const struct fscrypt_operations ext4_cryptops = {
- .key_prefix = "ext4:",
+ .legacy_key_prefix_for_backcompat = "ext4:",
.get_context= ext4_get_context,
.set_context= ext4_set_context,
.get_dummy_policy = ext4_get_dummy_policy,
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index a8c8232852bb1..8de799a8bad04 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -3231,7 +3231,7 @@ static struct block_device **f2fs_get_devices(struct
super_block *sb,
}
static const struct fscrypt_operations f2fs_cryptops = {
- .key_prefix = "f2fs:",
+ .legacy_key_prefix_for_backcompat = "f2fs:",
.get_context= f2fs_get_context,
.set_context= f2fs_set_context,
.get_dummy_policy = f2fs_get_dummy_policy,
diff --git a/fs/ubifs/crypto.c b/fs/ubifs/crypto.c
index 3125e76376ee6..fab90f9a8eaff 100644
--- a/fs/ubifs/crypto.c
+++ b/fs/ubifs/crypto.c
@@ -89,7 +89,7 @@ int ubifs_decrypt(const struct inode *inode, struct
ubifs_data_node *dn,
const struct fscrypt_operations ubifs_crypt_operations = {
.flags = FS_CFLG_OWN_PAGES,
- .key_prefix = "ubifs:",
+ .legacy_key_prefix_for_backcompat = "ubifs:",
.get_context= ubifs_crypt_get_context,
.set_context= ubifs_crypt_set_context,
.empty_dir = ubifs_crypt_empty_dir,
diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
index c895b12737a19..85574282c7e52 100644
--- a/include/linux/fscrypt.h
+++ b/include/linux/fscrypt.h
@@ -73,12 +73,16 @@ struct fscrypt_operations {
unsigned int flags;
/*
-* If set, this is a filesystem-specific key description prefix that
-* will be accepted for "logon" keys for v1 fscrypt policies, in
-* addition to the generic prefix "fscrypt:". This functionality is
-* deprecated, so new filesystems shouldn't set this field.
+* This field exists only for backwards compatibility reasons and should
+* only be set by the filesystems that are setting it already. It
+* contains the filesystem-specific key description prefix that is
+* accepted for "logon" keys for v1 fscrypt policies, in addition to the
+* generic prefix "fscrypt:". This functionality is deprecated in favor
+* of "fscrypt:", which itself is deprecated in favor of the filesystem
+* keyring ioctls such as FS_IOC_ADD_ENCRYPTION_KEY. Filesystems that
+* are newly adding fscrypt support should not set th
[f2fs-dev] [PATCH 5/5] fscrypt: support crypto data unit size less than filesystem block size
From: Eric Biggers Until now, fscrypt has always used the filesystem block size as the granularity of file contents encryption. Two scenarios have come up where a sub-block granularity of contents encryption would be useful: 1. Inline encryption hardware that only supports a crypto data unit size that is less than the filesystem block size. 2. Support for direct I/O at a granularity less than the filesystem block size, for example at the block device's logical block size in order to match the traditional direct I/O alignment requirement. (1) first came up with older eMMC inline crypto hardware that only supports a crypto data unit size of 512 bytes. That specific case ultimately went away because all systems with that hardware continued using out of tree code and never actually upgraded to the upstream inline crypto framework. But, now it's coming back in a new way: some current UFS controllers only support a data unit size of 4096 bytes, and there is a proposal to increase the filesystem block size to 16K. (2) was discussed as a "nice to have" feature, though not essential, when support for direct I/O on encrypted files was being upstreamed. Still, the fact that this feature has come up several times does suggest it would be wise to have available. Therefore, this patch implements it by using one of the reserved bytes in fscrypt_policy_v2 to allow users to select a sub-block data unit size. Supported values are powers of 2 between 512 bytes and the filesystem block size, inclusively. Support is implemented for both the FS-layer and inline crypto cases. This feature is incompatible with filesystems that encrypt variable length blocks as a result of compression, i.e. UBIFS. Therefore, this feature is made available only for filesystems that opt in via their fscrypt_operations. Make ext4 and f2fs opt in. Signed-off-by: Eric Biggers --- Documentation/filesystems/fscrypt.rst | 115 +++-- fs/crypto/bio.c | 39 fs/crypto/crypto.c| 139 ++ fs/crypto/fscrypt_private.h | 49 ++--- fs/crypto/inline_crypt.c | 41 ++-- fs/crypto/keysetup.c | 21 +++- fs/crypto/policy.c| 28 +- fs/ext4/crypto.c | 1 + fs/f2fs/super.c | 1 + include/linux/fscrypt.h | 12 +++ include/uapi/linux/fscrypt.h | 3 +- 11 files changed, 310 insertions(+), 139 deletions(-) diff --git a/Documentation/filesystems/fscrypt.rst b/Documentation/filesystems/fscrypt.rst index a624e92f2687f..010c066cf384c 100644 --- a/Documentation/filesystems/fscrypt.rst +++ b/Documentation/filesystems/fscrypt.rst @@ -261,9 +261,9 @@ DIRECT_KEY policies The Adiantum encryption mode (see `Encryption modes and usage`_) is suitable for both contents and filenames encryption, and it accepts -long IVs --- long enough to hold both an 8-byte logical block number -and a 16-byte per-file nonce. Also, the overhead of each Adiantum key -is greater than that of an AES-256-XTS key. +long IVs --- long enough to hold both an 8-byte data unit index and a +16-byte per-file nonce. Also, the overhead of each Adiantum key is +greater than that of an AES-256-XTS key. Therefore, to improve performance and save memory, for Adiantum a "direct key" configuration is supported. When the user has enabled @@ -300,8 +300,8 @@ IV_INO_LBLK_32 policies IV_INO_LBLK_32 policies work like IV_INO_LBLK_64, except that for IV_INO_LBLK_32, the inode number is hashed with SipHash-2-4 (where the -SipHash key is derived from the master key) and added to the file -logical block number mod 2^32 to produce a 32-bit IV. +SipHash key is derived from the master key) and added to the file data +unit index mod 2^32 to produce a 32-bit IV. This format is optimized for use with inline encryption hardware compliant with the eMMC v5.2 standard, which supports only 32 IV bits @@ -451,31 +451,62 @@ acceleration is recommended: Contents encryption --- -For file contents, each filesystem block is encrypted independently. -Starting from Linux kernel 5.5, encryption of filesystems with block -size less than system's page size is supported. - -Each block's IV is set to the logical block number within the file as -a little endian number, except that: - -- With CBC mode encryption, ESSIV is also used. Specifically, each IV - is encrypted with AES-256 where the AES-256 key is the SHA-256 hash - of the file's data encryption key. - -- With `DIRECT_KEY policies`_, the file's nonce is appended to the IV. - Currently this is only allowed with the Adiantum encryption mode. - -- With `IV_INO_LBLK_64 policies`_, the logical block number is limited - to 32 bits and is placed in bits 0-31 of the IV. The inode number - (which is also limited to 32 bits) is placed in bits 32-63. - -- With `IV_INO_LBLK_32 pol
[f2fs-dev] [PATCH 3/5] fscrypt: use s_maxbytes instead of filesystem lblk_bits
From: Eric Biggers
For a given filesystem, the number of bits used by the maximum file
logical block number is computable from the maximum file size and block
size. These values are always present in struct super_block.
Therefore, compute it this way instead of using the value from
fscrypt_operations::get_ino_and_lblk_bits. Since filesystems always
have to set the super_block fields anyway, this avoids having to provide
this information redundantly via fscrypt_operations.
This change is in preparation for adding support for sub-block data
units. For that, the value that is needed will become "the maximum file
data unit number". A hardcoded value won't suffice for that; it will
need to be computed anyway.
Signed-off-by: Eric Biggers
---
fs/crypto/fscrypt_private.h | 10 ++
fs/crypto/inline_crypt.c| 7 ++-
fs/crypto/policy.c | 20 +++-
3 files changed, 23 insertions(+), 14 deletions(-)
diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
index 2d63da48635ab..4b113214b53af 100644
--- a/fs/crypto/fscrypt_private.h
+++ b/fs/crypto/fscrypt_private.h
@@ -296,6 +296,16 @@ union fscrypt_iv {
void fscrypt_generate_iv(union fscrypt_iv *iv, u64 lblk_num,
const struct fscrypt_info *ci);
+/*
+ * Return the number of bits used by the maximum file logical block number that
+ * is possible on the given filesystem.
+ */
+static inline int
+fscrypt_max_file_lblk_bits(const struct super_block *sb)
+{
+ return fls64(sb->s_maxbytes - 1) - sb->s_blocksize_bits;
+}
+
/* fname.c */
bool __fscrypt_fname_encrypted_size(const union fscrypt_policy *policy,
u32 orig_len, u32 max_len,
diff --git a/fs/crypto/inline_crypt.c b/fs/crypto/inline_crypt.c
index 8bfb3ce864766..7d9f6c167de58 100644
--- a/fs/crypto/inline_crypt.c
+++ b/fs/crypto/inline_crypt.c
@@ -41,9 +41,8 @@ static struct block_device **fscrypt_get_devices(struct
super_block *sb,
static unsigned int fscrypt_get_dun_bytes(const struct fscrypt_info *ci)
{
- struct super_block *sb = ci->ci_inode->i_sb;
+ const struct super_block *sb = ci->ci_inode->i_sb;
unsigned int flags = fscrypt_policy_flags(>ci_policy);
- int ino_bits = 64, lblk_bits = 64;
if (flags & FSCRYPT_POLICY_FLAG_DIRECT_KEY)
return offsetofend(union fscrypt_iv, nonce);
@@ -55,9 +54,7 @@ static unsigned int fscrypt_get_dun_bytes(const struct
fscrypt_info *ci)
return sizeof(__le32);
/* Default case: IVs are just the file logical block number */
- if (sb->s_cop->get_ino_and_lblk_bits)
- sb->s_cop->get_ino_and_lblk_bits(sb, _bits, _bits);
- return DIV_ROUND_UP(lblk_bits, 8);
+ return DIV_ROUND_UP(fscrypt_max_file_lblk_bits(sb), 8);
}
/*
diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
index f4456ecb3f877..36bffc4d6228d 100644
--- a/fs/crypto/policy.c
+++ b/fs/crypto/policy.c
@@ -119,8 +119,7 @@ static bool supported_direct_key_modes(const struct inode
*inode,
static bool supported_iv_ino_lblk_policy(const struct fscrypt_policy_v2
*policy,
const struct inode *inode,
-const char *type,
-int max_ino_bits, int max_lblk_bits)
+const char *type, int max_ino_bits)
{
struct super_block *sb = inode->i_sb;
int ino_bits = 64, lblk_bits = 64;
@@ -154,13 +153,18 @@ static bool supported_iv_ino_lblk_policy(const struct
fscrypt_policy_v2 *policy,
sb->s_cop->get_ino_and_lblk_bits(sb, _bits, _bits);
if (ino_bits > max_ino_bits) {
fscrypt_warn(inode,
-"Can't use %s policy on filesystem '%s' because
its inode numbers are too long",
+"Can't use %s policy on filesystem '%s' because
its maximum inode number is too large",
type, sb->s_id);
return false;
}
- if (lblk_bits > max_lblk_bits) {
+
+ /*
+* IV_INO_LBLK_64 and IV_INO_LBLK_32 both require that file logical
+* block numbers fit in 32 bits.
+*/
+ if (fscrypt_max_file_lblk_bits(sb) > 32) {
fscrypt_warn(inode,
-"Can't use %s policy on filesystem '%s' because
its block numbers are too long",
+"Can't use %s policy on filesystem '%s' because
its maximum file size is too large",
type, sb->s_id);
return false;
}
@@ -239,8 +243,7 @@ static bool fscrypt_supported_v2_policy(const struct
fscrypt_policy_v2 *policy,
return false;
if ((policy->flags & FSCRYPT_POLICY_FLAG_IV_INO_LBLK_64) &&am
[f2fs-dev] [PATCH 4/5] fscrypt: replace get_ino_and_lblk_bits with just has_32bit_inodes
From: Eric Biggers
Now that fs/crypto/ computes the filesystem's lblk_bits from its maximum
file size, it is no longer necessary for filesystems to provide
lblk_bits via fscrypt_operations::get_ino_and_lblk_bits.
It is still necessary for fs/crypto/ to retrieve ino_bits from the
filesystem. However, this is used only to decide whether inode numbers
fit in 32 bits. Also, ino_bits is static for all relevant filesystems,
i.e. it doesn't depend on the filesystem instance.
Therefore, in the interest of keeping things as simple as possible,
replace 'get_ino_and_lblk_bits' with a flag 'has_32bit_inodes'. This
can always be changed back to a function if a filesystem needs it to be
dynamic, but for now a static flag is all that's needed.
Signed-off-by: Eric Biggers
---
fs/crypto/policy.c | 33 +++--
fs/ext4/crypto.c| 9 +
fs/f2fs/super.c | 9 +
include/linux/fscrypt.h | 26 +++---
4 files changed, 28 insertions(+), 49 deletions(-)
diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
index 36bffc4d6228d..c8072a634af8f 100644
--- a/fs/crypto/policy.c
+++ b/fs/crypto/policy.c
@@ -118,11 +118,11 @@ static bool supported_direct_key_modes(const struct inode
*inode,
}
static bool supported_iv_ino_lblk_policy(const struct fscrypt_policy_v2
*policy,
-const struct inode *inode,
-const char *type, int max_ino_bits)
+const struct inode *inode)
{
+ const char *type = (policy->flags & FSCRYPT_POLICY_FLAG_IV_INO_LBLK_64)
+ ? "IV_INO_LBLK_64" : "IV_INO_LBLK_32";
struct super_block *sb = inode->i_sb;
- int ino_bits = 64, lblk_bits = 64;
/*
* IV_INO_LBLK_* exist only because of hardware limitations, and
@@ -149,9 +149,15 @@ static bool supported_iv_ino_lblk_policy(const struct
fscrypt_policy_v2 *policy,
type, sb->s_id);
return false;
}
- if (sb->s_cop->get_ino_and_lblk_bits)
- sb->s_cop->get_ino_and_lblk_bits(sb, _bits, _bits);
- if (ino_bits > max_ino_bits) {
+
+ /*
+* IV_INO_LBLK_64 requires that inode numbers fit in 32 bits.
+* IV_INO_LBLK_32 hashes the inode number, so in principle it can
+* support any length; however, currently the inode number is gotten
+* from inode::i_ino which is 'unsigned long'. So for now the
+* implementation limit is 32 bits, the same as IV_INO_LBLK_64.
+*/
+ if (!sb->s_cop->has_32bit_inodes) {
fscrypt_warn(inode,
"Can't use %s policy on filesystem '%s' because
its maximum inode number is too large",
type, sb->s_id);
@@ -242,18 +248,9 @@ static bool fscrypt_supported_v2_policy(const struct
fscrypt_policy_v2 *policy,
policy->filenames_encryption_mode))
return false;
- if ((policy->flags & FSCRYPT_POLICY_FLAG_IV_INO_LBLK_64) &&
- !supported_iv_ino_lblk_policy(policy, inode, "IV_INO_LBLK_64", 32))
- return false;
-
- /*
-* IV_INO_LBLK_32 hashes the inode number, so in principle it can
-* support any ino_bits. However, currently the inode number is gotten
-* from inode::i_ino which is 'unsigned long'. So for now the
-* implementation limit is 32 bits.
-*/
- if ((policy->flags & FSCRYPT_POLICY_FLAG_IV_INO_LBLK_32) &&
- !supported_iv_ino_lblk_policy(policy, inode, "IV_INO_LBLK_32", 32))
+ if ((policy->flags & (FSCRYPT_POLICY_FLAG_IV_INO_LBLK_64 |
+ FSCRYPT_POLICY_FLAG_IV_INO_LBLK_32)) &&
+ !supported_iv_ino_lblk_policy(policy, inode))
return false;
if (memchr_inv(policy->__reserved, 0, sizeof(policy->__reserved))) {
diff --git a/fs/ext4/crypto.c b/fs/ext4/crypto.c
index a9221be67f2a7..2859d9569aa74 100644
--- a/fs/ext4/crypto.c
+++ b/fs/ext4/crypto.c
@@ -232,20 +232,13 @@ static bool ext4_has_stable_inodes(struct super_block *sb)
return ext4_has_feature_stable_inodes(sb);
}
-static void ext4_get_ino_and_lblk_bits(struct super_block *sb,
- int *ino_bits_ret, int *lblk_bits_ret)
-{
- *ino_bits_ret = 8 * sizeof(EXT4_SB(sb)->s_es->s_inodes_count);
- *lblk_bits_ret = 8 * sizeof(ext4_lblk_t);
-}
-
const struct fscrypt_operations ext4_cryptops = {
.needs_bounce_pages = 1,
+ .has_32bit_inodes = 1,
.legacy_key_prefix_for_backcompat = "ext4:",
.get_context= ext4_get_context,
.set_context= ext4_set_context,
[f2fs-dev] [PATCH 0/5] fscrypt: add support for data_unit_size < fs_block_size
Until now, fscrypt has always used the filesystem block size as the granularity of file contents encryption. Two scenarios have come up where a sub-block granularity of contents encryption would be useful: 1. Inline encryption hardware that only supports a crypto data unit size that is less than the filesystem block size. 2. Support for direct I/O at a granularity less than the filesystem block size, for example at the block device's logical block size in order to match the traditional direct I/O alignment requirement. (1) first came up with older eMMC inline crypto hardware that only supports a crypto data unit size of 512 bytes. That specific case ultimately went away because all systems with that hardware continued using out of tree code and never actually upgraded to the upstream inline crypto framework. But, now it's coming back in a new way: some current UFS controllers only support a data unit size of 4096 bytes, and there is a proposal to increase the filesystem block size to 16K. (2) was discussed as a "nice to have" feature, though not essential, when support for direct I/O on encrypted files was being upstreamed. Still, the fact that this feature has come up several times does suggest it would be wise to have available. Therefore, this patchset implements it by using one of the reserved bytes in fscrypt_policy_v2 to allow users to select a sub-block data unit size. Supported values are powers of 2 between 512 bytes and the filesystem block size, inclusively. Patch 1-4 are preparatory patches. Patch 5 is the actual feature. Testing and userspace support are still in progress; there may be bugs in this patchset. I just wanted to get this out early in case anyone has feedback on the feature itself and its likely implementation. Note: this is unrelated to the work on extent based encryption that is ongoing by the btrfs folks. This is basically an orthogonal feature. This patchset is based on mainline commit 708283abf896dd48 Eric Biggers (5): fscrypt: make it extra clear that key_prefix is deprecated fscrypt: make the bounce page pool opt-in instead of opt-out fscrypt: use s_maxbytes instead of filesystem lblk_bits fscrypt: replace get_ino_and_lblk_bits with just has_32bit_inodes fscrypt: support crypto data unit size less than filesystem block size Documentation/filesystems/fscrypt.rst | 115 ++-- fs/crypto/bio.c | 39 --- fs/crypto/crypto.c| 148 +++--- fs/crypto/fscrypt_private.h | 51 +++-- fs/crypto/inline_crypt.c | 46 ++-- fs/crypto/keysetup.c | 21 +++- fs/crypto/keysetup_v1.c | 5 +- fs/crypto/policy.c| 69 +++- fs/ext4/crypto.c | 13 +-- fs/f2fs/super.c | 13 +-- fs/ubifs/crypto.c | 3 +- include/linux/fscrypt.h | 71 +++- include/uapi/linux/fscrypt.h | 3 +- 13 files changed, 385 insertions(+), 212 deletions(-) base-commit: 708283abf896dd4853e673cc8cba70acaf9bf4ea -- 2.42.0 ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
[f2fs-dev] [PATCH 2/5] fscrypt: make the bounce page pool opt-in instead of opt-out
From: Eric Biggers
Replace FS_CFLG_OWN_PAGES with a bit flag 'needs_bounce_pages' which has
the opposite meaning. I.e., filesystems now opt into the bounce page
pool instead of opt out. Make fscrypt_alloc_bounce_page() check that
the bounce page pool has been initialized.
I believe the opt in makes more sense, since nothing else in
fscrypt_operations is opt out, and these days filesystems can choose to
use blk-crypto which doesn't need the fscrypt bounce page pool. Also, I
happen to be planning to add two more flags, and I wanted to fix the
"FS_CFLG_" name anyway as it wasn't prefixed with "FSCRYPT_".
Signed-off-by: Eric Biggers
---
fs/crypto/crypto.c | 9 -
fs/ext4/crypto.c| 1 +
fs/f2fs/super.c | 1 +
fs/ubifs/crypto.c | 1 -
include/linux/fscrypt.h | 19 ++-
5 files changed, 20 insertions(+), 11 deletions(-)
diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
index 6a837e4b80dcb..803347a5d0a6d 100644
--- a/fs/crypto/crypto.c
+++ b/fs/crypto/crypto.c
@@ -49,6 +49,13 @@ EXPORT_SYMBOL(fscrypt_enqueue_decrypt_work);
struct page *fscrypt_alloc_bounce_page(gfp_t gfp_flags)
{
+ if (WARN_ON_ONCE(!fscrypt_bounce_page_pool)) {
+ /*
+* Oops, the filesystem called a function that uses the bounce
+* page pool, but it forgot to set needs_bounce_pages.
+*/
+ return NULL;
+ }
return mempool_alloc(fscrypt_bounce_page_pool, gfp_flags);
}
@@ -325,7 +332,7 @@ int fscrypt_initialize(struct super_block *sb)
return 0;
/* No need to allocate a bounce page pool if this FS won't use it. */
- if (sb->s_cop->flags & FS_CFLG_OWN_PAGES)
+ if (!sb->s_cop->needs_bounce_pages)
return 0;
mutex_lock(_init_mutex);
diff --git a/fs/ext4/crypto.c b/fs/ext4/crypto.c
index 8cdb7bbc655b0..a9221be67f2a7 100644
--- a/fs/ext4/crypto.c
+++ b/fs/ext4/crypto.c
@@ -240,6 +240,7 @@ static void ext4_get_ino_and_lblk_bits(struct super_block
*sb,
}
const struct fscrypt_operations ext4_cryptops = {
+ .needs_bounce_pages = 1,
.legacy_key_prefix_for_backcompat = "ext4:",
.get_context= ext4_get_context,
.set_context= ext4_set_context,
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 8de799a8bad04..276535af5bf3c 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -3231,6 +3231,7 @@ static struct block_device **f2fs_get_devices(struct
super_block *sb,
}
static const struct fscrypt_operations f2fs_cryptops = {
+ .needs_bounce_pages = 1,
.legacy_key_prefix_for_backcompat = "f2fs:",
.get_context= f2fs_get_context,
.set_context= f2fs_set_context,
diff --git a/fs/ubifs/crypto.c b/fs/ubifs/crypto.c
index fab90f9a8eaff..f0ca403777d9a 100644
--- a/fs/ubifs/crypto.c
+++ b/fs/ubifs/crypto.c
@@ -88,7 +88,6 @@ int ubifs_decrypt(const struct inode *inode, struct
ubifs_data_node *dn,
}
const struct fscrypt_operations ubifs_crypt_operations = {
- .flags = FS_CFLG_OWN_PAGES,
.legacy_key_prefix_for_backcompat = "ubifs:",
.get_context= ubifs_crypt_get_context,
.set_context= ubifs_crypt_set_context,
diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
index 85574282c7e52..ac684f688d488 100644
--- a/include/linux/fscrypt.h
+++ b/include/linux/fscrypt.h
@@ -59,18 +59,19 @@ struct fscrypt_name {
#ifdef CONFIG_FS_ENCRYPTION
-/*
- * If set, the fscrypt bounce page pool won't be allocated (unless another
- * filesystem needs it). Set this if the filesystem always uses its own bounce
- * pages for writes and therefore won't need the fscrypt bounce page pool.
- */
-#define FS_CFLG_OWN_PAGES (1U << 1)
-
/* Crypto operations for filesystems */
struct fscrypt_operations {
- /* Set of optional flags; see above for allowed flags */
- unsigned int flags;
+ /*
+* If set, then fs/crypto/ will allocate a global bounce page pool. The
+* bounce page pool is required by the following functions:
+*
+* - fscrypt_encrypt_pagecache_blocks()
+* - fscrypt_zeroout_range() for files not using inline crypto
+*
+* If the filesystem doesn't use those, it doesn't need to set this.
+*/
+ unsigned int needs_bounce_pages : 1;
/*
* This field exists only for backwards compatibility reasons and should
--
2.42.0
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
[f2fs-dev] [PATCH] fsck.f2fs: use clearer info message for -a option
From: Eric Biggers The message "Info: Fix the reported corruption" makes people think that fsck has detected filesystem corruption. Replace this message with "Info: Automatic fix mode enabled" which is more accurate. Signed-off-by: Eric Biggers --- fsck/main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fsck/main.c b/fsck/main.c index 3690c74..dd8643c 100644 --- a/fsck/main.c +++ b/fsck/main.c @@ -263,7 +263,7 @@ void f2fs_parse_options(int argc, char *argv[]) break; case 'a': c.auto_fix = 1; - MSG(0, "Info: Fix the reported corruption.\n"); + MSG(0, "Info: Automatic fix mode enabled.\n"); break; case 'c': c.cache_config.num_cache_entry = atoi(optarg); base-commit: b15b6cc56ac7764be17acbdbf96448f388992adc -- 2.42.0 ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
[f2fs-dev] [PATCH] quota: explicitly forbid quota files from being encrypted
From: Eric Biggers
Since commit d7e7b9af104c ("fscrypt: stop using keyrings subsystem for
fscrypt_master_key"), xfstest generic/270 causes a WARNING when run on
f2fs with test_dummy_encryption in the mount options:
$ kvm-xfstests -c f2fs/encrypt generic/270
[...]
WARNING: CPU: 1 PID: 2453 at fs/crypto/keyring.c:240
fscrypt_destroy_keyring+0x1f5/0x260
The cause of the WARNING is that not all encrypted inodes have been
evicted before fscrypt_destroy_keyring() is called, which violates an
assumption. This happens because the test uses an external quota file,
which gets automatically encrypted due to test_dummy_encryption.
Encryption of quota files has never really been supported. On ext4,
ext4_quota_read() does not decrypt the data, so encrypted quota files
are always considered invalid on ext4. On f2fs, f2fs_quota_read() uses
the pagecache, so trying to use an encrypted quota file gets farther,
resulting in the issue described above being possible. But this was
never intended to be possible, and there is no use case for it.
Therefore, make the quota support layer explicitly reject using
IS_ENCRYPTED inodes when quotaon is attempted.
Cc: sta...@vger.kernel.org
Signed-off-by: Eric Biggers
---
fs/quota/dquot.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
index 9e72bfe8bbad9..7e268cd2727cc 100644
--- a/fs/quota/dquot.c
+++ b/fs/quota/dquot.c
@@ -2339,6 +2339,20 @@ static int vfs_setup_quota_inode(struct inode *inode,
int type)
if (sb_has_quota_loaded(sb, type))
return -EBUSY;
+ /*
+* Quota files should never be encrypted. They should be thought of as
+* filesystem metadata, not user data. New-style internal quota files
+* cannot be encrypted by users anyway, but old-style external quota
+* files could potentially be incorrectly created in an encrypted
+* directory, hence this explicit check. Some reasons why encrypted
+* quota files don't work include: (1) some filesystems that support
+* encryption don't handle it in their quota_read and quota_write, and
+* (2) cleaning up encrypted quota files at unmount would need special
+* consideration, as quota files are cleaned up later than user files.
+*/
+ if (IS_ENCRYPTED(inode))
+ return -EINVAL;
+
dqopt->files[type] = igrab(inode);
if (!dqopt->files[type])
return -EIO;
base-commit: 708283abf896dd4853e673cc8cba70acaf9bf4ea
--
2.42.0
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v6 0/9] Support negative dentries on case-insensitive ext4 and f2fs
On Wed, Aug 16, 2023 at 01:07:54AM -0400, Gabriel Krisman Bertazi wrote: > Hi, > > This is v6 of the negative dentry on case-insensitive directories. > Thanks Eric for the review of the last iteration. This version > drops the patch to expose the helper to check casefolding directories, > since it is not necessary in ecryptfs and it might be going away. It > also addresses some documentation details, fix a build bot error and > simplifies the commit messages. See the changelog in each patch for > more details. > > Thanks, > > --- > > Gabriel Krisman Bertazi (9): > ecryptfs: Reject casefold directory inodes > 9p: Split ->weak_revalidate from ->revalidate > fs: Expose name under lookup to d_revalidate hooks > fs: Add DCACHE_CASEFOLDED_NAME flag > libfs: Validate negative dentries in case-insensitive directories > libfs: Chain encryption checks after case-insensitive revalidation > libfs: Merge encrypted_ci_dentry_ops and ci_dentry_ops > ext4: Enable negative dentries on case-insensitive lookup > f2fs: Enable negative dentries on case-insensitive lookup > Looks good, Reviewed-by: Eric Biggers - Eric ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] circular locking dependency warning in f2fs
On Thu, Aug 17, 2023 at 10:26:12PM +0800, Chao Yu wrote: > > > > > > > > lock(new_inode#2->i_sem) > > > > > > > > lock(dir->i_xattr_sem) > > > > lock(new_inode#1->i_sem) > > > > > > > > This looks fine to me. > > > > > > > > > > Based on your feedback, am I correct assuming that you don't plan > > > to fix this ? > > > > I'm quite open to something that I may miss. Chao, what do you think? > > Jaegeuk, I agree with you, it looks like a false alarm. > False positive lockdep reports still need to be eliminated, for example by fixing the lockdep annotations. Otherwise it's impossible to distinguish them from true positives. - Eric ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH 0/1] Add 16K Support for f2fs
On Tue, Aug 15, 2023 at 06:14:31PM -0700, Daniel Rosenberg via Linux-f2fs-devel wrote: > F2fs filesystems currently have two large restrictions around block size. > The block size must equal the page size, and the block size must be 4096. > > The following patch, along with the associated f2fs-tools patch set, relax the > latter restriction, allowing you to use 16K block size f2fs on a 16K page size > system. It does not allow mounting 4K block size f2fs on a 16k page system. > > Doing that would require a lot more work, requiring a refactor of all block > sized struct similar to the userspace patches, as well as handling the block > reading/writing at sub page boundaries. As far as I know, buffer_heads are > still the main way this is handled in other filesystems. Is there a different > option there? I know there's a general desire to move away from buffer_heads, > but I don't know of any replacements covering that use case. And it would feel > a bit silly to not be able to read older filesystems from a 16k system... iomap is the replacement for buffer heads. See https://lwn.net/Articles/935934 - Eric ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v5 06/10] libfs: Validate negative dentries in case-insensitive directories
On Mon, Aug 14, 2023 at 03:21:33PM -0400, Gabriel Krisman Bertazi wrote:
> Eric Biggers writes:
>
> > On Mon, Aug 14, 2023 at 10:50:13AM -0400, Gabriel Krisman Bertazi wrote:
> >> Eric Biggers writes:
> >>
> >> > On Fri, Aug 11, 2023 at 08:41:42PM -0400, Gabriel Krisman Bertazi wrote:
> >> >> + /*
> >> >> +* Filesystems will call into d_revalidate without setting
> >> >> +* LOOKUP_ flags even for file creation (see lookup_one*
> >> >> +* variants). Reject negative dentries in this case, since we
> >> >> +* can't know for sure it won't be used for creation.
> >> >> +*/
> >> >> + if (!flags)
> >> >> + return 0;
> >> >> +
> >> >> + /*
> >> >> +* If the lookup is for creation, then a negative dentry can
> >> >> +* only be reused if it's a case-sensitive match, not just a
> >> >> +* case-insensitive one. This is needed to make the new file be
> >> >> +* created with the name the user specified, preserving case.
> >> >> +*/
> >> >> + if (flags & (LOOKUP_CREATE | LOOKUP_RENAME_TARGET)) {
> >> >> + /*
> >> >> +* ->d_name won't change from under us in the creation
> >> >> +* path only, since d_revalidate during creation and
> >> >> +* renames is always called with the parent inode
> >> >> +* locked. It isn't the case for all lookup callpaths,
> >> >> +* so ->d_name must not be touched outside
> >> >> +* (LOOKUP_CREATE|LOOKUP_RENAME_TARGET) context.
> >> >> +*/
> >> >> + if (dentry->d_name.len != name->len ||
> >> >> + memcmp(dentry->d_name.name, name->name, name->len))
> >> >> + return 0;
> >> >> + }
> >> >
> >> > This is still really confusing to me. Can you consider the below? The
> >> > code is
> >> > the same except for the reordering, but the explanation is reworked to
> >> > be much
> >> > clearer (IMO). Anything I am misunderstanding?
> >> >
> >> > /*
> >> > * If the lookup is for creation, then a negative dentry can only be
> >> > * reused if it's a case-sensitive match, not just a case-insensitive
> >> > * one. This is needed to make the new file be created with the name
> >> > * the user specified, preserving case.
> >> > *
> >> > * LOOKUP_CREATE or LOOKUP_RENAME_TARGET cover most creations. In these
> >> > * cases, ->d_name is stable and can be compared to 'name' without
> >> > * taking ->d_lock because the caller holds dir->i_rwsem for write.
> >> > * (This is because the directory lock blocks the dentry from being
> >> > * concurrently instantiated, and negative dentries are never moved.)
> >> > *
> >> > * All other creations actually use flags==0. These come from the edge
> >> > * case of filesystems calling functions like lookup_one() that do a
> >> > * lookup without setting the lookup flags at all. Such lookups might
> >> > * or might not be for creation, and if not don't guarantee stable
> >> > * ->d_name. Therefore, invalidate all negative dentries when flags==0.
> >> > */
> >> > if (flags & (LOOKUP_CREATE | LOOKUP_RENAME_TARGET)) {
> >> > if (dentry->d_name.len != name->len ||
> >> > memcmp(dentry->d_name.name, name->name, name->len))
> >> > return 0;
> >> > }
> >> > if (!flags)
> >> > return 0;
> >>
> >> I don't see it as particularly better or less confusing than the
> >> original. but I also don't mind taking it into the next iteration.
> >>
> >
> > Your commit message is still much longer and covers some quite different
> > details
> > which seem irrelevant to me. So if you don't see my explanation as being
> > much
> > different, I think we're still not on the same page. I hope that I'm not
> > misunderstanding anything, in which I believe that what I wrote above is a
> > good
> > explanation and your commit me
Re: [f2fs-dev] [PATCH 1/3] ext4: reject casefold inode flag without casefold feature
On Mon, Aug 14, 2023 at 03:09:33PM -0400, Gabriel Krisman Bertazi wrote: > Eric Biggers writes: > > > From: Eric Biggers > > > > It is invalid for the casefold inode flag to be set without the casefold > > superblock feature flag also being set. e2fsck already considers this > > case to be invalid and handles it by offering to clear the casefold flag > > on the inode. __ext4_iget() also already considered this to be invalid, > > sort of, but it only got so far as logging an error message; it didn't > > actually reject the inode. Make it reject the inode so that other code > > doesn't have to handle this case. This matches what f2fs does. > > > > Note: we could check 's_encoding != NULL' instead of > > ext4_has_feature_casefold(). This would make the check robust against > > the casefold feature being enabled by userspace writing to the page > > cache of the mounted block device. However, it's unsolvable in general > > for filesystems to be robust against concurrent writes to the page cache > > of the mounted block device. Though this very particular scenario > > involving the casefold feature is solvable, we should not pretend that > > we can support this model, so let's just check the casefold feature. > > tune2fs already forbids enabling casefold on a mounted filesystem. > > just because we can't fix the general issue for the entire filesystem > doesn't mean this case *must not* ever be addressed. What is the > advantage of making the code less robust against the syzbot code? Just > check sb->s_encoding and be safe later knowing the unicode map is > available. > Just to make sure, it sounds like you agree that the late checks of ->s_encoding are not needed and only __ext4_iget() should handle it, right? That simplifies the code so it is obviously beneficial if we can do it. As for whether __ext4_iget() should check the casefold feature or ->s_encoding, we should simply go with the one that makes the code clearer, as per what I've said. I think it's casefold, but it could be ->s_encoding if others prefer. - Eric ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v5 01/10] fs: Expose helper to check if a directory needs casefolding
On Mon, Aug 14, 2023 at 11:02:38AM -0400, Gabriel Krisman Bertazi wrote: > > Also, this patchset has been sitting for years before the latest > discussions, and I'm tired of it, so I'm happy to keep this > discussion for another time. Will drop this patch and just check > IS_CASEFOLDED in ecryptfs for the next iteration. > > I'll follow up with another case-insensitive cleanup patchset I've been > siting on forever, which includes this patch and will restart this > discussion, among others. > Well, as we know unfortunately filesystem developers are in short supply, and doing proper reviews (identifying issues and working closely with the patchset author over multiple iterations to address them, as opposed to just slapping on a Reviewed-by) is very time consuming. Earlier this year I tried to get the Android Systems team, which is ostensibly responsible for Android's use of casefolding, to take a look, but their entire feedback was just "looks good to me". Also, the fact that this patchset originally excluded the casefold+encrypt case technically made it not applicable to Android, and discouraged me from taking a look since encryption is my focus. Sorry for not taking a look sooner. Anyway, thanks for doing this, and I think it's near the finish line now. Once you address the latest feedback and get a couple acks, I think that Christian should take this through the VFS tree. BTW, in my opinion, as the maintainer of the "Unicode subsystem" you are also authorized to send a pull request for this to Linus yourself. But VFS does seem ideal in this case, given the diffstat, and Christian has been fairly active with taking patches. - Eric ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v5 06/10] libfs: Validate negative dentries in case-insensitive directories
On Mon, Aug 14, 2023 at 10:50:13AM -0400, Gabriel Krisman Bertazi wrote:
> Eric Biggers writes:
>
> > On Fri, Aug 11, 2023 at 08:41:42PM -0400, Gabriel Krisman Bertazi wrote:
> >> + /*
> >> + * Filesystems will call into d_revalidate without setting
> >> + * LOOKUP_ flags even for file creation (see lookup_one*
> >> + * variants). Reject negative dentries in this case, since we
> >> + * can't know for sure it won't be used for creation.
> >> + */
> >> + if (!flags)
> >> + return 0;
> >> +
> >> + /*
> >> + * If the lookup is for creation, then a negative dentry can
> >> + * only be reused if it's a case-sensitive match, not just a
> >> + * case-insensitive one. This is needed to make the new file be
> >> + * created with the name the user specified, preserving case.
> >> + */
> >> + if (flags & (LOOKUP_CREATE | LOOKUP_RENAME_TARGET)) {
> >> + /*
> >> + * ->d_name won't change from under us in the creation
> >> + * path only, since d_revalidate during creation and
> >> + * renames is always called with the parent inode
> >> + * locked. It isn't the case for all lookup callpaths,
> >> + * so ->d_name must not be touched outside
> >> + * (LOOKUP_CREATE|LOOKUP_RENAME_TARGET) context.
> >> + */
> >> + if (dentry->d_name.len != name->len ||
> >> + memcmp(dentry->d_name.name, name->name, name->len))
> >> + return 0;
> >> + }
> >
> > This is still really confusing to me. Can you consider the below? The
> > code is
> > the same except for the reordering, but the explanation is reworked to be
> > much
> > clearer (IMO). Anything I am misunderstanding?
> >
> > /*
> > * If the lookup is for creation, then a negative dentry can only be
> > * reused if it's a case-sensitive match, not just a case-insensitive
> > * one. This is needed to make the new file be created with the name
> > * the user specified, preserving case.
> > *
> > * LOOKUP_CREATE or LOOKUP_RENAME_TARGET cover most creations. In these
> > * cases, ->d_name is stable and can be compared to 'name' without
> > * taking ->d_lock because the caller holds dir->i_rwsem for write.
> > * (This is because the directory lock blocks the dentry from being
> > * concurrently instantiated, and negative dentries are never moved.)
> > *
> > * All other creations actually use flags==0. These come from the edge
> > * case of filesystems calling functions like lookup_one() that do a
> > * lookup without setting the lookup flags at all. Such lookups might
> > * or might not be for creation, and if not don't guarantee stable
> > * ->d_name. Therefore, invalidate all negative dentries when flags==0.
> > */
> > if (flags & (LOOKUP_CREATE | LOOKUP_RENAME_TARGET)) {
> > if (dentry->d_name.len != name->len ||
> > memcmp(dentry->d_name.name, name->name, name->len))
> > return 0;
> > }
> > if (!flags)
> > return 0;
>
> I don't see it as particularly better or less confusing than the
> original. but I also don't mind taking it into the next iteration.
>
Your commit message is still much longer and covers some quite different details
which seem irrelevant to me. So if you don't see my explanation as being much
different, I think we're still not on the same page. I hope that I'm not
misunderstanding anything, in which I believe that what I wrote above is a good
explanation and your commit message should be substantially simplified.
Remember, longer != better. Keep things as simple as possible.
- Eric
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
[f2fs-dev] [PATCH 2/3] ext4: remove redundant checks of s_encoding
From: Eric Biggers
Now that ext4 does not allow inodes with the casefold flag to be
instantiated when unsupported, it's unnecessary to repeatedly check for
support later on during random filesystem operations.
Signed-off-by: Eric Biggers
---
fs/ext4/hash.c | 2 +-
fs/ext4/namei.c | 6 +++---
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/fs/ext4/hash.c b/fs/ext4/hash.c
index 46c3423ddfa1..deabe29da7fb 100644
--- a/fs/ext4/hash.c
+++ b/fs/ext4/hash.c
@@ -300,7 +300,7 @@ int ext4fs_dirhash(const struct inode *dir, const char
*name, int len,
unsigned char *buff;
struct qstr qstr = {.name = name, .len = len };
- if (len && IS_CASEFOLDED(dir) && um &&
+ if (len && IS_CASEFOLDED(dir) &&
(!IS_ENCRYPTED(dir) || fscrypt_has_encryption_key(dir))) {
buff = kzalloc(sizeof(char) * PATH_MAX, GFP_KERNEL);
if (!buff)
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 0caf6c730ce3..f9a5663b9d23 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -1445,7 +1445,7 @@ int ext4_fname_setup_ci_filename(struct inode *dir, const
struct qstr *iname,
struct dx_hash_info *hinfo = >hinfo;
int len;
- if (!IS_CASEFOLDED(dir) || !dir->i_sb->s_encoding ||
+ if (!IS_CASEFOLDED(dir) ||
(IS_ENCRYPTED(dir) && !fscrypt_has_encryption_key(dir))) {
cf_name->name = NULL;
return 0;
@@ -1496,7 +1496,7 @@ static bool ext4_match(struct inode *parent,
#endif
#if IS_ENABLED(CONFIG_UNICODE)
- if (parent->i_sb->s_encoding && IS_CASEFOLDED(parent) &&
+ if (IS_CASEFOLDED(parent) &&
(!IS_ENCRYPTED(parent) || fscrypt_has_encryption_key(parent))) {
if (fname->cf_name.name) {
struct qstr cf = {.name = fname->cf_name.name,
@@ -2393,7 +2393,7 @@ static int ext4_add_entry(handle_t *handle, struct dentry
*dentry,
#if IS_ENABLED(CONFIG_UNICODE)
if (sb_has_strict_encoding(sb) && IS_CASEFOLDED(dir) &&
- sb->s_encoding && utf8_validate(sb->s_encoding, >d_name))
+ utf8_validate(sb->s_encoding, >d_name))
return -EINVAL;
#endif
--
2.41.0
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
[f2fs-dev] [PATCH 1/3] ext4: reject casefold inode flag without casefold feature
From: Eric Biggers
It is invalid for the casefold inode flag to be set without the casefold
superblock feature flag also being set. e2fsck already considers this
case to be invalid and handles it by offering to clear the casefold flag
on the inode. __ext4_iget() also already considered this to be invalid,
sort of, but it only got so far as logging an error message; it didn't
actually reject the inode. Make it reject the inode so that other code
doesn't have to handle this case. This matches what f2fs does.
Note: we could check 's_encoding != NULL' instead of
ext4_has_feature_casefold(). This would make the check robust against
the casefold feature being enabled by userspace writing to the page
cache of the mounted block device. However, it's unsolvable in general
for filesystems to be robust against concurrent writes to the page cache
of the mounted block device. Though this very particular scenario
involving the casefold feature is solvable, we should not pretend that
we can support this model, so let's just check the casefold feature.
tune2fs already forbids enabling casefold on a mounted filesystem.
Signed-off-by: Eric Biggers
---
fs/ext4/inode.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 43775a6ca505..390dedbb7e8a 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -4940,9 +4940,12 @@ struct inode *__ext4_iget(struct super_block *sb,
unsigned long ino,
"iget: bogus i_mode (%o)", inode->i_mode);
goto bad_inode;
}
- if (IS_CASEFOLDED(inode) && !ext4_has_feature_casefold(inode->i_sb))
+ if (IS_CASEFOLDED(inode) && !ext4_has_feature_casefold(inode->i_sb)) {
ext4_error_inode(inode, function, line, 0,
"casefold flag without casefold feature");
+ ret = -EFSCORRUPTED;
+ goto bad_inode;
+ }
if ((err_str = check_igot_inode(inode, flags)) != NULL) {
ext4_error_inode(inode, function, line, 0, err_str);
ret = -EFSCORRUPTED;
--
2.41.0
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
[f2fs-dev] [PATCH 3/3] libfs: remove redundant checks of s_encoding
From: Eric Biggers
Now that neither ext4 nor f2fs allows inodes with the casefold flag to
be instantiated when unsupported, it's unnecessary to repeatedly check
for support later on during random filesystem operations.
Signed-off-by: Eric Biggers
---
fs/libfs.c | 14 ++
1 file changed, 2 insertions(+), 12 deletions(-)
diff --git a/fs/libfs.c b/fs/libfs.c
index 5b851315eeed..5197ea8c66d3 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -1381,16 +1381,6 @@ bool is_empty_dir_inode(struct inode *inode)
}
#if IS_ENABLED(CONFIG_UNICODE)
-/*
- * Determine if the name of a dentry should be casefolded.
- *
- * Return: if names will need casefolding
- */
-static bool needs_casefold(const struct inode *dir)
-{
- return IS_CASEFOLDED(dir) && dir->i_sb->s_encoding;
-}
-
/**
* generic_ci_d_compare - generic d_compare implementation for casefolding
filesystems
* @dentry:dentry whose name we are checking against
@@ -1411,7 +1401,7 @@ static int generic_ci_d_compare(const struct dentry
*dentry, unsigned int len,
char strbuf[DNAME_INLINE_LEN];
int ret;
- if (!dir || !needs_casefold(dir))
+ if (!dir || !IS_CASEFOLDED(dir))
goto fallback;
/*
* If the dentry name is stored in-line, then it may be concurrently
@@ -1453,7 +1443,7 @@ static int generic_ci_d_hash(const struct dentry *dentry,
struct qstr *str)
const struct unicode_map *um = sb->s_encoding;
int ret = 0;
- if (!dir || !needs_casefold(dir))
+ if (!dir || !IS_CASEFOLDED(dir))
return 0;
ret = utf8_casefold_hash(um, dentry, str);
--
2.41.0
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
[f2fs-dev] [PATCH 0/3] Simplify rejection of unexpected casefold inode flag
This series makes unexpected casefold flags on inodes be consistently rejected early on so that additional validation isn't needed later on during random filesystem operations. For additional context, refer to the discussion on patch 1 of https://lore.kernel.org/linux-fsdevel/20230812004146.30980-1-kris...@suse.de/T/#u Applies to v6.5-rc6 Eric Biggers (3): ext4: reject casefold inode flag without casefold feature ext4: remove redundant checks of s_encoding libfs: remove redundant checks of s_encoding fs/ext4/hash.c | 2 +- fs/ext4/inode.c | 5 - fs/ext4/namei.c | 6 +++--- fs/libfs.c | 14 ++ 4 files changed, 10 insertions(+), 17 deletions(-) base-commit: 2ccdd1b13c591d306f0401d98dedc4bdcd02b421 -- 2.41.0 ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
