Re: [PATCH v1] sched/fair: Fix memory leak in alloc_fair_sched_group()

[Zihuan Zhang]

Hi Prateek,
Thanks a lot for the careful review and clarification.

在 2025/6/24 12:31, K Prateek Nayak 写道:
> On 6/23/2025 11:49 AM, Zihuan Zhang wrote:
> > alloc_fair_sched_group() allocates per-CPU cfs_rq[] and se[] arrays
> > for a task group. However, if either allocation fails, or a per-CPU
> > allocation fails during the loop, the function may leak memory.
>
> alloc_fair_sched_group() is only called by sched_create_group()
> which does a sched_free_group() on failure that calls
> free_fair_sched_group(). I don't see the memory leak in this scenario.
> What am I missing?
You're absolutely right — I missed the fact that

`sched_create_group()` will always call `sched_free_group()` on failure,
which makes my concern about memory leaks in `alloc_fair_sched_group()` 
invalid.
> > This patch fixes the memory leak by:
> > - Using sizeof(*ptr) instead of sizeof(ptr) for correctness.
> > - Using the existing free_fair_sched_group() function to clean up
> > Note: Calling free_fair_sched_group() unconditionally in the failure
> > path is safe, as kfree(NULL) is a no-op in the kernel. This avoids
> > duplicating cleanup logic and improves robustness.
> >
> > Signed-off-by: Zihuan Zhang <zhangzihuan@xxxxxxxxxx>
> > ---
> >   kernel/sched/fair.c | 10 +++++-----
> >   1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index 7a14da5396fb..920174245517 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -13372,12 +13372,12 @@ int alloc_fair_sched_group(struct 
> > task_group *tg, struct task_group *parent)
> >       struct cfs_rq *cfs_rq;>       int i;
> >   -    tg->cfs_rq = kcalloc(nr_cpu_ids, sizeof(cfs_rq), GFP_KERNEL);
> > +    tg->cfs_rq = kcalloc(nr_cpu_ids, sizeof(*tg->cfs_rq), GFP_KERNEL);
> >       if (!tg->cfs_rq)
> >           goto err;
> > -    tg->se = kcalloc(nr_cpu_ids, sizeof(se), GFP_KERNEL);
> > +    tg->se = kcalloc(nr_cpu_ids, sizeof(*tg->se), GFP_KERNEL);
> >       if (!tg->se)
> > -        goto err;
> > +        goto err_free_rq;
> >         tg->shares = NICE_0_LOAD;
> >   @@ -13387,7 +13387,7 @@ int alloc_fair_sched_group(struct 
> > task_group *tg, struct task_group *parent)
> >           cfs_rq = kzalloc_node(sizeof(struct cfs_rq),
> >                         GFP_KERNEL, cpu_to_node(i));
> >           if (!cfs_rq)
> > -            goto err;
> > +            goto err_free_rq;
> >             se = kzalloc_node(sizeof(struct sched_entity_stats),
> >                     GFP_KERNEL, cpu_to_node(i));
> > @@ -13402,7 +13402,7 @@ int alloc_fair_sched_group(struct task_group 
> > *tg, struct task_group *parent)
> >       return 1;
> >     err_free_rq:
> > -    kfree(cfs_rq);
>
> This will actually introducing a memory leak. If allocation of "se"
> fails, the "cfs_rq" won't be linked to "tg" and needs to be freed here.
>
> > +    free_fair_sched_group(tg);
>
> free_fair_sched_group() doesn't NULL out the "tg->cfs_rq" and "tg->se"
> after freeing them which now introduces double-free via
> free_fair_sched_group() on failure in alloc_fair_sched_group().
Also, thanks for pointing out the potential issue of leaking the
locally allocated `cfs_rq` if `se` allocation fails, and the risk of
double free if `free_fair_sched_group()` is called too early.

I will drop this patch and look deeper into the call chain next time
before proposing such cleanup.

Thanks again for your time and detailed feedback!

> >   err:
> >       return 0;
> >   }
Best regards,
Zihuan Zhang