Re: [PATCH] EDAC/versalnet: fix memory leak on device registration failure

[Shubhrajyoti Datta]

On Thu, Sep 24, 2026 at 7:45 PM Guangshuo Li <lgs201920130244@xxxxxxxxx> wrote:
>
> Hi Shubhrajyoti,
>
> Thanks for pointing that out.
>
> On Wed, 23 Sept 2026 at 23:03, Shubhrajyoti Datta
> <shubhrajyoti.datta@xxxxxxxxx> wrote:
> >
> > On Sun, Sep 20, 2026 at 11:25 AM Guangshuo Li <lgs201920130244@xxxxxxxxx> wrote:
> > >
> > > init_one_mc() allocates dev with kzalloc() and registers it with
> > > device_register(). If device_register() fails, the device has already
> > > been initialized and holds its initial device reference.
> > >
> > > The current error path eventually frees dev directly with kfree().
> > > This bypasses the device release path and can leak driver-core
> > > resources associated with the initialized device. The existing
> > > versal_edac_release() callback is responsible for freeing dev once the
> > > device reference reaches zero.
> > >
> > > Call put_device() when device_register() fails so the initialized
> > > device reference is dropped and versal_edac_release() performs the
> > > proper cleanup. Return after freeing mci to avoid falling through to
> > > the direct kfree() path, which remains necessary for failures that
> > > occur before device_register() is called.
> > >
> > > The issue was identified by a static analysis tool I developed and
> > > confirmed by manual review.
> > >
> > > Fixes: d5fe2fec6c40 ("EDAC: Add a driver for the AMD Versal NET DDR controller")
> > > Cc: stable@xxxxxxxxxxxxxxx
> > > Signed-off-by: Guangshuo Li <lgs201920130244@xxxxxxxxx>
> > > ---
> > >  drivers/edac/versalnet_edac.c | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/edac/versalnet_edac.c b/drivers/edac/versalnet_edac.c
> > > index 3a06b41c1d84..db3bf5a1345b 100644
> > > --- a/drivers/edac/versalnet_edac.c
> > > +++ b/drivers/edac/versalnet_edac.c
> > > @@ -829,8 +829,10 @@ static int init_one_mc(struct mc_priv *priv, struct platform_device *pdev, int i
> > >         dev->release = versal_edac_release;
> > >
> > >         rc = device_register(dev);
> > > -       if (rc)
> > > +       if (rc) {
> > > +               put_device(dev);
> > >                 goto err_mc_free;
> > > +       }
> >
> > There was a comment earlier from sashiko
> >  failure path calls put_device() with dev->init_name still pointing at a stack
> > buffer before device_add() copies it. That's unsafe in
> > principle (dev_name() would follow init_name)
> > >
> > >         mci->pdev = dev;
> > >         mc_init(mci, dev);
> > > @@ -852,9 +854,9 @@ static int init_one_mc(struct mc_priv *priv, struct platform_device *pdev, int i
> > >         device_unregister(mci->pdev);
> > >  err_mc_free:
> > >         edac_mc_free(mci);
> > > +       return rc;
> > >  err_dev_free:
> > >         kfree(dev);
> > > -
> > >         return rc;
> > >  }
> > >
> > > --
> > > 2.43.0
> > >
> > >
>
> Would splitting device_register() into device_initialize(), dev_set_name(),
> and device_add() be a reasonable way to handle this? For example:
>
> diff --git a/drivers/edac/versalnet_edac.c b/drivers/edac/versalnet_edac.c
> --- a/drivers/edac/versalnet_edac.c
> +++ b/drivers/edac/versalnet_edac.c
> @@ -64,7 +64,6 @@
>  #define XDDR5_BUS_WIDTH_64 0
>  #define XDDR5_BUS_WIDTH_32 1
>  #define XDDR5_BUS_WIDTH_16 2
> -#define MC_NAME_LEN 32
>
>  static int init_one_mc(struct mc_priv *priv, struct platform_device
> *pdev, int i)
>  {
> @@ -730,7 +729,6 @@ static int init_one_mc(struct mc_priv *priv,
> struct platform_device *pdev, int i)
>   u32 num_chans, rank, dwidth, config;
>   struct edac_mc_layer layers[2];
>   struct mem_ctl_info *mci;
> - char name[MC_NAME_LEN];
>   struct device *dev;
>   enum dev_type dt;
>   int rc;
> @@ -773,13 +771,17 @@ static int init_one_mc(struct mc_priv *priv,
> struct platform_device *pdev, int i)
>   goto err_dev_free;
>   }
>
> - sprintf(name, "versal-net-ddrmc5-edac-%d", i);
> -
> - dev->init_name = name;
> + device_initialize(dev);
>   dev->release = versal_edac_release;
>
> - rc = device_register(dev);
> + rc = dev_set_name(dev, "versal-net-ddrmc5-edac-%d", i);
> + if (rc)
> + goto err_mc_free;
> +
> + rc = device_add(dev);
>   if (rc)
>   goto err_mc_free;
>
> @@ -800,10 +802,13 @@ static int init_one_mc(struct mc_priv *priv,
> struct platform_device *pdev, int i)
>
>  err_unreg:
>   device_unregister(mci->pdev);
> + edac_mc_free(mci);
> + return rc;
>  err_mc_free:
>   edac_mc_free(mci);
> + put_device(dev);
> + return rc;
>  err_dev_free:
>   kfree(dev);
>
>   return rc;
>  }
>
> This avoids keeping dev->init_name pointing at stack storage and uses
> put_device() only after the device has been initialized. Would this look
> reasonable to you? If this looks good to you, I’ll send v2 with this approach.

This is already addressed in the ongoing patch series:

https://lore.kernel.org/all/20260724171945.2812749-5-shubhrajyoti.datta@xxxxxxx/

Please have a look and let me know if you have any concerns.
>
> Thanks,
> Guangshuo