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

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

[Vitalii Demianets]

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 <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);
- bad2:
+out:
 	return ret;
 }
 
-- 
1.7.8.6

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

[Hans J. Koch]

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.

> - bad2:
> +out:
>  	return ret;
>  }
>  
> -- 
> 1.7.8.6
> 

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

[Cong Ding]

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?

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

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

[Hans J. Koch]

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

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

[Cong Ding]

On Wed, Nov 28, 2012 at 1:37 AM, Hans J. Koch <hjk@hansjkoch.de> 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 <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.
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 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/
>>

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

[Vitalii Demianets]

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

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

[Hans J. Koch]

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 <hjk@hansjkoch.de> 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 <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.
> 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 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/
> >>
> 

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

[Hans J. Koch]

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

[PATCH] drivers/uio/uio_pdrv_genirq.c: Fix memory freeing issues

[Vitalii Demianets]

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 <vitas@nppfactor.kiev.ua>
---
 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

Re: [PATCH] drivers/uio/uio_pdrv_genirq.c: Fix memory freeing issues

[Hans J. Koch]

On Thu, Nov 29, 2012 at 01:47:28PM +0200, Vitalii Demianets wrote:
> 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.

Your arguments sound right to me. But there are some minor issues, see below.

Otherwise looks good.
Thanks,
Hans

> 
> Signed-off-by: Vitalii Demianets <vitas@nppfactor.kiev.ua>
> ---
>  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,

Please do not use CamelCase. This is neither Windows nor C++ or Java.
And you don't need to tell users about the type (like "bit"). That should
be clear by looking at the code. I'd prefer UPPERCASE to make it clear that
these are constants, e.g. IRQ_IS_DISABLED and UIO_INFO_IS_ALLOCATED or
something like that.

> +};
> +
>  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
> 

[PATCH v2] drivers/uio/uio_pdrv_genirq.c: Fix memory freeing issues

[Vitalii Demianets]

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 <vitas@nppfactor.kiev.ua>

---
v2: Constants naming style

---
 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..75aaeee 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 {
+	UIO_IRQ_DISABLED = 0,
+	UIO_INFO_ALLOCED = 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(UIO_IRQ_DISABLED, &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(UIO_IRQ_DISABLED, &priv->flags))
 			enable_irq(dev_info->irq);
 	} else {
-		if (!test_and_set_bit(0, &priv->flags))
+		if (!test_and_set_bit(UIO_IRQ_DISABLED, &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 << UIO_INFO_ALLOCED) : 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 << UIO_INFO_ALLOCED))
 		kfree(priv->uioinfo);
 
 	kfree(priv);
-- 
1.7.8.6

Re: [PATCH v2] drivers/uio/uio_pdrv_genirq.c: Fix memory freeing issues

[Hans J. Koch]

On Wed, Dec 05, 2012 at 11:22:57AM +0200, Vitalii Demianets wrote:
> 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.

I noticed two more issues. See below.

> 
> Signed-off-by: Vitalii Demianets <vitas@nppfactor.kiev.ua>
> 
> ---
> v2: Constants naming style
> 
> ---
>  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..75aaeee 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 {
> +	UIO_IRQ_DISABLED = 0,
> +	UIO_INFO_ALLOCED = 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(UIO_IRQ_DISABLED, &priv->flags))
>  		disable_irq_nosync(irq);

That is not safe. That flag can only be changed by userspace, and if the
userspace part is not running, the CPU on which the irq is pending will hang.
The handler has to disable the interrupt in any case.
(The original version had the same problem, I know...)

>  
>  	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(UIO_IRQ_DISABLED, &priv->flags))
>  			enable_irq(dev_info->irq);
>  	} else {
> -		if (!test_and_set_bit(0, &priv->flags))
> +		if (!test_and_set_bit(UIO_IRQ_DISABLED, &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 << UIO_INFO_ALLOCED) : 0;

The comment doesn't describe the line it's supposed to comment.

>  	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 << UIO_INFO_ALLOCED))
>  		kfree(priv->uioinfo);
>  
>  	kfree(priv);
> -- 
> 1.7.8.6
> 

Re: [PATCH v2] drivers/uio/uio_pdrv_genirq.c: Fix memory freeing issues

[Vitalii Demianets]

On Thursday 06 December 2012 04:41:01 Hans J. Koch wrote:
> > @@ -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(UIO_IRQ_DISABLED, &priv->flags))
> >  		disable_irq_nosync(irq);
> 
> That is not safe. That flag can only be changed by userspace, and if the
> userspace part is not running, the CPU on which the irq is pending will
> hang. 
> The handler has to disable the interrupt in any case.
> (The original version had the same problem, I know...)
> 

Perhaps I'm missing something here, but I don't see your point. If userspace 
is not (implicitly) calling uio_pdrv_genirq_irqcontrol() by writing to the 
device file, then on the first invocation of interrupt handler that IRQ is 
disabled by disable_irq_nosync() and that's the end of story - no more irqs, 
no problems. Until userspace writes non-zero to the device file, of course.

> >  	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 << UIO_INFO_ALLOCED) : 0;
> 
> The comment doesn't describe the line it's supposed to comment.
> 

The comment draws attention to the fact that UIO_IRQ_DISABLED is not set 
initially, and this is done on purpose. Particularly it is done because of 
the potential problem you noted earlier - if userspace is never setting this 
flag, the interrupt handler will disable irq on the first invocation thanks 
to the absence of the flag (if(!test_and_set_bit(UIO_IRQ_DISABLED,...) will 
succeed).
So, I think we really need some comment explaining how the things are arranged 
here; after all even you haven't got the whole picture on the first glance.
Maybe the current wording of the comment is not the best. I've just left what 
was there before.

Re: [PATCH v2] drivers/uio/uio_pdrv_genirq.c: Fix memory freeing issues

[Hans J. Koch]

On Thu, Dec 06, 2012 at 11:11:38AM +0200, Vitalii Demianets wrote:
> On Thursday 06 December 2012 04:41:01 Hans J. Koch wrote:
> > > @@ -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(UIO_IRQ_DISABLED, &priv->flags))
> > >  		disable_irq_nosync(irq);
> > 
> > That is not safe. That flag can only be changed by userspace, and if the
> > userspace part is not running, the CPU on which the irq is pending will
> > hang. 
> > The handler has to disable the interrupt in any case.
> > (The original version had the same problem, I know...)
> > 
> 
> Perhaps I'm missing something here, but I don't see your point. If userspace 
> is not (implicitly) calling uio_pdrv_genirq_irqcontrol() by writing to the 
> device file, then on the first invocation of interrupt handler that IRQ is 
> disabled by disable_irq_nosync() and that's the end of story - no more irqs, 
> no problems. Until userspace writes non-zero to the device file, of course.

If you run into the irq handler, the interrupt was obviously enabled. Since
irq sharing is not supported by this driver, you ALWAYS have to disable it
because it IS your interrupt. Storing the enabled/disabled status in a
variable is not needed at all here. Or am I missing something?

> 
> > >  	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 << UIO_INFO_ALLOCED) : 0;
> > 
> > The comment doesn't describe the line it's supposed to comment.
> > 
> 
> The comment draws attention to the fact that UIO_IRQ_DISABLED is not set 
> initially, and this is done on purpose.

And that's right because the interrupt is initially enabled by
uio_register_device().

> Particularly it is done because of 
> the potential problem you noted earlier - if userspace is never setting this 
> flag, the interrupt handler will disable irq on the first invocation thanks 
> to the absence of the flag (if(!test_and_set_bit(UIO_IRQ_DISABLED,...) will 
> succeed).
> So, I think we really need some comment explaining how the things are arranged 
> here; after all even you haven't got the whole picture on the first glance.
> Maybe the current wording of the comment is not the best. I've just left what 
> was there before.

OK, but we humans tend to read a text from top to bottom, so a comment should
describe the code immediately following it. And that is a line that deals with
memory allocation, not with interrupts.

Thanks,
Hans

Re: [PATCH v2] drivers/uio/uio_pdrv_genirq.c: Fix memory freeing issues

[Vitalii Demianets]

On Friday 07 December 2012 00:15:59 Hans J. Koch wrote:
> On Thu, Dec 06, 2012 at 11:11:38AM +0200, Vitalii Demianets wrote:
> > On Thursday 06 December 2012 04:41:01 Hans J. Koch wrote:
> > > > @@ -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(UIO_IRQ_DISABLED, &priv->flags))
> > > >  		disable_irq_nosync(irq);
> > >
> > > That is not safe. That flag can only be changed by userspace, and if
> > > the userspace part is not running, the CPU on which the irq is pending
> > > will hang.
> > > The handler has to disable the interrupt in any case.
> > > (The original version had the same problem, I know...)
> >
> > Perhaps I'm missing something here, but I don't see your point. If
> > userspace is not (implicitly) calling uio_pdrv_genirq_irqcontrol() by
> > writing to the device file, then on the first invocation of interrupt
> > handler that IRQ is disabled by disable_irq_nosync() and that's the end
> > of story - no more irqs, no problems. Until userspace writes non-zero to
> > the device file, of course.
>
> If you run into the irq handler, the interrupt was obviously enabled. Since
> irq sharing is not supported by this driver, you ALWAYS have to disable it
> because it IS your interrupt. Storing the enabled/disabled status in a
> variable is not needed at all here. Or am I missing something?
>

Good point. I agree that having UIO_IRQ_DISABLED flag is redundant. Inside the 
irq handler we know for sure that irq is enabled. In 
uio_pdrv_genirq_irqcontrol() this knowledge is not mandatory, we could 
enable/disable irq unconditionally.

The only question: I feel uncomfortable mixing in one patch bug fixes (i.e. 
memory freeing issues) and optimization (removing UIO_IRQ_DISABLED flag). 
What do you think is the best course of action:
a) respin the current patch to the v3 with removing UIO_IRQ_DISABLED flag;
 or
b) do the removing of the flag in a separate patch, dependent on the "Fix 
memory freeing issues"?

> > > >  	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 << UIO_INFO_ALLOCED) : 0;
> > >
> > > The comment doesn't describe the line it's supposed to comment.
> >
> > The comment draws attention to the fact that UIO_IRQ_DISABLED is not set
> > initially, and this is done on purpose.
>
> And that's right because the interrupt is initially enabled by
> uio_register_device().
>
> > Particularly it is done because of
> > the potential problem you noted earlier - if userspace is never setting
> > this flag, the interrupt handler will disable irq on the first invocation
> > thanks to the absence of the flag
> > (if(!test_and_set_bit(UIO_IRQ_DISABLED,...) will succeed).
> > So, I think we really need some comment explaining how the things are
> > arranged here; after all even you haven't got the whole picture on the
> > first glance. Maybe the current wording of the comment is not the best.
> > I've just left what was there before.
>
> OK, but we humans tend to read a text from top to bottom, so a comment
> should describe the code immediately following it. And that is a line that
> deals with memory allocation, not with interrupts.
>

That line deals with initialization of flags. The comment draws attention to 
the fact that UIO_IRQ_DISABLED flag is initialized to zero.
Anyways, if we agreed on the removing of that flag altogether, this would be 
no issue and the comment would be simply removed.

Re: [PATCH v2] drivers/uio/uio_pdrv_genirq.c: Fix memory freeing issues

[Vitalii Demianets]

On Friday 07 December 2012 11:41:45 Vitalii Demianets wrote:
> On Friday 07 December 2012 00:15:59 Hans J. Koch wrote:
> > On Thu, Dec 06, 2012 at 11:11:38AM +0200, Vitalii Demianets wrote:
> > > On Thursday 06 December 2012 04:41:01 Hans J. Koch wrote:
> > > > > @@ -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(UIO_IRQ_DISABLED, &priv->flags))
> > > > >  		disable_irq_nosync(irq);
> > > >
> > > > That is not safe. That flag can only be changed by userspace, and if
> > > > the userspace part is not running, the CPU on which the irq is
> > > > pending will hang.
> > > > The handler has to disable the interrupt in any case.
> > > > (The original version had the same problem, I know...)
> > >
> > > Perhaps I'm missing something here, but I don't see your point. If
> > > userspace is not (implicitly) calling uio_pdrv_genirq_irqcontrol() by
> > > writing to the device file, then on the first invocation of interrupt
> > > handler that IRQ is disabled by disable_irq_nosync() and that's the end
> > > of story - no more irqs, no problems. Until userspace writes non-zero
> > > to the device file, of course.
> >
> > If you run into the irq handler, the interrupt was obviously enabled.
> > Since irq sharing is not supported by this driver, you ALWAYS have to
> > disable it because it IS your interrupt. Storing the enabled/disabled
> > status in a variable is not needed at all here. Or am I missing
> > something?
>
> Good point. I agree that having UIO_IRQ_DISABLED flag is redundant. Inside
> the irq handler we know for sure that irq is enabled. In
> uio_pdrv_genirq_irqcontrol() this knowledge is not mandatory, we could
> enable/disable irq unconditionally.
>

On second thought, we can't call enable_irq()/disable_irq() unconditionally 
because of the potential disable counter (irq_desc->depth) disbalance. That's 
why we need UIO_IRQ_DISABLED flag, and that's why we should check it in 
uio_pdrv_genirq_irqcontrol().
On the other hand, you are right in that we don't need to check it inside irq 
handler. Inside irq handler we can disable irq and set the flag 
unconditionally, because:
a) We know for sure that irqs are enabled, because we are inside (not-shared) 
irq handler;
 and
b) We are guarded from potential race conditions by spin_lock_irqsave() call 
in uio_pdrv_genirq_irqcontrol().

So,yes, we can get rid of costly atomic call to 
test_and_set_bit(UIO_IRQ_DISABLED,..) inside irq handler. But I still don't 
like the idea of mixing this optimization with bug fixes in a single patch.

Re: [PATCH v2] drivers/uio/uio_pdrv_genirq.c: Fix memory freeing issues

[Vitalii Demianets]

On Friday 07 December 2012 15:51:02 Vitalii Demianets wrote:
> On Friday 07 December 2012 11:41:45 Vitalii Demianets wrote:
> > On Friday 07 December 2012 00:15:59 Hans J. Koch wrote:
> > > On Thu, Dec 06, 2012 at 11:11:38AM +0200, Vitalii Demianets wrote:
> > > > On Thursday 06 December 2012 04:41:01 Hans J. Koch wrote:
> > > > > > @@ -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(UIO_IRQ_DISABLED, &priv->flags))
> > > > > >  		disable_irq_nosync(irq);
> > > > >
> > > > > That is not safe. That flag can only be changed by userspace, and
> > > > > if the userspace part is not running, the CPU on which the irq is
> > > > > pending will hang.
> > > > > The handler has to disable the interrupt in any case.
> > > > > (The original version had the same problem, I know...)
> > > >
> > > > Perhaps I'm missing something here, but I don't see your point. If
> > > > userspace is not (implicitly) calling uio_pdrv_genirq_irqcontrol() by
> > > > writing to the device file, then on the first invocation of interrupt
> > > > handler that IRQ is disabled by disable_irq_nosync() and that's the
> > > > end of story - no more irqs, no problems. Until userspace writes
> > > > non-zero to the device file, of course.
> > >
> > > If you run into the irq handler, the interrupt was obviously enabled.
> > > Since irq sharing is not supported by this driver, you ALWAYS have to
> > > disable it because it IS your interrupt. Storing the enabled/disabled
> > > status in a variable is not needed at all here. Or am I missing
> > > something?
> >
> > Good point. I agree that having UIO_IRQ_DISABLED flag is redundant.
> > Inside the irq handler we know for sure that irq is enabled. In
> > uio_pdrv_genirq_irqcontrol() this knowledge is not mandatory, we could
> > enable/disable irq unconditionally.
>
> On second thought, we can't call enable_irq()/disable_irq() unconditionally
> because of the potential disable counter (irq_desc->depth) disbalance.
> That's why we need UIO_IRQ_DISABLED flag, and that's why we should check it
> in uio_pdrv_genirq_irqcontrol().
> On the other hand, you are right in that we don't need to check it inside
> irq handler. Inside irq handler we can disable irq and set the flag
> unconditionally, because:
> a) We know for sure that irqs are enabled, because we are inside
> (not-shared) irq handler;
>  and
> b) We are guarded from potential race conditions by spin_lock_irqsave()
> call in uio_pdrv_genirq_irqcontrol().
>
> So,yes, we can get rid of costly atomic call to
> test_and_set_bit(UIO_IRQ_DISABLED,..) inside irq handler. But I still don't
> like the idea of mixing this optimization with bug fixes in a single patch.

On the third thought, we can't ;)
Imagine the SMP system where uio_pdrv_genirq_irqcontrol() is being executed on 
CPU0 and irq handler is running concurrently on CPU1. To protect from 
disable_irq counter disbalance we must first check current irq status, and in 
atomic manner. Thus we prevent  double-disable, one from 
uio_pdrv_genirq_irqcontrol() running on CPU0 and another form irq handler 
running on CPU1.
Above consideration justifies current code.

But it seems we have potential concurrency problem here anyway. Here is 
theoretical scenario:
1) Initial state: irq is enabled, uio_pdrv_genirq_irqcontrol() starts being 
executed on CPU0 with irq_on=1 and at the same time, concurrently, irq 
handler starts being executed on CPU1;
2) irq handler executes line
   if (!test_and_set_bit(UIO_IRQ_DISABLED, &priv->flags))
as irq was enabled, the condition holds. And now UIO_IRQ_DISABLED is set.
3) uio_pdrv_genirq_irqcontrol() executes line
   if (test_and_clear_bit(UIO_IRQ_DISABLED, &priv->flags))
as UIO_IRQ_DISABLED was set by CPU1, the condition holds. And now 
UIO_IRQ_DISABLED is cleared.
4) CPU0 executes enable_irq() . IRQ is enabled. "Unbalanced enable for 
IRQ %d\n" warning is issued.
5) irq handler on CPU1 executes disable_irq_nosync(). IRQ is disabled.

And now we are in situation where IRQ is disabled, but UIO_IRQ_DISABLED is 
cleared. Bad.

The above scenario is purely theoretical, I have no means to check it in 
hardware.
The (theoretical) solution is to guard UIO_IRQ_DISABLED test and actual irq 
disabling inside irq handler by priv->lock.

What do you think about it? Is it worth worrying about?

Re: [PATCH v2] drivers/uio/uio_pdrv_genirq.c: Fix memory freeing issues

[Hans J. Koch]

On Fri, Dec 07, 2012 at 05:00:54PM +0200, Vitalii Demianets wrote:
> >
> > On second thought, we can't call enable_irq()/disable_irq() unconditionally
> > because of the potential disable counter (irq_desc->depth) disbalance.
> > That's why we need UIO_IRQ_DISABLED flag, and that's why we should check it
> > in uio_pdrv_genirq_irqcontrol().
> > On the other hand, you are right in that we don't need to check it inside
> > irq handler. Inside irq handler we can disable irq and set the flag
> > unconditionally, because:
> > a) We know for sure that irqs are enabled, because we are inside
> > (not-shared) irq handler;
> >  and
> > b) We are guarded from potential race conditions by spin_lock_irqsave()
> > call in uio_pdrv_genirq_irqcontrol().
> >
> > So,yes, we can get rid of costly atomic call to
> > test_and_set_bit(UIO_IRQ_DISABLED,..) inside irq handler. But I still don't
> > like the idea of mixing this optimization with bug fixes in a single patch.
> 
> On the third thought, we can't ;)
> Imagine the SMP system where uio_pdrv_genirq_irqcontrol() is being executed on 
> CPU0 and irq handler is running concurrently on CPU1. To protect from 
> disable_irq counter disbalance we must first check current irq status, and in 
> atomic manner. Thus we prevent  double-disable, one from 
> uio_pdrv_genirq_irqcontrol() running on CPU0 and another form irq handler 
> running on CPU1.
> Above consideration justifies current code.
> 
> But it seems we have potential concurrency problem here anyway. Here is 
> theoretical scenario:
> 1) Initial state: irq is enabled, uio_pdrv_genirq_irqcontrol() starts being 
> executed on CPU0 with irq_on=1 and at the same time, concurrently, irq 
> handler starts being executed on CPU1;
> 2) irq handler executes line
>    if (!test_and_set_bit(UIO_IRQ_DISABLED, &priv->flags))
> as irq was enabled, the condition holds. And now UIO_IRQ_DISABLED is set.
> 3) uio_pdrv_genirq_irqcontrol() executes line
>    if (test_and_clear_bit(UIO_IRQ_DISABLED, &priv->flags))
> as UIO_IRQ_DISABLED was set by CPU1, the condition holds. And now 
> UIO_IRQ_DISABLED is cleared.
> 4) CPU0 executes enable_irq() . IRQ is enabled. "Unbalanced enable for 
> IRQ %d\n" warning is issued.
> 5) irq handler on CPU1 executes disable_irq_nosync(). IRQ is disabled.
> 
> And now we are in situation where IRQ is disabled, but UIO_IRQ_DISABLED is 
> cleared. Bad.
> 
> The above scenario is purely theoretical, I have no means to check it in 
> hardware.
> The (theoretical) solution is to guard UIO_IRQ_DISABLED test and actual irq 
> disabling inside irq handler by priv->lock.
> 
> What do you think about it? Is it worth worrying about?

Hi Vitalii,
thanks a lot for analyzing the problem so thoroughly. It made me review
uio_pdrv_genirq.c again, and I noticed several issues and came to the
following conclusions:

1.) priv->lock is completely unnecessary. It is only used in one function,
so there's nothing it could possibly protect.

2.) All these "test_and_clear_bit" and "test_and_set_bit" calls are also
unnecessary. We can simply use enable_irq and disable_irq in both the
irq handler and in uio_pdrv_genirq_irqcontrol.

We should go "back to the roots" and have a look at how UIO works.
The workflow it is intended for is like this:

1.) Hardware is in Reset State (e.g. after boot). Any decent hardware
has its interrupt disabled at that time.

2.) uio_pdrv_genirq is loaded. Kernel enables the irq.

3.) Userspace part of the driver comes up. It will initialize the hardware
(including setting the bits that enable the interrupt).

4.) Userspace will then issue a blocking read() on /dev/uioX. Typically,
there'll be a loop or a thread with this blocking read() at the beginning
(usually using the select() call).

5.) At some time, a hardware interrupt will occur. The irq handler in kernel
space will be called, only to disable the irq. This will also cause the UIO
core to make /dev/uioX readable.

6.) Userspace's blocking read returns. Userspace does its work by
reading/writing device memory.

7.) As the last thing, Userspace writes a "1" to /dev/uioX, which causes
uio_pdrv_genirq_irqcontrol to be called, re-enabling the interrupt.

8.) Goto 4.)

We should also remember that uio_pdrv_genirq_handler() is NOT a real hardware
irq handler. The real handler is in the UIO core, which will increment the
event number and wake up userspace.

So, although your scenario clearly shows a subtle race condition, there is
none. If userspace does stupid things, no harm will be done to the kernel.
If userspace is designed the way described above (and in the documentation),
it will always wake up with its interrupt disabled, do its work, and then
re-enable the interrupt. You can probably think of a few things userspace
could do to screw things up. But that's not our problem.

Could you hack up a patch for that? I think it should start with removing
uio_pdrv_genirq_platdata->lock and uio_pdrv_genirq_platdata->flags...

Thanks again for your work. What do you think about my theory?

Thanks,
Hans

Re: [PATCH v2] drivers/uio/uio_pdrv_genirq.c: Fix memory freeing issues

[Vitalii Demianets]

On Saturday 08 December 2012 01:47:21 Hans J. Koch wrote:
> On Fri, Dec 07, 2012 at 05:00:54PM +0200, Vitalii Demianets wrote:
> > > On second thought, we can't call enable_irq()/disable_irq()
> > > unconditionally because of the potential disable counter
> > > (irq_desc->depth) disbalance. That's why we need UIO_IRQ_DISABLED flag,
> > > and that's why we should check it in uio_pdrv_genirq_irqcontrol().
> > > On the other hand, you are right in that we don't need to check it
> > > inside irq handler. Inside irq handler we can disable irq and set the
> > > flag unconditionally, because:
> > > a) We know for sure that irqs are enabled, because we are inside
> > > (not-shared) irq handler;
> > >  and
> > > b) We are guarded from potential race conditions by spin_lock_irqsave()
> > > call in uio_pdrv_genirq_irqcontrol().
> > >
> > > So,yes, we can get rid of costly atomic call to
> > > test_and_set_bit(UIO_IRQ_DISABLED,..) inside irq handler. But I still
> > > don't like the idea of mixing this optimization with bug fixes in a
> > > single patch.
> >
> > On the third thought, we can't ;)
> > Imagine the SMP system where uio_pdrv_genirq_irqcontrol() is being
> > executed on CPU0 and irq handler is running concurrently on CPU1. To
> > protect from disable_irq counter disbalance we must first check current
> > irq status, and in atomic manner. Thus we prevent  double-disable, one
> > from
> > uio_pdrv_genirq_irqcontrol() running on CPU0 and another form irq handler
> > running on CPU1.
> > Above consideration justifies current code.
> >
> > But it seems we have potential concurrency problem here anyway. Here is
> > theoretical scenario:
> > 1) Initial state: irq is enabled, uio_pdrv_genirq_irqcontrol() starts
> > being executed on CPU0 with irq_on=1 and at the same time, concurrently,
> > irq handler starts being executed on CPU1;
> > 2) irq handler executes line
> >    if (!test_and_set_bit(UIO_IRQ_DISABLED, &priv->flags))
> > as irq was enabled, the condition holds. And now UIO_IRQ_DISABLED is set.
> > 3) uio_pdrv_genirq_irqcontrol() executes line
> >    if (test_and_clear_bit(UIO_IRQ_DISABLED, &priv->flags))
> > as UIO_IRQ_DISABLED was set by CPU1, the condition holds. And now
> > UIO_IRQ_DISABLED is cleared.
> > 4) CPU0 executes enable_irq() . IRQ is enabled. "Unbalanced enable for
> > IRQ %d\n" warning is issued.
> > 5) irq handler on CPU1 executes disable_irq_nosync(). IRQ is disabled.
> >
> > And now we are in situation where IRQ is disabled, but UIO_IRQ_DISABLED
> > is cleared. Bad.
> >
> > The above scenario is purely theoretical, I have no means to check it in
> > hardware.
> > The (theoretical) solution is to guard UIO_IRQ_DISABLED test and actual
> > irq disabling inside irq handler by priv->lock.
> >
> > What do you think about it? Is it worth worrying about?
>
> Hi Vitalii,
> thanks a lot for analyzing the problem so thoroughly. It made me review
> uio_pdrv_genirq.c again, and I noticed several issues and came to the
> following conclusions:
>
> 1.) priv->lock is completely unnecessary. It is only used in one function,
> so there's nothing it could possibly protect.
>
> 2.) All these "test_and_clear_bit" and "test_and_set_bit" calls are also
> unnecessary. We can simply use enable_irq and disable_irq in both the
> irq handler and in uio_pdrv_genirq_irqcontrol.
>
> We should go "back to the roots" and have a look at how UIO works.
> The workflow it is intended for is like this:
>
> 1.) Hardware is in Reset State (e.g. after boot). Any decent hardware
> has its interrupt disabled at that time.
>
> 2.) uio_pdrv_genirq is loaded. Kernel enables the irq.
>
> 3.) Userspace part of the driver comes up. It will initialize the hardware
> (including setting the bits that enable the interrupt).
>
> 4.) Userspace will then issue a blocking read() on /dev/uioX. Typically,
> there'll be a loop or a thread with this blocking read() at the beginning
> (usually using the select() call).
>
> 5.) At some time, a hardware interrupt will occur. The irq handler in
> kernel space will be called, only to disable the irq. This will also cause
> the UIO core to make /dev/uioX readable.
>
> 6.) Userspace's blocking read returns. Userspace does its work by
> reading/writing device memory.
>
> 7.) As the last thing, Userspace writes a "1" to /dev/uioX, which causes
> uio_pdrv_genirq_irqcontrol to be called, re-enabling the interrupt.
>
> 8.) Goto 4.)
>
> We should also remember that uio_pdrv_genirq_handler() is NOT a real
> hardware irq handler. The real handler is in the UIO core, which will
> increment the event number and wake up userspace.
>
> So, although your scenario clearly shows a subtle race condition, there is
> none. If userspace does stupid things, no harm will be done to the kernel.

With all due respect, I disagree here. If userspace does stupid things, 
unintentionally because of poorly written code or intentionally because of a 
malicious purpose, we could have irq depth counter disbalanced which, in 
turn, could lead to the interrupt not enabled when it should be. In which 
case the whole system would stop doing useful things which it was designed to 
do.

> If userspace is designed the way described above (and in the
> documentation), it will always wake up with its interrupt disabled, do its
> work, and then re-enable the interrupt. You can probably think of a few
> things userspace could do to screw things up. But that's not our problem.
>

Disagree again. I think we can not leave a hole in kernel which opens DOS 
possibilities and hope userspace will behave well. There are always errors in 
the code. And security issues, too.

> Could you hack up a patch for that? I think it should start with removing
> uio_pdrv_genirq_platdata->lock and uio_pdrv_genirq_platdata->flags...
>
> Thanks again for your work. What do you think about my theory?
>

I think that we should fix the bug, not ignore it.

Ok, less words, more code. I am posting a new series of two patches. First 
is "Fix memory freeing issues" (with improved wording in the comment which 
you found unclear) and the second is about fixing SMP race condition.

Re: [PATCH v2] drivers/uio/uio_pdrv_genirq.c: Fix memory freeing issues

[Hans J. Koch]

On Mon, Dec 10, 2012 at 11:03:59AM +0200, Vitalii Demianets wrote:
> On Saturday 08 December 2012 01:47:21 Hans J. Koch wrote:
> > On Fri, Dec 07, 2012 at 05:00:54PM +0200, Vitalii Demianets wrote:
> > > > On second thought, we can't call enable_irq()/disable_irq()
> > > > unconditionally because of the potential disable counter
> > > > (irq_desc->depth) disbalance. That's why we need UIO_IRQ_DISABLED flag,
> > > > and that's why we should check it in uio_pdrv_genirq_irqcontrol().
> > > > On the other hand, you are right in that we don't need to check it
> > > > inside irq handler. Inside irq handler we can disable irq and set the
> > > > flag unconditionally, because:
> > > > a) We know for sure that irqs are enabled, because we are inside
> > > > (not-shared) irq handler;
> > > >  and
> > > > b) We are guarded from potential race conditions by spin_lock_irqsave()
> > > > call in uio_pdrv_genirq_irqcontrol().
> > > >
> > > > So,yes, we can get rid of costly atomic call to
> > > > test_and_set_bit(UIO_IRQ_DISABLED,..) inside irq handler. But I still
> > > > don't like the idea of mixing this optimization with bug fixes in a
> > > > single patch.
> > >
> > > On the third thought, we can't ;)
> > > Imagine the SMP system where uio_pdrv_genirq_irqcontrol() is being
> > > executed on CPU0 and irq handler is running concurrently on CPU1. To
> > > protect from disable_irq counter disbalance we must first check current
> > > irq status, and in atomic manner. Thus we prevent  double-disable, one
> > > from
> > > uio_pdrv_genirq_irqcontrol() running on CPU0 and another form irq handler
> > > running on CPU1.
> > > Above consideration justifies current code.
> > >
> > > But it seems we have potential concurrency problem here anyway. Here is
> > > theoretical scenario:
> > > 1) Initial state: irq is enabled, uio_pdrv_genirq_irqcontrol() starts
> > > being executed on CPU0 with irq_on=1 and at the same time, concurrently,
> > > irq handler starts being executed on CPU1;
> > > 2) irq handler executes line
> > >    if (!test_and_set_bit(UIO_IRQ_DISABLED, &priv->flags))
> > > as irq was enabled, the condition holds. And now UIO_IRQ_DISABLED is set.
> > > 3) uio_pdrv_genirq_irqcontrol() executes line
> > >    if (test_and_clear_bit(UIO_IRQ_DISABLED, &priv->flags))
> > > as UIO_IRQ_DISABLED was set by CPU1, the condition holds. And now
> > > UIO_IRQ_DISABLED is cleared.
> > > 4) CPU0 executes enable_irq() . IRQ is enabled. "Unbalanced enable for
> > > IRQ %d\n" warning is issued.
> > > 5) irq handler on CPU1 executes disable_irq_nosync(). IRQ is disabled.
> > >
> > > And now we are in situation where IRQ is disabled, but UIO_IRQ_DISABLED
> > > is cleared. Bad.
> > >
> > > The above scenario is purely theoretical, I have no means to check it in
> > > hardware.
> > > The (theoretical) solution is to guard UIO_IRQ_DISABLED test and actual
> > > irq disabling inside irq handler by priv->lock.
> > >
> > > What do you think about it? Is it worth worrying about?
> >
> > Hi Vitalii,
> > thanks a lot for analyzing the problem so thoroughly. It made me review
> > uio_pdrv_genirq.c again, and I noticed several issues and came to the
> > following conclusions:
> >
> > 1.) priv->lock is completely unnecessary. It is only used in one function,
> > so there's nothing it could possibly protect.
> >
> > 2.) All these "test_and_clear_bit" and "test_and_set_bit" calls are also
> > unnecessary. We can simply use enable_irq and disable_irq in both the
> > irq handler and in uio_pdrv_genirq_irqcontrol.
> >
> > We should go "back to the roots" and have a look at how UIO works.
> > The workflow it is intended for is like this:
> >
> > 1.) Hardware is in Reset State (e.g. after boot). Any decent hardware
> > has its interrupt disabled at that time.
> >
> > 2.) uio_pdrv_genirq is loaded. Kernel enables the irq.
> >
> > 3.) Userspace part of the driver comes up. It will initialize the hardware
> > (including setting the bits that enable the interrupt).
> >
> > 4.) Userspace will then issue a blocking read() on /dev/uioX. Typically,
> > there'll be a loop or a thread with this blocking read() at the beginning
> > (usually using the select() call).
> >
> > 5.) At some time, a hardware interrupt will occur. The irq handler in
> > kernel space will be called, only to disable the irq. This will also cause
> > the UIO core to make /dev/uioX readable.
> >
> > 6.) Userspace's blocking read returns. Userspace does its work by
> > reading/writing device memory.
> >
> > 7.) As the last thing, Userspace writes a "1" to /dev/uioX, which causes
> > uio_pdrv_genirq_irqcontrol to be called, re-enabling the interrupt.
> >
> > 8.) Goto 4.)
> >
> > We should also remember that uio_pdrv_genirq_handler() is NOT a real
> > hardware irq handler. The real handler is in the UIO core, which will
> > increment the event number and wake up userspace.
> >
> > So, although your scenario clearly shows a subtle race condition, there is
> > none. If userspace does stupid things, no harm will be done to the kernel.
> 
> With all due respect, I disagree here. If userspace does stupid things, 
> unintentionally because of poorly written code or intentionally because of a 
> malicious purpose, we could have irq depth counter disbalanced which, in 
> turn, could lead to the interrupt not enabled when it should be.

It's just the interrupt of a broken UIO userspace driver. Note that this is
also a security mechanism, disabling the irq when it is not handled properly.

> In which 
> case the whole system would stop doing useful things which it was designed to 
> do.

If the kernel irq depth counter can't track the irq, I don't really see a
point in fixing it by adding extra flags in a simple driver.

> 
> > If userspace is designed the way described above (and in the
> > documentation), it will always wake up with its interrupt disabled, do its
> > work, and then re-enable the interrupt. You can probably think of a few
> > things userspace could do to screw things up. But that's not our problem.
> >
> 
> Disagree again. I think we can not leave a hole in kernel which opens DOS 
> possibilities and hope userspace will behave well. There are always errors in 
> the code. And security issues, too.

UIO is a risky thing to begin with. There are many more possibilities to
do bad things. That's why UIO devices are root-only.

> 
> > Could you hack up a patch for that? I think it should start with removing
> > uio_pdrv_genirq_platdata->lock and uio_pdrv_genirq_platdata->flags...
> >
> > Thanks again for your work. What do you think about my theory?
> >
> 
> I think that we should fix the bug, not ignore it.

I don't see a bug here.

Thanks,
Hans

> 
> Ok, less words, more code. I am posting a new series of two patches. First 
> is "Fix memory freeing issues" (with improved wording in the comment which 
> you found unclear) and the second is about fixing SMP race condition.
> 

Re: [PATCH v2] drivers/uio/uio_pdrv_genirq.c: Fix memory freeing issues

[Vitalii Demianets]

On Monday 10 December 2012 11:52:18 Hans J. Koch wrote:
> On Mon, Dec 10, 2012 at 11:03:59AM +0200, Vitalii Demianets wrote:
> > On Saturday 08 December 2012 01:47:21 Hans J. Koch wrote:
> > > On Fri, Dec 07, 2012 at 05:00:54PM +0200, Vitalii Demianets wrote:
> > > > > On second thought, we can't call enable_irq()/disable_irq()
> > > > > unconditionally because of the potential disable counter
> > > > > (irq_desc->depth) disbalance. That's why we need UIO_IRQ_DISABLED 
flag,
> > > > > and that's why we should check it in uio_pdrv_genirq_irqcontrol().
> > > > > On the other hand, you are right in that we don't need to check it
> > > > > inside irq handler. Inside irq handler we can disable irq and set 
the
> > > > > flag unconditionally, because:
> > > > > a) We know for sure that irqs are enabled, because we are inside
> > > > > (not-shared) irq handler;
> > > > >  and
> > > > > b) We are guarded from potential race conditions by 
spin_lock_irqsave()
> > > > > call in uio_pdrv_genirq_irqcontrol().
> > > > >
> > > > > So,yes, we can get rid of costly atomic call to
> > > > > test_and_set_bit(UIO_IRQ_DISABLED,..) inside irq handler. But I 
still
> > > > > don't like the idea of mixing this optimization with bug fixes in a
> > > > > single patch.
> > > >
> > > > On the third thought, we can't ;)
> > > > Imagine the SMP system where uio_pdrv_genirq_irqcontrol() is being
> > > > executed on CPU0 and irq handler is running concurrently on CPU1. To
> > > > protect from disable_irq counter disbalance we must first check 
current
> > > > irq status, and in atomic manner. Thus we prevent  double-disable, one
> > > > from
> > > > uio_pdrv_genirq_irqcontrol() running on CPU0 and another form irq 
handler
> > > > running on CPU1.
> > > > Above consideration justifies current code.
> > > >
> > > > But it seems we have potential concurrency problem here anyway. Here 
is
> > > > theoretical scenario:
> > > > 1) Initial state: irq is enabled, uio_pdrv_genirq_irqcontrol() starts
> > > > being executed on CPU0 with irq_on=1 and at the same time, 
concurrently,
> > > > irq handler starts being executed on CPU1;
> > > > 2) irq handler executes line
> > > >    if (!test_and_set_bit(UIO_IRQ_DISABLED, &priv->flags))
> > > > as irq was enabled, the condition holds. And now UIO_IRQ_DISABLED is 
set.
> > > > 3) uio_pdrv_genirq_irqcontrol() executes line
> > > >    if (test_and_clear_bit(UIO_IRQ_DISABLED, &priv->flags))
> > > > as UIO_IRQ_DISABLED was set by CPU1, the condition holds. And now
> > > > UIO_IRQ_DISABLED is cleared.
> > > > 4) CPU0 executes enable_irq() . IRQ is enabled. "Unbalanced enable for
> > > > IRQ %d\n" warning is issued.
> > > > 5) irq handler on CPU1 executes disable_irq_nosync(). IRQ is disabled.
> > > >
> > > > And now we are in situation where IRQ is disabled, but 
UIO_IRQ_DISABLED
> > > > is cleared. Bad.
> > > >
> > > > The above scenario is purely theoretical, I have no means to check it 
in
> > > > hardware.
> > > > The (theoretical) solution is to guard UIO_IRQ_DISABLED test and 
actual
> > > > irq disabling inside irq handler by priv->lock.
> > > >
> > > > What do you think about it? Is it worth worrying about?
> > >
> > > Hi Vitalii,
> > > thanks a lot for analyzing the problem so thoroughly. It made me review
> > > uio_pdrv_genirq.c again, and I noticed several issues and came to the
> > > following conclusions:
> > >
> > > 1.) priv->lock is completely unnecessary. It is only used in one 
function,
> > > so there's nothing it could possibly protect.
> > >
> > > 2.) All these "test_and_clear_bit" and "test_and_set_bit" calls are also
> > > unnecessary. We can simply use enable_irq and disable_irq in both the
> > > irq handler and in uio_pdrv_genirq_irqcontrol.
> > >
> > > We should go "back to the roots" and have a look at how UIO works.
> > > The workflow it is intended for is like this:
> > >
> > > 1.) Hardware is in Reset State (e.g. after boot). Any decent hardware
> > > has its interrupt disabled at that time.
> > >
> > > 2.) uio_pdrv_genirq is loaded. Kernel enables the irq.
> > >
> > > 3.) Userspace part of the driver comes up. It will initialize the 
hardware
> > > (including setting the bits that enable the interrupt).
> > >
> > > 4.) Userspace will then issue a blocking read() on /dev/uioX. Typically,
> > > there'll be a loop or a thread with this blocking read() at the 
beginning
> > > (usually using the select() call).
> > >
> > > 5.) At some time, a hardware interrupt will occur. The irq handler in
> > > kernel space will be called, only to disable the irq. This will also 
cause
> > > the UIO core to make /dev/uioX readable.
> > >
> > > 6.) Userspace's blocking read returns. Userspace does its work by
> > > reading/writing device memory.
> > >
> > > 7.) As the last thing, Userspace writes a "1" to /dev/uioX, which causes
> > > uio_pdrv_genirq_irqcontrol to be called, re-enabling the interrupt.
> > >
> > > 8.) Goto 4.)
> > >
> > > We should also remember that uio_pdrv_genirq_handler() is NOT a real
> > > hardware irq handler. The real handler is in the UIO core, which will
> > > increment the event number and wake up userspace.
> > >
> > > So, although your scenario clearly shows a subtle race condition, there 
is
> > > none. If userspace does stupid things, no harm will be done to the 
kernel.
> > 
> > With all due respect, I disagree here. If userspace does stupid things, 
> > unintentionally because of poorly written code or intentionally because of 
a 
> > malicious purpose, we could have irq depth counter disbalanced which, in 
> > turn, could lead to the interrupt not enabled when it should be.
> 
> It's just the interrupt of a broken UIO userspace driver. Note that this is
> also a security mechanism, disabling the irq when it is not handled 
properly.
> 
> > In which 
> > case the whole system would stop doing useful things which it was designed 
to 
> > do.
> 
> If the kernel irq depth counter can't track the irq, I don't really see a
> point in fixing it by adding extra flags in a simple driver.
> 

The flag in driver would be needed if we supposed that userspace can do stupid 
things. For example, userspace can call write() twice with zero data. Which 
would lead to disabling irq twice. Which means that someone would need to 
enable irq twice. Which is not in the userspace-kernel interaction scenario 
you so clearly described above previously.

Also, without that flag the irq could be disabled twice even when userspace do 
not do double-disable. It could happen when userspace is writing to the 
device file AND irq is running at this same moment on another CPU core 
concurrently.
Yes, you are right that such a situation could be avoided if userspace code 
was written carefully. But how can you assure that every user will write good 
code?

> > 
> > > If userspace is designed the way described above (and in the
> > > documentation), it will always wake up with its interrupt disabled, do 
its
> > > work, and then re-enable the interrupt. You can probably think of a few
> > > things userspace could do to screw things up. But that's not our 
problem.
> > >
> > 
> > Disagree again. I think we can not leave a hole in kernel which opens DOS 
> > possibilities and hope userspace will behave well. There are always errors 
in 
> > the code. And security issues, too.
> 
> UIO is a risky thing to begin with. There are many more possibilities to
> do bad things. That's why UIO devices are root-only.
> 

We can assume that code running under root does not do bad things on purpose. 
But that doesn't mean that code running under root is bug-free.

> > 
> > > Could you hack up a patch for that? I think it should start with 
removing
> > > uio_pdrv_genirq_platdata->lock and uio_pdrv_genirq_platdata->flags...
> > >
> > > Thanks again for your work. What do you think about my theory?
> > >
> > 
> > I think that we should fix the bug, not ignore it.
> 
> I don't see a bug here.
> 

There is no bug as long as you trust userspace code.
Personally I don't trust it. I don't trust even my own code. There were plenty 
of times when I looked at my code ant thought: "Oh gosh, how could I be so 
dumb!"
And kernel should behave properly no matter what stupid things userspace does, 
shouldn't it? Isn't it one of the reasons why we have distinction between 
kernel and user space at all? Because kernel is by definition trusted code, 
and userspace is not.

Re: [PATCH v2] drivers/uio/uio_pdrv_genirq.c: Fix memory freeing issues

[Hans J. Koch]

On Mon, Dec 10, 2012 at 12:24:04PM +0200, Vitalii Demianets wrote:
> > > > Hi Vitalii,
> > > > thanks a lot for analyzing the problem so thoroughly. It made me review
> > > > uio_pdrv_genirq.c again, and I noticed several issues and came to the
> > > > following conclusions:
> > > >
> > > > 1.) priv->lock is completely unnecessary. It is only used in one 
> > > >function,
> > > > so there's nothing it could possibly protect.
> > > >
> > > > 2.) All these "test_and_clear_bit" and "test_and_set_bit" calls are also
> > > > unnecessary. We can simply use enable_irq and disable_irq in both the
> > > > irq handler and in uio_pdrv_genirq_irqcontrol.
> > > >
> > > > We should go "back to the roots" and have a look at how UIO works.
> > > > The workflow it is intended for is like this:
> > > >
> > > > 1.) Hardware is in Reset State (e.g. after boot). Any decent hardware
> > > > has its interrupt disabled at that time.
> > > >
> > > > 2.) uio_pdrv_genirq is loaded. Kernel enables the irq.
> > > >
> > > > 3.) Userspace part of the driver comes up. It will initialize the 
> > > > hardware
> > > > (including setting the bits that enable the interrupt).
> > > >
> > > > 4.) Userspace will then issue a blocking read() on /dev/uioX. Typically,
> > > > there'll be a loop or a thread with this blocking read() at the 
> > > > beginning
> > > > (usually using the select() call).
> > > >
> > > > 5.) At some time, a hardware interrupt will occur. The irq handler in
> > > > kernel space will be called, only to disable the irq. This will also 
> > > > cause the UIO core to make /dev/uioX readable.
> > > >
> > > > 6.) Userspace's blocking read returns. Userspace does its work by
> > > > reading/writing device memory.
> > > >
> > > > 7.) As the last thing, Userspace writes a "1" to /dev/uioX, which causes
> > > > uio_pdrv_genirq_irqcontrol to be called, re-enabling the interrupt.
> > > >
> > > > 8.) Goto 4.)
> > > >
> > > > We should also remember that uio_pdrv_genirq_handler() is NOT a real
> > > > hardware irq handler. The real handler is in the UIO core, which will
> > > > increment the event number and wake up userspace.
> > > >
> > > > So, although your scenario clearly shows a subtle race condition, there 
> > > > is none. If userspace does stupid things, no harm will be done to the 
> > > > kernel.
> > > 
> > > With all due respect, I disagree here. If userspace does stupid things, 
> > > unintentionally because of poorly written code or intentionally because of 
> a 
> > > malicious purpose, we could have irq depth counter disbalanced which, in 
> > > turn, could lead to the interrupt not enabled when it should be.
> > 
> > It's just the interrupt of a broken UIO userspace driver. Note that this is
> > also a security mechanism, disabling the irq when it is not handled 
> > properly.
> > 
> > > In which 
> > > case the whole system would stop doing useful things which it was designed 
> > > to do.
> > 
> > If the kernel irq depth counter can't track the irq, I don't really see a
> > point in fixing it by adding extra flags in a simple driver.
> > 
> 
> The flag in driver would be needed if we supposed that userspace can do stupid 
> things. For example, userspace can call write() twice with zero data. Which 
> would lead to disabling irq twice. Which means that someone would need to 
> enable irq twice. Which is not in the userspace-kernel interaction scenario 
> you so clearly described above previously.

Indeed. It means that somebody wrote bad userspace code, and it needs to be
fixed there.

> 
> Also, without that flag the irq could be disabled twice even when userspace do 
> not do double-disable. It could happen when userspace is writing to the 
> device file AND irq is running at this same moment on another CPU core 
> concurrently.

If that happens, then either userspace is broken or the device cannot be
handled by a UIO driver (e.g. because irq rate is too high which is usually
an indicator for bad hardware design).

> Yes, you are right that such a situation could be avoided if userspace code 
> was written carefully. But how can you assure that every user will write good 
> code?

I don't care at all. I know the quality of code written in industry too well.
They'll write bad code. All we can do is prevent damage to others.

> 
> > > 
> > > > If userspace is designed the way described above (and in the
> > > > documentation), it will always wake up with its interrupt disabled, do 
> > > > its work, and then re-enable the interrupt. You can probably think of
> > > > a few things userspace could do to screw things up. But that's not our 
> > > > problem.
> > > >
> > > 
> > > Disagree again. I think we can not leave a hole in kernel which opens DOS 
> > > possibilities and hope userspace will behave well. There are always errors  
> > > in the code. And security issues, too.
> > 
> > UIO is a risky thing to begin with. There are many more possibilities to
> > do bad things. That's why UIO devices are root-only.
> > 
> 
> We can assume that code running under root does not do bad things on purpose.

If that was true, there would be no security problems in the world. Making
UIO devices root-only is only a prerequisite for constructing a safe system.
BTW, there are more safety-critical devices in a typical Linux system,
e.g. /dev/mem.

> But that doesn't mean that code running under root is bug-free.

Is that so? Then root has to fix his/her code, like anyone else.

> 
> > > 
> > > > Could you hack up a patch for that? I think it should start with 
> > > > removing
> > > > uio_pdrv_genirq_platdata->lock and uio_pdrv_genirq_platdata->flags...
> > > >
> > > > Thanks again for your work. What do you think about my theory?
> > > >
> > > 
> > > I think that we should fix the bug, not ignore it.
> > 
> > I don't see a bug here.
> > 
> 
> There is no bug as long as you trust userspace code.

I don't.

> Personally I don't trust it. I don't trust even my own code. There were plenty 
> of times when I looked at my code ant thought: "Oh gosh, how could I be so 
> dumb!"

Same here.

> And kernel should behave properly no matter what stupid things userspace does, 
> shouldn't it?

Exactly. That's what it's all about.

> Isn't it one of the reasons why we have distinction between 
> kernel and user space at all? Because kernel is by definition trusted code, 
> and userspace is not.

Yes. And with what I have suggested, we are in the position that userspace
might create a situation in which it won't work anymore. That is alright
for me, because it is its own fault. The kernel has to provide an interface
that allows a proper userspace to be written. But it's not our task to fix
or prevent userspace problems in the kernel. We have to do our best that
buggy userspace cannot harm the rest of the kernel or other processes. But
if some crap userspace code doesn't work or causes a few error messages in
the logs, I still sleep very well.

Thanks,
Hans

Re: [PATCH v2] drivers/uio/uio_pdrv_genirq.c: Fix memory freeing issues

[Vitalii Demianets]

On Tuesday 11 December 2012 00:37:59 Hans J. Koch wrote:
> On Mon, Dec 10, 2012 at 12:24:04PM +0200, Vitalii Demianets wrote:
> > > > > Hi Vitalii,
> > > > > thanks a lot for analyzing the problem so thoroughly. It made me
> > > > > review uio_pdrv_genirq.c again, and I noticed several issues and
> > > > > came to the following conclusions:
> > > > >
> > > > > 1.) priv->lock is completely unnecessary. It is only used in one
> > > > >function,
> > > > > so there's nothing it could possibly protect.
> > > > >
> > > > > 2.) All these "test_and_clear_bit" and "test_and_set_bit" calls are
> > > > > also unnecessary. We can simply use enable_irq and disable_irq in
> > > > > both the irq handler and in uio_pdrv_genirq_irqcontrol.
> > > > >
> > > > > We should go "back to the roots" and have a look at how UIO works.
> > > > > The workflow it is intended for is like this:
> > > > >
> > > > > 1.) Hardware is in Reset State (e.g. after boot). Any decent
> > > > > hardware has its interrupt disabled at that time.
> > > > >
> > > > > 2.) uio_pdrv_genirq is loaded. Kernel enables the irq.
> > > > >
> > > > > 3.) Userspace part of the driver comes up. It will initialize the
> > > > > hardware
> > > > > (including setting the bits that enable the interrupt).
> > > > >
> > > > > 4.) Userspace will then issue a blocking read() on /dev/uioX.
> > > > > Typically, there'll be a loop or a thread with this blocking read()
> > > > > at the beginning
> > > > > (usually using the select() call).
> > > > >
> > > > > 5.) At some time, a hardware interrupt will occur. The irq handler
> > > > > in kernel space will be called, only to disable the irq. This will
> > > > > also cause the UIO core to make /dev/uioX readable.
> > > > >
> > > > > 6.) Userspace's blocking read returns. Userspace does its work by
> > > > > reading/writing device memory.
> > > > >
> > > > > 7.) As the last thing, Userspace writes a "1" to /dev/uioX, which
> > > > > causes uio_pdrv_genirq_irqcontrol to be called, re-enabling the
> > > > > interrupt.
> > > > >
> > > > > 8.) Goto 4.)
> > > > >
> > > > > We should also remember that uio_pdrv_genirq_handler() is NOT a
> > > > > real hardware irq handler. The real handler is in the UIO core,
> > > > > which will increment the event number and wake up userspace.
> > > > >
> > > > > So, although your scenario clearly shows a subtle race condition,
> > > > > there is none. If userspace does stupid things, no harm will be
> > > > > done to the kernel.
> > > >
> > > > With all due respect, I disagree here. If userspace does stupid
> > > > things, unintentionally because of poorly written code or
> > > > intentionally because of
> >
> > a
> >
> > > > malicious purpose, we could have irq depth counter disbalanced which,
> > > > in turn, could lead to the interrupt not enabled when it should be.
> > >
> > > It's just the interrupt of a broken UIO userspace driver. Note that
> > > this is also a security mechanism, disabling the irq when it is not
> > > handled properly.
> > >
> > > > In which
> > > > case the whole system would stop doing useful things which it was
> > > > designed to do.
> > >
> > > If the kernel irq depth counter can't track the irq, I don't really see
> > > a point in fixing it by adding extra flags in a simple driver.
> >
> > The flag in driver would be needed if we supposed that userspace can do
> > stupid things. For example, userspace can call write() twice with zero
> > data. Which would lead to disabling irq twice. Which means that someone
> > would need to enable irq twice. Which is not in the userspace-kernel
> > interaction scenario you so clearly described above previously.
>
> Indeed. It means that somebody wrote bad userspace code, and it needs to be
> fixed there.
>
> > Also, without that flag the irq could be disabled twice even when
> > userspace do not do double-disable. It could happen when userspace is
> > writing to the device file AND irq is running at this same moment on
> > another CPU core concurrently.
>
> If that happens, then either userspace is broken or the device cannot be
> handled by a UIO driver (e.g. because irq rate is too high which is usually
> an indicator for bad hardware design).
>
> > Yes, you are right that such a situation could be avoided if userspace
> > code was written carefully. But how can you assure that every user will
> > write good code?
>
> I don't care at all. I know the quality of code written in industry too
> well. They'll write bad code. All we can do is prevent damage to others.
>
> > > > > If userspace is designed the way described above (and in the
> > > > > documentation), it will always wake up with its interrupt disabled,
> > > > > do its work, and then re-enable the interrupt. You can probably
> > > > > think of a few things userspace could do to screw things up. But
> > > > > that's not our problem.
> > > >
> > > > Disagree again. I think we can not leave a hole in kernel which opens
> > > > DOS possibilities and hope userspace will behave well. There are
> > > > always errors in the code. And security issues, too.
> > >
> > > UIO is a risky thing to begin with. There are many more possibilities
> > > to do bad things. That's why UIO devices are root-only.
> >
> > We can assume that code running under root does not do bad things on
> > purpose.
>
> If that was true, there would be no security problems in the world. Making
> UIO devices root-only is only a prerequisite for constructing a safe
> system. BTW, there are more safety-critical devices in a typical Linux
> system, e.g. /dev/mem.
>
> > But that doesn't mean that code running under root is bug-free.
>
> Is that so? Then root has to fix his/her code, like anyone else.
>
> > > > > Could you hack up a patch for that? I think it should start with
> > > > > removing
> > > > > uio_pdrv_genirq_platdata->lock and
> > > > > uio_pdrv_genirq_platdata->flags...
> > > > >
> > > > > Thanks again for your work. What do you think about my theory?
> > > >
> > > > I think that we should fix the bug, not ignore it.
> > >
> > > I don't see a bug here.
> >
> > There is no bug as long as you trust userspace code.
>
> I don't.
>
> > Personally I don't trust it. I don't trust even my own code. There were
> > plenty of times when I looked at my code ant thought: "Oh gosh, how could
> > I be so dumb!"
>
> Same here.
>
> > And kernel should behave properly no matter what stupid things userspace
> > does, shouldn't it?
>
> Exactly. That's what it's all about.
>
> > Isn't it one of the reasons why we have distinction between
> > kernel and user space at all? Because kernel is by definition trusted
> > code, and userspace is not.
>
> Yes. And with what I have suggested, we are in the position that userspace
> might create a situation in which it won't work anymore. That is alright
> for me, because it is its own fault. The kernel has to provide an interface
> that allows a proper userspace to be written. But it's not our task to fix
> or prevent userspace problems in the kernel. We have to do our best that
> buggy userspace cannot harm the rest of the kernel or other processes. But
> if some crap userspace code doesn't work or causes a few error messages in
> the logs, I still sleep very well.
>

I understand your position. I simply don't agree with it. I can't sleep well 
knowing that there is a potential concurrency issue that could be fixed, but 
wasn't. Even if it harms nobody else except the (userspace) driver which 
caused the issue in the first place.
At least now this discussion and patch for the issue sits in the LKML archive 
so that anybody can find it in case of necessity.
So, let's agree to disagree.

Please, review the v3 of "Fix memory freeing issues" patch (first in the 
series I posted yesterday) and ignore the second, as we haven't agreed 
on it.

Re: [PATCH v2] drivers/uio/uio_pdrv_genirq.c: Fix memory freeing issues

[Hans J. Koch]

On Tue, Dec 11, 2012 at 12:47:35PM +0200, Vitalii Demianets wrote:
> 
> Please, review the v3 of "Fix memory freeing issues" patch (first in the 
> series I posted yesterday) and ignore the second, as we haven't agreed 
> on it.
> 

I can't find a v3. Please resend it.

Thanks,
Hans

Re: [PATCH v2] drivers/uio/uio_pdrv_genirq.c: Fix memory freeing issues

[Vitalii Demianets]

On Thursday 13 December 2012 19:11:09 Hans J. Koch wrote:
> On Tue, Dec 11, 2012 at 12:47:35PM +0200, Vitalii Demianets wrote:
> > Please, review the v3 of "Fix memory freeing issues" patch (first in the
> > series I posted yesterday) and ignore the second, as we haven't agreed
> > on it.
>
> I can't find a v3. Please resend it.
>

I've posted v3 as a [PATCH 1/2 v3] in series:
http://www.spinics.net/lists/kernel/msg1450101.html

You were CC-ed. If for some reason you didn't get it in your mailbox, I'll 
resend.

Re: [PATCH v2] drivers/uio/uio_pdrv_genirq.c: Fix memory freeing issues

[Hans J. Koch]

On Thu, Dec 13, 2012 at 07:23:21PM +0200, Vitalii Demianets wrote:
> On Thursday 13 December 2012 19:11:09 Hans J. Koch wrote:
> > On Tue, Dec 11, 2012 at 12:47:35PM +0200, Vitalii Demianets wrote:
> > > Please, review the v3 of "Fix memory freeing issues" patch (first in the
> > > series I posted yesterday) and ignore the second, as we haven't agreed
> > > on it.
> >
> > I can't find a v3. Please resend it.
> >
> 
> I've posted v3 as a [PATCH 1/2 v3] in series:
> http://www.spinics.net/lists/kernel/msg1450101.html
> 
> You were CC-ed. If for some reason you didn't get it in your mailbox, I'll 
> resend.

OK, found it, sorry. I ignored that one because it does the same flag-testing
stuff. It is unnecessary and only tries to fix userspace stupidity in the
kernel. I won't buy that one, and I already gave an explanation why.
I won't take it just because you disagree with my opinion.

Thanks,
Hans

Re: [PATCH v2] drivers/uio/uio_pdrv_genirq.c: Fix memory freeing issues

[Vitalii Demianets]

On Thursday 13 December 2012 19:34:00 Hans J. Koch wrote:
> On Thu, Dec 13, 2012 at 07:23:21PM +0200, Vitalii Demianets wrote:
> > On Thursday 13 December 2012 19:11:09 Hans J. Koch wrote:
> > > On Tue, Dec 11, 2012 at 12:47:35PM +0200, Vitalii Demianets wrote:
> > > > Please, review the v3 of "Fix memory freeing issues" patch (first in
> > > > the series I posted yesterday) and ignore the second, as we haven't
> > > > agreed on it.
> > >
> > > I can't find a v3. Please resend it.
> >
> > I've posted v3 as a [PATCH 1/2 v3] in series:
> > http://www.spinics.net/lists/kernel/msg1450101.html
> >
> > You were CC-ed. If for some reason you didn't get it in your mailbox,
> > I'll resend.
>
> OK, found it, sorry. I ignored that one because it does the same
> flag-testing stuff. It is unnecessary and only tries to fix userspace
> stupidity in the kernel. I won't buy that one, and I already gave an
> explanation why. I won't take it just because you disagree with my opinion.
>

Hans, it keeps flag-testing code in place and does not change it. It keeps 
previous behaviour. I divided the patch in two parts and posted it in series 
specifically for that purpose, first patch in series does only memory-related 
stuff.
It happens so that it needs another flag (UIO_INFO_ALLOCED) to do that 
memory-related stuff well. That's the only reason I gave the name to the 
already existing previously unnamed flag (bit 0, now UIO_IRQ_DISABLED).
Again, the "Fix memory freeing issues" patch fixes only what it says on the 
tin: memory freeing issues. All flag-manipulation changes belong to the patch 
2/2 in the series, which you could soundly reject, because we disagree on 
that matter.

Keeping this flag-testing stuff as it was doesn't do any harm. Are you saying 
that you reject memory-related patch only because it doesn't change something 
else, totally unrelated?