MIME-Version: 1.0
References: <20220106114801.20563-1-prabhakar.mahadev-lad.rj@bp.renesas.com> <164148400405.10801.375398277921411297@Monstersaurus>
In-Reply-To: <164148400405.10801.375398277921411297@Monstersaurus>
From:   "Lad, Prabhakar" <prabhakar.csengg@gmail.com>
Date:   Thu, 6 Jan 2022 17:46:25 +0000
Message-ID: <CA+V-a8saaXGBJweW9dE6+Vp8k2fpYob2tbVbhxCqK-PU_y1PWg@mail.gmail.com>
Subject: Re: [PATCH] can: rcar_canfd: Make sure we free CAN network device
To:     Kieran Bingham <kieran.bingham@ideasonboard.com>
Cc:     Biju Das <biju.das.jz@bp.renesas.com>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>,
        Marc Kleine-Budde <mkl@pengutronix.de>,
        Wolfgang Grandegger <wg@grandegger.com>,
        Pavel Machek <pavel@denx.de>,
        Linux-Renesas <linux-renesas-soc@vger.kernel.org>,
        linux-can@vger.kernel.org, netdev <netdev@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Kieran,

Thank you for the  review.

On Thu, Jan 6, 2022 at 3:46 PM Kieran Bingham
<kieran.bingham@ideasonboard.com> 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@denx.de>
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> >  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@ideasonboard.com>
>
> >         netif_napi_del(&priv->napi);
> > -       free_candev(ndev);
> >  fail:
> > +       free_candev(ndev);
>
>
>
> >         return err;
> >  }
> >
> > --
> > 2.17.1
> >

Cheers,
Prabhakar