On Wed 14-05-25 22:08:55, Jeongjun Park wrote: > Since handle->h_transaction may be a NULL pointer, so we should change it > to call is_handle_aborted(handle) first before dereferencing it. > > And the following data-race was reported in my fuzzer: > > ================================================================== > BUG: KCSAN: data-race in jbd2_journal_dirty_metadata / jbd2_journal_dirty_metadata > > write to 0xffff888011024104 of 4 bytes by task 10881 on cpu 1: > jbd2_journal_dirty_metadata+0x2a5/0x770 fs/jbd2/transaction.c:1556 > __ext4_handle_dirty_metadata+0xe7/0x4b0 fs/ext4/ext4_jbd2.c:358 > ext4_do_update_inode fs/ext4/inode.c:5220 [inline] > ext4_mark_iloc_dirty+0x32c/0xd50 fs/ext4/inode.c:5869 > __ext4_mark_inode_dirty+0xe1/0x450 fs/ext4/inode.c:6074 > ext4_dirty_inode+0x98/0xc0 fs/ext4/inode.c:6103 > .... > > read to 0xffff888011024104 of 4 bytes by task 10880 on cpu 0: > jbd2_journal_dirty_metadata+0xf2/0x770 fs/jbd2/transaction.c:1512 > __ext4_handle_dirty_metadata+0xe7/0x4b0 fs/ext4/ext4_jbd2.c:358 > ext4_do_update_inode fs/ext4/inode.c:5220 [inline] > ext4_mark_iloc_dirty+0x32c/0xd50 fs/ext4/inode.c:5869 > __ext4_mark_inode_dirty+0xe1/0x450 fs/ext4/inode.c:6074 > ext4_dirty_inode+0x98/0xc0 fs/ext4/inode.c:6103 > .... > > value changed: 0x00000000 -> 0x00000001 > ================================================================== > > This issue is caused by missing data-race annotation for jh->b_modified. > Therefore, the missing annotation needs to be added. > > Reported-by: syzbot+de24c3fe3c4091051710@xxxxxxxxxxxxxxxxxxxxxxxxx > Closes: https://syzkaller.appspot.com/bug?extid=de24c3fe3c4091051710 > Fixes: 6e06ae88edae ("jbd2: speedup jbd2_journal_dirty_metadata()") > Signed-off-by: Jeongjun Park <aha310510@xxxxxxxxx> Looks good now! Thanks! Feel free to add: Reviewed-by: Jan Kara <jack@xxxxxxx> Honza > --- > v2: Changed to annotate jh->b_modified to avoid performance overhead > - Link to v1: https://lore.kernel.org/all/20250513170441.54658-1-aha310510@xxxxxxxxx/ > --- > fs/jbd2/transaction.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c > index cbc4785462f5..c7867139af69 100644 > --- a/fs/jbd2/transaction.c > +++ b/fs/jbd2/transaction.c > @@ -1509,7 +1509,7 @@ int jbd2_journal_dirty_metadata(handle_t *handle, struct buffer_head *bh) > jh->b_next_transaction == transaction); > spin_unlock(&jh->b_state_lock); > } > - if (jh->b_modified == 1) { > + if (data_race(jh->b_modified == 1)) { > /* If it's in our transaction it must be in BJ_Metadata list. */ > if (data_race(jh->b_transaction == transaction && > jh->b_jlist != BJ_Metadata)) { > @@ -1528,7 +1528,6 @@ int jbd2_journal_dirty_metadata(handle_t *handle, struct buffer_head *bh) > goto out; > } > > - journal = transaction->t_journal; > spin_lock(&jh->b_state_lock); > > if (is_handle_aborted(handle)) { > @@ -1543,6 +1542,8 @@ int jbd2_journal_dirty_metadata(handle_t *handle, struct buffer_head *bh) > goto out_unlock_bh; > } > > + journal = transaction->t_journal; > + > if (jh->b_modified == 0) { > /* > * This buffer's got modified and becoming part > -- -- Jan Kara <jack@xxxxxxxx> SUSE Labs, CR
