lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <20220714123714.xxqo7nnde6xacriu@riteshh-domain> Date: Thu, 14 Jul 2022 18:07:14 +0530 From: Ritesh Harjani <ritesh.list@...il.com> To: Jan Kara <jack@...e.cz> Cc: Ted Tso <tytso@....edu>, linux-ext4@...r.kernel.org Subject: Re: [PATCH 06/10] ext2: Factor our freeing of xattr block reference On 22/07/12 12:54PM, Jan Kara wrote: > Free of xattr block reference is opencode in two places. Factor it out > into a separate function and use it. Looked into the refactoring logic. The patch looks good to me. Small queries below - > > Signed-off-by: Jan Kara <jack@...e.cz> > --- > fs/ext2/xattr.c | 90 +++++++++++++++++++++---------------------------- > 1 file changed, 38 insertions(+), 52 deletions(-) > > diff --git a/fs/ext2/xattr.c b/fs/ext2/xattr.c > index 841fa6d9d744..9885294993ef 100644 > --- a/fs/ext2/xattr.c > +++ b/fs/ext2/xattr.c > @@ -651,6 +651,42 @@ ext2_xattr_set(struct inode *inode, int name_index, const char *name, > return error; > } > > +static void ext2_xattr_release_block(struct inode *inode, > + struct buffer_head *bh) > +{ > + struct mb_cache *ea_block_cache = EA_BLOCK_CACHE(inode); > + > + lock_buffer(bh); > + if (HDR(bh)->h_refcount == cpu_to_le32(1)) { > + __u32 hash = le32_to_cpu(HDR(bh)->h_hash); > + > + /* > + * This must happen under buffer lock for > + * ext2_xattr_set2() to reliably detect freed block > + */ > + mb_cache_entry_delete(ea_block_cache, hash, > + bh->b_blocknr); > + /* Free the old block. */ > + ea_bdebug(bh, "freeing"); > + ext2_free_blocks(inode, bh->b_blocknr, 1); > + /* We let our caller release bh, so we > + * need to duplicate the buffer before. */ > + get_bh(bh); > + bforget(bh); > + unlock_buffer(bh); > + } else { > + /* Decrement the refcount only. */ > + le32_add_cpu(&HDR(bh)->h_refcount, -1); > + dquot_free_block(inode, 1); > + mark_buffer_dirty(bh); > + unlock_buffer(bh); > + ea_bdebug(bh, "refcount now=%d", > + le32_to_cpu(HDR(bh)->h_refcount)); > + if (IS_SYNC(inode)) > + sync_dirty_buffer(bh); > + } > +} > + > /* > * Second half of ext2_xattr_set(): Update the file system. > */ > @@ -747,34 +783,7 @@ ext2_xattr_set2(struct inode *inode, struct buffer_head *old_bh, > * If there was an old block and we are no longer using it, > * release the old block. > */ > - lock_buffer(old_bh); > - if (HDR(old_bh)->h_refcount == cpu_to_le32(1)) { > - __u32 hash = le32_to_cpu(HDR(old_bh)->h_hash); > - > - /* > - * This must happen under buffer lock for > - * ext2_xattr_set2() to reliably detect freed block > - */ > - mb_cache_entry_delete(ea_block_cache, hash, > - old_bh->b_blocknr); > - /* Free the old block. */ > - ea_bdebug(old_bh, "freeing"); > - ext2_free_blocks(inode, old_bh->b_blocknr, 1); > - mark_inode_dirty(inode); ^^^ this is not needed because ext2_free_blocks() will take care of it. Hence you have dropped this in ext2_xattr_release_block() > - /* We let our caller release old_bh, so we > - * need to duplicate the buffer before. */ > - get_bh(old_bh); > - bforget(old_bh); > - } else { > - /* Decrement the refcount only. */ > - le32_add_cpu(&HDR(old_bh)->h_refcount, -1); > - dquot_free_block_nodirty(inode, 1); > - mark_inode_dirty(inode); Quick qn -> Don't we need mark_inode_dirty() here? > - mark_buffer_dirty(old_bh); > - ea_bdebug(old_bh, "refcount now=%d", > - le32_to_cpu(HDR(old_bh)->h_refcount)); > - } > - unlock_buffer(old_bh); > + ext2_xattr_release_block(inode, old_bh); > } > > cleanup: > @@ -828,30 +837,7 @@ ext2_xattr_delete_inode(struct inode *inode) > EXT2_I(inode)->i_file_acl); > goto cleanup; > } > - lock_buffer(bh); > - if (HDR(bh)->h_refcount == cpu_to_le32(1)) { > - __u32 hash = le32_to_cpu(HDR(bh)->h_hash); > - > - /* > - * This must happen under buffer lock for ext2_xattr_set2() to > - * reliably detect freed block > - */ > - mb_cache_entry_delete(EA_BLOCK_CACHE(inode), hash, > - bh->b_blocknr); > - ext2_free_blocks(inode, EXT2_I(inode)->i_file_acl, 1); > - get_bh(bh); > - bforget(bh); > - unlock_buffer(bh); > - } else { > - le32_add_cpu(&HDR(bh)->h_refcount, -1); > - ea_bdebug(bh, "refcount now=%d", > - le32_to_cpu(HDR(bh)->h_refcount)); > - unlock_buffer(bh); > - mark_buffer_dirty(bh); > - if (IS_SYNC(inode)) > - sync_dirty_buffer(bh); > - dquot_free_block_nodirty(inode, 1); > - } > + ext2_xattr_release_block(inode, bh); > EXT2_I(inode)->i_file_acl = 0; > > cleanup: > -- > 2.35.3 >
Powered by blists - more mailing lists