Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <7233c25b-d20c-2c65-295e-ebfc5520fda4@bytedance.com>
Date:   Wed, 21 Jun 2023 16:19:10 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.12.0
Subject: Re: [PATCH] sched/topology: fix potential memoryleak in
 sched_init_numa()
Content-Language: en-US
To:     Miaohe Lin <linmiaohe@huawei.com>, mingo@redhat.com,
        peterz@infradead.org, juri.lelli@redhat.com,
        vincent.guittot@linaro.org
Cc:     dietmar.eggemann@arm.com, rostedt@goodmis.org, bsegall@google.com,
        mgorman@suse.de, bristot@redhat.com, vschneid@redhat.com,
        linux-kernel@vger.kernel.org
References: <20230621063817.3753617-1-linmiaohe@huawei.com>
From:   Abel Wu <wuyun.abel@bytedance.com>
In-Reply-To: <20230621063817.3753617-1-linmiaohe@huawei.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 6/21/23 2:38 PM, Miaohe Lin wrote:
> When sched_init_numa() fails to allocate enough memory for sched domains
> numa masks, it forgot to free the allocated memory leading to memoryleak.
> Add a helper to help release the resource.
> 
> Fixes: cb83b629bae0 ("sched/numa: Rewrite the CONFIG_NUMA sched domain support")
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
>   kernel/sched/topology.c | 22 ++++++++++++++++++++--
>   1 file changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
> index 290509383419..dcec4d653ae3 100644
> --- a/kernel/sched/topology.c
> +++ b/kernel/sched/topology.c
> @@ -1807,6 +1807,20 @@ static void init_numa_topology_type(int offline_node)
>   
>   #define NR_DISTANCE_VALUES (1 << DISTANCE_BITS)
>   
> +static void sched_free_numa_mask(struct cpumask ***masks, int nr_levels)
> +{
> +	int i, j;
> +
> +	for (i = 0; i < nr_levels; i++) {
> +		if (!masks[i])
> +			continue;
> +		for_each_node(j)
> +			kfree(masks[i][j]);
> +		kfree(masks[i]);
> +	}
> +	kfree(masks);
> +}
> +
>   void sched_init_numa(int offline_node)
>   {
>   	struct sched_domain_topology_level *tl;
> @@ -1886,15 +1900,19 @@ void sched_init_numa(int offline_node)
>   	 */
>   	for (i = 0; i < nr_levels; i++) {
>   		masks[i] = kzalloc(nr_node_ids * sizeof(void *), GFP_KERNEL);
> -		if (!masks[i])
> +		if (!masks[i]) {
> +			sched_free_numa_mask(masks, nr_levels);
>   			return;
> +		}
>   
>   		for_each_cpu_node_but(j, offline_node) {
>   			struct cpumask *mask = kzalloc(cpumask_size(), GFP_KERNEL);
>   			int k;
>   
> -			if (!mask)
> +			if (!mask) {
> +				sched_free_numa_mask(masks, nr_levels);
>   				return;
> +			}
>   
>   			masks[i][j] = mask;
>   

Allocation can also fail in @tl (topology level), and if that is the
case, masks[][] IMHO also needs be freed. So I think it might be better
if call sched_reset_numa() at proper place.