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

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

> 
> 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();
> >  }
> >
>