On Sun 13-11-22 17:24:39, Svyatoslav Feldsherov wrote: > After commit cbfecb927f42 ("fs: record I_DIRTY_TIME even if inode > already has I_DIRTY_INODE") writeiback_single_inode can push inode with > I_DIRTY_TIME set to b_dirty_time list. In case of freeing inode with > I_DIRTY_TIME set this can happened after deletion of inode io_list at > evict. Stack trace is following. > > evict > fat_evict_inode > fat_truncate_blocks > fat_flush_inodes > writeback_inode > sync_inode_metadata > writeback_single_inode > > This will lead to use after free in flusher thread. > > Fixes: cbfecb927f42 ("fs: record I_DIRTY_TIME even if inode already has I_DIRTY_INODE") > Reported-by: syzbot+6ba92bd00d5093f7e371@xxxxxxxxxxxxxxxxxxxxxxxxx > Signed-off-by: Svyatoslav Feldsherov <feldsherov@xxxxxxxxxx> Thanks for the analysis! I was scratching my head over this syzbot report for a while and it didn't occur to me somebody could be calling writeback_single_inode() from the .evict callback. Also what contributes to the problem is that FAT calls sync_inode_metadata(inode, 0) so it is not marking this final flush as data integrity sync and so we happily leave the I_DIRTY_TIME bit set. > diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c > index 443f83382b9b..31c93cbdb3fe 100644 > --- a/fs/fs-writeback.c > +++ b/fs/fs-writeback.c > @@ -1718,7 +1718,7 @@ static int writeback_single_inode(struct inode *inode, > */ > if (!(inode->i_state & I_DIRTY_ALL)) > inode_cgwb_move_to_attached(inode, wb); > - else if (!(inode->i_state & I_SYNC_QUEUED)) { > + else if (!(inode->i_state & (I_SYNC_QUEUED | I_FREEING))) { > if ((inode->i_state & I_DIRTY)) > redirty_tail_locked(inode, wb); > else if (inode->i_state & I_DIRTY_TIME) { So even calling inode_cgwb_move_to_attached() is not safe when I_FREEING is already set. So I belive the I_FREEING bit check needs to be before this whole if block. I also think we should add some assertions into i_io_list handling functions to complain if I_FREEING bit is set to catch these problems earlier which means to be also more careful in __mark_inode_dirty(). But this is for a separate cleanup. Honza -- Jan Kara <jack@xxxxxxxx> SUSE Labs, CR