Re: [PATCH] ext4: make concurrent DEFAULT shutdown waits killable

[Jan Kara]

On Tue 10-11-26 21:21:18, diasyzhangqing@xxxxxxxxx wrote:
> From: Qing Zhang <diasyzhang@xxxxxxxxxxx>
> 
> Concurrent DEFAULT shutdown callers can all pass the shutdown flag
> check before the first caller finishes bdev_freeze().  They then wait
> uninterruptibly on bd_fsfreeze_mutex and may perform redundant
> freeze/thaw operations, triggering hung-task reports if the initial
> freeze takes long enough.
> 
> Serialize DEFAULT shutdown attempts with a per-superblock mutex
> acquired killably.  Re-check the shutdown flag after acquiring it to
> skip the freeze after a successful attempt, or retry if the previous
> attempt failed.
> 
> Leave LOGFLUSH and NOLOGFLUSH unchanged.  The ->shutdown callback uses
> NOLOGFLUSH with s_umount held, so it must not acquire a mutex held
> across a freeze.  This does not shorten the initial freeze or prevent
> hung-task reports for the active freezer and other freeze-blocked
> tasks.
> 
> Fixes: 783d94854499 ("ext4: add EXT4_IOC_GOINGDOWN ioctl")
> Reported-by: syzbot+d4ecb9a1e5b3dcf8baec@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=d4ecb9a1e5b3dcf8baec
> Signed-off-by: Qing Zhang <diasyzhang@xxxxxxxxxxx>

Err, no. This is really syzkaller self-inflicted damage we aren't going to
work around. If anything you could make the wait on bd_fsfreeze_mutex
in bdev_freeze() killable. That will actually be a sensible change and
should address syzbot warning as well.

								Honza

> ---
> 
> #syz test: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
>  fs/ext4/ext4.h  |  1 +
>  fs/ext4/ioctl.c | 14 +++++++++++++-
>  fs/ext4/super.c |  1 +
>  3 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index f38fec3167fb..c4e44e3d58cc 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -1647,6 +1647,7 @@ struct ext4_sb_info {
>  	/* Journaling */
>  	struct journal_s *s_journal;
>  	unsigned long s_ext4_flags;		/* Ext4 superblock flags */
> +	struct mutex s_shutdown_lock;	/* Serializes DEFAULT shutdown requests */
>  	struct mutex s_orphan_lock;	/* Protects on disk list changes */
>  	struct list_head s_orphan;	/* List of orphaned inodes in on disk
>  					   list */
> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
> index e8e579498c99..3019b16320a0 100644
> --- a/fs/ext4/ioctl.c
> +++ b/fs/ext4/ioctl.c
> @@ -818,16 +818,28 @@ int ext4_force_shutdown(struct super_block *sb, u32 flags)
>  	if (ext4_forced_shutdown(sb))
>  		return 0;
>  
> +	if (flags == EXT4_GOING_FLAGS_DEFAULT) {
> +		if (mutex_lock_killable(&sbi->s_shutdown_lock))
> +			return -EINTR;
> +		if (ext4_forced_shutdown(sb)) {
> +			mutex_unlock(&sbi->s_shutdown_lock);
> +			return 0;
> +		}
> +	}
> +
>  	ext4_msg(sb, KERN_ALERT, "shut down requested (%d)", flags);
>  	trace_ext4_shutdown(sb, flags);
>  
>  	switch (flags) {
>  	case EXT4_GOING_FLAGS_DEFAULT:
>  		ret = bdev_freeze(sb->s_bdev);
> -		if (ret)
> +		if (ret) {
> +			mutex_unlock(&sbi->s_shutdown_lock);
>  			return ret;
> +		}
>  		set_bit(EXT4_FLAGS_SHUTDOWN, &sbi->s_ext4_flags);
>  		bdev_thaw(sb->s_bdev);
> +		mutex_unlock(&sbi->s_shutdown_lock);
>  		break;
>  	case EXT4_GOING_FLAGS_LOGFLUSH:
>  		/*
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 9cae52471dee..a1d4fc5c0a68 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -5547,6 +5547,7 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
>  
>  	INIT_LIST_HEAD(&sbi->s_orphan); /* unlinked but open files */
>  	mutex_init(&sbi->s_orphan_lock);
> +	mutex_init(&sbi->s_shutdown_lock);
>  
>  	spin_lock_init(&sbi->s_bdev_wb_lock);
>  
> -- 
> 2.43.7
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR