linux-ext4 - Re: [patch 1/2 v2] ext4: simplify some code in read_mmp

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 linux-cve-announce 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, 14 Aug 2015 11:03:46 -0600
From:	Andreas Dilger <adilger@...ger.ca>
To:	Dan Carpenter <dan.carpenter@...cle.com>
Cc:	Theodore Ts'o <tytso@....edu>,
	Ext4 Developers List <linux-ext4@...r.kernel.org>,
	kernel-janitors@...r.kernel.org
Subject: Re: [patch 1/2 v2] ext4: simplify some code in read_mmp_block()


> On Aug 14, 2015, at 3:47 AM, Dan Carpenter <dan.carpenter@...cle.com> wrote:
> 
> My static check complains because we have:
> 
> 	if (!*bh)
> 		return -ENOMEM;
> 	if (*bh) {
> 
> The second check is unnecessary.
> 
> I've simplified this code by moving the "if (!*bh)" checks around.  Also
> Andreas Dilger says we should probably print a warning if sb_getblk()
> fails.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@...cle.com>

Reviewed-by: Andreas Dilger <adilger@...ger.ca>

> ---
> v2: move the code around even more, add a warning message
> 
> diff --git a/fs/ext4/mmp.c b/fs/ext4/mmp.c
> index 8313ca3..0880ec9 100644
> --- a/fs/ext4/mmp.c
> +++ b/fs/ext4/mmp.c
> @@ -69,6 +69,7 @@ static int read_mmp_block(struct super_block *sb, struct buffer_head **bh,
> 			  ext4_fsblk_t mmp_block)
> {
> 	struct mmp_struct *mmp;
> +	int ret;
> 
> 	if (*bh)
> 		clear_buffer_uptodate(*bh);
> @@ -76,25 +77,24 @@ static int read_mmp_block(struct super_block *sb, struct buffer_head **bh,
> 	/* This would be sb_bread(sb, mmp_block), except we need to be sure
> 	 * that the MD RAID device cache has been bypassed, and that the read
> 	 * is not blocked in the elevator. */
> -	if (!*bh)
> +	if (!*bh) {
> 		*bh = sb_getblk(sb, mmp_block);
> -	if (!*bh)
> -		return -ENOMEM;
> -	if (*bh) {
> -		get_bh(*bh);
> -		lock_buffer(*bh);
> -		(*bh)->b_end_io = end_buffer_read_sync;
> -		submit_bh(READ_SYNC | REQ_META | REQ_PRIO, *bh);
> -		wait_on_buffer(*bh);
> -		if (!buffer_uptodate(*bh)) {
> -			brelse(*bh);
> -			*bh = NULL;
> +		if (!*bh) {
> +			ret = -ENOMEM;
> +			goto warn_exit;
> 		}
> 	}
> -	if (unlikely(!*bh)) {
> -		ext4_warning(sb, "Error while reading MMP block %llu",
> -			     mmp_block);
> -		return -EIO;
> +
> +	get_bh(*bh);
> +	lock_buffer(*bh);
> +	(*bh)->b_end_io = end_buffer_read_sync;
> +	submit_bh(READ_SYNC | REQ_META | REQ_PRIO, *bh);
> +	wait_on_buffer(*bh);
> +	if (!buffer_uptodate(*bh)) {
> +		brelse(*bh);
> +		*bh = NULL;
> +		ret = -EIO;
> +		goto warn_exit;
> 	}
> 
> 	mmp = (struct mmp_struct *)((*bh)->b_data);
> @@ -103,6 +103,10 @@ static int read_mmp_block(struct super_block *sb, struct buffer_head **bh,
> 		return -EINVAL;
> 
> 	return 0;
> +
> +warn_exit:
> +	ext4_warning(sb, "Error while reading MMP block %llu", mmp_block);

It wouldn't be terrible to include the error in this message, but
since it will also be returned to userspace it isn't critical.

> +	return ret;
> }
> 
> /*


Cheers, Andreas





--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html