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

[Guangshuo Li]

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.

Thanks,
Guangshuo