linux-ext4 - Re: [PATCH v2] ext4: Remove unnecessary assignments

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] [day] [month] [year] [list]

Date:   Wed, 5 Jan 2022 18:24:36 -0500
From:   "Theodore Ts'o" <tytso@....edu>
To:     cgel.zte@...il.com
Cc:     Andreas Dilger <adilger.kernel@...ger.ca>,
        linux-ext4@...r.kernel.org, linux-kernel@...r.kernel.org,
        luo penghao <luo.penghao@....com.cn>,
        Zeal Robot <zealci@....com.cn>,
        Harshad Shirwadkar <harshadshirwadkar@...il.com>
Subject: Re: [PATCH v2] ext4: Remove unnecessary assignments

On Thu, Nov 04, 2021 at 06:36:37AM +0000, cgel.zte@...il.com wrote:
> From: luo penghao <luo.penghao@....com.cn>
> 
> The assignment at the end of the function is not necessary
> 
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 089c958..f1258a7 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -3886,7 +3886,7 @@ void ext4_mb_mark_bb(struct super_block *sb, ext4_fsblk_t block,
>  	if (err)
>  		goto out_err;
>  	sync_dirty_buffer(bitmap_bh);
> -	err = ext4_handle_dirty_metadata(NULL, NULL, gdp_bh);
> +	ext4_handle_dirty_metadata(NULL, NULL, gdp_bh);
>  	sync_dirty_buffer(gdp_bh);
>  
>  out_err:

There's actually a bigger issue here than the Clang analyzer
complaining about the unnecessasary assignment --- and that is we
*should* be propagating the error up to ext4_mb_mark_bb's callers, and
those callers should be logging errors and potentially aborting the
fast_commit replay.

There might be some errors that can be ignored, if an idempotent
operation doesn't need to be redone.  However, in cases like
ext4_handle_dirty_metadata(), or ext4_read_block_bitmap(), any
failures are probably due to something fatal happening --- either an
ENOMEM, or an I/O error, etc., and simply silently aborting the
current function without logging any kind of problem is going to make
it much harder to root cause a potential fast commit replay failure.

So what we should probably do is make ext4_mb_mark_bb return an error,
and then we'll need to look at all of the callers of ext4_mb_mark_bb,
and fix them up as necessary.

					- Ted