Re: [PATCH] can: rcar_canfd: Make sure we free CAN network device

[Lad, Prabhakar]

Hi Kieran,

Thank you for the  review.

On Thu, Jan 6, 2022 at 3:46 PM Kieran Bingham
<kieran.bingham@xxxxxxxxxxxxxxxx> wrote:
>
> Quoting Lad Prabhakar (2022-01-06 11:48:00)
> > Make sure we free CAN network device in the error path. There are several
> > jumps to fail label after allocating the CAN network device successfully.
> > This patch places the free_candev() under fail label so that in failure
> > path a jump to fail label frees the CAN network device.
> >
> > Fixes: 76e9353a80e9 ("can: rcar_canfd: Add support for RZ/G2L family")
> > Reported-by: Pavel Machek <pavel@xxxxxxx>
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> > ---
> >  drivers/net/can/rcar/rcar_canfd.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
> > index ff9d0f5ae0dd..388521e70837 100644
> > --- a/drivers/net/can/rcar/rcar_canfd.c
> > +++ b/drivers/net/can/rcar/rcar_canfd.c
> > @@ -1640,8 +1640,7 @@ static int rcar_canfd_channel_probe(struct rcar_canfd_global *gpriv, u32 ch,
> >         ndev = alloc_candev(sizeof(*priv), RCANFD_FIFO_DEPTH);
> >         if (!ndev) {
> >                 dev_err(&pdev->dev, "alloc_candev() failed\n");
> > -               err = -ENOMEM;
> > -               goto fail;
> > +               return -ENOMEM;
>
> Aha good - so we don't try to call free_candev() on a null pointer.
> (which doesn't look null-safe, in free_netdev).
>
Yep.

> >         }
> >         priv = netdev_priv(ndev);
> >
> > @@ -1735,8 +1734,8 @@ static int rcar_canfd_channel_probe(struct rcar_canfd_global *gpriv, u32 ch,
> >
> >  fail_candev:
>
> Is this label still appropriately named now that the free_candev is
> moved out of it? I wonder if it should be fail_netif:
>
I was tempted for this change, but wanted to keep the changes minimal.
Maybe I'll do it anyway to improve the readability.

> So aside from potential naming, the !ndev case is safely handled, so it
> looks fine to me.
>
>
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx>
>
> >         netif_napi_del(&priv->napi);
> > -       free_candev(ndev);
> >  fail:
> > +       free_candev(ndev);
>
>
>
> >         return err;
> >  }
> >
> > --
> > 2.17.1
> >

Cheers,
Prabhakar