Move fscrypt and fsverity info out of struct inode
Subsystems (9)
Eric Biggers
maintainerv5 cover Move fscrypt and fsverity info out of struct inode
This is a cleaned-up implementation of moving the i_crypt_info and i_verity_info pointers out of 'struct inode' and into the fs-specific part of the inode, as proposed previously by Christian at
https://lore.kernel.org/r/[email protected]/
The high-level concept is still the same: fs/crypto/ and fs/verity/ locate the pointer by adding an offset to the address of struct inode. The offset is retrieved from fscrypt_operations or fsverity_operations.
I've cleaned up a lot of the details, including:
- Grouped changes into patches differently
- Rewrote commit messages and comments to be clearer
- Adjusted code formatting to be consistent with existing code
- Removed unneeded #ifdefs
- Improved choice and location of VFS_WARN_ON_ONCE() statements
- Added missing kerneldoc for ubifs_inode::i_crypt_info
- Moved field initialization to init_once functions when they exist
- Improved ceph offset calculation and removed unneeded static_asserts
- fsverity_get_info() now checks IS_VERITY() instead of v_ops
- fscrypt_put_encryption_info() no longer checks IS_ENCRYPTED(), since I no longer think it's actually correct there.
- verity_data_blocks() now keeps doing a raw dereference
- Dropped fscrypt_set_inode_info()
- Renamed some functions
- Do offset calculation using int, so we don't rely on unsigned overflow
- And more.
For v4 and earlier, see
https://lore.kernel.org/r/[email protected]/
I'd like to take this series through the fscrypt tree for 6.18. (fsverity normally has a separate tree, but by choosing just one tree for this, we'll avoid conflicts in some places.)
Eric Biggers (13):
fscrypt: replace raw loads of info pointer with helper function
fscrypt: add support for info in fs-specific part of inode
ext4: move crypt info pointer to fs-specific part of inode
f2fs: move crypt info pointer to fs-specific part of inode
ubifs: move crypt info pointer to fs-specific part of inode
ceph: move crypt info pointer to fs-specific part of inode
fs: remove inode::i_crypt_info
fsverity: add support for info in fs-specific part of inode
ext4: move verity info pointer to fs-specific part of inode
f2fs: move verity info pointer to fs-specific part of inode
btrfs: move verity info pointer to fs-specific part of inode
fs: remove inode::i_verity_info
fsverity: check IS_VERITY() in fsverity_cleanup_inode()
fs/btrfs/btrfs_inode.h | 5 ++++ fs/btrfs/inode.c | 3 ++ fs/btrfs/verity.c | 2 ++ fs/ceph/crypto.c | 2 ++ fs/ceph/inode.c | 1 + fs/ceph/super.h | 1 + fs/crypto/bio.c | 2 +- fs/crypto/crypto.c | 14 +++++---- fs/crypto/fname.c | 11 +++---- fs/crypto/fscrypt_private.h | 4 +-- fs/crypto/hooks.c | 2 +- fs/crypto/inline_crypt.c | 12 ++++---- fs/crypto/keysetup.c | 43 ++++++++++++++++----------- fs/crypto/policy.c | 7 +++-- fs/ext4/crypto.c | 2 ++ fs/ext4/ext4.h | 8 +++++ fs/ext4/super.c | 6 ++++ fs/ext4/verity.c | 2 ++ fs/f2fs/f2fs.h | 6 ++++ fs/f2fs/super.c | 10 ++++++- fs/f2fs/verity.c | 2 ++ fs/ubifs/crypto.c | 2 ++ fs/ubifs/ubifs.h | 4 +++ fs/verity/enable.c | 6 ++-- fs/verity/fsverity_private.h | 9 +++--- fs/verity/open.c | 23 ++++++++------- fs/verity/verify.c | 2 +- include/linux/fs.h | 10 ------- include/linux/fscrypt.h | 40 +++++++++++++++++++++++-- include/linux/fsverity.h | 57 +++++++++++++++++++++++++++++------- 30 files changed, 215 insertions(+), 83 deletions(-)
base-commit: 561c80369df0733ba0574882a1635287b20f9de2
Signature · 1 line
2.50.1
Eric Biggers
maintainerPATCH v5 1/13 fscrypt: replace raw loads of info pointer with helper function
Add and use a helper function fscrypt_get_inode_info_raw(). It loads an inode's fscrypt info pointer using a raw dereference, which is appropriate when the caller knows the key setup already happened.
This eliminates most occurrences of inode::i_crypt_info in the source, in preparation for replacing that with a filesystem-specific field.
--- fs/crypto/bio.c | 2 +- fs/crypto/crypto.c | 14 ++++++++------ fs/crypto/fname.c | 11 ++++++----- fs/crypto/hooks.c | 2 +- fs/crypto/inline_crypt.c | 12 +++++++----- fs/crypto/policy.c | 7 ++++--- include/linux/fscrypt.h | 16 ++++++++++++++++ 7 files changed, 43 insertions(+), 21 deletions(-)
fs/crypto/bio.c +1 -1
@@ -111,11 +111,11 @@ static int fscrypt_zeroout_range_inline_crypt(const struct inode *inode,* Return: 0 on success; -errno on failure. */ int fscrypt_zeroout_range (const struct inode *inode,pgoff_t lblk,sector_t pblk,unsigned int len) { -const struct fscrypt_inode_info *ci = inode->i_crypt_info; +const struct fscrypt_inode_info *ci =fscrypt_get_inode_info_raw (inode);const unsigned int du_bits = ci->ci_data_unit_bits;const unsigned int du_size =1U << du_bits;const unsigned int du_per_page_bits =PAGE_SHIFT - du_bits;const unsigned int du_per_page =1U << du_per_page_bits;u64 du_index = (u64 )lblk << (inode->i_blkbits - du_bits);
fs/crypto/crypto.c +8 -6
@@ -171,11 +171,11 @@ int fscrypt_crypt_data_unit(const struct fscrypt_inode_info *ci,*/ struct page *fscrypt_encrypt_pagecache_blocks (struct folio *folio,size_t len,size_t offs,gfp_t gfp_flags) {const struct inode *inode = folio->mapping->host; -const struct fscrypt_inode_info *ci = inode->i_crypt_info; +const struct fscrypt_inode_info *ci =fscrypt_get_inode_info_raw (inode);const unsigned int du_bits = ci->ci_data_unit_bits;const unsigned int du_size =1U << du_bits;struct page *ciphertext_page;u64 index = ((u64 )folio->index << (PAGE_SHIFT - du_bits)) + (offs >> du_bits);
@@ -230,12 +230,13 @@ int fscrypt_encrypt_block_inplace(const struct inode *inode, struct page *page, unsigned int len, unsigned int offs, u64 lblk_num) { if (WARN_ON_ONCE(inode->i_sb->s_cop->supports_subblock_data_units)) return -EOPNOTSUPP; -return fscrypt_crypt_data_unit (inode->i_crypt_info,FS_ENCRYPT , - lblk_num, page, page, len, offs); +return fscrypt_crypt_data_unit (fscrypt_get_inode_info_raw (inode), +FS_ENCRYPT , lblk_num, page, page, len, + offs); }EXPORT_SYMBOL (fscrypt_encrypt_block_inplace);/** * fscrypt_decrypt_pagecache_blocks() - Decrypt data from a pagecache folio
@@ -253,11 +254,11 @@ EXPORT_SYMBOL(fscrypt_encrypt_block_inplace);*/ int fscrypt_decrypt_pagecache_blocks (struct folio *folio,size_t len,size_t offs) {const struct inode *inode = folio->mapping->host; -const struct fscrypt_inode_info *ci = inode->i_crypt_info; +const struct fscrypt_inode_info *ci =fscrypt_get_inode_info_raw (inode);const unsigned int du_bits = ci->ci_data_unit_bits;const unsigned int du_size =1U << du_bits;u64 index = ((u64 )folio->index << (PAGE_SHIFT - du_bits)) + (offs >> du_bits);size_t i;
@@ -303,12 +304,13 @@ int fscrypt_decrypt_block_inplace(const struct inode *inode, struct page *page, unsigned int len, unsigned int offs, u64 lblk_num) { if (WARN_ON_ONCE(inode->i_sb->s_cop->supports_subblock_data_units)) return -EOPNOTSUPP; -return fscrypt_crypt_data_unit (inode->i_crypt_info,FS_DECRYPT , - lblk_num, page, page, len, offs); +return fscrypt_crypt_data_unit (fscrypt_get_inode_info_raw (inode), +FS_DECRYPT , lblk_num, page, page, len, + offs); }EXPORT_SYMBOL (fscrypt_decrypt_block_inplace);/** * fscrypt_initialize() - allocate major buffers for fs encryption.
fs/crypto/fname.c +6 -5
@@ -92,11 +92,11 @@ static inline bool fscrypt_is_dot_dotdot(const struct qstr *str)* Return: 0 on success, -errno on failure */ int fscrypt_fname_encrypt (const struct inode *inode,const struct qstr *iname,u8 *out,unsigned int olen) { -const struct fscrypt_inode_info *ci = inode->i_crypt_info; +const struct fscrypt_inode_info *ci =fscrypt_get_inode_info_raw (inode);struct crypto_sync_skcipher *tfm = ci->ci_enc_key.tfm;SYNC_SKCIPHER_REQUEST_ON_STACK (req, tfm);union fscrypt_iv iv;struct scatterlist sg;int err;
@@ -136,11 +136,11 @@ EXPORT_SYMBOL_GPL(fscrypt_fname_encrypt);*/ static int fname_decrypt (const struct inode *inode,const struct fscrypt_str *iname,struct fscrypt_str *oname) { -const struct fscrypt_inode_info *ci = inode->i_crypt_info; +const struct fscrypt_inode_info *ci =fscrypt_get_inode_info_raw (inode);struct crypto_sync_skcipher *tfm = ci->ci_enc_key.tfm;SYNC_SKCIPHER_REQUEST_ON_STACK (req, tfm);union fscrypt_iv iv;struct scatterlist src_sg, dst_sg;int err;
@@ -272,12 +272,13 @@ bool __fscrypt_fname_encrypted_size(const union fscrypt_policy *policy,* fill out encrypted_len_ret with the length (up to max_len). */ bool fscrypt_fname_encrypted_size (const struct inode *inode,u32 orig_len,u32 max_len,u32 *encrypted_len_ret) { -return __fscrypt_fname_encrypted_size (&inode->i_crypt_info->ci_policy, - orig_len, max_len, +const struct fscrypt_inode_info *ci =fscrypt_get_inode_info_raw (inode); + +return __fscrypt_fname_encrypted_size (&ci->ci_policy, orig_len, max_len, encrypted_len_ret); }EXPORT_SYMBOL_GPL (fscrypt_fname_encrypted_size);/**
@@ -541,11 +542,11 @@ EXPORT_SYMBOL_GPL(fscrypt_match_name);* * Return: the SipHash of @name using the hash key of @dir */ u64 fscrypt_fname_siphash (const struct inode *dir,const struct qstr *name) { -const struct fscrypt_inode_info *ci = dir->i_crypt_info; +const struct fscrypt_inode_info *ci =fscrypt_get_inode_info_raw (dir);WARN_ON_ONCE (!ci->ci_dirhash_key_initialized);return siphash (name->name, name->len, &ci->ci_dirhash_key); }
fs/crypto/hooks.c +1 -1
@@ -197,11 +197,11 @@ int fscrypt_prepare_setflags(struct inode *inode,*/ if (IS_ENCRYPTED (inode) && (flags & ~oldflags &FS_CASEFOLD_FL )) { err =fscrypt_require_key (inode);if (err)return err; - ci = inode->i_crypt_info; + ci =fscrypt_get_inode_info_raw (inode);if (ci->ci_policy.version !=FSCRYPT_POLICY_V2 )return -EINVAL ; mk = ci->ci_master_key;down_read (&mk->mk_sem);if (mk->mk_present)
fs/crypto/inline_crypt.c +7 -5
@@ -261,11 +261,11 @@ int fscrypt_derive_sw_secret(struct super_block *sb,return err; }bool __fscrypt_inode_uses_inline_crypto (const struct inode *inode) { -return inode->i_crypt_info->ci_inlinecrypt; +return fscrypt_get_inode_info_raw (inode)->ci_inlinecrypt; }EXPORT_SYMBOL_GPL (__fscrypt_inode_uses_inline_crypto); static void fscrypt_generate_dun(const struct fscrypt_inode_info *ci, u64 lblk_num,
@@ -305,11 +305,11 @@ void fscrypt_set_bio_crypt_ctx(struct bio *bio, const struct inode *inode,const struct fscrypt_inode_info *ci;u64 dun[BLK_CRYPTO_DUN_ARRAY_SIZE ];if (!fscrypt_inode_uses_inline_crypto (inode))return ; - ci = inode->i_crypt_info; + ci =fscrypt_get_inode_info_raw (inode);fscrypt_generate_dun (ci, first_lblk, dun);bio_crypt_set_ctx (bio, ci->ci_enc_key.blk_key, dun, gfp_mask); }EXPORT_SYMBOL_GPL (fscrypt_set_bio_crypt_ctx);
@@ -383,26 +383,28 @@ EXPORT_SYMBOL_GPL(fscrypt_set_bio_crypt_ctx_bh);*/ bool fscrypt_mergeable_bio (struct bio *bio,const struct inode *inode,u64 next_lblk) {const struct bio_crypt_ctx *bc = bio->bi_crypt_context; +const struct fscrypt_inode_info *ci;u64 next_dun[BLK_CRYPTO_DUN_ARRAY_SIZE ];if (!!bc !=fscrypt_inode_uses_inline_crypto (inode))return false;if (!bc)return true; + ci =fscrypt_get_inode_info_raw (inode);/* * Comparing the key pointers is good enough, as all I/O for each key * uses the same pointer. I.e., there's currently no need to support * merging requests where the keys are the same but the pointers differ. */ -if (bc->bc_key != inode->i_crypt_info->ci_enc_key.blk_key) +if (bc->bc_key != ci->ci_enc_key.blk_key)return false; -fscrypt_generate_dun (inode->i_crypt_info, next_lblk, next_dun); +fscrypt_generate_dun (ci, next_lblk, next_dun);return bio_crypt_dun_is_contiguous (bc, bio->bi_iter.bi_size, next_dun); }EXPORT_SYMBOL_GPL (fscrypt_mergeable_bio);/**
@@ -500,11 +502,11 @@ u64 fscrypt_limit_io_blocks(const struct inode *inode, u64 lblk, u64 nr_blocks)return nr_blocks;if (nr_blocks <=1 )return nr_blocks; - ci = inode->i_crypt_info; + ci =fscrypt_get_inode_info_raw (inode);if (!(fscrypt_policy_flags (&ci->ci_policy) &FSCRYPT_POLICY_FLAG_IV_INO_LBLK_32 ))return nr_blocks;/* With IV_INO_LBLK_32, the DUN can wrap around from U32_MAX to 0. */
fs/crypto/policy.c +4 -3
@@ -725,11 +725,11 @@ const union fscrypt_policy *fscrypt_policy_to_inherit(struct inode *dir)if (IS_ENCRYPTED (dir)) { err =fscrypt_require_key (dir);if (err)return ERR_PTR (err); -return &dir->i_crypt_info->ci_policy; +return &fscrypt_get_inode_info_raw (dir)->ci_policy; }return fscrypt_get_dummy_policy (dir->i_sb); }
@@ -744,11 +744,11 @@ const union fscrypt_policy *fscrypt_policy_to_inherit(struct inode *dir)* * Return: size of the resulting context or a negative error code. */ int fscrypt_context_for_new_inode (void *ctx,struct inode *inode) { -struct fscrypt_inode_info *ci = inode->i_crypt_info; +struct fscrypt_inode_info *ci =fscrypt_get_inode_info_raw (inode);BUILD_BUG_ON (sizeof (union fscrypt_context ) !=FSCRYPT_SET_CONTEXT_MAX_SIZE );/* fscrypt_prepare_new_inode() should have set up the key already. */
@@ -769,11 +769,11 @@ EXPORT_SYMBOL_GPL(fscrypt_context_for_new_inode);* * Return: 0 on success, -errno on failure */ int fscrypt_set_context (struct inode *inode,void *fs_data) { -struct fscrypt_inode_info *ci = inode->i_crypt_info; +struct fscrypt_inode_info *ci;union fscrypt_context ctx;int ctxsize; ctxsize =fscrypt_context_for_new_inode (&ctx, inode);if (ctxsize <0 )
@@ -781,10 +781,11 @@ int fscrypt_set_context(struct inode *inode, void *fs_data)/* * This may be the first time the inode number is available, so do any * delayed key setup that requires the inode number. */ + ci =fscrypt_get_inode_info_raw (inode);if (ci->ci_policy.version ==FSCRYPT_POLICY_V2 && (ci->ci_policy.v2.flags &FSCRYPT_POLICY_FLAG_IV_INO_LBLK_32 ))fscrypt_hash_inode_number (ci, ci->ci_master_key);return inode->i_sb->s_cop->set_context (inode, &ctx, ctxsize, fs_data);
include/linux/fscrypt.h +16 -0
@@ -193,10 +193,26 @@ struct fscrypt_operations { };int fscrypt_d_revalidate (struct inode *dir,const struct qstr *name,struct dentry *dentry,unsigned int flags); +/* +* Load the inode's fscrypt info pointer, using a raw dereference. Since this +* uses a raw dereference with no memory barrier, it is appropriate to use only +* when the caller knows the inode's key setup already happened, resulting in +* non-NULL fscrypt info. E.g., the file contents en/decryption functions use +* this, since fscrypt_file_open() set up the key. +*/ +static inline struct fscrypt_inode_info * +fscrypt_get_inode_info_raw (const struct inode *inode) +{ +struct fscrypt_inode_info *ci = inode->i_crypt_info; + +VFS_WARN_ON_ONCE (ci ==NULL ); +return ci; +} +static inline struct fscrypt_inode_info *fscrypt_get_inode_info (const struct inode *inode) {/* * Pairs with the cmpxchg_release() in fscrypt_setup_encryption_info().
Signature · 1 line
2.50.1
Eric Biggers
maintainerPATCH v5 2/13 fscrypt: add support for info in fs-specific part of inode
In reply to Eric BiggersAdd an inode_info_offs field to struct fscrypt_operations, and update fs/crypto/ to support it. When set to a nonzero value, it specifies the offset to the fscrypt_inode_info pointer within the filesystem-specific part of the inode structure, to be used instead of inode::i_crypt_info.
Since this makes inode::i_crypt_info no longer necessarily used, update comments that mentioned it.
This is a prerequisite for a later commit that removes inode::i_crypt_info, saving memory and improving cache efficiency with filesystems that don't support fscrypt.
--- fs/crypto/fscrypt_private.h | 4 ++-- fs/crypto/keysetup.c | 43 ++++++++++++++++++++++--------------- include/linux/fscrypt.h | 22 +++++++++++++++---- 3 files changed, 46 insertions(+), 23 deletions(-)
fs/crypto/fscrypt_private.h +2 -2
@@ -247,12 +247,12 @@ struct fscrypt_prepared_key {/* * fscrypt_inode_info - the "encryption key" for an inode * * When an encrypted file's key is made available, an instance of this struct is -* allocated and stored in ->i_crypt_info. Once created, it remains until the -* inode is evicted. +* allocated and a pointer to it is stored in the file's in-memory inode. Once +* created, it remains until the inode is evicted. */ struct fscrypt_inode_info {/* The key in a form prepared for actual encryption/decryption */ struct fscrypt_prepared_key ci_enc_key;
fs/crypto/keysetup.c +26 -17
97 lines not on this page · raw message
include/linux/fscrypt.h +18 -4
@@ -59,10 +59,16 @@ struct fscrypt_name { #ifdef CONFIG_FS_ENCRYPTION /* Crypto operations for filesystems */ struct fscrypt_operations { + /* + * The offset of the pointer to struct fscrypt_inode_info in the + * filesystem-specific part of the inode, relative to the beginning of + * the common part of the inode (the 'struct inode'). + */ + ptrdiff_t inode_info_offs; /* * If set, then fs/crypto/ will allocate a global bounce page pool the * first time an encryption key is set up for a file. The bounce page * pool is required by the following functions:
@@ -193,36 +199,44 @@ struct fscrypt_operations { };int fscrypt_d_revalidate (struct inode *dir,const struct qstr *name,struct dentry *dentry,unsigned int flags); +static inline struct fscrypt_inode_info ** +fscrypt_inode_info_addr (const struct inode *inode) +{ +if (inode->i_sb->s_cop->inode_info_offs ==0 ) +return (struct fscrypt_inode_info **)&inode->i_crypt_info; +return (void *)inode + inode->i_sb->s_cop->inode_info_offs; +} +/* * Load the inode's fscrypt info pointer, using a raw dereference. Since this * uses a raw dereference with no memory barrier, it is appropriate to use only * when the caller knows the inode's key setup already happened, resulting in * non-NULL fscrypt info. E.g., the file contents en/decryption functions use * this, since fscrypt_file_open() set up the key. */ static inline struct fscrypt_inode_info *fscrypt_get_inode_info_raw (const struct inode *inode) { -struct fscrypt_inode_info *ci = inode->i_crypt_info; +struct fscrypt_inode_info *ci = *fscrypt_inode_info_addr (inode);VFS_WARN_ON_ONCE (ci ==NULL );return ci; }static inline struct fscrypt_inode_info *fscrypt_get_inode_info (const struct inode *inode) {/* * Pairs with the cmpxchg_release() in fscrypt_setup_encryption_info(). -* I.e., another task may publish ->i_crypt_info concurrently, executing -* a RELEASE barrier. We need to use smp_load_acquire() here to safely +* I.e., another task may publish the fscrypt info concurrently, +* executing a RELEASE barrier. Use smp_load_acquire() here to safely * ACQUIRE the memory the other task published. */ -return smp_load_acquire (&inode->i_crypt_info); +return smp_load_acquire (fscrypt_inode_info_addr (inode)); }/** * fscrypt_needs_contents_encryption() - check whether an inode needs * contents encryption
Signature · 1 line
2.50.1
Eric Biggers
maintainerPATCH v5 3/13 ext4: move crypt info pointer to fs-specific part of inode
In reply to Eric BiggersMove the fscrypt_inode_info pointer into the filesystem-specific part of the inode by adding the field ext4_inode_info::i_crypt_info and configuring fscrypt_operations::inode_info_offs accordingly.
This is a prerequisite for a later commit that removes inode::i_crypt_info, saving memory and improving cache efficiency with filesystems that don't support fscrypt.
--- fs/ext4/crypto.c | 2 ++ fs/ext4/ext4.h | 4 ++++ fs/ext4/super.c | 3 +++ 3 files changed, 9 insertions(+)
fs/ext4/crypto.c +2 -0
@@ -225,10 +225,12 @@ static bool ext4_has_stable_inodes(struct super_block *sb) {return ext4_has_feature_stable_inodes (sb); }const struct fscrypt_operations ext4_cryptops = { + .inode_info_offs = (int )offsetof(struct ext4_inode_info , i_crypt_info) - + (int )offsetof(struct ext4_inode_info , vfs_inode), .needs_bounce_pages =1 , .has_32bit_inodes =1 , .supports_subblock_data_units =1 , .legacy_key_prefix ="ext4:" , .get_context = ext4_get_context,
fs/ext4/ext4.h +4 -0
@@ -1180,10 +1180,14 @@ struct ext4_inode_info {/* Precomputed uuid+inum+igen checksum for seeding inode checksums */ __u32 i_csum_seed;kprojid_t i_projid; + +#ifdef CONFIG_FS_ENCRYPTION +struct fscrypt_inode_info *i_crypt_info; +#endif };/* * File system states */
fs/ext4/super.c +3 -0
@@ -1468,10 +1468,13 @@ static void init_once(void *foo)INIT_LIST_HEAD (&ei->i_orphan);init_rwsem (&ei->xattr_sem);init_rwsem (&ei->i_data_sem);inode_init_once (&ei->vfs_inode);ext4_fc_init_inode (&ei->vfs_inode); +#ifdef CONFIG_FS_ENCRYPTION + ei->i_crypt_info =NULL ; +#endif }static int __init init_inodecache (void ) { ext4_inode_cachep = kmem_cache_create_usercopy("ext4_inode_cache",
Signature · 1 line
2.50.1
Theodore Ts'o
maintainerRe: [PATCH v5 03/13] ext4: move crypt info pointer to fs-specific part of inode
Quoted 11 lines · Eric Biggers
Move the
fscrypt_inode_infopointer into the filesystem-specific part of the inode by adding the fieldext4_inode_info::i_crypt_infoand configuringfscrypt_operations::inode_info_offsaccordingly.This is a prerequisite for a later commit that removes
inode::i_crypt_info, saving memory and improving cache efficiency with filesystems that don't support fscrypt.Co-developed-by Christian Brauner <[email protected]>Signed-off-by Christian Brauner <[email protected]>Signed-off-by Eric Biggers <[email protected]>
Eric Biggers
maintainerPATCH v5 4/13 f2fs: move crypt info pointer to fs-specific part of inode
In reply to Eric BiggersMove the fscrypt_inode_info pointer into the filesystem-specific part of the inode by adding the field f2fs_inode_info::i_crypt_info and configuring fscrypt_operations::inode_info_offs accordingly.
This is a prerequisite for a later commit that removes inode::i_crypt_info, saving memory and improving cache efficiency with filesystems that don't support fscrypt.
--- fs/f2fs/f2fs.h | 3 +++ fs/f2fs/super.c | 7 ++++++- 2 files changed, 9 insertions(+), 1 deletion(-)
fs/f2fs/f2fs.h +3 -0
@@ -905,10 +905,13 @@ struct f2fs_inode_info {unsigned char i_compress_flag;/* compress flag */ unsigned int i_cluster_size;/* cluster size */ unsigned int atomic_write_cnt;loff_t original_i_size;/* original i_size before atomic write */ +#ifdef CONFIG_FS_ENCRYPTION +struct fscrypt_inode_info *i_crypt_info;/* filesystem encryption info */ +#endif };static inline void get_read_extent_info (struct extent_info *ext,struct f2fs_extent *i_ext) {
fs/f2fs/super.c +6 -1
@@ -478,10 +478,13 @@ static inline void adjust_unusable_cap_perc(struct f2fs_sb_info *sbi)static void init_once (void *foo) {struct f2fs_inode_info *fi = (struct f2fs_inode_info *) foo;inode_init_once (&fi->vfs_inode); +#ifdef CONFIG_FS_ENCRYPTION + fi->i_crypt_info =NULL ; +#endif }#ifdef CONFIG_QUOTA static const char *const quotatypes[] =INITQFNAMES ;#define QTYPE2NAME (t) (quotatypes[t])
@@ -3568,10 +3571,12 @@ static struct block_device **f2fs_get_devices(struct super_block *sb, *num_devs = sbi->s_ndevs;return devs; }static const struct fscrypt_operations f2fs_cryptops = { + .inode_info_offs = (int )offsetof(struct f2fs_inode_info , i_crypt_info) - + (int )offsetof(struct f2fs_inode_info , vfs_inode), .needs_bounce_pages =1 , .has_32bit_inodes =1 , .supports_subblock_data_units =1 , .legacy_key_prefix ="f2fs:" , .get_context = f2fs_get_context,
@@ -3579,11 +3584,11 @@ static const struct fscrypt_operations f2fs_cryptops = { .get_dummy_policy = f2fs_get_dummy_policy, .empty_dir = f2fs_empty_dir, .has_stable_inodes = f2fs_has_stable_inodes, .get_devices = f2fs_get_devices, }; -#endif +#endif /* CONFIG_FS_ENCRYPTION */ static struct inode *f2fs_nfs_get_inode (struct super_block *sb,u64 ino,u32 generation) {struct f2fs_sb_info *sbi =F2FS_SB (sb);
Signature · 1 line
2.50.1
Eric Biggers
maintainerPATCH v5 5/13 ubifs: move crypt info pointer to fs-specific part of inode
In reply to Eric BiggersMove the fscrypt_inode_info pointer into the filesystem-specific part of the inode by adding the field ubifs_inode::i_crypt_info and configuring fscrypt_operations::inode_info_offs accordingly.
This is a prerequisite for a later commit that removes inode::i_crypt_info, saving memory and improving cache efficiency with filesystems that don't support fscrypt.
Note that the initialization of ubifs_inode::i_crypt_info to NULL on inode allocation is handled by the memset() in ubifs_alloc_inode().
--- fs/ubifs/crypto.c | 2 ++ fs/ubifs/ubifs.h | 4 ++++ 2 files changed, 6 insertions(+)
fs/ubifs/crypto.c +2 -0
@@ -86,10 +86,12 @@ int ubifs_decrypt(const struct inode *inode, struct ubifs_data_node *dn,return 0 ; }const struct fscrypt_operations ubifs_crypt_operations = { + .inode_info_offs = (int )offsetof(struct ubifs_inode , i_crypt_info) - + (int )offsetof(struct ubifs_inode , vfs_inode), .legacy_key_prefix ="ubifs:" , .get_context = ubifs_crypt_get_context, .set_context = ubifs_crypt_set_context, .empty_dir = ubifs_crypt_empty_dir, };
fs/ubifs/ubifs.h +4 -0
@@ -363,10 +363,11 @@ struct ubifs_gced_idx_leb {* @compr_type: default compression type used for this inode * @last_page_read: page number of last page read (for bulk read) * @read_in_a_row: number of consecutive pages read in a row (for bulk read) * @data_len: length of the data attached to the inode * @data: inode's data +* @i_crypt_info: inode's fscrypt information * * @ui_mutex exists for two main reasons. At first it prevents inodes from * being written back while UBIFS changing them, being in the middle of an VFS * operation. This way UBIFS makes sure the inode fields are consistent. For * example, in 'ubifs_rename()' we change 4 inodes simultaneously, and
@@ -414,10 +415,13 @@ struct ubifs_inode {int flags;pgoff_t last_page_read;pgoff_t read_in_a_row;int data_len;void *data; +#ifdef CONFIG_FS_ENCRYPTION +struct fscrypt_inode_info *i_crypt_info; +#endif };/** * struct ubifs_unclean_leb - records a LEB recovered under read-only mode. * @list: list
Signature · 1 line
2.50.1
Eric Biggers
maintainerPATCH v5 6/13 ceph: move crypt info pointer to fs-specific part of inode
In reply to Eric BiggersMove the fscrypt_inode_info pointer into the filesystem-specific part of the inode by adding the field ceph_inode_info::i_crypt_info and configuring fscrypt_operations::inode_info_offs accordingly.
This is a prerequisite for a later commit that removes inode::i_crypt_info, saving memory and improving cache efficiency with filesystems that don't support fscrypt.
--- fs/ceph/crypto.c | 2 ++ fs/ceph/inode.c | 1 + fs/ceph/super.h | 1 + 3 files changed, 4 insertions(+)
fs/ceph/crypto.c +2 -0
@@ -131,10 +131,12 @@ static const union fscrypt_policy *ceph_get_dummy_policy(struct super_block *sb) {return ceph_sb_to_fs_client (sb)->fsc_dummy_enc_policy.policy; }static struct fscrypt_operations ceph_fscrypt_ops = { + .inode_info_offs = (int )offsetof(struct ceph_inode_info , i_crypt_info) - + (int)offsetof(struct ceph_inode_info, netfs.inode), .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,
fs/ceph/inode.c +1 -0
@@ -663,10 +663,11 @@ struct inode *ceph_alloc_inode(struct super_block *sb)INIT_WORK (&ci->i_work, ceph_inode_work); ci->i_work_mask =0 ;memset (&ci->i_btime,'\0' ,sizeof (ci->i_btime));#ifdef CONFIG_FS_ENCRYPTION + ci->i_crypt_info =NULL ; ci->fscrypt_auth =NULL ; ci->fscrypt_auth_len =0 ;#endif return &ci->netfs.inode; }
fs/ceph/super.h +1 -0
@@ -461,10 +461,11 @@ struct ceph_inode_info {struct work_struct i_work;unsigned long i_work_mask;#ifdef CONFIG_FS_ENCRYPTION +struct fscrypt_inode_info *i_crypt_info;u32 fscrypt_auth_len;u32 fscrypt_file_len;u8 *fscrypt_auth;u8 *fscrypt_file;#endif
Signature · 1 line
2.50.1
Eric Biggers
maintainerPATCH v5 7/13 fs: remove inode::i_crypt_info
In reply to Eric BiggersNow that all fscrypt-capable filesystems store the pointer to fscrypt_inode_info in the filesystem-specific part of the inode structure, inode::i_crypt_info is no longer needed. Update fscrypt_inode_info_addr() to no longer support the fallback to inode::i_crypt_info. Finally, remove inode::i_crypt_info itself along with the now-unnecessary forward declaration of fscrypt_inode_info.
The end result of the migration to the filesystem-specific pointer is memory savings on CONFIG_FS_ENCRYPTION=y kernels for all filesystems that don't support fscrypt. Specifically, their in-memory inodes are now smaller by the size of a pointer: either 4 or 8 bytes.
--- include/linux/fs.h | 5 ----- include/linux/fscrypt.h | 8 ++++++-- 2 files changed, 6 insertions(+), 7 deletions(-)
include/linux/fs.h +0 -5
@@ -70,11 +70,10 @@ struct vfsmount;struct cred ;struct swap_info_struct ;struct seq_file ;struct workqueue_struct ;struct iov_iter ; -struct fscrypt_inode_info ;struct fscrypt_operations ;struct fsverity_info ;struct fsverity_operations ;struct fsnotify_mark_connector ;struct fsnotify_sb_info ;
@@ -778,14 +777,10 @@ struct inode {__u32 i_fsnotify_mask;/* all events this inode cares about */ /* 32-bit hole reserved for expanding i_fsnotify_mask */ struct fsnotify_mark_connector __rcu *i_fsnotify_marks;#endif -#ifdef CONFIG_FS_ENCRYPTION -struct fscrypt_inode_info *i_crypt_info; -#endif -#ifdef CONFIG_FS_VERITY struct fsverity_info *i_verity_info;#endif void *i_private;/* fs or device private pointer */
include/linux/fscrypt.h +6 -2
@@ -199,15 +199,19 @@ struct fscrypt_operations { };int fscrypt_d_revalidate (struct inode *dir,const struct qstr *name,struct dentry *dentry,unsigned int flags); +/* +* Returns the address of the fscrypt info pointer within the +* filesystem-specific part of the inode. (To save memory on filesystems that +* don't support fscrypt, a field in 'struct inode' itself is no longer used.) +*/ static inline struct fscrypt_inode_info **fscrypt_inode_info_addr (const struct inode *inode) { -if (inode->i_sb->s_cop->inode_info_offs ==0 ) -return (struct fscrypt_inode_info **)&inode->i_crypt_info; +VFS_WARN_ON_ONCE (inode->i_sb->s_cop->inode_info_offs ==0 );return (void *)inode + inode->i_sb->s_cop->inode_info_offs; }/* * Load the inode's fscrypt info pointer, using a raw dereference. Since this
Signature · 1 line
2.50.1
Eric Biggers
maintainerPATCH v5 8/13 fsverity: add support for info in fs-specific part of inode
In reply to Eric BiggersAdd an inode_info_offs field to struct fsverity_operations, and update fs/verity/ to support it. When set to a nonzero value, it specifies the offset to the fsverity_info pointer within the filesystem-specific part of the inode structure, to be used instead of inode::i_verity_info.
Since this makes inode::i_verity_info no longer necessarily used, update comments that mentioned it.
This is a prerequisite for a later commit that removes inode::i_verity_info, saving memory and improving cache efficiency on filesystems that don't support fsverity.
--- fs/verity/enable.c | 6 ++--- fs/verity/fsverity_private.h | 9 ++++---- fs/verity/open.c | 23 ++++++++++--------- fs/verity/verify.c | 2 +- include/linux/fsverity.h | 44 ++++++++++++++++++++++++++++-------- 5 files changed, 55 insertions(+), 29 deletions(-)
fs/verity/enable.c +3 -3
@@ -282,13 +282,13 @@ static int enable_verity(struct file *filp,fsverity_free_info (vi); }else {/* Successfully enabled verity */ /* -* Readers can start using ->i_verity_info immediately, so it -* can't be rolled back once set. So don't set it until just -* after the filesystem has successfully enabled verity. +* Readers can start using the inode's verity info immediately, +* so it can't be rolled back once set. So don't set it until +* just after the filesystem has successfully enabled verity. */ fsverity_set_info (inode, vi); } out:kfree (params.hashstate);
fs/verity/fsverity_private.h +5 -4
@@ -61,14 +61,15 @@ struct merkle_tree_params {/* * fsverity_info - cached verity metadata for an inode * * When a verity file is first opened, an instance of this struct is allocated -* and stored in ->i_verity_info; it remains until the inode is evicted. It -* caches information about the Merkle tree that's needed to efficiently verify -* data read from the file. It also caches the file digest. The Merkle tree -* pages themselves are not cached here, but the filesystem may cache them. +* and a pointer to it is stored in the file's in-memory inode. It remains +* until the inode is evicted. It caches information about the Merkle tree +* that's needed to efficiently verify data read from the file. It also caches +* the file digest. The Merkle tree pages themselves are not cached here, but +* the filesystem may cache them. */ struct fsverity_info {struct merkle_tree_params tree_params;u8 root_hash[FS_VERITY_MAX_DIGEST_SIZE ];u8 file_digest[FS_VERITY_MAX_DIGEST_SIZE ];
fs/verity/open.c +12 -11
@@ -242,21 +242,21 @@ struct fsverity_info *fsverity_create_info(const struct inode *inode, }void fsverity_set_info (struct inode *inode,struct fsverity_info *vi) {/* -* Multiple tasks may race to set ->i_verity_info, so use -* cmpxchg_release(). This pairs with the smp_load_acquire() in -* fsverity_get_info(). I.e., here we publish ->i_verity_info with a -* RELEASE barrier so that other tasks can ACQUIRE it. +* Multiple tasks may race to set the inode's verity info pointer, so +* use cmpxchg_release(). This pairs with the smp_load_acquire() in +* fsverity_get_info(). I.e., publish the pointer with a RELEASE +* barrier so that other tasks can ACQUIRE it. */ -if (cmpxchg_release (&inode->i_verity_info,NULL , vi) !=NULL ) { -/* Lost the race, so free the fsverity_info we allocated. */ +if (cmpxchg_release (fsverity_info_addr (inode),NULL , vi) !=NULL ) { +/* Lost the race, so free the verity info we allocated. */ fsverity_free_info (vi);/* -* Afterwards, the caller may access ->i_verity_info directly, -* so make sure to ACQUIRE the winning fsverity_info. +* Afterwards, the caller may access the inode's verity info +* directly, so make sure to ACQUIRE the winning verity info. */ (void )fsverity_get_info (inode); } }
@@ -348,11 +348,10 @@ int fsverity_get_descriptor(struct inode *inode, *desc_ret = desc;return 0 ; } -/* Ensure the inode has an ->i_verity_info */ static int ensure_verity_info (struct inode *inode) {struct fsverity_info *vi =fsverity_get_info (inode);struct fsverity_descriptor *desc;int err;
@@ -393,12 +392,14 @@ int __fsverity_prepare_setattr(struct dentry *dentry, struct iattr *attr) }EXPORT_SYMBOL_GPL (__fsverity_prepare_setattr);void __fsverity_cleanup_inode (struct inode *inode) { -fsverity_free_info (inode->i_verity_info); - inode->i_verity_info =NULL ; +struct fsverity_info **vi_addr =fsverity_info_addr (inode); + +fsverity_free_info (*vi_addr); + *vi_addr =NULL ; }EXPORT_SYMBOL_GPL (__fsverity_cleanup_inode);void __init fsverity_init_info_cache (void ) {
fs/verity/verify.c +1 -1
@@ -243,11 +243,11 @@ verify_data_block(struct inode *inode, struct fsverity_info *vi,static bool verify_data_blocks (struct folio *data_folio,size_t len,size_t offset,unsigned long max_ra_pages) {struct inode *inode = data_folio->mapping->host; -struct fsverity_info *vi = inode->i_verity_info; +struct fsverity_info *vi = *fsverity_info_addr (inode);const unsigned int block_size = vi->tree_params.block_size;u64 pos = (u64 )data_folio->index <<PAGE_SHIFT ;if (WARN_ON_ONCE (len <=0 || !IS_ALIGNED (len | offset, block_size)))return false;
include/linux/fsverity.h +34 -10
105 lines not on this page · raw message
Signature · 1 line
2.50.1
Eric Biggers
maintainerPATCH v5 9/13 ext4: move verity info pointer to fs-specific part of inode
In reply to Eric BiggersMove the fsverity_info pointer into the filesystem-specific part of the inode by adding the field ext4_inode_info::i_verity_info and configuring fsverity_operations::inode_info_offs accordingly.
This is a prerequisite for a later commit that removes inode::i_verity_info, saving memory and improving cache efficiency on filesystems that don't support fsverity.
--- fs/ext4/ext4.h | 4 ++++ fs/ext4/super.c | 3 +++ fs/ext4/verity.c | 2 ++ 3 files changed, 9 insertions(+)
fs/ext4/ext4.h +4 -0
@@ -1184,10 +1184,14 @@ struct ext4_inode_info {kprojid_t i_projid;#ifdef CONFIG_FS_ENCRYPTION struct fscrypt_inode_info *i_crypt_info;#endif + +#ifdef CONFIG_FS_VERITY +struct fsverity_info *i_verity_info; +#endif };/* * File system states */
fs/ext4/super.c +3 -0
@@ -1471,10 +1471,13 @@ static void init_once(void *foo)inode_init_once (&ei->vfs_inode);ext4_fc_init_inode (&ei->vfs_inode);#ifdef CONFIG_FS_ENCRYPTION ei->i_crypt_info =NULL ;#endif +#ifdef CONFIG_FS_VERITY + ei->i_verity_info =NULL ; +#endif }static int __init init_inodecache (void ) { ext4_inode_cachep = kmem_cache_create_usercopy("ext4_inode_cache",
fs/ext4/verity.c +2 -0
@@ -387,10 +387,12 @@ static int ext4_write_merkle_tree_block(struct inode *inode, const void *buf,return pagecache_write (inode, buf, size, pos); }const struct fsverity_operations ext4_verityops = { + .inode_info_offs = (int )offsetof(struct ext4_inode_info , i_verity_info) - + (int )offsetof(struct ext4_inode_info , vfs_inode), .begin_enable_verity = ext4_begin_enable_verity, .end_enable_verity = ext4_end_enable_verity, .get_verity_descriptor = ext4_get_verity_descriptor, .read_merkle_tree_page = ext4_read_merkle_tree_page, .write_merkle_tree_block = ext4_write_merkle_tree_block,
Signature · 1 line
2.50.1
Re: [PATCH v5 09/13] ext4: move verity info pointer to fs-specific part of inode
Quoted 11 lines · Eric Biggers
Move the
fsverity_infopointer into the filesystem-specific part of the inode by adding the fieldext4_inode_info::i_verity_infoand configuringfsverity_operations::inode_info_offsaccordingly.This is a prerequisite for a later commit that removes
inode::i_verity_info, saving memory and improving cache efficiency on filesystems that don't support fsverity.Co-developed-by Christian Brauner <[email protected]>Signed-off-by Christian Brauner <[email protected]>Signed-off-by Eric Biggers <[email protected]>
Eric Biggers
maintainerPATCH v5 10/13 f2fs: move verity info pointer to fs-specific part of inode
In reply to Eric BiggersMove the fsverity_info pointer into the filesystem-specific part of the inode by adding the field f2fs_inode_info::i_verity_info and configuring fsverity_operations::inode_info_offs accordingly.
This is a prerequisite for a later commit that removes inode::i_verity_info, saving memory and improving cache efficiency on filesystems that don't support fsverity.
--- fs/f2fs/f2fs.h | 3 +++ fs/f2fs/super.c | 3 +++ fs/f2fs/verity.c | 2 ++ 3 files changed, 8 insertions(+)
fs/f2fs/f2fs.h +3 -0
@@ -908,10 +908,13 @@ struct f2fs_inode_info {unsigned int atomic_write_cnt;loff_t original_i_size;/* original i_size before atomic write */ #ifdef CONFIG_FS_ENCRYPTION struct fscrypt_inode_info *i_crypt_info;/* filesystem encryption info */ #endif +#ifdef CONFIG_FS_VERITY +struct fsverity_info *i_verity_info;/* filesystem verity info */ +#endif };static inline void get_read_extent_info (struct extent_info *ext,struct f2fs_extent *i_ext) {
fs/f2fs/super.c +3 -0
@@ -481,10 +481,13 @@ static void init_once(void *foo)inode_init_once (&fi->vfs_inode);#ifdef CONFIG_FS_ENCRYPTION fi->i_crypt_info =NULL ;#endif +#ifdef CONFIG_FS_VERITY + fi->i_verity_info =NULL ; +#endif }#ifdef CONFIG_QUOTA static const char *const quotatypes[] =INITQFNAMES ;#define QTYPE2NAME (t) (quotatypes[t])
fs/f2fs/verity.c +2 -0
@@ -285,10 +285,12 @@ static int f2fs_write_merkle_tree_block(struct inode *inode, const void *buf,return pagecache_write (inode, buf, size, pos); }const struct fsverity_operations f2fs_verityops = { + .inode_info_offs = (int )offsetof(struct f2fs_inode_info , i_verity_info) - + (int )offsetof(struct f2fs_inode_info , vfs_inode), .begin_enable_verity = f2fs_begin_enable_verity, .end_enable_verity = f2fs_end_enable_verity, .get_verity_descriptor = f2fs_get_verity_descriptor, .read_merkle_tree_page = f2fs_read_merkle_tree_page, .write_merkle_tree_block = f2fs_write_merkle_tree_block,
Signature · 1 line
2.50.1
Eric Biggers
maintainerPATCH v5 11/13 btrfs: move verity info pointer to fs-specific part of inode
In reply to Eric BiggersMove the fsverity_info pointer into the filesystem-specific part of the inode by adding the field btrfs_inode::i_verity_info and configuring fsverity_operations::inode_info_offs accordingly.
This is a prerequisite for a later commit that removes inode::i_verity_info, saving memory and improving cache efficiency on filesystems that don't support fsverity.
--- fs/btrfs/btrfs_inode.h | 5 +++++ fs/btrfs/inode.c | 3 +++ fs/btrfs/verity.c | 2 ++ 3 files changed, 10 insertions(+)
fs/btrfs/btrfs_inode.h +5 -0
@@ -336,10 +336,15 @@ struct btrfs_inode {/* Hook into fs_info->delayed_iputs */ struct list_head delayed_iput;struct rw_semaphore i_mmap_lock; + +#ifdef CONFIG_FS_VERITY +struct fsverity_info *i_verity_info; +#endif +struct inode vfs_inode; };static inline u64 btrfs_get_first_dir_index_to_log (const struct btrfs_inode *inode) {
fs/btrfs/inode.c +3 -0
@@ -7959,10 +7959,13 @@ int btrfs_drop_inode(struct inode *inode)static void init_once (void *foo) {struct btrfs_inode *ei = foo;inode_init_once (&ei->vfs_inode); +#ifdef CONFIG_FS_VERITY + ei->i_verity_info =NULL ; +#endif }void __cold btrfs_destroy_cachep (void ) {/*
fs/btrfs/verity.c +2 -0
@@ -800,10 +800,12 @@ static int btrfs_write_merkle_tree_block(struct inode *inode, const void *buf,return write_key_bytes (BTRFS_I (inode),BTRFS_VERITY_MERKLE_ITEM_KEY , pos, buf, size); }const struct fsverity_operations btrfs_verityops = { + .inode_info_offs = (int )offsetof(struct btrfs_inode , i_verity_info) - + (int )offsetof(struct btrfs_inode , vfs_inode), .begin_enable_verity = btrfs_begin_enable_verity, .end_enable_verity = btrfs_end_enable_verity, .get_verity_descriptor = btrfs_get_verity_descriptor, .read_merkle_tree_page = btrfs_read_merkle_tree_page, .write_merkle_tree_block = btrfs_write_merkle_tree_block,
Signature · 1 line
2.50.1
Eric Biggers
maintainerPATCH v5 12/13 fs: remove inode::i_verity_info
In reply to Eric BiggersNow that all fsverity-capable filesystems store the pointer to fsverity_info in the filesystem-specific part of the inode structure, inode::i_verity_info is no longer needed. Update fsverity_info_addr() to no longer support the fallback to inode::i_verity_info. Finally, remove inode::i_verity_info itself, and move the forward declaration of struct fsverity_info from fs.h (which no longer needs it) to fsverity.h.
The end result of the migration to the filesystem-specific pointer is memory savings on CONFIG_FS_VERITY=y kernels for all filesystems that don't support fsverity. Specifically, their in-memory inodes are now smaller by the size of a pointer: either 4 or 8 bytes.
--- include/linux/fs.h | 5 ----- include/linux/fsverity.h | 10 ++++++++-- 2 files changed, 8 insertions(+), 7 deletions(-)
include/linux/fs.h +0 -5
@@ -71,11 +71,10 @@ struct cred;struct swap_info_struct ;struct seq_file ;struct workqueue_struct ;struct iov_iter ;struct fscrypt_operations ; -struct fsverity_info ;struct fsverity_operations ;struct fsnotify_mark_connector ;struct fsnotify_sb_info ;struct fs_context ;struct fs_parameter_spec ;
@@ -777,14 +776,10 @@ struct inode {__u32 i_fsnotify_mask;/* all events this inode cares about */ /* 32-bit hole reserved for expanding i_fsnotify_mask */ struct fsnotify_mark_connector __rcu *i_fsnotify_marks;#endif -#ifdef CONFIG_FS_VERITY -struct fsverity_info *i_verity_info; -#endif -void *i_private;/* fs or device private pointer */ } __randomize_layout;static inline void inode_set_cached_link (struct inode *inode,char *link,int linklen) {
include/linux/fsverity.h +8 -2
@@ -24,10 +24,12 @@#define FS_VERITY_MAX_DIGEST_SIZE SHA512_DIGEST_SIZE/* Arbitrary limit to bound the kmalloc() size. Can be changed. */ #define FS_VERITY_MAX_DESCRIPTOR_SIZE 16384 +struct fsverity_info ; +/* Verity operations for filesystems */ struct fsverity_operations {/** * The offset of the pointer to struct fsverity_info in the * filesystem-specific part of the inode, relative to the beginning of
@@ -128,15 +130,19 @@ struct fsverity_operations { u64 pos, unsigned int size); };#ifdef CONFIG_FS_VERITY +/* +* Returns the address of the verity info pointer within the filesystem-specific +* part of the inode. (To save memory on filesystems that don't support +* fsverity, a field in 'struct inode' itself is no longer used.) +*/ static inline struct fsverity_info **fsverity_info_addr (const struct inode *inode) { -if (inode->i_sb->s_vop->inode_info_offs ==0 ) -return (struct fsverity_info **)&inode->i_verity_info; +VFS_WARN_ON_ONCE (inode->i_sb->s_vop->inode_info_offs ==0 );return (void *)inode + inode->i_sb->s_vop->inode_info_offs; }static inline struct fsverity_info *fsverity_get_info (const struct inode *inode) {
Signature · 1 line
2.50.1
Eric Biggers
maintainerPATCH v5 13/13 fsverity: check IS_VERITY() in fsverity_cleanup_inode()
In reply to Eric BiggersSince getting the address of the fsverity_info has gotten a bit more expensive, make fsverity_cleanup_inode() check for IS_VERITY() instead. This avoids adding more overhead to non-verity files.
This assumes that verity info is never set when !IS_VERITY(), which is currently true, but add a VFS_WARN_ON_ONCE() that asserts that. (This of course defeats the optimization, but only when CONFIG_VFS_DEBUG=y.)
--- include/linux/fsverity.h | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
include/linux/fsverity.h +8 -1
@@ -188,12 +188,19 @@ void __fsverity_cleanup_inode(struct inode *inode);* * Filesystems must call this on inode eviction to free the inode's verity info. */ static inline void fsverity_cleanup_inode (struct inode *inode) { -if (*fsverity_info_addr (inode)) +/* +* Only IS_VERITY() inodes can have verity info, so start by checking +* for IS_VERITY() (which is faster than retrieving the pointer to the +* verity info). This minimizes overhead for non-verity inodes. +*/ +if (IS_VERITY (inode))__fsverity_cleanup_inode (inode); +else +VFS_WARN_ON_ONCE (*fsverity_info_addr (inode) !=NULL ); }/* read_metadata.c */ int fsverity_ioctl_read_metadata (struct file *filp,const void __user *uarg);
Signature · 1 line
2.50.1
Christian Brauner
maintainerQuoted 27 lines · Eric Biggers
This is a cleaned-up implementation of moving the
i_crypt_infoandi_verity_infopointers out of 'struct inode' and into the fs-specific part of the inode, as proposed previously by Christian at
https://lore.kernel.org/r/[email protected]/The high-level concept is still the same:
fs/crypto/andfs/verity/locate the pointer by adding an offset to the address of struct inode. The offset is retrieved fromfscrypt_operationsorfsverity_operations.I've cleaned up a lot of the details, including:
- Grouped changes into patches differently
- Rewrote commit messages and comments to be clearer
- Adjusted code formatting to be consistent with existing code
- Removed unneeded #ifdefs
- Improved choice and location ofVFS_WARN_ON_ONCE()statements
- Added missing kerneldoc forubifs_inode::i_crypt_info
- Moved field initialization toinit_oncefunctions when they exist
- Improved ceph offset calculation and removed unneededstatic_asserts
-fsverity_get_info()now checksIS_VERITY()instead ofv_ops
-fscrypt_put_encryption_info()no longer checksIS_ENCRYPTED(), since I no longer think it's actually correct there.
-verity_data_blocks()now keeps doing a raw dereference
- Droppedfscrypt_set_inode_info()
- Renamed some functions
- Do offset calculation using int, so we don't rely on unsigned overflow
- And more.
Quoted 6 lines
For v4 and earlier, see
https://lore.kernel.org/r/[email protected]/I'd like to take this series through the fscrypt tree for 6.18. (fsverity normally has a separate tree, but by choosing just one tree for this, we'll avoid conflicts in some places.)
Woh woh. First, I had a cleaned up version ready for v6.18 so if you plan on taking over someone's series and resend then maybe ask the author first whether that's ok or not. I haven't seen you do that. You just caused duplicated work for no reason.
And second general infrastructure changes that touch multiple fses and generic fs infrastructure I very much want to go through VFS trees. We'll simply use a shared tree.
Eric Biggers
maintainerQuoted 31 lines · Christian Brauner
Quoted 30 lines · Eric Biggers
This is a cleaned-up implementation of moving the
i_crypt_infoandi_verity_infopointers out of 'struct inode' and into the fs-specific part of the inode, as proposed previously by Christian at
https://lore.kernel.org/r/[email protected]/The high-level concept is still the same:
fs/crypto/andfs/verity/locate the pointer by adding an offset to the address of struct inode. The offset is retrieved fromfscrypt_operationsorfsverity_operations.I've cleaned up a lot of the details, including:
- Grouped changes into patches differently
- Rewrote commit messages and comments to be clearer
- Adjusted code formatting to be consistent with existing code
- Removed unneeded #ifdefs
- Improved choice and location ofVFS_WARN_ON_ONCE()statements
- Added missing kerneldoc forubifs_inode::i_crypt_info
- Moved field initialization toinit_oncefunctions when they exist
- Improved ceph offset calculation and removed unneededstatic_asserts
-fsverity_get_info()now checksIS_VERITY()instead ofv_ops
-fscrypt_put_encryption_info()no longer checksIS_ENCRYPTED(), since I no longer think it's actually correct there.
-verity_data_blocks()now keeps doing a raw dereference
- Droppedfscrypt_set_inode_info()
- Renamed some functions
- Do offset calculation using int, so we don't rely on unsigned overflow
- And more.For v4 and earlier, see
https://lore.kernel.org/r/[email protected]/
Quoted 8 lines
Quoted 3 lines
I'd like to take this series through the fscrypt tree for 6.18. (fsverity normally has a separate tree, but by choosing just one tree for this, we'll avoid conflicts in some places.)
Woh woh. First, I had a cleaned up version ready for v6.18 so if you plan on taking over someone's series and resend then maybe ask the author first whether that's ok or not. I haven't seen you do that. You just caused duplicated work for no reason.
Ah, sorry about that. When I started looking at it again yesterday there turned out to be way too many cleanups and fixes I wanted to make (beyond the comments I gave earlier), and I hadn't seen activity from you on it in a while. So I figured it would be easier to just send a series myself. But I should have asked you first, sorry.
Quoted 3 lines
And second general infrastructure changes that touch multiple fses and generic fs infrastructure I very much want to go through VFS trees. We'll simply use a shared tree.
So you'd like to discontinue the fscrypt and fsverity trees? That's what they are for: general infrastructure shared by multiple filesystems. Or is this comment just for this series in particular, presumably because it touches 'struct inode'?
- Eric
Christian Brauner
maintainerQuoted 41 lines · Eric Biggers
Quoted 31 lines · Christian Brauner
Quoted 30 lines · Eric Biggers
This is a cleaned-up implementation of moving the
i_crypt_infoandi_verity_infopointers out of 'struct inode' and into the fs-specific part of the inode, as proposed previously by Christian at
https://lore.kernel.org/r/[email protected]/The high-level concept is still the same:
fs/crypto/andfs/verity/locate the pointer by adding an offset to the address of struct inode. The offset is retrieved fromfscrypt_operationsorfsverity_operations.I've cleaned up a lot of the details, including:
- Grouped changes into patches differently
- Rewrote commit messages and comments to be clearer
- Adjusted code formatting to be consistent with existing code
- Removed unneeded #ifdefs
- Improved choice and location ofVFS_WARN_ON_ONCE()statements
- Added missing kerneldoc forubifs_inode::i_crypt_info
- Moved field initialization toinit_oncefunctions when they exist
- Improved ceph offset calculation and removed unneededstatic_asserts
-fsverity_get_info()now checksIS_VERITY()instead ofv_ops
-fscrypt_put_encryption_info()no longer checksIS_ENCRYPTED(), since I no longer think it's actually correct there.
-verity_data_blocks()now keeps doing a raw dereference
- Droppedfscrypt_set_inode_info()
- Renamed some functions
- Do offset calculation using int, so we don't rely on unsigned overflow
- And more.For v4 and earlier, see
https://lore.kernel.org/r/[email protected]/Quoted 8 lines
Quoted 3 lines
I'd like to take this series through the fscrypt tree for 6.18. (fsverity normally has a separate tree, but by choosing just one tree for this, we'll avoid conflicts in some places.)
Woh woh. First, I had a cleaned up version ready for v6.18 so if you plan on taking over someone's series and resend then maybe ask the author first whether that's ok or not. I haven't seen you do that. You just caused duplicated work for no reason.
Quoted 5 lines
Ah, sorry about that. When I started looking at it again yesterday there turned out to be way too many cleanups and fixes I wanted to make (beyond the comments I gave earlier), and I hadn't seen activity from you on it in a while. So I figured it would be easier to just send a series myself. But I should have asked you first, sorry.
So I started working on this pretty much right away. And I had planned on sending it out rather soon but then thought to better wait for -rc1 to be released because I saw you had a bunch of crypto changes in for -rc1 that would've caused merge conflicts. It's no big deal overall but I just don't like that I wasted massaging all that stuff. So next time a heads-up would be nice. Thank you!
Quoted 9 lines
Quoted 4 lines
And second general infrastructure changes that touch multiple fses and generic fs infrastructure I very much want to go through VFS trees. We'll simply use a shared tree.
So you'd like to discontinue the fscrypt and fsverity trees? That's what they are for: general infrastructure shared by multiple filesystems. Or is this comment just for this series in particular, presumably because it touches 'struct inode'?
My comment just applies this series. I'm not here to take away your trees ofc unless you would like to have them go through the VFS batch. That's something that some people like Amir have started doing.
I'll put the series into vfs-6.17.inode and push it out then you can base any additional changes on top of that. I'll not touch it unless you tell me to. Linus knows that VFS trees often have work that is used as the base for other trees so he will merge VFS trees before any of the smaller trees and I always mention this to him.
Christian Brauner
maintainerQuoted 48 lines · Christian Brauner
Quoted 41 lines · Eric Biggers
Quoted 31 lines · Christian Brauner
Quoted 30 lines · Eric Biggers
This is a cleaned-up implementation of moving the
i_crypt_infoandi_verity_infopointers out of 'struct inode' and into the fs-specific part of the inode, as proposed previously by Christian at
https://lore.kernel.org/r/[email protected]/The high-level concept is still the same:
fs/crypto/andfs/verity/locate the pointer by adding an offset to the address of struct inode. The offset is retrieved fromfscrypt_operationsorfsverity_operations.I've cleaned up a lot of the details, including:
- Grouped changes into patches differently
- Rewrote commit messages and comments to be clearer
- Adjusted code formatting to be consistent with existing code
- Removed unneeded #ifdefs
- Improved choice and location ofVFS_WARN_ON_ONCE()statements
- Added missing kerneldoc forubifs_inode::i_crypt_info
- Moved field initialization toinit_oncefunctions when they exist
- Improved ceph offset calculation and removed unneededstatic_asserts
-fsverity_get_info()now checksIS_VERITY()instead ofv_ops
-fscrypt_put_encryption_info()no longer checksIS_ENCRYPTED(), since I no longer think it's actually correct there.
-verity_data_blocks()now keeps doing a raw dereference
- Droppedfscrypt_set_inode_info()
- Renamed some functions
- Do offset calculation using int, so we don't rely on unsigned overflow
- And more.For v4 and earlier, see
https://lore.kernel.org/r/[email protected]/Quoted 8 lines
Quoted 3 lines
I'd like to take this series through the fscrypt tree for 6.18. (fsverity normally has a separate tree, but by choosing just one tree for this, we'll avoid conflicts in some places.)
Woh woh. First, I had a cleaned up version ready for v6.18 so if you plan on taking over someone's series and resend then maybe ask the author first whether that's ok or not. I haven't seen you do that. You just caused duplicated work for no reason.
Quoted 5 lines
Ah, sorry about that. When I started looking at it again yesterday there turned out to be way too many cleanups and fixes I wanted to make (beyond the comments I gave earlier), and I hadn't seen activity from you on it in a while. So I figured it would be easier to just send a series myself. But I should have asked you first, sorry.
Quoted 6 lines
So I started working on this pretty much right away. And I had planned on sending it out rather soon but then thought to better wait for -rc1 to be released because I saw you had a bunch of crypto changes in for -rc1 that would've caused merge conflicts. It's no big deal overall but I just don't like that I wasted massaging all that stuff. So next time a heads-up would be nice. Thank you!
I just pulled the series and now I see that you also changed the authorship of every single patch in the series from me to you and put my Co-developed-by in there.
I mean I acknowledge that there's changes between the branches and there's some function renaming but it's not to the point where authorship should be changed. And if you think that's necessary than it would be something you would want to talk to me about first.
I don't care about the stats but it was always hugely frustrating to me when I started kernel development when senior kernel developers waltzed in and thought they'd just take things over so I try very hard to not do that unless this is agreed upon first.
Quoted 18 lines
Quoted 8 lines
Quoted 3 lines
And second general infrastructure changes that touch multiple fses and generic fs infrastructure I very much want to go through VFS trees. We'll simply use a shared tree.
So you'd like to discontinue the fscrypt and fsverity trees? That's what they are for: general infrastructure shared by multiple filesystems. Or is this comment just for this series in particular, presumably because it touches 'struct inode'?
My comment just applies this series. I'm not here to take away your trees ofc unless you would like to have them go through the VFS batch. That's something that some people like Amir have started doing.
I'll put the series into vfs-6.17.inode and push it out then you can base any additional changes on top of that. I'll not touch it unless you tell me to. Linus knows that VFS trees often have work that is used as the base for other trees so he will merge VFS trees before any of the smaller trees and I always mention this to him.
Eric Biggers
maintainerQuoted 65 lines · Christian Brauner
Quoted 48 lines · Christian Brauner
Quoted 41 lines · Eric Biggers
Quoted 31 lines · Christian Brauner
Quoted 30 lines · Eric Biggers
This is a cleaned-up implementation of moving the
i_crypt_infoandi_verity_infopointers out of 'struct inode' and into the fs-specific part of the inode, as proposed previously by Christian at
https://lore.kernel.org/r/[email protected]/The high-level concept is still the same:
fs/crypto/andfs/verity/locate the pointer by adding an offset to the address of struct inode. The offset is retrieved fromfscrypt_operationsorfsverity_operations.I've cleaned up a lot of the details, including:
- Grouped changes into patches differently
- Rewrote commit messages and comments to be clearer
- Adjusted code formatting to be consistent with existing code
- Removed unneeded #ifdefs
- Improved choice and location ofVFS_WARN_ON_ONCE()statements
- Added missing kerneldoc forubifs_inode::i_crypt_info
- Moved field initialization toinit_oncefunctions when they exist
- Improved ceph offset calculation and removed unneededstatic_asserts
-fsverity_get_info()now checksIS_VERITY()instead ofv_ops
-fscrypt_put_encryption_info()no longer checksIS_ENCRYPTED(), since I no longer think it's actually correct there.
-verity_data_blocks()now keeps doing a raw dereference
- Droppedfscrypt_set_inode_info()
- Renamed some functions
- Do offset calculation using int, so we don't rely on unsigned overflow
- And more.For v4 and earlier, see
https://lore.kernel.org/r/[email protected]/Quoted 8 lines
Quoted 3 lines
I'd like to take this series through the fscrypt tree for 6.18. (fsverity normally has a separate tree, but by choosing just one tree for this, we'll avoid conflicts in some places.)
Woh woh. First, I had a cleaned up version ready for v6.18 so if you plan on taking over someone's series and resend then maybe ask the author first whether that's ok or not. I haven't seen you do that. You just caused duplicated work for no reason.
Quoted 5 lines
Ah, sorry about that. When I started looking at it again yesterday there turned out to be way too many cleanups and fixes I wanted to make (beyond the comments I gave earlier), and I hadn't seen activity from you on it in a while. So I figured it would be easier to just send a series myself. But I should have asked you first, sorry.
Quoted 6 lines
So I started working on this pretty much right away. And I had planned on sending it out rather soon but then thought to better wait for -rc1 to be released because I saw you had a bunch of crypto changes in for -rc1 that would've caused merge conflicts. It's no big deal overall but I just don't like that I wasted massaging all that stuff. So next time a heads-up would be nice. Thank you!
I just pulled the series and now I see that you also changed the authorship of every single patch in the series from me to you and put my Co-developed-by in there.
I mean I acknowledge that there's changes between the branches and there's some function renaming but it's not to the point where authorship should be changed. And if you think that's necessary than it would be something you would want to talk to me about first.
Quoted 4 lines
I don't care about the stats but it was always hugely frustrating to me when I started kernel development when senior kernel developers waltzed in and thought they'd just take things over so I try very hard to not do that unless this is agreed upon first.
If you want to keep the authorship that's fine with me. Make sure you've checked the diff: the diff between v4 and v5 is larger than the diff between the base and either version. And as I mentioned, I rewrote the commit messages and divided some of the changes into commits differently as well. If the situation was flipped, I wouldn't want to be kept as the author. But I realize there are different opinions about this sort of thing, and I'm totally fine with whatever you prefer.
Thanks,
- Eric
Christian Brauner
maintainerQuoted 71 lines · Eric Biggers
Quoted 65 lines · Christian Brauner
Quoted 48 lines · Christian Brauner
Quoted 41 lines · Eric Biggers
Quoted 31 lines · Christian Brauner
Quoted 30 lines · Eric Biggers
This is a cleaned-up implementation of moving the
i_crypt_infoandi_verity_infopointers out of 'struct inode' and into the fs-specific part of the inode, as proposed previously by Christian at
https://lore.kernel.org/r/[email protected]/The high-level concept is still the same:
fs/crypto/andfs/verity/locate the pointer by adding an offset to the address of struct inode. The offset is retrieved fromfscrypt_operationsorfsverity_operations.I've cleaned up a lot of the details, including:
- Grouped changes into patches differently
- Rewrote commit messages and comments to be clearer
- Adjusted code formatting to be consistent with existing code
- Removed unneeded #ifdefs
- Improved choice and location ofVFS_WARN_ON_ONCE()statements
- Added missing kerneldoc forubifs_inode::i_crypt_info
- Moved field initialization toinit_oncefunctions when they exist
- Improved ceph offset calculation and removed unneededstatic_asserts
-fsverity_get_info()now checksIS_VERITY()instead ofv_ops
-fscrypt_put_encryption_info()no longer checksIS_ENCRYPTED(), since I no longer think it's actually correct there.
-verity_data_blocks()now keeps doing a raw dereference
- Droppedfscrypt_set_inode_info()
- Renamed some functions
- Do offset calculation using int, so we don't rely on unsigned overflow
- And more.For v4 and earlier, see
https://lore.kernel.org/r/[email protected]/Quoted 8 lines
Quoted 3 lines
I'd like to take this series through the fscrypt tree for 6.18. (fsverity normally has a separate tree, but by choosing just one tree for this, we'll avoid conflicts in some places.)
Woh woh. First, I had a cleaned up version ready for v6.18 so if you plan on taking over someone's series and resend then maybe ask the author first whether that's ok or not. I haven't seen you do that. You just caused duplicated work for no reason.
Quoted 5 lines
Ah, sorry about that. When I started looking at it again yesterday there turned out to be way too many cleanups and fixes I wanted to make (beyond the comments I gave earlier), and I hadn't seen activity from you on it in a while. So I figured it would be easier to just send a series myself. But I should have asked you first, sorry.
Quoted 6 lines
So I started working on this pretty much right away. And I had planned on sending it out rather soon but then thought to better wait for -rc1 to be released because I saw you had a bunch of crypto changes in for -rc1 that would've caused merge conflicts. It's no big deal overall but I just don't like that I wasted massaging all that stuff. So next time a heads-up would be nice. Thank you!
I just pulled the series and now I see that you also changed the authorship of every single patch in the series from me to you and put my Co-developed-by in there.
I mean I acknowledge that there's changes between the branches and there's some function renaming but it's not to the point where authorship should be changed. And if you think that's necessary than it would be something you would want to talk to me about first.
Quoted 4 lines
I don't care about the stats but it was always hugely frustrating to me when I started kernel development when senior kernel developers waltzed in and thought they'd just take things over so I try very hard to not do that unless this is agreed upon first.
Quoted 7 lines
If you want to keep the authorship that's fine with me. Make sure you've checked the diff: the diff between v4 and v5 is larger than the diff between the base and either version. And as I mentioned, I rewrote the commit messages and divided some of the changes into commits differently as well. If the situation was flipped, I wouldn't want to be kept as the author. But I realize there are different opinions about this sort of thing, and I'm totally fine with whatever you prefer.
It's not that I oppose it per se it's just that it would be nice to have gotten a heads-up about both the rewrite and the authorship change. (Sorry, still on vacation and so answers are delayed.)
Quoted 4 lines · Eric Biggers
This is a cleaned-up implementation of moving the
i_crypt_infoandi_verity_infopointers out of 'struct inode' and into the fs-specific part of the inode, as proposed previously by Christian at
https://lore.kernel.org/r/[email protected]/
I would really much prefer to move fscrypt to use a hash lookup instead of bloating all inodes for a each file system supporting it, even if very few files on very few file systems are using it. With the fsverity xfs series posted again this is becoming personal :)
You mentioned you were looking into it but didn't like the rhashtable API. My offer to help with that still stands.
Eric Biggers
maintainerQuoted 6 lines · Christoph Hellwig
Quoted 4 lines · Eric Biggers
This is a cleaned-up implementation of moving the
i_crypt_infoandi_verity_infopointers out of 'struct inode' and into the fs-specific part of the inode, as proposed previously by Christian at
https://lore.kernel.org/r/[email protected]/
Quoted 7 lines
I would really much prefer to move fscrypt to use a hash lookup instead of bloating all inodes for a each file system supporting it, even if very few files on very few file systems are using it. With the fsverity xfs series posted again this is becoming personal :)
You mentioned you were looking into it but didn't like the rhashtable API. My offer to help with that still stands.
I assume you actually still mean fsverity, not fscrypt. First, it would be helpful not to use one solution for fscrypt and a totally different solution for fsverity, as that would increase the maintenance cost well beyond that of either solution individually.
Second, the fsverity info can be loaded very frequently. For example, curently it's loaded for each 4K data block processed. Also, there *are* use cases in which most files on the filesystem have fsverity enabled. Not super common, but they exist.
Yes, the rhashtable is technically O(1), but its code is a mess, both source code and generated code. Let's look at the x86_64 code generated by the fs-specific field solution in this patchset:
a74: 48 8b 47 28 mov 0x28(%rdi),%rax
a78: 48 8b 80 a8 00 00 00 mov 0xa8(%rax),%rax
a7f: 48 8b 00 mov (%rax),%rax
a82: 48 8b 04 07 mov (%rdi,%rax,1),%raxA few dependent loads, but nice and simple.
Now let's look at the rhashtable version of the load. (I used the 'struct inode *' as the rhashtable key):
ffffffff81487f00 <fsverity_get_info>:
ffffffff81487f00: f3 0f 1e fa endbr64
ffffffff81487f04: 55 push %rbp
ffffffff81487f05: 48 89 e5 mov %rsp,%rbp
ffffffff81487f08: 41 57 push %r15
ffffffff81487f0a: 41 56 push %r14
ffffffff81487f0c: 41 55 push %r13
ffffffff81487f0e: 41 54 push %r12
ffffffff81487f10: 53 push %rbx
ffffffff81487f11: 48 83 ec 10 sub $0x10,%rsp
ffffffff81487f15: 48 89 7d d0 mov %rdi,-0x30(%rbp)
ffffffff81487f19: e8 d2 c6 e9 ff call ffffffff813245f0 <__rcu_read_lock>
ffffffff81487f1e: 48 8b 1d 5b 15 c9 00 mov 0xc9155b(%rip),%rbx # ffffffff82119480 <fsverity_info_hashtable>
ffffffff81487f25: 8b 43 08 mov 0x8(%rbx),%eax
ffffffff81487f28: 8b 55 d4 mov -0x2c(%rbp),%edx
ffffffff81487f2b: 8b 4d d0 mov -0x30(%rbp),%ecx
ffffffff81487f2e: 8b 3b mov (%rbx),%edi
ffffffff81487f30: 2d 09 41 52 21 sub $0x21524109,%eax
ffffffff81487f35: 01 c2 add %eax,%edx
ffffffff81487f37: 01 c1 add %eax,%ecx
ffffffff81487f39: 89 d6 mov %edx,%esi
ffffffff81487f3b: 31 d0 xor %edx,%eax
ffffffff81487f3d: c1 c6 0e rol $0xe,%esi
ffffffff81487f40: 29 f0 sub %esi,%eax
ffffffff81487f42: 89 c6 mov %eax,%esi
ffffffff81487f44: 31 c1 xor %eax,%ecx
ffffffff81487f46: c1 c6 0b rol $0xb,%esi
ffffffff81487f49: 29 f1 sub %esi,%ecx
ffffffff81487f4b: 89 ce mov %ecx,%esi
ffffffff81487f4d: 31 ca xor %ecx,%edx
ffffffff81487f4f: c1 ce 07 ror $0x7,%esi
ffffffff81487f52: 29 f2 sub %esi,%edx
ffffffff81487f54: 89 d6 mov %edx,%esi
ffffffff81487f56: 31 d0 xor %edx,%eax
ffffffff81487f58: c1 c6 10 rol $0x10,%esi
ffffffff81487f5b: 29 f0 sub %esi,%eax
ffffffff81487f5d: 89 c6 mov %eax,%esi
ffffffff81487f5f: 31 c1 xor %eax,%ecx
ffffffff81487f61: c1 c6 04 rol $0x4,%esi
ffffffff81487f64: 29 f1 sub %esi,%ecx
ffffffff81487f66: 8d 77 ff lea -0x1(%rdi),%esi
ffffffff81487f69: 31 ca xor %ecx,%edx
ffffffff81487f6b: c1 c1 0e rol $0xe,%ecx
ffffffff81487f6e: 29 ca sub %ecx,%edx
ffffffff81487f70: 31 d0 xor %edx,%eax
ffffffff81487f72: c1 ca 08 ror $0x8,%edx
ffffffff81487f75: 29 d0 sub %edx,%eax
ffffffff81487f77: 21 c6 and %eax,%esi
ffffffff81487f79: 8b 43 04 mov 0x4(%rbx),%eax
ffffffff81487f7c: 4c 8d 6c f3 40 lea 0x40(%rbx,%rsi,8),%r13
ffffffff81487f81: 85 c0 test %eax,%eax
ffffffff81487f83: 0f 85 8c 00 00 00 jne ffffffff81488015 <fsverity_get_info+0x115>
ffffffff81487f89: 49 8b 4d 00 mov 0x0(%r13),%rcx
ffffffff81487f8d: 48 83 e1 fe and $0xfffffffffffffffe,%rcx
ffffffff81487f91: 74 71 je ffffffff81488004 <fsverity_get_info+0x104>
ffffffff81487f93: 0f b7 05 f8 14 c9 00 movzwl 0xc914f8(%rip),%eax # ffffffff82119492 <fsverity_info_hashtable+0x12>
ffffffff81487f9a: 44 0f b7 25 f2 14 c9 movzwl 0xc914f2(%rip),%r12d # ffffffff82119494 <fsverity_info_hashtable+0x14>
ffffffff81487fa1: 00
ffffffff81487fa2: 49 89 ce mov %rcx,%r14
ffffffff81487fa5: 44 0f b7 3d e9 14 c9 movzwl 0xc914e9(%rip),%r15d # ffffffff82119496 <fsverity_info_hashtable+0x16>
ffffffff81487fac: 00
ffffffff81487fad: 48 89 45 c8 mov %rax,-0x38(%rbp)
ffffffff81487fb1: 4d 29 fc sub %r15,%r12
ffffffff81487fb4: 48 8b 55 c8 mov -0x38(%rbp),%rdx
ffffffff81487fb8: 4b 8d 3c 26 lea (%r14,%r12,1),%rdi
ffffffff81487fbc: 48 8d 75 d0 lea -0x30(%rbp),%rsi
ffffffff81487fc0: e8 2b 53 4b 00 call ffffffff8193d2f0 <memcmp>
ffffffff81487fc5: 85 c0 test %eax,%eax
ffffffff81487fc7: 75 26 jne ffffffff81487fef <fsverity_get_info+0xef>
ffffffff81487fc9: 4d 85 f6 test %r14,%r14
ffffffff81487fcc: 74 43 je ffffffff81488011 <fsverity_get_info+0x111>
ffffffff81487fce: 4c 89 f3 mov %r14,%rbx
ffffffff81487fd1: 4c 29 fb sub %r15,%rbx
ffffffff81487fd4: e8 67 18 ea ff call ffffffff81329840 <__rcu_read_unlock>
ffffffff81487fd9: 48 89 d8 mov %rbx,%rax
ffffffff81487fdc: 48 83 c4 10 add $0x10,%rsp
ffffffff81487fe0: 5b pop %rbx
ffffffff81487fe1: 41 5c pop %r12
ffffffff81487fe3: 41 5d pop %r13
ffffffff81487fe5: 41 5e pop %r14
ffffffff81487fe7: 41 5f pop %r15
ffffffff81487fe9: 5d pop %rbp
ffffffff81487fea: e9 01 53 4d 00 jmp ffffffff8195d2f0 <__pi___x86_return_thunk>
ffffffff81487fef: 4d 8b 36 mov (%r14),%r14
ffffffff81487ff2: 41 f6 c6 01 test $0x1,%r14b
ffffffff81487ff6: 74 bc je ffffffff81487fb4 <fsverity_get_info+0xb4>
ffffffff81487ff8: 4c 89 e8 mov %r13,%rax
ffffffff81487ffb: 48 83 c8 01 or $0x1,%rax
ffffffff81487fff: 49 39 c6 cmp %rax,%r14
ffffffff81488002: 75 85 jne ffffffff81487f89 <fsverity_get_info+0x89>
ffffffff81488004: 48 8b 5b 30 mov 0x30(%rbx),%rbx
ffffffff81488008: 48 85 db test %rbx,%rbx
ffffffff8148800b: 0f 85 14 ff ff ff jne ffffffff81487f25 <fsverity_get_info+0x25>
ffffffff81488011: 31 db xor %ebx,%ebx
ffffffff81488013: eb bf jmp ffffffff81487fd4 <fsverity_get_info+0xd4>
ffffffff81488015: 48 89 df mov %rbx,%rdi
ffffffff81488018: e8 33 94 11 00 call ffffffff815a1450 <rht_bucket_nested>
ffffffff8148801d: 49 89 c5 mov %rax,%r13
ffffffff81488020: e9 64 ff ff ff jmp ffffffff81487f89 <fsverity_get_info+0x89>
ffffffff81488025: 66 66 2e 0f 1f 84 00 data16 cs nopw 0x0(%rax,%rax,1)
ffffffff8148802c: 00 00 00 00 It doesn't really seem like the kind of solution that's a good choice for a frequently-loaded field. And that's only the load; it's not getting into the insertion (and resizing) part.
Reliability due to the code complexity is also a concern, of course. 'git log --grep="rhashtable:" --grep="Fixes:" --all-match' turns up quite a few issues in rhashtable over time...
If we're going so far as to use a rhashtable, I have to wonder why we aren't first prioritizing other fields. For example ext4_inode_info unconditionally has 40 bytes for fast_commit information, even though fast_commit is an experimental ext4 feature that isn't enabled on most filesystems. That's 5 times as much as i_verity_info. And quota has 24 bytes under CONFIG_QUOTA. And there are even holes in the ext4_inode_info struct; we could also just improve the field packing!
The fs-specific field solution from this patchset is much more efficient than the rhashtable: efficient enough that we don't really have to worry about it, regardless of fscrypt or fsverity. So I think it's a good middle ground, and I'd like to just do it this way.
- Eric
Quoted 1 line · Eric Biggers
I assume you actually still mean fsverity, not fscrypt.
Yes, sorry.
Quoted 4 lines
First, it would
be helpful not to use one solution for fscrypt and a totally different solution for fsverity, as that would increase the maintenance cost well beyond that of either solution individually.
I agree that reducing the number of infrastructures is a goal. But I don't think we should limit us to a single "solution" for different kinds of problems.
Quoted 3 lines
Second, the fsverity info can be loaded very frequently. For example, curently it's loaded for each 4K data block processed.
Well, we can easily keep a once looked up data structure around for any operation that does not leave file system control. So for writing that's a single ioctl context. For read that is a single call into ->readahead, or maybe even ->read_iter.
Quoted 3 lines
Also, there
*are* use cases in which most files on the filesystem have fsverity enabled. Not super common, but they exist.
Sure. But the typical use case is a few files, and even that is just a tiny minority of all ext4/f2fs/xfs file systems.
Quoted 3 lines
It doesn't really seem like the kind of solution that's a good choice for a frequently-loaded field. And that's only the load; it's not getting into the insertion (and resizing) part.
Assuming you actually get it down to once per high-level operation above, it will still be absolute noise compared to the I/O generated.
Quoted 7 lines
If we're going so far as to use a rhashtable, I have to wonder why we aren't first prioritizing other fields. For example
ext4_inode_infounconditionally has 40 bytes forfast_commitinformation, even thoughfast_commitis an experimental ext4 feature that isn't enabled on most filesystems. That's 5 times as much asi_verity_info. And quota has 24 bytes underCONFIG_QUOTA. And there are even holes in theext4_inode_infostruct; we could also just improve the field packing!
All that does sound like a good idea, independent of what we are discussing here.
Christian Brauner
maintainerQuoted 4 lines
The fs-specific field solution from this patchset is much more efficient than the rhashtable: efficient enough that we don't really have to worry about it, regardless of fscrypt or fsverity. So I think it's a good middle ground, and I'd like to just do it this way.
I agree.