Re: [PATCH 2/3] hw/mem/cxl_type3: Fix special_ops memory leak on msix_init_exclusive_bar() failure

[Jonathan Cameron]

On Tue, 21 Jan 2025 14:58:12 +0000
Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:

> On Mon, 20 Jan 2025 11:09:46 +0800
> Li Zhijian <lizhijian@fujitsu.com> wrote:
> 
> > Address a memory leak issue by ensuring `regs->special_ops` is freed when
> > `msix_init_exclusive_bar()` encounters an error during CXL Type3 device
> > initialization.
> > 
> > Additionally, this patch renames err_address_space_free to err_msix_uninit
> > for better clarity and logical flow
> > 
> > Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>  
> Hi.
> 
> The need to reorder the cleanup calls made me look more
> closely at this patch. Either the order was wrong here or in the previous
> patch. I think the issue is here.
> 
> Jonathan
> 
> > ---
> >  hw/mem/cxl_type3.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> > index 9dad250f56d5..9eb3d0979cf5 100644
> > --- a/hw/mem/cxl_type3.c
> > +++ b/hw/mem/cxl_type3.c
> > @@ -885,7 +885,7 @@ static void ct3_realize(PCIDevice *pci_dev, Error 
> > **errp)
> >      /* MSI(-X) Initialization */
> >      rc = msix_init_exclusive_bar(pci_dev, CXL_T3_MSIX_VECTOR_NR, 4, NULL);
> >      if (rc) {
> > -        goto err_address_space_free;
> > +        goto err_free_special_ops;
> >      }
> >      for (i = 0; i < CXL_T3_MSIX_VECTOR_NR; i++) {
> >          msix_vector_use(pci_dev, i);
> > @@ -899,7 +899,7 @@ static void ct3_realize(PCIDevice *pci_dev, Error 
> > **errp)
> >      cxl_cstate->cdat.free_cdat_table = ct3_free_cdat_table;
> >      cxl_cstate->cdat.private = ct3d;
> >      if (!cxl_doe_cdat_init(cxl_cstate, errp)) {
> > -        goto err_free_special_ops;
> > +        goto err_msix_uninit;
> >      }
> >  
> >      pcie_cap_deverr_init(pci_dev);
> > @@ -936,9 +936,9 @@ static void ct3_realize(PCIDevice *pci_dev, Error 
> > **errp)
> >  err_release_cdat:
> >      cxl_doe_cdat_release(cxl_cstate);
> >  err_free_special_ops:
> > -    msix_uninit_exclusive_bar(pci_dev);
> >      g_free(regs->special_ops);
> > -err_address_space_free:
> > +err_msix_uninit:
> > +    msix_uninit_exclusive_bar(pci_dev);  
> 
> This reorder doesn't look correct.
> 
> Should end up I think as
> err_release_cdata:
>     cxl_doe_cdata_release(cxl_cstate);
> err_msix_uninit:
>     msix_uninit_eclusive_bar(pci_dev);
> err_free_special_ops:
>     g_free(regs->special_ops)
> err_address_space_free:
This last label can go away.

I've applied your series with order modified as here to my CXL staging git
tree. I'll push out later today hopefully.  

Thanks,

Jonathan

> etc.
> 
> >      if (ct3d->dc.host_dc) {
> >          cxl_destroy_dc_regions(ct3d);
> >          address_space_destroy(&ct3d->dc.host_dc_as);  
> 
>