Re: [PATCH 2/7] ext4: avoid potential buffer_head leak in __ext4_new_inode

[Kemeng Shi <shikemeng@huaweicloud.com>]

on 8/14/2024 4:48 PM, Zhang Yi wrote:
> On 2024/8/13 20:07, Kemeng Shi wrote:
>> If a group is marked EXT4_GROUP_INFO_IBITMAP_CORRUPT after it's inode
>> bitmap buffer_head was successfully verified, then __ext4_new_inode
>> will get a valid inode_bitmap_bh of a corrupted group from
>> ext4_read_inode_bitmap in which case inode_bitmap_bh misses a release.
>> Hnadle "IS_ERR(inode_bitmap_bh)" and group corruption separately like
>> how ext4_free_inode does to avoid buffer_head leak.
>>
>> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
>> ---
>>  fs/ext4/ialloc.c | 10 +++++++---
>>  1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
>> index ad7f13976dc6..f24a238b6b09 100644
>> --- a/fs/ext4/ialloc.c
>> +++ b/fs/ext4/ialloc.c
>> @@ -1054,9 +1054,13 @@ struct inode *__ext4_new_inode(struct mnt_idmap *idmap,
>>  		brelse(inode_bitmap_bh);
>>  		inode_bitmap_bh = ext4_read_inode_bitmap(sb, group);
>>  		/* Skip groups with suspicious inode tables */
>> -		if (((!(sbi->s_mount_state & EXT4_FC_REPLAY))
>> -		     && EXT4_MB_GRP_IBITMAP_CORRUPT(grp)) ||
>> -		    IS_ERR(inode_bitmap_bh)) {
>> +		if (IS_ERR(inode_bitmap_bh)) {
>> +			inode_bitmap_bh = NULL;
>> +			goto next_group;
>> +		}
>> +		if (!(sbi->s_mount_state & EXT4_FC_REPLAY) &&
>> +		    EXT4_MB_GRP_IBITMAP_CORRUPT(grp)) {
>> +			brelse(inode_bitmap_bh);
>>  			inode_bitmap_bh = NULL;
> 
> Wouldn't the inode_bitmap_bh be brelsed in the next loop or the end of this
> function? why not just goto next_group?
Sure, goto next_group directly will be better. Will do it in next version.

Thanks,
Kemeng
> 
> Thanks,
> Yi.
> 
>>  			goto next_group;
>>  		}
>>
>