Re: [PATCH] memory: fsl_ifc: fix leak of irq and nand_irq in fsl_ifc_ctrl_probe

[Dongliang Mu]

On Tue, Aug 3, 2021 at 3:57 PM Krzysztof Kozlowski
<krzysztof.kozlowski@xxxxxxxxxxxxx> wrote:
>
> On 03/08/2021 09:51, Dongliang Mu wrote:
> > In fsl_ifc_ctrl_probe, if fsl_ifc_ctrl_init fails, we should free the
> > resources allocated by irq_of_parse_and_map.
>
> Your code is doing much more. You also touch nand_irq, not only
> fsl_ifc_ctrl_init(). This looks incorrect as IRQ is optional, isn't it?
>
> The problem is entirely different than you described here - the error
> paths of fsl_ifc_ctrl_init() and request_irq() are wrong. They do not
> release resources in proper paths.

Yes, you're right :). This patch rewrites the whole error handling
code. Any comment on the code changes?

I will rewrite the commit message in the patch v2.

BTW, there is a minor issue: if request_irq fails, we should not
invoke free_irq.

>
>
>
> Best regards,
> Krzysztof
>
> >
> > Fix this by adjusting the error handling code.
> >
> > Signed-off-by: Dongliang Mu <mudongliangabcd@xxxxxxxxx>
> > ---
> >  drivers/memory/fsl_ifc.c | 19 ++++++++++++-------
> >  1 file changed, 12 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/memory/fsl_ifc.c b/drivers/memory/fsl_ifc.c
> > index d062c2f8250f..391390dd3dcb 100644
> > --- a/drivers/memory/fsl_ifc.c
> > +++ b/drivers/memory/fsl_ifc.c
> > @@ -258,12 +258,17 @@ static int fsl_ifc_ctrl_probe(struct platform_device *dev)
> >       /* get the nand machine irq */
> >       fsl_ifc_ctrl_dev->nand_irq =
> >                       irq_of_parse_and_map(dev->dev.of_node, 1);
> > +     if (fsl_ifc_ctrl_dev->nand_irq == 0) {
> > +             dev_err(&dev->dev, "failed to get nand_irq resource for IFC\n");
> > +             ret = -ENODEV;
> > +             goto err_unmap_irq;
> > +     }
> >
> >       fsl_ifc_ctrl_dev->dev = &dev->dev;
> >
> >       ret = fsl_ifc_ctrl_init(fsl_ifc_ctrl_dev);
> >       if (ret < 0)
> > -             goto err;
> > +             goto err_unmap_nandirq;
> >
> >       init_waitqueue_head(&fsl_ifc_ctrl_dev->nand_wait);
> >
> > @@ -272,7 +277,7 @@ static int fsl_ifc_ctrl_probe(struct platform_device *dev)
> >       if (ret != 0) {
> >               dev_err(&dev->dev, "failed to install irq (%d)\n",
> >                       fsl_ifc_ctrl_dev->irq);
> > -             goto err_irq;
> > +             goto err_unmap_nandirq;
> >       }
> >
> >       if (fsl_ifc_ctrl_dev->nand_irq) {
> > @@ -281,17 +286,17 @@ static int fsl_ifc_ctrl_probe(struct platform_device *dev)
> >               if (ret != 0) {
> >                       dev_err(&dev->dev, "failed to install irq (%d)\n",
> >                               fsl_ifc_ctrl_dev->nand_irq);
> > -                     goto err_nandirq;
> > +                     goto err_free_irq;
> >               }
> >       }
> >
> >       return 0;
> >
> > -err_nandirq:
> > -     free_irq(fsl_ifc_ctrl_dev->nand_irq, fsl_ifc_ctrl_dev);
> > -     irq_dispose_mapping(fsl_ifc_ctrl_dev->nand_irq);
> > -err_irq:
> > +err_free_irq:
> >       free_irq(fsl_ifc_ctrl_dev->irq, fsl_ifc_ctrl_dev);
> > +err_unmap_nandirq:
> > +     irq_dispose_mapping(fsl_ifc_ctrl_dev->nand_irq);
> > +err_unmap_irq:
> >       irq_dispose_mapping(fsl_ifc_ctrl_dev->irq);
> >  err:
> >       iounmap(fsl_ifc_ctrl_dev->gregs);
> >
>