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: <20220714145538.jbrbobhi5ppvuxka@quack3> Date: Thu, 14 Jul 2022 16:55:38 +0200 From: Jan Kara <jack@...e.cz> To: Ritesh Harjani <ritesh.list@...il.com> Cc: Jan Kara <jack@...e.cz>, Ted Tso <tytso@....edu>, linux-ext4@...r.kernel.org Subject: Re: [PATCH 06/10] ext2: Factor our freeing of xattr block reference On Thu 14-07-22 18:07:14, Ritesh Harjani wrote: > 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() Correct. ext2_free_blocks() always dirties the inode (unless there is metadata inconsistency found in which case we don't really care). > > - /* 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? Notice that I've changed dquot_free_block_nodirty() to dquot_free_block() because quota info update is the only reason why we need to dirty the inode so why not let quota code handle it... > > > - 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); > > } Honza -- Jan Kara <jack@...e.com> SUSE Labs, CR
Powered by blists - more mailing lists