Re: [PATCH] ext4: skip extra isize expansion while unmounting

[Jan Kara]

On Mon 24-08-26 15:15:12, Hemanth Selam wrote:
> syzbot reports a WARN from ext4_xattr_inode_create() reached through the
> unmount path:
> 
>   EXT4-fs warning (device loop0): ext4_xattr_inode_create:1485: refuse to
>   create EA inode when umounting
>   WARNING: fs/ext4/xattr.c:1486 at ext4_xattr_inode_lookup_create
>    ext4_xattr_block_set
>    ext4_expand_extra_isize_ea
>    __ext4_expand_extra_isize
>    __ext4_mark_inode_dirty
>    ext4_dirty_inode
>    __mark_inode_dirty
>    sync_lazytime
>    iput
>    dentry_kill
>    shrink_dentry_list
>    shrink_dcache_for_umount
>    generic_shutdown_super
>    kill_block_super
>    ext4_kill_sb
> 
> shrink_dcache_for_umount() clears s_root before generic_shutdown_super()
> clears SB_ACTIVE, so during the dcache shrink the last iput() of a
> lazytime inode still redirties it and reaches the isize expansion.  The
> expansion can move xattrs out to a block, and creating the EA inode for
> them needs s_root, which ext4_xattr_inode_create() refuses without.
> 
> ext4_try_to_expand_extra_isize() already declines to expand when the
> superblock is not active, but that test does not cover this window.
> Decline while s_root is gone as well.  The expansion is best effort and
> __ext4_mark_inode_dirty() ignores its return value, so nothing else
> changes; the inode can be expanded on a later mount.
> 
> Running the syzbot reproducer for 60 seconds produced 3583 splats before
> this change and none after it, with the same number of mount cycles.
> 
> Reported-by: syzbot+4b03894b6ec5753ddf24@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=4b03894b6ec5753ddf24
> Fixes: f31173c19901 ("ext4: refuse to create ea block when umounted")
> Signed-off-by: Hemanth Selam <hemanth.selam@xxxxxxxxx>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza

> ---
>  fs/ext4/inode.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index bd4b778df9eb..6e523a5c8230 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -6598,8 +6598,14 @@ static int ext4_try_to_expand_extra_isize(struct inode *inode,
>  	 * When !SB_ACTIVE, iput triggers write_inode_now() which acquires
>  	 * s_writepages_rwsem, causing a deadlock with the caller's active
>  	 * jbd2 handle (lock order: s_writepages_rwsem -> jbd2_handle).
> +	 *
> +	 * Skip it while unmounting as well.  shrink_dcache_for_umount()
> +	 * clears s_root before generic_shutdown_super() clears SB_ACTIVE, and
> +	 * the last iput() of a lazytime inode in that window redirties it and
> +	 * lands here.  Moving xattrs out to a block then needs a new EA inode,
> +	 * which ext4_xattr_inode_create() refuses without s_root.
>  	 */
> -	if (unlikely(!(inode->i_sb->s_flags & SB_ACTIVE)))
> +	if (unlikely(!(inode->i_sb->s_flags & SB_ACTIVE) || !inode->i_sb->s_root))
>  		return -EBUSY;
>  
>  	/*
> -- 
> 2.43.7
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR