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

[Greg Kurz <groug@kaod.org>]

[-- 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 --]