Hi--
Please see a small nit below.
On 11/14/22 13:21, 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(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->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@xxxxxxxxxxxxxxxxxxxxxxxxx
> Signed-off-by: Svyatoslav Feldsherov <feldsherov@xxxxxxxxxx>
> ---
> 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..c4aea096689c 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, it's io_list shoudn't be updated
its
> + * 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);
> + }
> }
> }
>
--
~Randy
