kernel/sched/fair.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
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.
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@kylinos.cn>
---
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);
+ free_fair_sched_group(tg);
err:
return 0;
}
--
2.25.1On 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? > 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@kylinos.cn> > --- > 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(). > err: > return 0; > } -- Thanks and Regards, Prateek
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@kylinos.cn> >> --- >> 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
© 2016 - 2025 Red Hat, Inc.