On Wed 29-06-22 10:29:39, Zhang Yi wrote:
> When evicting an inode with default dioread_nolock, it could be raced by
> the unwritten extents converting kworker after writeback some new
> allocated dirty blocks. It convert unwritten extents to written, the
> extents could be merged to upper level and free extent blocks, so it
> could mark the inode dirty again even this inode has been marked
> I_FREEING. But the inode->i_io_list check and warning in
> ext4_evict_inode() missing this corner case. Fortunately,
> ext4_evict_inode() will wait all extents converting finished before this
> check, so it will not lead to inode use-after-free problem, every thing
> is OK besides this warning. The WARN_ON_ONCE was originally designed
> for finding inode use-after-free issues in advance, but if we add
> current dioread_nolock case in, it will become not quite useful, so fix
> this warning by just remove this check.
>
> ======
> WARNING: CPU: 7 PID: 1092 at fs/ext4/inode.c:227
> ext4_evict_inode+0x875/0xc60
> ...
> RIP: 0010:ext4_evict_inode+0x875/0xc60
> ...
> Call Trace:
> <TASK>
> evict+0x11c/0x2b0
> iput+0x236/0x3a0
> do_unlinkat+0x1b4/0x490
> __x64_sys_unlinkat+0x4c/0xb0
> do_syscall_64+0x3b/0x90
> entry_SYSCALL_64_after_hwframe+0x46/0xb0
> RIP: 0033:0x7fa933c1115b
> ======
>
> rm kworker
> ext4_end_io_end()
> vfs_unlink()
> ext4_unlink()
> ext4_convert_unwritten_io_end_vec()
> ext4_convert_unwritten_extents()
> ext4_map_blocks()
> ext4_ext_map_blocks()
> ext4_ext_try_to_merge_up()
> __mark_inode_dirty()
> check !I_FREEING
> locked_inode_to_wb_and_lock_list()
> iput()
> iput_final()
> evict()
> ext4_evict_inode()
> truncate_inode_pages_final() //wait release io_end
> inode_io_list_move_locked()
> ext4_release_io_end()
> trigger WARN_ON_ONCE()
>
> Fixes: ceff86fddae8 ("ext4: Avoid freeing inodes on dirty list")
> Signed-off-by: Zhang Yi <yi.zhang@xxxxxxxxxx>
Thanks! The patch looks good. Feel free to add:
Reviewed-by: Jan Kara <jack@xxxxxxx>
Honza
>
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 84c0eb55071d..702cc208689a 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -220,13 +220,13 @@ void ext4_evict_inode(struct inode *inode)
>
> /*
> * For inodes with journalled data, transaction commit could have
> - * dirtied the inode. Flush worker is ignoring it because of I_FREEING
> - * flag but we still need to remove the inode from the writeback lists.
> + * dirtied the inode. And for inodes with dioread_nolock, unwritten
> + * extents converting worker could merge extents and also have dirtied
> + * the inode. Flush worker is ignoring it because of I_FREEING flag but
> + * we still need to remove the inode from the writeback lists.
> */
> - if (!list_empty_careful(&inode->i_io_list)) {
> - WARN_ON_ONCE(!ext4_should_journal_data(inode));
> + if (!list_empty_careful(&inode->i_io_list))
> inode_io_list_del(inode);
> - }
>
> /*
> * Protect us against freezing - iput() caller didn't have to have any
> --
> 2.31.1
>
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
