Re: [PATCH] drivers/uio/uio_pdrv_genirq.c: Fix memory leak & confusing labels

["Hans J. Koch" <hjk@hansjkoch.de>]

On Wed, Nov 28, 2012 at 01:07:26AM +0100, Cong Ding wrote:
> On Wed, Nov 28, 2012 at 12:07 AM, Hans J. Koch <hjk@hansjkoch.de> wrote:
> > On Tue, Nov 27, 2012 at 07:29:32PM +0200, Vitalii Demianets wrote:
> >> Memory leak was caused by jumping to the wrong exit label. So, it is good time
> >> to improve misleading label names too.
> >
> > I agree that bad0, bad1, and bad2 are not the best choice for label names...
> > I don't have any objections to your renaming.
> >
> > But there's a more serious bug, maybe you can fix that as well while you're
> > at it? (See below)
> >
> > Thanks,
> > Hans
> >
> >>
> >> Signed-off-by: Vitalii Demianets <vitas@nppfactor.kiev.ua>
> >> ---
> >>  drivers/uio/uio_pdrv_genirq.c |   21 +++++++++++----------
> >>  1 files changed, 11 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/drivers/uio/uio_pdrv_genirq.c b/drivers/uio/uio_pdrv_genirq.c
> >> index 42202cd..b88cf7b 100644
> >> --- a/drivers/uio/uio_pdrv_genirq.c
> >> +++ b/drivers/uio/uio_pdrv_genirq.c
> >> @@ -110,7 +110,7 @@ static int uio_pdrv_genirq_probe(struct platform_device *pdev)
> >>               if (!uioinfo) {
> >>                       ret = -ENOMEM;
> >>                       dev_err(&pdev->dev, "unable to kmalloc\n");
> >> -                     goto bad2;
> >> +                     goto out;
> >>               }
> >>               uioinfo->name = pdev->dev.of_node->name;
> >>               uioinfo->version = "devicetree";
> >> @@ -125,20 +125,20 @@ static int uio_pdrv_genirq_probe(struct platform_device *pdev)
> >>
> >>       if (!uioinfo || !uioinfo->name || !uioinfo->version) {
> >>               dev_err(&pdev->dev, "missing platform_data\n");
> >> -             goto bad0;
> >> +             goto out_uioinfo;
> >>       }
> >>
> >>       if (uioinfo->handler || uioinfo->irqcontrol ||
> >>           uioinfo->irq_flags & IRQF_SHARED) {
> >>               dev_err(&pdev->dev, "interrupt configuration error\n");
> >> -             goto bad0;
> >> +             goto out_uioinfo;
> >>       }
> >>
> >>       priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> >>       if (!priv) {
> >>               ret = -ENOMEM;
> >>               dev_err(&pdev->dev, "unable to kmalloc\n");
> >> -             goto bad0;
> >> +             goto out_uioinfo;
> >>       }
> >>
> >>       priv->uioinfo = uioinfo;
> >> @@ -150,7 +150,7 @@ static int uio_pdrv_genirq_probe(struct platform_device *pdev)
> >>               ret = platform_get_irq(pdev, 0);
> >>               if (ret < 0) {
> >>                       dev_err(&pdev->dev, "failed to get IRQ\n");
> >> -                     goto bad0;
> >> +                     goto out_priv;
> >>               }
> >>               uioinfo->irq = ret;
> >>       }
> >> @@ -205,19 +205,20 @@ static int uio_pdrv_genirq_probe(struct platform_device *pdev)
> >>       ret = uio_register_device(&pdev->dev, priv->uioinfo);
> >>       if (ret) {
> >>               dev_err(&pdev->dev, "unable to register uio device\n");
> >> -             goto bad1;
> >> +             goto out_pm;
> >>       }
> >>
> >>       platform_set_drvdata(pdev, priv);
> >>       return 0;
> >> - bad1:
> >> -     kfree(priv);
> >> +out_pm:
> >>       pm_runtime_disable(&pdev->dev);
> >> - bad0:
> >> +out_priv:
> >> +     kfree(priv);
> >> +out_uioinfo:
> >>       /* kfree uioinfo for OF */
> >>       if (pdev->dev.of_node)
> >>               kfree(uioinfo);
> >
> > The free() depends on pdev->dev.of_node, while the allocation doesn't!!!!
> > That's another source of memory leaks.
> I don't agree. In line 99, it is
> struct uio_info *uioinfo = pdev->dev.platform_data;
> if uioinfo doesn't equal to NULL, it will run to line 126,
> if (!uioinfo || !uioinfo->name || !uioinfo->version) {
> and then if uioinfo->name equals to NULL, it runs to line 127 and 128,
> and then goto bad0. If in this flow, we have to check
> pdev->dev.of_node before free(uioinfo), right?

Hmmm. The idea is that uioinfo==NULL means OF. In that case,
a struct uio_info is allocated and filled with the necessary values (name,
version, irq). It is assumed (without check...) that pdev->dev.of_node
is not NULL. If it were NULL we would crash here when dereferencing
 pdev->dev.of_node->name, leaving a memory leak.

After bad0 it is also assumed that pdev->dev.of_node is an indicator for
OF or not OF.

In other words, the case of uioinfo AND pdev->dev.of_node both being NULL
is not handled properly and will have ugly results.

> 
> btw, I think in line 126 it is not necessary to check (!uioinfo),
> because if uioinfo equals to NULL, it will go to line 109, and if the
> alloc fails, it will go to bad2. uioinfo has no chance to be NULL when
> runs to line 126. So I'd like to suggest a patch to avoid unnecessary
> check like this
> 
> diff --git a/drivers/uio/uio_pdrv_genirq.c b/drivers/uio/uio_pdrv_genirq.c
>  index 42202cd..3eb4fa2 100644
>  --- a/drivers/uio/uio_pdrv_genirq.c
>  +++ b/drivers/uio/uio_pdrv_genirq.c
>  @@ -123,7 +123,7 @@ static int uio_pdrv_genirq_probe(struct
> platform_device *pdev)
>              uioinfo->irq = irq;
>      }
> 
>  -   if (!uioinfo || !uioinfo->name || !uioinfo->version) {
>  +   if (!uioinfo->name || !uioinfo->version) {

That's wrong. We need a valid uioinfo at this point.

>          dev_err(&pdev->dev, "missing platform_data\n");
>          goto bad0;
>      }
> 
> 
> >
> >> - bad2:
> >> +out:
> >>       return ret;
> >>  }
> >>
> >> --
> >> 1.7.8.6
> >>
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>