Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 3 Aug 2023 18:14:46 +0200
From:   Jan Kara <jack@suse.cz>
To:     Zhang Yi <yi.zhang@huaweicloud.com>
Cc:     linux-ext4@vger.kernel.org, tytso@mit.edu,
        adilger.kernel@dilger.ca, jack@suse.cz, yi.zhang@huawei.com,
        chengzhihao1@huawei.com, yukuai3@huawei.com
Subject: Re: [PATCH 11/12] ext4: cleanup ext4_get_dev_journal() and
 ext4_get_journal()
Message-ID: <20230803161446.6ac3ffhvfihmpyr6@quack3>
References: <20230704134233.110812-1-yi.zhang@huaweicloud.com>
 <20230704134233.110812-12-yi.zhang@huaweicloud.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230704134233.110812-12-yi.zhang@huaweicloud.com>
Precedence: bulk

On Tue 04-07-23 21:42:32, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@huawei.com>
> 
> Factor out a new helper form ext4_get_dev_journal() to get external
> journal bdev and check validation of this device, drop ext4_blkdev_get()
> helper, and also remove duplicate check of journal feature. It makes
> ext4_get_dev_journal() more clear than before.
> 
> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>

One comment below:

> @@ -5838,25 +5815,25 @@ static journal_t *ext4_get_journal(struct super_block *sb,
>  	return journal;
>  }
>  
> -static journal_t *ext4_get_dev_journal(struct super_block *sb,
> -				       dev_t j_dev)
> +static struct block_device *ext4_get_journal_dev(struct super_block *sb,
> +					dev_t j_dev, ext4_fsblk_t *j_start,
> +					ext4_fsblk_t *j_len)
>  {
>  	struct buffer_head *bh;
> -	journal_t *journal;
> -	ext4_fsblk_t start;
> -	ext4_fsblk_t len;
> +	struct block_device *bdev;
>  	int hblock, blocksize;
>  	ext4_fsblk_t sb_block;
>  	unsigned long offset;
>  	struct ext4_super_block *es;
> -	struct block_device *bdev;
>  
> -	if (WARN_ON_ONCE(!ext4_has_feature_journal(sb)))
> -		return NULL;
> -
> -	bdev = ext4_blkdev_get(j_dev, sb);
> -	if (bdev == NULL)
> +	bdev = blkdev_get_by_dev(j_dev, BLK_OPEN_READ | BLK_OPEN_WRITE, sb,
> +				 &ext4_holder_ops);
> +	if (IS_ERR(bdev)) {
> +		ext4_msg(sb, KERN_ERR,
> +			 "failed to open journal device unknown-block(%u,%u) %ld",
> +			 MAJOR(j_dev), MINOR(j_dev), PTR_ERR(bdev));
>  		return NULL;
> +	}
>  
>  	blocksize = sb->s_blocksize;
>  	hblock = bdev_logical_block_size(bdev);
> @@ -5869,7 +5846,8 @@ static journal_t *ext4_get_dev_journal(struct super_block *sb,
>  	sb_block = EXT4_MIN_BLOCK_SIZE / blocksize;
>  	offset = EXT4_MIN_BLOCK_SIZE % blocksize;
>  	set_blocksize(bdev, blocksize);
> -	if (!(bh = __bread(bdev, sb_block, blocksize))) {
> +	bh = __bread(bdev, sb_block, blocksize);
> +	if (!bh) {
>  		ext4_msg(sb, KERN_ERR, "couldn't read superblock of "
>  		       "external journal");
>  		goto out_bdev;
> @@ -5879,56 +5857,67 @@ static journal_t *ext4_get_dev_journal(struct super_block *sb,
>  	if ((le16_to_cpu(es->s_magic) != EXT4_SUPER_MAGIC) ||
>  	    !(le32_to_cpu(es->s_feature_incompat) &
>  	      EXT4_FEATURE_INCOMPAT_JOURNAL_DEV)) {
> -		ext4_msg(sb, KERN_ERR, "external journal has "
> -					"bad superblock");
> -		brelse(bh);
> -		goto out_bdev;
> +		ext4_msg(sb, KERN_ERR, "external journal has bad superblock");
> +		goto out_bh;
>  	}
>  
>  	if ((le32_to_cpu(es->s_feature_ro_compat) &
>  	     EXT4_FEATURE_RO_COMPAT_METADATA_CSUM) &&
>  	    es->s_checksum != ext4_superblock_csum(sb, es)) {
> -		ext4_msg(sb, KERN_ERR, "external journal has "
> -				       "corrupt superblock");
> -		brelse(bh);
> -		goto out_bdev;
> +		ext4_msg(sb, KERN_ERR, "external journal has corrupt superblock");
> +		goto out_bh;
>  	}
>  
>  	if (memcmp(EXT4_SB(sb)->s_es->s_journal_uuid, es->s_uuid, 16)) {
>  		ext4_msg(sb, KERN_ERR, "journal UUID does not match");
> -		brelse(bh);
> -		goto out_bdev;
> +		goto out_bh;
>  	}
>  
> -	len = ext4_blocks_count(es);
> -	start = sb_block + 1;
> -	brelse(bh);	/* we're done with the superblock */
> +	brelse(bh);
> +	*j_start = sb_block + 1;
> +	*j_len = ext4_blocks_count(es);

Here the ext4_blocks_count() is a use-after-free since you've released the
bh a few lines above.

Otherwise the patch looks good to me.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR