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@xxxxxxxxxx>
>>
>> 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@xxxxxxxxxx>
>
> 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.
