linux-kernel - Re: [PATCH] sched/fair: Remove redundant error handling path in alloc_fair_sched

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <7455ea1d-24a0-4f36-a744-3cd42b81c13a@linux.ibm.com>
Date: Wed, 10 Dec 2025 23:08:11 +0530
From: Madadi Vineeth Reddy <vineethr@...ux.ibm.com>
To: Wanwu Li <liwanwu9113@....com>
Cc: mingo@...hat.com, peterz@...radead.org, juri.lelli@...hat.com,
        vincent.guittot@...aro.org, dietmar.eggemann@....com,
        rostedt@...dmis.org, bsegall@...gle.com, mgorman@...e.de,
        vschneid@...hat.com, linux-kernel@...r.kernel.org,
        Wanwu Li <liwanwu@...inos.cn>,
        Madadi Vineeth Reddy <vineethr@...ux.ibm.com>
Subject: Re: [PATCH] sched/fair: Remove redundant error handling path in
 alloc_fair_sched_group

Hi Wanwu,

On 08/12/25 08:10, Wanwu Li wrote:
> From: Wanwu Li <liwanwu@...inos.cn>
> 
> 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@...inos.cn>
> ---
>  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