[Qemu-devel] [PATCH] spapr: fix potential memory leak in spapr_core_plug()

[Qemu-devel] [PATCH] spapr: fix potential memory leak in spapr_core_plug()

[Greg Kurz]

Since commit 5c1da81215c7 ("spapr: Remove unnecessary differences between
hotplug and coldplug paths"), the CPU DT for the DRC is always allocated.
This causes a memory leak for pseries-2.6 and older machine types, that
don't support CPU hotplug and don't allocate DRCs for CPUs.

Reported-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 12b3f099d4c9..4a480e1c1dd9 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2993,8 +2993,6 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
     CPUState *cs = CPU(core->threads);
     sPAPRDRConnector *drc;
     Error *local_err = NULL;
-    void *fdt = NULL;
-    int fdt_offset = 0;
     int smt = kvmppc_smt_threads();
     CPUArchId *core_slot;
     int index;
@@ -3009,9 +3007,12 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
 
     g_assert(drc || !mc->has_hotpluggable_cpus);
 
-    fdt = spapr_populate_hotplug_cpu_dt(cs, &fdt_offset, spapr);
-
     if (drc) {
+        void *fdt;
+        int fdt_offset;
+
+        fdt = spapr_populate_hotplug_cpu_dt(cs, &fdt_offset, spapr);
+
         spapr_drc_attach(drc, dev, fdt, fdt_offset, &local_err);
         if (local_err) {
             g_free(fdt);

Re: [Qemu-devel] [PATCH] spapr: fix potential memory leak in spapr_core_plug()

[Bharata B Rao]

On Wed, Jul 12, 2017 at 11:48:39AM +0200, Greg Kurz wrote:
> Since commit 5c1da81215c7 ("spapr: Remove unnecessary differences between
> hotplug and coldplug paths"), the CPU DT for the DRC is always allocated.
> This causes a memory leak for pseries-2.6 and older machine types, that
> don't support CPU hotplug and don't allocate DRCs for CPUs.
> 
> Reported-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/ppc/spapr.c |    9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 12b3f099d4c9..4a480e1c1dd9 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2993,8 +2993,6 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>      CPUState *cs = CPU(core->threads);
>      sPAPRDRConnector *drc;
>      Error *local_err = NULL;
> -    void *fdt = NULL;
> -    int fdt_offset = 0;
>      int smt = kvmppc_smt_threads();
>      CPUArchId *core_slot;
>      int index;
> @@ -3009,9 +3007,12 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> 
>      g_assert(drc || !mc->has_hotpluggable_cpus);
> 
> -    fdt = spapr_populate_hotplug_cpu_dt(cs, &fdt_offset, spapr);
> -
>      if (drc) {
> +        void *fdt;
> +        int fdt_offset;
> +
> +        fdt = spapr_populate_hotplug_cpu_dt(cs, &fdt_offset, spapr);
> +
>          spapr_drc_attach(drc, dev, fdt, fdt_offset, &local_err);
>          if (local_err) {
>              g_free(fdt);

You say this in the patch description already, but want to note explicitly
that this prevents double allocation for pseries-2.6 and ealier types and
not for newer machine types.

Regards,
Bharata.

Re: [Qemu-devel] [PATCH] spapr: fix potential memory leak in spapr_core_plug()

[Greg Kurz]

[-- Attachment #1: Type: text/plain, Size: 3261 bytes --]

On Wed, 12 Jul 2017 15:52:37 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> On Wed, Jul 12, 2017 at 11:48:39AM +0200, Greg Kurz wrote:
> > Since commit 5c1da81215c7 ("spapr: Remove unnecessary differences between
> > hotplug and coldplug paths"), the CPU DT for the DRC is always allocated.
> > This causes a memory leak for pseries-2.6 and older machine types, that
> > don't support CPU hotplug and don't allocate DRCs for CPUs.
> > 
> > Reported-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  hw/ppc/spapr.c |    9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 12b3f099d4c9..4a480e1c1dd9 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -2993,8 +2993,6 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >      CPUState *cs = CPU(core->threads);
> >      sPAPRDRConnector *drc;
> >      Error *local_err = NULL;
> > -    void *fdt = NULL;
> > -    int fdt_offset = 0;
> >      int smt = kvmppc_smt_threads();
> >      CPUArchId *core_slot;
> >      int index;
> > @@ -3009,9 +3007,12 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > 
> >      g_assert(drc || !mc->has_hotpluggable_cpus);
> > 
> > -    fdt = spapr_populate_hotplug_cpu_dt(cs, &fdt_offset, spapr);
> > -
> >      if (drc) {
> > +        void *fdt;
> > +        int fdt_offset;
> > +
> > +        fdt = spapr_populate_hotplug_cpu_dt(cs, &fdt_offset, spapr);
> > +
> >          spapr_drc_attach(drc, dev, fdt, fdt_offset, &local_err);
> >          if (local_err) {
> >              g_free(fdt);  
> 
> You say this in the patch description already, but want to note explicitly
> that this prevents double allocation for pseries-2.6 and ealier types and
> not for newer machine types.
> 

This DT node doesn't have the exact same use as the one allocated in
spapr_populate_cpus_dt_node(). Here, it is needed by the DRC logic:

$ git grep 'drc->fdt' hw/ppc/spapr_drc.c
hw/ppc/spapr_drc.c:    if (!drc->fdt) {
hw/ppc/spapr_drc.c:    fdt = drc->fdt;
hw/ppc/spapr_drc.c:    fdt_offset = drc->fdt_start_offset;
hw/ppc/spapr_drc.c:    drc->fdt = fdt;
hw/ppc/spapr_drc.c:    drc->fdt_start_offset = fdt_start_offset;
hw/ppc/spapr_drc.c:    g_free(drc->fdt);
hw/ppc/spapr_drc.c:    drc->fdt = NULL;
hw/ppc/spapr_drc.c:    drc->fdt_start_offset = 0;
hw/ppc/spapr_drc.c:    if (!drc->fdt) {
hw/ppc/spapr_drc.c:        ccs->fdt_offset = drc->fdt_start_offset;
hw/ppc/spapr_drc.c:        tag = fdt_next_tag(drc->fdt, ccs->fdt_offset, &fdt_offset_next);
hw/ppc/spapr_drc.c:            name = fdt_get_name(drc->fdt, ccs->fdt_offset, NULL);
hw/ppc/spapr_drc.c:            prop = fdt_get_property_by_offset(drc->fdt, ccs->fdt_offset,
hw/ppc/spapr_drc.c:            name = fdt_string(drc->fdt, fdt32_to_cpu(prop->nameoff));

and

void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
                      int fdt_start_offset, Error **errp)
{
    ...
    g_assert(fdt);

So I'm not sure it is worth to mention the double allocation.

Cheers,

--
Greg

> Regards,
> Bharata.
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [Qemu-devel] [PATCH] spapr: fix potential memory leak in spapr_core_plug()

[Bharata B Rao]

On Wed, Jul 12, 2017 at 01:11:30PM +0200, Greg Kurz wrote:
> On Wed, 12 Jul 2017 15:52:37 +0530
> Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> 
> > On Wed, Jul 12, 2017 at 11:48:39AM +0200, Greg Kurz wrote:
> > > Since commit 5c1da81215c7 ("spapr: Remove unnecessary differences between
> > > hotplug and coldplug paths"), the CPU DT for the DRC is always allocated.
> > > This causes a memory leak for pseries-2.6 and older machine types, that
> > > don't support CPU hotplug and don't allocate DRCs for CPUs.
> > > 
> > > Reported-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > ---
> > >  hw/ppc/spapr.c |    9 +++++----
> > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index 12b3f099d4c9..4a480e1c1dd9 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -2993,8 +2993,6 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > >      CPUState *cs = CPU(core->threads);
> > >      sPAPRDRConnector *drc;
> > >      Error *local_err = NULL;
> > > -    void *fdt = NULL;
> > > -    int fdt_offset = 0;
> > >      int smt = kvmppc_smt_threads();
> > >      CPUArchId *core_slot;
> > >      int index;
> > > @@ -3009,9 +3007,12 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > > 
> > >      g_assert(drc || !mc->has_hotpluggable_cpus);
> > > 
> > > -    fdt = spapr_populate_hotplug_cpu_dt(cs, &fdt_offset, spapr);
> > > -
> > >      if (drc) {
> > > +        void *fdt;
> > > +        int fdt_offset;
> > > +
> > > +        fdt = spapr_populate_hotplug_cpu_dt(cs, &fdt_offset, spapr);
> > > +
> > >          spapr_drc_attach(drc, dev, fdt, fdt_offset, &local_err);
> > >          if (local_err) {
> > >              g_free(fdt);  
> > 
> > You say this in the patch description already, but want to note explicitly
> > that this prevents double allocation for pseries-2.6 and ealier types and
> > not for newer machine types.
> > 
> 
> This DT node doesn't have the exact same use as the one allocated in
> spapr_populate_cpus_dt_node(). Here, it is needed by the DRC logic:

What I mean to say is that even after this fix, for machines types > 2.6, we
still allocate memory for FDT and go through the code that creates DT entries
for CPUs twice. What we create via spapr_populate_cpus_dt_node() is used
while the CPU DT entries created by spapr_populate_hotplug_cpu_dt() won't
be used and corresponding drc->fdt is never freed, afaics for boot time
and cold-plugged CPUs.

May be this can be fixed during DT code reorg effort that David is
planning.

Regards,
Bharata.

Re: [Qemu-devel] [PATCH] spapr: fix potential memory leak in spapr_core_plug()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3285 bytes --]

On Thu, Jul 13, 2017 at 08:54:58AM +0530, Bharata B Rao wrote:
> On Wed, Jul 12, 2017 at 01:11:30PM +0200, Greg Kurz wrote:
> > On Wed, 12 Jul 2017 15:52:37 +0530
> > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > 
> > > On Wed, Jul 12, 2017 at 11:48:39AM +0200, Greg Kurz wrote:
> > > > Since commit 5c1da81215c7 ("spapr: Remove unnecessary differences between
> > > > hotplug and coldplug paths"), the CPU DT for the DRC is always allocated.
> > > > This causes a memory leak for pseries-2.6 and older machine types, that
> > > > don't support CPU hotplug and don't allocate DRCs for CPUs.
> > > > 
> > > > Reported-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > > ---
> > > >  hw/ppc/spapr.c |    9 +++++----
> > > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > index 12b3f099d4c9..4a480e1c1dd9 100644
> > > > --- a/hw/ppc/spapr.c
> > > > +++ b/hw/ppc/spapr.c
> > > > @@ -2993,8 +2993,6 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > > >      CPUState *cs = CPU(core->threads);
> > > >      sPAPRDRConnector *drc;
> > > >      Error *local_err = NULL;
> > > > -    void *fdt = NULL;
> > > > -    int fdt_offset = 0;
> > > >      int smt = kvmppc_smt_threads();
> > > >      CPUArchId *core_slot;
> > > >      int index;
> > > > @@ -3009,9 +3007,12 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > > > 
> > > >      g_assert(drc || !mc->has_hotpluggable_cpus);
> > > > 
> > > > -    fdt = spapr_populate_hotplug_cpu_dt(cs, &fdt_offset, spapr);
> > > > -
> > > >      if (drc) {
> > > > +        void *fdt;
> > > > +        int fdt_offset;
> > > > +
> > > > +        fdt = spapr_populate_hotplug_cpu_dt(cs, &fdt_offset, spapr);
> > > > +
> > > >          spapr_drc_attach(drc, dev, fdt, fdt_offset, &local_err);
> > > >          if (local_err) {
> > > >              g_free(fdt);  
> > > 
> > > You say this in the patch description already, but want to note explicitly
> > > that this prevents double allocation for pseries-2.6 and ealier types and
> > > not for newer machine types.
> > > 
> > 
> > This DT node doesn't have the exact same use as the one allocated in
> > spapr_populate_cpus_dt_node(). Here, it is needed by the DRC logic:
> 
> What I mean to say is that even after this fix, for machines types > 2.6, we
> still allocate memory for FDT and go through the code that creates DT entries
> for CPUs twice. What we create via spapr_populate_cpus_dt_node() is used
> while the CPU DT entries created by spapr_populate_hotplug_cpu_dt() won't
> be used and corresponding drc->fdt is never freed, afaics for boot time
> and cold-plugged CPUs.

Sure, I just don't care that much.  It's not a leak in the sense that
it can continue to lose memory - it just means we allocate (and track)
a bit more than we need to.

> May be this can be fixed during DT code reorg effort that David is
> planning.

Maybe.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH] spapr: fix potential memory leak in spapr_core_plug()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1788 bytes --]

On Wed, Jul 12, 2017 at 11:48:39AM +0200, Greg Kurz wrote:
> Since commit 5c1da81215c7 ("spapr: Remove unnecessary differences between
> hotplug and coldplug paths"), the CPU DT for the DRC is always allocated.
> This causes a memory leak for pseries-2.6 and older machine types, that
> don't support CPU hotplug and don't allocate DRCs for CPUs.
> 
> Reported-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> Signed-off-by: Greg Kurz <groug@kaod.org>

Applied, thanks.

> ---
>  hw/ppc/spapr.c |    9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 12b3f099d4c9..4a480e1c1dd9 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2993,8 +2993,6 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>      CPUState *cs = CPU(core->threads);
>      sPAPRDRConnector *drc;
>      Error *local_err = NULL;
> -    void *fdt = NULL;
> -    int fdt_offset = 0;
>      int smt = kvmppc_smt_threads();
>      CPUArchId *core_slot;
>      int index;
> @@ -3009,9 +3007,12 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>  
>      g_assert(drc || !mc->has_hotpluggable_cpus);
>  
> -    fdt = spapr_populate_hotplug_cpu_dt(cs, &fdt_offset, spapr);
> -
>      if (drc) {
> +        void *fdt;
> +        int fdt_offset;
> +
> +        fdt = spapr_populate_hotplug_cpu_dt(cs, &fdt_offset, spapr);
> +
>          spapr_drc_attach(drc, dev, fdt, fdt_offset, &local_err);
>          if (local_err) {
>              g_free(fdt);
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]