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.
Otherwise the patch looks good to me.
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
