Re: [PATCH] sched/fair: Remove redundant error handling path in alloc_fair_sched_group

[Madadi Vineeth Reddy]

Hi Wanwu,

On 08/12/25 08:10, Wanwu Li wrote:
> From: Wanwu Li <liwanwu@xxxxxxxxxx>
> 
> On error, the sched_free_group function already centrally handles
> resource cleanup, making the explicit kfree(cfs_rq) in
> alloc_fair_sched_group redundant.
> 
> Signed-off-by: Wanwu Li <liwanwu@xxxxxxxxxx>
> ---
>  kernel/sched/fair.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index da46c3164537..b657d3281f44 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -13645,7 +13645,7 @@ int alloc_fair_sched_group(struct task_group *tg, struct task_group *parent)
>  		se = kzalloc_node(sizeof(struct sched_entity_stats),
>  				  GFP_KERNEL, cpu_to_node(i));
>  		if (!se)
> -			goto err_free_rq;
> +			goto err;
>  
>  		init_cfs_rq(cfs_rq);
>  		init_tg_cfs_entry(tg, cfs_rq, se, i, parent->se[i]);
> @@ -13654,8 +13654,6 @@ int alloc_fair_sched_group(struct task_group *tg, struct task_group *parent)
>  
>  	return 1;
>  
> -err_free_rq:
> -	kfree(cfs_rq);
>  err:
>  	return 0;
>  }

I believe this change introduces a memory leak. 

The `err_free_rq` label serves a specific purpose - it frees the cfs_rq that was allocated in the current loop
iteration but failed before being registered with the task_group structure.

Here is the flow with your patch:

	for_each_possible_cpu(i) {
		cfs_rq = kzalloc_node(sizeof(struct cfs_rq),
				      GFP_KERNEL, cpu_to_node(i));     // Memory allocated, stored in local variable
		if (!cfs_rq)
			goto err;

		se = kzalloc_node(sizeof(struct sched_entity_stats),
				  GFP_KERNEL, cpu_to_node(i));         // Say allocation fails here
		if (!se)
			goto err;			               // Jump directly to err (with your change)

		// Below is NEVER REACHED - cfs_rq not registered to tg->cfs_rq[i]

		init_cfs_rq(cfs_rq);
		init_tg_cfs_entry(tg, cfs_rq, se, i, parent->se[i]);
		init_entity_runnable_average(se);

The locally allocated cfs_rq pointer is lost because in `sched_free_group` that calls `free_fair_sched_group` is as below

	if (tg->cfs_rq)
		kfree(tg->cfs_rq[i]);                                 // This is kfree(NULL) - the actual cfs_rq is leaked

Thanks,
Vineeth