| Message ID | 20221115202001.324188-1-feldsherov@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] fs: do not update freeing inode i_io_list | expand |
On Tue 15-11-22 20:20:01, Svyatoslav Feldsherov wrote: > After commit cbfecb927f42 ("fs: record I_DIRTY_TIME even if inode > already has I_DIRTY_INODE") writeback_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 happen after deletion of inode from i_io_list > at evict. Stack trace is following. > > evict > fat_evict_inode > fat_truncate_blocks > fat_flush_inodes > writeback_inode > sync_inode_metadata(inode, sync=0) > writeback_single_inode(inode, wbc) <- wbc->sync_mode == WB_SYNC_NONE > > This will lead to use after free in flusher thread. > > Similar issue can be triggered if writeback_single_inode in the > stack trace update inode->i_io_list. Add explicit check to avoid it. > > Fixes: cbfecb927f42 ("fs: record I_DIRTY_TIME even if inode already has I_DIRTY_INODE") > Reported-by: syzbot+6ba92bd00d5093f7e371@syzkaller.appspotmail.com > Reviewed-by: Jan Kara <jack@suse.cz> > Signed-off-by: Svyatoslav Feldsherov <feldsherov@google.com> Ted, I guess you will merge this patch since you've merged the one from Lukas this patch is fixing? Honza > --- > V2 -> V3: > - fix grammar in commit message and comments > > V1 -> V2: > - address review comments > - skip inode_cgwb_move_to_attached for freeing inode > > fs/fs-writeback.c | 30 +++++++++++++++++++----------- > 1 file changed, 19 insertions(+), 11 deletions(-) > > diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c > index 443f83382b9b..9958d4020771 100644 > --- a/fs/fs-writeback.c > +++ b/fs/fs-writeback.c > @@ -1712,18 +1712,26 @@ static int writeback_single_inode(struct inode *inode, > wb = inode_to_wb_and_lock_list(inode); > spin_lock(&inode->i_lock); > /* > - * If the inode is now fully clean, then it can be safely removed from > - * its writeback list (if any). Otherwise the flusher threads are > - * responsible for the writeback lists. > + * If the inode is freeing, its i_io_list shoudn't be updated > + * as it can be finally deleted at this moment. > */ > - if (!(inode->i_state & I_DIRTY_ALL)) > - inode_cgwb_move_to_attached(inode, wb); > - else if (!(inode->i_state & I_SYNC_QUEUED)) { > - if ((inode->i_state & I_DIRTY)) > - redirty_tail_locked(inode, wb); > - else if (inode->i_state & I_DIRTY_TIME) { > - inode->dirtied_when = jiffies; > - inode_io_list_move_locked(inode, wb, &wb->b_dirty_time); > + if (!(inode->i_state & I_FREEING)) { > + /* > + * If the inode is now fully clean, then it can be safely > + * removed from its writeback list (if any). Otherwise the > + * flusher threads are responsible for the writeback lists. > + */ > + if (!(inode->i_state & I_DIRTY_ALL)) > + inode_cgwb_move_to_attached(inode, wb); > + else if (!(inode->i_state & I_SYNC_QUEUED)) { > + if ((inode->i_state & I_DIRTY)) > + redirty_tail_locked(inode, wb); > + else if (inode->i_state & I_DIRTY_TIME) { > + inode->dirtied_when = jiffies; > + inode_io_list_move_locked(inode, > + wb, > + &wb->b_dirty_time); > + } > } > } > > -- > 2.38.1.431.g37b22c650d-goog >
On Wed, Nov 16, 2022 at 12:15:39PM +0100, Jan Kara wrote: > On Tue 15-11-22 20:20:01, Svyatoslav Feldsherov wrote: > > After commit cbfecb927f42 ("fs: record I_DIRTY_TIME even if inode > > already has I_DIRTY_INODE") writeback_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 happen after deletion of inode from i_io_list > > at evict. Stack trace is following. > > > > evict > > fat_evict_inode > > fat_truncate_blocks > > fat_flush_inodes > > writeback_inode > > sync_inode_metadata(inode, sync=0) > > writeback_single_inode(inode, wbc) <- wbc->sync_mode == WB_SYNC_NONE > > > > This will lead to use after free in flusher thread. > > > > Similar issue can be triggered if writeback_single_inode in the > > stack trace update inode->i_io_list. Add explicit check to avoid it. > > > > Fixes: cbfecb927f42 ("fs: record I_DIRTY_TIME even if inode already has I_DIRTY_INODE") > > Reported-by: syzbot+6ba92bd00d5093f7e371@syzkaller.appspotmail.com > > Reviewed-by: Jan Kara <jack@suse.cz> > > Signed-off-by: Svyatoslav Feldsherov <feldsherov@google.com> > > Ted, I guess you will merge this patch since you've merged the one from > Lukas this patch is fixing? Sorry, I forgot to ack this earlier, but this was pushed to Linus and it's in 6.1-rc7. - Ted
diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c index 443f83382b9b..9958d4020771 100644 --- a/fs/fs-writeback.c +++ b/fs/fs-writeback.c @@ -1712,18 +1712,26 @@ static int writeback_single_inode(struct inode *inode, wb = inode_to_wb_and_lock_list(inode); spin_lock(&inode->i_lock); /* - * If the inode is now fully clean, then it can be safely removed from - * its writeback list (if any). Otherwise the flusher threads are - * responsible for the writeback lists. + * If the inode is freeing, its i_io_list shoudn't be updated + * as it can be finally deleted at this moment. */ - if (!(inode->i_state & I_DIRTY_ALL)) - inode_cgwb_move_to_attached(inode, wb); - else if (!(inode->i_state & I_SYNC_QUEUED)) { - if ((inode->i_state & I_DIRTY)) - redirty_tail_locked(inode, wb); - else if (inode->i_state & I_DIRTY_TIME) { - inode->dirtied_when = jiffies; - inode_io_list_move_locked(inode, wb, &wb->b_dirty_time); + if (!(inode->i_state & I_FREEING)) { + /* + * If the inode is now fully clean, then it can be safely + * removed from its writeback list (if any). Otherwise the + * flusher threads are responsible for the writeback lists. + */ + if (!(inode->i_state & I_DIRTY_ALL)) + inode_cgwb_move_to_attached(inode, wb); + else if (!(inode->i_state & I_SYNC_QUEUED)) { + if ((inode->i_state & I_DIRTY)) + redirty_tail_locked(inode, wb); + else if (inode->i_state & I_DIRTY_TIME) { + inode->dirtied_when = jiffies; + inode_io_list_move_locked(inode, + wb, + &wb->b_dirty_time); + } } }
After commit cbfecb927f42 ("fs: record I_DIRTY_TIME even if inode already has I_DIRTY_INODE") writeback_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 happen after deletion of inode from i_io_list at evict. Stack trace is following. evict fat_evict_inode fat_truncate_blocks fat_flush_inodes writeback_inode sync_inode_metadata(inode, sync=0) writeback_single_inode(inode, wbc) <- wbc->sync_mode == WB_SYNC_NONE This will lead to use after free in flusher thread. Similar issue can be triggered if writeback_single_inode in the stack trace update inode->i_io_list. Add explicit check to avoid it. Fixes: cbfecb927f42 ("fs: record I_DIRTY_TIME even if inode already has I_DIRTY_INODE") Reported-by: syzbot+6ba92bd00d5093f7e371@syzkaller.appspotmail.com Reviewed-by: Jan Kara <jack@suse.cz> Signed-off-by: Svyatoslav Feldsherov <feldsherov@google.com> --- V2 -> V3: - fix grammar in commit message and comments V1 -> V2: - address review comments - skip inode_cgwb_move_to_attached for freeing inode fs/fs-writeback.c | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-)