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

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

[Zqiang]

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++)
-		brelse(group_desc[i]);
-	kvfree(group_desc);
+	if (group_desc) {
+		for (i = 0; i < sbi->s_gdb_count; i++)
+			brelse(group_desc[i]);
+		kvfree(group_desc);
+	}
 	rcu_read_unlock();
 }
 
-- 
2.17.1

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

[Baokun Li]

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.


Cheers,
Baokun

> -		brelse(group_desc[i]);
> -	kvfree(group_desc);
> +	if (group_desc) {
> +		for (i = 0; i < sbi->s_gdb_count; i++)
> +			brelse(group_desc[i]);
> +		kvfree(group_desc);
> +	}
>  	rcu_read_unlock();
>  }
>  

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

[Zqiang]

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

May be use rcu_access_pointer() to access sbi->s_group_desc
is enough.

Thanks
Zqiang


> 
> Cheers,
> Baokun
> 
> > 
> > - brelse(group_desc[i]);
> >  - kvfree(group_desc);
> >  + if (group_desc) {
> >  + for (i = 0; i < sbi->s_gdb_count; i++)
> >  + brelse(group_desc[i]);
> >  + kvfree(group_desc);
> >  + }
> >  rcu_read_unlock();
> >  }
> >
>

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

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

[Zqiang]

> 
> 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
>