by Zhang Yi

[permalink] [raw]

Subject: Re: [PATCH 11/12] ext4: cleanup ext4_get_dev_journal() and ext4_get_journal()

On 2023/8/4 0:14, Jan Kara wrote:
> On Tue 04-07-23 21:42:32, Zhang Yi wrote:
>> From: Zhang Yi <[email protected]>
>>
>> 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 <[email protected]>
>
> 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.

2023-08-07 14:30:27

by Jan Kara

[permalink] [raw]

Subject: Re: [PATCH 07/12] jbd2: add fast_commit space check

On Mon 07-08-23 18:53:09, Zhang Yi wrote:
> On 2023/8/3 22:38, Jan Kara wrote:
> > On Tue 04-07-23 21:42:28, Zhang Yi wrote:
> >> From: Zhang Yi <[email protected]>
> >>
> >> If JBD2_FEATURE_INCOMPAT_FAST_COMMIT bit is set, it means the journal
> >> have fast commit records need to recover, so the fast commit size
> >> should not be zero, and also the leftover normal journal size should
> >> never less than JBD2_MIN_JOURNAL_BLOCKS. Add a check into the
> >> journal_check_superblock() and drop the pointless branch when
> >> initializing in-memory fastcommit parameters.
> >>
> >> Signed-off-by: Zhang Yi <[email protected]>
> >
> > Some comments below.
> >
> >
> >> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> >> index efdb8db3c06e..210b532a3673 100644
> >> --- a/fs/jbd2/journal.c
> >> +++ b/fs/jbd2/journal.c
> >> @@ -1392,6 +1392,18 @@ static int journal_check_superblock(journal_t *journal)
> >> return err;
> >> }
> >>
> >> + if (jbd2_has_feature_fast_commit(journal)) {
> >> + int num_fc_blks = be32_to_cpu(sb->s_num_fc_blks);
> >> +
> >> + if (!num_fc_blks ||
> >> + (be32_to_cpu(sb->s_maxlen) - num_fc_blks <
> >> + JBD2_MIN_JOURNAL_BLOCKS)) {
> >> + printk(KERN_ERR "JBD2: Invalid fast commit size %d\n",
> >> + num_fc_blks);
> >> + return err;
> >> + }
> >
> > This is wrong sb->s_num_fc_blks == 0 means that the fast-commit area should
> > have the default size of 256 blocks. At least that's how it behaves
> > currently and we should not change the behavior.
>
> Thanks for the review and correcting me. I missed the fc_debug_force
> mount option, this option enable fast commit feature without init
> sb->s_num_fc_blks to disk, so it could left over an unclean image with
> fast_commit feature but sb->s_num_fc_blks is still zero. And the mke2fs
> could also set sb->s_num_fc_blks to 0.

Yes.

> > Similarly if the number of fastcommit blocks was too big (i.e. there was
> > not enough space left for ordinary journal), we effectively silently
> > disable fastcommit and you break this behavior in this patch.
> >
>
> If the fastcommit is too big, jbd2_journal_initialize_fast_commit()
> will detect this corruption and refuse to mount.
>
> [ 1213.810719] JBD2: Cannot enable fast commits.
> [ 1213.812282] EXT4-fs (pmem1): Failed to set fast commit journal feature
>
> It only silently disable fastcommit while recovering the journal, but it
> doesn't seem to make much sense, because the journal->j_last is likely to
> be wrong (not point to the correct end of normal journal range) and will
> probably lead to incorrect recovery. It seems better to report the error
> and exit as early as possible. So I suppose we could keep this "too big"
> check in journal_check_superblock(). How does that sound ?

Ah, you are right. So let's keep the "space for journal too small" check as
you suggest.

Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR