2022-01-06 11:48:15

by Prabhakar Mahadev Lad

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

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 <[email protected]>
Signed-off-by: Lad Prabhakar <[email protected]>
---
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;
}
priv = netdev_priv(ndev);

@@ -1735,8 +1734,8 @@ static int rcar_canfd_channel_probe(struct rcar_canfd_global *gpriv, u32 ch,

fail_candev:
netif_napi_del(&priv->napi);
- free_candev(ndev);
fail:
+ free_candev(ndev);
return err;
}

--
2.17.1

2022-01-06 17:47:17

by Lad, Prabhakar

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

Hi Kieran,

Thank you for the review.

On Thu, Jan 6, 2022 at 3:46 PM Kieran Bingham
<[email protected]> 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 <[email protected]>
> > Signed-off-by: Lad Prabhakar <[email protected]>
> > ---
> > 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 <[email protected]>
>
> > netif_napi_del(&priv->napi);
> > - free_candev(ndev);
> > fail:
> > + free_candev(ndev);
>
>
>
> > return err;
> > }
> >
> > --
> > 2.17.1
> >

Cheers,
Prabhakar