Re: [PATCH] ext4: Fix possible NULL pointer dereference in ext4_group_desc_free()

[Baokun Li]

On 3/17/26 7:33 AM, Zqiang wrote:
>> On 3/16/26 4:20 PM, Zqiang wrote:
>>
>>> This can happen if the kvmalloc_objs() fails and sbi->s_group_desc pointer
>>>  is NULL in the ext4_group_desc_init(), and then the ext4_group_desc_free()
>>>  is called, leading to a NULL group_desc pointer dereference.
>>>
>>>  This commit therefore adds a NULL check for sbi->s_group_desc before
>>>  accessing its internal members.
>>>
>>>  Signed-off-by: Zqiang <qiang.zhang@xxxxxxxxx>
>>>  ---
>>>  fs/ext4/super.c | 8 +++++---
>>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>>
>>>  diff --git a/fs/ext4/super.c b/fs/ext4/super.c
>>>  index 43f680c750ae..c4307dc04687 100644
>>>  --- a/fs/ext4/super.c
>>>  +++ b/fs/ext4/super.c
>>>  @@ -1256,9 +1256,11 @@ static void ext4_group_desc_free(struct ext4_sb_info *sbi)
>>>  
>>>  rcu_read_lock();
>>>  group_desc = rcu_dereference(sbi->s_group_desc);
>>>  - for (i = 0; i < sbi->s_gdb_count; i++)
>>>
>> In ext4_group_desc_init(), s_gdb_count is only assigned after kvmalloc_array
>> allocation succeeds. Therefore, when kvmalloc_array fails, the
>> brelse(group_desc[i]) in ext4_group_desc_free() will not actually be
>> executed,
>> and thus this NULL pointer dereference issue will not be triggered.
>
> Thanks for replay, got it, sorry for make noise.
>
> Just then, I find that warning may be trigger:
>
> the kvfree() is called in RCU read critical section, if
> the sbi->s_group_desc pointer comes from vmalloc(),
> the vfree() is called to release it, but the might_sleep()
> is called in the vfree(), this may be trigger warnings in
> rcu_sleep_check() when the enable CONFIG_DEBUG_ATOMIC_SLEEP.

Indeed, vfree triggers the following warning: ```
===========================================================================
EXT4-fs (vdc): unmounting filesystem
c478da00-c52c-4dd4-81c1-d4f93e12ab50. BUG: sleeping function called from
invalid context at mm/vmalloc.c:3441 in_atomic(): 0, irqs_disabled(): 0,
non_block: 0, pid: 457, name: umount preempt_count: 0, expected: 0 RCU
nest depth: 1, expected: 0 CPU: 0 UID: 0 PID: 457 Comm: umount Tainted:
G W 6.19.0-rc4-g4f5e8e6f0123-dirty #10 PREEMPT(none) Tainted: [W]=WARN
Call Trace: <TASK> dump_stack_lvl+0x55/0x70 __might_resched+0x116/0x160
vfree+0x38/0x60 ext4_put_super+0x1ac/0x490
generic_shutdown_super+0x81/0x180 kill_block_super+0x1a/0x40
ext4_kill_sb+0x22/0x40 deactivate_locked_super+0x35/0xb0
cleanup_mnt+0x101/0x170 task_work_run+0x5c/0xa0
exit_to_user_mode_loop+0xe2/0x460 do_syscall_64+0x1de/0x1f0
entry_SYSCALL_64_after_hwframe+0x76/0x7e
===========================================================================
``` And ext4_mb_init_backend, ext4_mb_release, ext4_flex_groups_free
have similar issues.Indeed, vfree triggers the following warning:

===========================================================================
EXT4-fs (vdc): unmounting filesystem c478da00-c52c-4dd4-81c1-d4f93e12ab50.
BUG: sleeping function called from invalid context at mm/vmalloc.c:3441
in_atomic(): 0, irqs_disabled(): 0, non_block: 0, pid: 457, name: umount
preempt_count: 0, expected: 0
RCU nest depth: 1, expected: 0
CPU: 0 UID: 0 PID: 457 Comm: umount Tainted: G W
               6.19.0-rc4-g4f5e8e6f0123-dirty #10 PREEMPT(none)
Tainted: [W]=WARN
Call Trace:
 <TASK>
 dump_stack_lvl+0x55/0x70
 __might_resched+0x116/0x160
 vfree+0x38/0x60
 ext4_put_super+0x1ac/0x490
 generic_shutdown_super+0x81/0x180
 kill_block_super+0x1a/0x40
 ext4_kill_sb+0x22/0x40
 deactivate_locked_super+0x35/0xb0
 cleanup_mnt+0x101/0x170
 task_work_run+0x5c/0xa0
 exit_to_user_mode_loop+0xe2/0x460
 do_syscall_64+0x1de/0x1f0
 entry_SYSCALL_64_after_hwframe+0x76/0x7e
===========================================================================

And ext4_mb_init_backend, ext4_mb_release, ext4_flex_groups_free have
similar issues.

> May be use rcu_access_pointer() to access sbi->s_group_desc
> is enough.
>
Yes, these are all initialization or teardown paths and cannot run
concurrently with
online resize, so using rcu_access_pointer() seems sufficient.

Also, should we add might_sleep() to kvfree() to prevent similar issues?


Cheers,
Baokun