Re: [PATCH net] gianfar: fix memory leak in gfar_parse_group error paths

[Pavan Chebbi]

On Mon, Jun 22, 2026 at 7:49 AM Rosen Penev <rosenp@xxxxxxxxx> wrote:
>
> In gfar_parse_group, if any allocation in the irqinfo[] loop fails,
> or if of_iomap fails, or if the IRQ parsing fails, the already-
> allocated irqinfo[] entries and mapped regs are not freed.  The
> caller's free_gfar_dev cannot clean them up because priv->num_grps
> is only incremented after a successful return.
>
> Fix by adding an err_out label that frees all successfully allocated
> irqinfo entries and unmaps regs if mapped.  Track the number of
> allocated entries with 'allocated' and propagate the correct error
> code.
>
> Fixes: 46ceb60ca80fa ("gianfar: Add Multiple group Support")
> Assisted-by: opencode:big-pickle
> Signed-off-by: Rosen Penev <rosenp@xxxxxxxxx>
> ---
>  drivers/net/ethernet/freescale/gianfar.c | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
> index 3271de5844f84..12ebb207be0be 100644
> --- a/drivers/net/ethernet/freescale/gianfar.c
> +++ b/drivers/net/ethernet/freescale/gianfar.c
> @@ -502,17 +502,19 @@ static int gfar_parse_group(struct device_node *np,
>                             struct gfar_private *priv, const char *model)
>  {
>         struct gfar_priv_grp *grp = &priv->gfargrp[priv->num_grps];
> -       int i;
> +       int err = -ENOMEM;
> +       int i, allocated = 0;

Declarations should follow reverse xmas-tree fashion.
BTW, why num_grps cannot be incremented quicker? Not sure it can cause
any issues, since all this is happening probe(), it should be safe to
do that?
That way you could avoid cleaning up at two different places...

>
>         for (i = 0; i < GFAR_NUM_IRQS; i++) {
>                 grp->irqinfo[i] = kzalloc_obj(struct gfar_irqinfo);
>                 if (!grp->irqinfo[i])
> -                       return -ENOMEM;
> +                       goto err_out;
> +               allocated++;
>         }
>
>         grp->regs = of_iomap(np, 0);
>         if (!grp->regs)
> -               return -ENOMEM;
> +               goto err_out;
>
>         gfar_irq(grp, TX)->irq = irq_of_parse_and_map(np, 0);
>
> @@ -522,8 +524,10 @@ static int gfar_parse_group(struct device_node *np,
>                 gfar_irq(grp, ER)->irq = irq_of_parse_and_map(np, 2);
>                 if (!gfar_irq(grp, TX)->irq ||
>                     !gfar_irq(grp, RX)->irq ||
> -                   !gfar_irq(grp, ER)->irq)
> -                       return -EINVAL;
> +                   !gfar_irq(grp, ER)->irq) {
> +                       err = -EINVAL;
> +                       goto err_out;
> +               }
>         }
>
>         grp->priv = priv;
> @@ -567,6 +571,13 @@ static int gfar_parse_group(struct device_node *np,
>         priv->num_grps++;
>
>         return 0;
> +
> +err_out:
> +       for (i = 0; i < allocated; i++)
> +               kfree(grp->irqinfo[i]);
> +       if (grp->regs)
> +               iounmap(grp->regs);
> +       return err;
>  }
>
>  /* Reads the controller's registers to determine what interface
> --
> 2.54.0
>
>

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature