Memory leak was caused by jumping to the wrong exit label. So, it is good time
to improve misleading label names too.
Signed-off-by: Vitalii Demianets <[email protected]>
---
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);
- bad2:
+out:
return ret;
}
--
1.7.8.6
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 <[email protected]>
> ---
> 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.
> - bad2:
> +out:
> return ret;
> }
>
> --
> 1.7.8.6
>
On Wed, Nov 28, 2012 at 12:07 AM, Hans J. Koch <[email protected]> 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 <[email protected]>
>> ---
>> 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?
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) {
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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
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 <[email protected]> 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 <[email protected]>
> >> ---
> >> 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 [email protected]
> > 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
On Wed, Nov 28, 2012 at 1:37 AM, Hans J. Koch <[email protected]> wrote:
> 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 <[email protected]> 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 <[email protected]>
>> >> ---
>> >> 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.
You are correct, we have to ensure they are valid before line 115 (or
109). Sorry for misunderstanding your idea in the former email.
>
>>
>> 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.
I agree that uioinfo has to be valid here, but it is checked in line
110 (and go to bad2 if invalid), why check again in line 126?
>
>> 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 [email protected]
>> > 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 [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at http://www.tux.org/lkml/
>>
On Wednesday 28 November 2012 02:37:50 Hans J. Koch wrote:
>
> In other words, the case of uioinfo AND pdev->dev.of_node both being NULL
> is not handled properly and will have ugly results.
>
Moreover, the case of (uioinfo != NULL) && (pdev->dev.of_node != NULL) leads
to equally ugly results too (freeing uoinfo when it is statically allocated).
I think, we should sort out these problems, but in another patch. It is
totally unrelated to the problem solved by the original patch (memory leak
caused by not freeing priv in case platform_get_irq() fails).
On Wed, Nov 28, 2012 at 10:29:29AM +0100, Cong Ding wrote:
> On Wed, Nov 28, 2012 at 1:37 AM, Hans J. Koch <[email protected]> wrote:
> > 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 <[email protected]> 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 <[email protected]>
> >> >> ---
> >> >> 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.
> You are correct, we have to ensure they are valid before line 115 (or
> 109). Sorry for misunderstanding your idea in the former email.
> >
> >>
> >> 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.
> I agree that uioinfo has to be valid here, but it is checked in line
> 110 (and go to bad2 if invalid), why check again in line 126?
OK, you're right. No need to check it here.
> >
> >> 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 [email protected]
> >> > 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 [email protected]
> >> More majordomo info at http://vger.kernel.org/majordomo-info.html
> >> Please read the FAQ at http://www.tux.org/lkml/
> >>
>
On Wed, Nov 28, 2012 at 11:37:23AM +0200, Vitalii Demianets wrote:
> On Wednesday 28 November 2012 02:37:50 Hans J. Koch wrote:
> >
> > In other words, the case of uioinfo AND pdev->dev.of_node both being NULL
> > is not handled properly and will have ugly results.
> >
>
> Moreover, the case of (uioinfo != NULL) && (pdev->dev.of_node != NULL) leads
> to equally ugly results too (freeing uoinfo when it is statically allocated).
You're right. That wants to be fixed as well.
>
> I think, we should sort out these problems, but in another patch. It is
> totally unrelated to the problem solved by the original patch (memory leak
> caused by not freeing priv in case platform_get_irq() fails).
So far, no patch was applied. I don't mind if all fixes for uio_pdrv_genirq
are in one patch as long as only this one file is altered.
Thanks,
Hans
1. uioinfo was kfreed based on the presence of pdev->dev.of_node, which was
obviously wrong and unrelated to the fact if uioinfo was allocated statically
or dynamically. This patch introduces new flag which clearly shows if uioinfo
was allocated dynamically and kfrees uioinfo based on that flag;
2. Fix: priv data was not freed in case platform_get_irq() failed. As it was
caused mainly by improper exit labels naming, labels were renamed too;
3. The case of uioinfo AND pdev->dev.of_node both NULL (not initialized
in platform data) was not treated properly.
Signed-off-by: Vitalii Demianets <[email protected]>
---
drivers/uio/uio_pdrv_genirq.c | 44 ++++++++++++++++++++++++----------------
1 files changed, 26 insertions(+), 18 deletions(-)
diff --git a/drivers/uio/uio_pdrv_genirq.c b/drivers/uio/uio_pdrv_genirq.c
index 42202cd..45126e3 100644
--- a/drivers/uio/uio_pdrv_genirq.c
+++ b/drivers/uio/uio_pdrv_genirq.c
@@ -30,6 +30,11 @@
#define DRIVER_NAME "uio_pdrv_genirq"
+enum {
+ bitIRQDisabled = 0,
+ bitUioinfoAlloced = 1,
+};
+
struct uio_pdrv_genirq_platdata {
struct uio_info *uioinfo;
spinlock_t lock;
@@ -63,7 +68,7 @@ static irqreturn_t uio_pdrv_genirq_handler(int irq, struct uio_info *dev_info)
* remember the state so we can allow user space to enable it later.
*/
- if (!test_and_set_bit(0, &priv->flags))
+ if (!test_and_set_bit(bitIRQDisabled, &priv->flags))
disable_irq_nosync(irq);
return IRQ_HANDLED;
@@ -83,10 +88,10 @@ static int uio_pdrv_genirq_irqcontrol(struct uio_info *dev_info, s32 irq_on)
spin_lock_irqsave(&priv->lock, flags);
if (irq_on) {
- if (test_and_clear_bit(0, &priv->flags))
+ if (test_and_clear_bit(bitIRQDisabled, &priv->flags))
enable_irq(dev_info->irq);
} else {
- if (!test_and_set_bit(0, &priv->flags))
+ if (!test_and_set_bit(bitIRQDisabled, &priv->flags))
disable_irq(dev_info->irq);
}
spin_unlock_irqrestore(&priv->lock, flags);
@@ -101,8 +106,9 @@ static int uio_pdrv_genirq_probe(struct platform_device *pdev)
struct uio_mem *uiomem;
int ret = -EINVAL;
int i;
+ bool uioinfo_alloced = false;
- if (!uioinfo) {
+ if (!uioinfo && pdev->dev.of_node) {
int irq;
/* alloc uioinfo for one device */
@@ -110,10 +116,11 @@ 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";
+ uioinfo_alloced = true;
/* Multiple IRQs are not supported */
irq = platform_get_irq(pdev, 0);
@@ -125,32 +132,33 @@ 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;
spin_lock_init(&priv->lock);
- priv->flags = 0; /* interrupt is enabled to begin with */
+ /* interrupt is enabled to begin with */
+ priv->flags = uioinfo_alloced ? (1 << bitUioinfoAlloced) : 0;
priv->pdev = pdev;
if (!uioinfo->irq) {
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 +213,19 @@ 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:
- /* kfree uioinfo for OF */
- if (pdev->dev.of_node)
+out_priv:
+ kfree(priv);
+out_uioinfo:
+ if (uioinfo_alloced)
kfree(uioinfo);
- bad2:
+out:
return ret;
}
@@ -232,7 +240,7 @@ static int uio_pdrv_genirq_remove(struct platform_device *pdev)
priv->uioinfo->irqcontrol = NULL;
/* kfree uioinfo for OF */
- if (pdev->dev.of_node)
+ if (priv->flags & (1 << bitUioinfoAlloced))
kfree(priv->uioinfo);
kfree(priv);
--
1.7.8.6