All of lore.kernel.org
 help / color / mirror / Atom feed
From: Nathan Lynch <nathanl@linux.ibm.com>
To: Zhang Xiaoxu <zhangxiaoxu5@huawei.com>
Cc: tyreld@linux.ibm.com, groug@kaod.org, paulus@samba.org,
	linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH] powerpc/pseries/hotplug-cpu: Fix memleak when cpus node not exist
Date: Tue, 10 Nov 2020 08:08:45 -0600	[thread overview]
Message-ID: <87ft5hjocy.fsf@linux.ibm.com> (raw)
In-Reply-To: <20201110123029.3767459-1-zhangxiaoxu5@huawei.com>

Zhang Xiaoxu <zhangxiaoxu5@huawei.com> writes:
> From: zhangxiaoxu <zhangxiaoxu5@huawei.com>
>
> If the cpus nodes not exist, we lost to free the 'cpu_drcs', which
> will leak memory.
>
> Fixes: a0ff72f9f5a7 ("powerpc/pseries/hotplug-cpu: Remove double free in error path")
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Signed-off-by: zhangxiaoxu <zhangxiaoxu5@huawei.com>
> ---
>  arch/powerpc/platforms/pseries/hotplug-cpu.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> index f2837e33bf5d..4bb1c9f2bb11 100644
> --- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
> +++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> @@ -743,6 +743,7 @@ static int dlpar_cpu_add_by_count(u32 cpus_to_add)
>  	parent = of_find_node_by_path("/cpus");
>  	if (!parent) {
>  		pr_warn("Could not find CPU root node in device tree\n");
> +		kfree(cpu_drcs);
>  		return -1;
>  	}

Thanks for finding this.

a0ff72f9f5a7 ("powerpc/pseries/hotplug-cpu: Remove double free in error
path") was posted in Sept 2019 but was not applied until July 2020:

https://lore.kernel.org/linuxppc-dev/20190919231633.1344-1-nathanl@linux.ibm.com/

Here is that change as posted; note the function context is
find_dlpar_cpus_to_add(), not dlpar_cpu_add_by_count():

--- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
+++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
@@ -726,7 +726,6 @@ static int find_dlpar_cpus_to_add(u32 *cpu_drcs, u32 cpus_to_add)
 	parent = of_find_node_by_path("/cpus");
 	if (!parent) {
 		pr_warn("Could not find CPU root node in device tree\n");
-		kfree(cpu_drcs);
 		return -1;
 	}

Meanwhile b015f6bc9547dbc056edde7177c7868ca8629c4c ("powerpc/pseries: Add
cpu DLPAR support for drc-info property") was posted in Nov 2019 and
committed a few days later:

https://lore.kernel.org/linux-pci/1573449697-5448-4-git-send-email-tyreld@linux.ibm.com/

This change reorganized the same code, removing
find_dlpar_cpus_to_add(), and it had the effect of fixing the same
issue.

However git apparently allowed the older change to still apply on top of
this (changing a function different from the one in the original
patch!), leading to a real bug.

Your patch is correct but it should be framed as a revert of
a0ff72f9f5a7 with this context in the commit message.

  reply	other threads:[~2020-11-10 14:11 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-10 12:30 [PATCH] powerpc/pseries/hotplug-cpu: Fix memleak when cpus node not exist Zhang Xiaoxu
2020-11-10 14:08 ` Nathan Lynch [this message]
2020-11-10 17:47   ` Tyrel Datwyler
2020-11-16 12:23     ` Michael Ellerman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87ft5hjocy.fsf@linux.ibm.com \
    --to=nathanl@linux.ibm.com \
    --cc=groug@kaod.org \
    --cc=linuxppc-dev@lists.ozlabs.org \
    --cc=paulus@samba.org \
    --cc=tyreld@linux.ibm.com \
    --cc=zhangxiaoxu5@huawei.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.