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

["Zqiang" <qiang.zhang@linux.dev>]

> 
> 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@linux.dev>
> >  ---
> >  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


This can also actually happen, depending on your kernel configuration:
 CONFIG_PREEMPT_NONE=y 
 CONFIG_PROVE_RCU=y
 CONFIG_DEBUG_ATOMIC_SLEEP=y
 CONFIG_PREEMPT_RCU=n
 CONFIG_PREEMPT_COUNT=y
 CONFIG_DEBUG_PREEMPT=y


./include/linux/rcupdate.h:409 Illegal context switch in RCU read-side critical section!

other info that might help us debug this:

rcu_scheduler_active = 2, debug_locks = 1
2 locks held by umount/555:
 #0: ffff88800da680e8 (&type->s_umount_key#29){++++}-{4:4}, at: deactivate_super+0x76/0xa0
 #1: ffffffff86a0e580 (rcu_read_lock){....}-{1:3}, at: ext4_group_desc_free+0x27/0x270

Call Trace:
 <TASK>
 dump_stack_lvl+0xbb/0xd0
 dump_stack+0x14/0x20
 lockdep_rcu_suspicious+0x15a/0x1b0
 __might_resched+0x375/0x4d0
 ? put_object.part.0+0x2c/0x50
 __might_sleep+0x108/0x160
 vfree+0x58/0x910
 ? ext4_group_desc_free+0x27/0x270
 kvfree+0x23/0x40
 ext4_group_desc_free+0x111/0x270
 ext4_put_super+0x3c8/0xd40
 generic_shutdown_super+0x14c/0x4a0
 ? __pfx_shrinker_free+0x10/0x10
 kill_block_super+0x40/0x90
 ext4_kill_sb+0x6d/0xb0
 deactivate_locked_super+0xb4/0x180
 deactivate_super+0x7e/0xa0
 cleanup_mnt+0x296/0x3e0
 __cleanup_mnt+0x16/0x20
 task_work_run+0x157/0x250
 ? __pfx_task_work_run+0x10/0x10
 ? exit_to_user_mode_loop+0x6a/0x550
 exit_to_user_mode_loop+0x102/0x550
 do_syscall_64+0x44a/0x500
 entry_SYSCALL_64_after_hwframe+0x77/0x7f



> ===========================================================================
> ``` 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.

Will make a patch and CC you :)

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

I think it makes sense.

Thanks
Zqiang

> 
> Cheers,
> Baokun
>