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

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

[Dongliang Mu]

In fsl_ifc_ctrl_probe, if fsl_ifc_ctrl_init fails, we should free the
resources allocated by irq_of_parse_and_map.

Fix this by adjusting the error handling code.

Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
---
 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);
-- 
2.25.1

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

[Krzysztof Kozlowski]

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.



Best regards,
Krzysztof

> 
> Fix this by adjusting the error handling code.
> 
> Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
> ---
>  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);
> 

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@canonical.com> 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@gmail.com>
> > ---
> >  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);
> >
>

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

[Krzysztof Kozlowski]

On 03/08/2021 11:28, Dongliang Mu wrote:
> On Tue, Aug 3, 2021 at 3:57 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@canonical.com> 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 did not check the exact error paths, I assume you are going to make
them correct. Therefore only the nand_irq which looks optional and your
code makes it required.

> 
> 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

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 5:51 PM Krzysztof Kozlowski
<krzysztof.kozlowski@canonical.com> wrote:
>
> On 03/08/2021 11:28, Dongliang Mu wrote:
> > On Tue, Aug 3, 2021 at 3:57 PM Krzysztof Kozlowski
> > <krzysztof.kozlowski@canonical.com> 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 did not check the exact error paths, I assume you are going to make
> them correct. Therefore only the nand_irq which looks optional and your
> code makes it required.

Well, yes. It seems unnecessary. I will revert this part of the code
changes and send a v2 patch.

>
> >
> > 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