linux-ext4 - Re: [PATCH v2] ext4: fix warning when submitting superblock in ext4_commit

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 20 May 2022 11:31:05 +0200
From:   Jan Kara <jack@...e.cz>
To:     Zhang Yi <yi.zhang@...wei.com>
Cc:     linux-ext4@...r.kernel.org, tytso@....edu,
        adilger.kernel@...ger.ca, jack@...e.cz, ritesh.list@...il.com,
        yukuai3@...wei.com
Subject: Re: [PATCH v2] ext4: fix warning when submitting superblock in
 ext4_commit_super()

On Fri 20-05-22 10:32:16, Zhang Yi wrote:
> We have already check the io_error and uptodate flag before submitting
> the superblock buffer, and re-set the uptodate flag if it has been
> failed to write out. But it was lockless and could be raced by another
> ext4_commit_super(), and finally trigger '!uptodate' WARNING when
> marking buffer dirty. Fix it by submit buffer directly.
> 
> Reported-by: Hulk Robot <hulkci@...wei.com>
> Signed-off-by: Zhang Yi <yi.zhang@...wei.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@...e.cz>

								Honza

> ---
> v2->v1:
>  - Add clear_buffer_dirty() before submit_bh().
> 
>  fs/ext4/super.c | 22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 1466fbdbc8e3..9f6892665be4 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -6002,7 +6002,6 @@ static void ext4_update_super(struct super_block *sb)
>  static int ext4_commit_super(struct super_block *sb)
>  {
>  	struct buffer_head *sbh = EXT4_SB(sb)->s_sbh;
> -	int error = 0;
>  
>  	if (!sbh)
>  		return -EINVAL;
> @@ -6011,6 +6010,13 @@ static int ext4_commit_super(struct super_block *sb)
>  
>  	ext4_update_super(sb);
>  
> +	lock_buffer(sbh);
> +	/* Buffer got discarded which means block device got invalidated */
> +	if (!buffer_mapped(sbh)) {
> +		unlock_buffer(sbh);
> +		return -EIO;
> +	}
> +
>  	if (buffer_write_io_error(sbh) || !buffer_uptodate(sbh)) {
>  		/*
>  		 * Oh, dear.  A previous attempt to write the
> @@ -6025,17 +6031,21 @@ static int ext4_commit_super(struct super_block *sb)
>  		clear_buffer_write_io_error(sbh);
>  		set_buffer_uptodate(sbh);
>  	}
> -	BUFFER_TRACE(sbh, "marking dirty");
> -	mark_buffer_dirty(sbh);
> -	error = __sync_dirty_buffer(sbh,
> -		REQ_SYNC | (test_opt(sb, BARRIER) ? REQ_FUA : 0));
> +	get_bh(sbh);
> +	/* Clear potential dirty bit if it was journalled update */
> +	clear_buffer_dirty(sbh);
> +	sbh->b_end_io = end_buffer_write_sync;
> +	submit_bh(REQ_OP_WRITE,
> +		  REQ_SYNC | (test_opt(sb, BARRIER) ? REQ_FUA : 0), sbh);
> +	wait_on_buffer(sbh);
>  	if (buffer_write_io_error(sbh)) {
>  		ext4_msg(sb, KERN_ERR, "I/O error while writing "
>  		       "superblock");
>  		clear_buffer_write_io_error(sbh);
>  		set_buffer_uptodate(sbh);
> +		return -EIO;
>  	}
> -	return error;
> +	return 0;
>  }
>  
>  /*
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR