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;
Subject: Re: [PATCH 11/12] ext4: cleanup ext4_get_dev_journal() and
 ext4_get_journal()
To:     Jan Kara <jack@suse.cz>
Cc:     linux-ext4@vger.kernel.org, tytso@mit.edu,
        adilger.kernel@dilger.ca, yi.zhang@huawei.com,
        chengzhihao1@huawei.com, yukuai3@huawei.com
References: <20230704134233.110812-1-yi.zhang@huaweicloud.com>
 <20230704134233.110812-12-yi.zhang@huaweicloud.com>
 <20230803161446.6ac3ffhvfihmpyr6@quack3>
From:   Zhang Yi <yi.zhang@huaweicloud.com>
Message-ID: <1dcada9b-77ba-504e-b2dd-b0437afa9962@huaweicloud.com>
Date:   Mon, 7 Aug 2023 19:36:45 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.12.0
MIME-Version: 1.0
In-Reply-To: <20230803161446.6ac3ffhvfihmpyr6@quack3>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 2023/8/4 0:14, Jan Kara wrote:
> 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.
> 

Indeed, will move it before the brelse(bh).

Thanks,
Yi.