[PATCH] coccinelle: api: check for propagation of error from platform_get_irq

[PATCH] coccinelle: api: check for propagation of error from platform_get_irq

[Julia Lawall]

The error return value of platform_get_irq seems to often get dropped.

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
 scripts/coccinelle/api/platform_get_irq_return.cocci |   53 ++++++++++
 1 file changed, 53 insertions(+)

diff --git a/scripts/coccinelle/api/platform_get_irq_return.cocci b/scripts/coccinelle/api/platform_get_irq_return.cocci
new file mode 100644
index 0000000..96fc560
--- /dev/null
+++ b/scripts/coccinelle/api/platform_get_irq_return.cocci
@@ -0,0 +1,53 @@
+/// Propagate the return value of platform_get_irq.
+//# Sometimes the return value of platform_get_irq is tested using <= 0, but 0
+//# might not be an appropriate return value in an error case.
+///
+// Confidence: Moderate
+// Copyright: (C) 2015 Julia Lawall, Inria. GPLv2.
+// URL: http://coccinelle.lip6.fr/
+// Options: --no-includes --include-headers
+
+virtual context
+virtual org
+virtual report
+
+// ----------------------------------------------------------------------------
+
+@r depends on context || org || report@
+constant C;
+expression e, ret;
+position j0, j1, j2;
+@@
+
+* e@j0 = platform_get_irq(...);
+if (...) {
+  ...
+  ret@j1 = -C;
+  ...
+  return ret@j2;
+}
+
+// ----------------------------------------------------------------------------
+
+@script:python r_org depends on org@
+j0 << r.j0;
+j1 << r.j1;
+j2 << r.j2;
+@@
+
+msg = "Propagate return value of platform_get_irq."
+coccilib.org.print_todo(j0[0], msg)
+coccilib.org.print_link(j1[0], "")
+coccilib.org.print_link(j2[0], "")
+
+// ----------------------------------------------------------------------------
+
+@script:python r_report depends on report@
+j0 << r.j0;
+j1 << r.j1;
+j2 << r.j2;
+@@
+
+msg = "Propagate return value of platform_get_irq around lines %s,%s." % (j1[0].line,j2[0].line)
+coccilib.report.print_report(j0[0], msg)
+

Re: [PATCH] coccinelle: api: check for propagation of error from platform_get_irq

[Sergei Shtylyov]

On 12/26/2015 10:24 PM, Julia Lawall wrote:

> The error return value of platform_get_irq seems to often get dropped.

    Maybe it was intentional (see my recent follow-up to your netdev patch). 
But if an error gets unconditionally overriden, deferred probing can't work. 
So your patches seem good things in the end. Thank you!

> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

[...]

MBR, Sergei

Re: [PATCH] coccinelle: api: check for propagation of error from platform_get_irq

[Julia Lawall]

On Sat, 26 Dec 2015, Sergei Shtylyov wrote:

> On 12/26/2015 10:24 PM, Julia Lawall wrote:
> 
> > The error return value of platform_get_irq seems to often get dropped.
> 
>    Maybe it was intentional (see my recent follow-up to your netdev patch).
> But if an error gets unconditionally overriden, deferred probing can't work.
> So your patches seem good things in the end. Thank you!

Actually, this semantic patch overloos the case where a constant is 
returned directly, which is apparently also popular.  I can update this if 
the whole concept is useful, but that's not clear at the moment...

julia

Re: [PATCH] coccinelle: api: check for propagation of error from platform_get_irq

[Julia Lawall]

The conclusion seems to be that it is useful to override the value, so we 
can just drop this semantic patch.

julia

Re: [PATCH] coccinelle: api: check for propagation of error from platform_get_irq

[Sergei Shtylyov]

On 12/26/2015 11:24 PM, Julia Lawall wrote:

> The conclusion seems to be that it is useful to override the value, so we
> can just drop this semantic patch.

    No! As I said, unconditionally overriding an error value breaks the 
deferred probing. It's actually a bug to override it, so the patch seems 
*very* useful.

> julia

MBR, Sergei

Re: [PATCH] coccinelle: api: check for propagation of error from platform_get_irq

[Julia Lawall]

On Sat, 26 Dec 2015, Sergei Shtylyov wrote:

> On 12/26/2015 11:24 PM, Julia Lawall wrote:
> 
> > The conclusion seems to be that it is useful to override the value, so we
> > can just drop this semantic patch.
> 
>    No! As I said, unconditionally overriding an error value breaks the
> deferred probing. It's actually a bug to override it, so the patch seems
> *very* useful.

Ah, OK, I'll send an improved version then :)

julia

[PATCH v2] coccinelle: api: check for propagation of error from platform_get_irq

[Julia Lawall]

The error return value of platform_get_irq seems to often get dropped.

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---

v2: Check for the direct return case also.  Added some mailing lists of
common offenders.

diff --git a/scripts/coccinelle/api/platform_get_irq_return.cocci b/scripts/coccinelle/api/platform_get_irq_return.cocci
new file mode 100644
index 0000000..44680d0
--- /dev/null
+++ b/scripts/coccinelle/api/platform_get_irq_return.cocci
@@ -0,0 +1,58 @@
+/// Propagate the return value of platform_get_irq.
+//# Sometimes the return value of platform_get_irq is tested using <= 0, but 0
+//# might not be an appropriate return value in an error case.
+///
+// Confidence: Moderate
+// Copyright: (C) 2015 Julia Lawall, Inria. GPLv2.
+// URL: http://coccinelle.lip6.fr/
+// Options: --no-includes --include-headers
+
+virtual context
+virtual org
+virtual report
+
+// ----------------------------------------------------------------------------
+
+@r depends on context || org || report@
+constant C;
+statement S;
+expression e, ret;
+position j0, j1;
+@@
+
+* e@j0 = platform_get_irq(...);
+(
+if@j1 (...) {
+  ...
+  return -C;
+} else S
+|
+if@j1 (...) {
+  ...
+  ret = -C;
+  ...
+  return ret;
+} else S
+)
+
+// ----------------------------------------------------------------------------
+
+@script:python r_org depends on org@
+j0 << r.j0;
+j1 << r.j1;
+@@
+
+msg = "Propagate return value of platform_get_irq."
+coccilib.org.print_todo(j0[0], msg)
+coccilib.org.print_link(j1[0], "")
+
+// ----------------------------------------------------------------------------
+
+@script:python r_report depends on report@
+j0 << r.j0;
+j1 << r.j1;
+@@
+
+msg = "Propagate return value of platform_get_irq around line %s." % (j1[0].line)
+coccilib.report.print_report(j0[0], msg)
+

Re: [PATCH v2] coccinelle: api: check for propagation of error from platform_get_irq

[Sergei Shtylyov]

On 12/26/2015 11:58 PM, Julia Lawall wrote:

> The error return value of platform_get_irq seems to often get dropped.
>
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
>
> ---
>
> v2: Check for the direct return case also.  Added some mailing lists of
> common offenders.
>
> diff --git a/scripts/coccinelle/api/platform_get_irq_return.cocci b/scripts/coccinelle/api/platform_get_irq_return.cocci
> new file mode 100644
> index 0000000..44680d0
> --- /dev/null
> +++ b/scripts/coccinelle/api/platform_get_irq_return.cocci
> @@ -0,0 +1,58 @@
> +/// Propagate the return value of platform_get_irq.
> +//# Sometimes the return value of platform_get_irq is tested using <= 0, but 0
> +//# might not be an appropriate return value in an error case.
> +///
> +// Confidence: Moderate
> +// Copyright: (C) 2015 Julia Lawall, Inria. GPLv2.
> +// URL: http://coccinelle.lip6.fr/
> +// Options: --no-includes --include-headers
> +
> +virtual context
> +virtual org
> +virtual report
> +
> +// ----------------------------------------------------------------------------
> +
> +@r depends on context || org || report@
> +constant C;
> +statement S;
> +expression e, ret;
> +position j0, j1;
> +@@
> +
> +* e@j0 = platform_get_irq(...);
> +(
> +if@j1 (...) {
> +  ...
> +  return -C;
> +} else S
> +|
> +if@j1 (...) {
> +  ...
> +  ret = -C;
> +  ...
> +  return ret;
> +} else S

    Well, this seems to also cover the (e <= 0) checks which do make same 
sense in the light of Linus considering IRQ0 invalid. So I'd be more specific 
about the checks here -- 0 should indeed be overridden with something if it's 
considered invalid.

MBR, Sergei

Re: [PATCH v2] coccinelle: api: check for propagation of error from platform_get_irq

[Julia Lawall]

On Sun, 27 Dec 2015, Sergei Shtylyov wrote:

> On 12/26/2015 11:58 PM, Julia Lawall wrote:
> 
> > The error return value of platform_get_irq seems to often get dropped.
> > 
> > Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
> > 
> > ---
> > 
> > v2: Check for the direct return case also.  Added some mailing lists of
> > common offenders.
> > 
> > diff --git a/scripts/coccinelle/api/platform_get_irq_return.cocci
> > b/scripts/coccinelle/api/platform_get_irq_return.cocci
> > new file mode 100644
> > index 0000000..44680d0
> > --- /dev/null
> > +++ b/scripts/coccinelle/api/platform_get_irq_return.cocci
> > @@ -0,0 +1,58 @@
> > +/// Propagate the return value of platform_get_irq.
> > +//# Sometimes the return value of platform_get_irq is tested using <= 0,
> > but 0
> > +//# might not be an appropriate return value in an error case.
> > +///
> > +// Confidence: Moderate
> > +// Copyright: (C) 2015 Julia Lawall, Inria. GPLv2.
> > +// URL: http://coccinelle.lip6.fr/
> > +// Options: --no-includes --include-headers
> > +
> > +virtual context
> > +virtual org
> > +virtual report
> > +
> > +//
> > ----------------------------------------------------------------------------
> > +
> > +@r depends on context || org || report@
> > +constant C;
> > +statement S;
> > +expression e, ret;
> > +position j0, j1;
> > +@@
> > +
> > +* e@j0 = platform_get_irq(...);
> > +(
> > +if@j1 (...) {
> > +  ...
> > +  return -C;
> > +} else S
> > +|
> > +if@j1 (...) {
> > +  ...
> > +  ret = -C;
> > +  ...
> > +  return ret;
> > +} else S
> 
>    Well, this seems to also cover the (e <= 0) checks which do make same sense
> in the light of Linus considering IRQ0 invalid. So I'd be more specific about
> the checks here -- 0 should indeed be overridden with something if it's
> considered invalid.

That's what the limitations section says (lines with #).  This doesn't 
make any changes, it only makes warnings, which should include the 
limitations information, so perhaps people can consider what it is that 
they really intend to do.

If you think this is not a good idea, then I can make the test more 
specific.

julia

Re: [PATCH v2] coccinelle: api: check for propagation of error from platform_get_irq

[Sergei Shtylyov]

On 12/27/2015 01:32 AM, Julia Lawall wrote:

>>> The error return value of platform_get_irq seems to often get dropped.
>>>
>>> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
>>>
>>> ---
>>>
>>> v2: Check for the direct return case also.  Added some mailing lists of
>>> common offenders.
>>>
>>> diff --git a/scripts/coccinelle/api/platform_get_irq_return.cocci
>>> b/scripts/coccinelle/api/platform_get_irq_return.cocci
>>> new file mode 100644
>>> index 0000000..44680d0
>>> --- /dev/null
>>> +++ b/scripts/coccinelle/api/platform_get_irq_return.cocci
>>> @@ -0,0 +1,58 @@
>>> +/// Propagate the return value of platform_get_irq.
>>> +//# Sometimes the return value of platform_get_irq is tested using <= 0,
>>> but 0
>>> +//# might not be an appropriate return value in an error case.
>>> +///
>>> +// Confidence: Moderate
>>> +// Copyright: (C) 2015 Julia Lawall, Inria. GPLv2.
>>> +// URL: http://coccinelle.lip6.fr/
>>> +// Options: --no-includes --include-headers
>>> +
>>> +virtual context
>>> +virtual org
>>> +virtual report
>>> +
>>> +//
>>> ----------------------------------------------------------------------------
>>> +
>>> +@r depends on context || org || report@
>>> +constant C;
>>> +statement S;
>>> +expression e, ret;
>>> +position j0, j1;
>>> +@@
>>> +
>>> +* e@j0 = platform_get_irq(...);
>>> +(
>>> +if@j1 (...) {
>>> +  ...
>>> +  return -C;
>>> +} else S
>>> +|
>>> +if@j1 (...) {
>>> +  ...
>>> +  ret = -C;
>>> +  ...
>>> +  return ret;
>>> +} else S
>>
>>     Well, this seems to also cover the (e <= 0) checks which do make same sense
>> in the light of Linus considering IRQ0 invalid. So I'd be more specific about
>> the checks here -- 0 should indeed be overridden with something if it's
>> considered invalid.
>
> That's what the limitations section says (lines with #).  This doesn't

    Ah, failed to notice those, only saw after replying.

> make any changes, it only makes warnings, which should include the
> limitations information, so perhaps people can consider what it is that
> they really intend to do.
 >
> If you think this is not a good idea, then I can make the test more
> specific.

    Well, looking again, the patch should be good. I just thought its goal was 
to fix the code as well...

> julia

MBR, Sergei

Re: [PATCH v2] coccinelle: api: check for propagation of error from platform_get_irq

[Julia Lawall]

>    Well, looking again, the patch should be good. I just thought its goal was
> to fix the code as well...

I could do that for the irq < 0 case, but I think that in that case, kbuild
will only run the patch version, and the <= cases will not be reported on.
I don't have a general fix for the <= 0.  Is it even correct to have < in
some cases and <= in others?

julia

Re: [PATCH v2] coccinelle: api: check for propagation of error from platform_get_irq

[Sergei Shtylyov]

On 12/27/2015 9:13 AM, Julia Lawall wrote:

>>     Well, looking again, the patch should be good. I just thought its goal was
>> to fix the code as well...
>
> I could do that for the irq < 0 case, but I think that in that case, kbuild
> will only run the patch version, and the <= cases will not be reported on.
> I don't have a general fix for the <= 0.  Is it even correct to have < in
> some cases and <= in others?

    That's a good question...
    In my prior fixes of this case I preferred to consider IRQ0 valid and so 
used 'irq < 0'. I myself don't share the "IRQ0 is invalid" sentiment...

> julia

MBR, Sergei

Re: [Cocci] [PATCH v2] coccinelle: api: check for propagation of error from platform_get_irq

[SF Markus Elfring]

> The error return value of platform_get_irq seems to often get dropped.

How do you think about any more fine-tuning here?

Commit message:
* … of the platform_get_irq() function seems to get dropped too often.

* Why do you concentrate on a single function name?
  Do you plan to extend this source code analysis approach?


> +@script:python r_report depends on report@
> +j0 << r.j0;
> +j1 << r.j1;
> +@@
> +
> +msg = "Propagate return value of platform_get_irq around line %s." % (j1[0].line)

Are there more unchecked return values which are interesting
for further considerations?
https://cwe.mitre.org/data/definitions/252.html

Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Cocci] [PATCH v2] coccinelle: api: check for propagation of error from platform_get_irq

[Julia Lawall]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 862 bytes --]



On Sun, 27 Dec 2015, SF Markus Elfring wrote:

> > The error return value of platform_get_irq seems to often get dropped.
> 
> How do you think about any more fine-tuning here?
> 
> Commit message:
> * … of the platform_get_irq() function seems to get dropped too often.
> 
> * Why do you concentrate on a single function name?
>   Do you plan to extend this source code analysis approach?
> 
> 
> > +@script:python r_report depends on report@
> > +j0 << r.j0;
> > +j1 << r.j1;
> > +@@
> > +
> > +msg = "Propagate return value of platform_get_irq around line %s." % (j1[0].line)
> 
> Are there more unchecked return values which are interesting
> for further considerations?
> https://cwe.mitre.org/data/definitions/252.html

The value is not unchecked.  I made a specific rule because the specific 
problem is quite common.

julia

Re: [Cocci] [PATCH v2] coccinelle: api: check for propagation of error from platform_get_irq

[SF Markus Elfring]

>> https://cwe.mitre.org/data/definitions/252.html
> 
> The value is not unchecked.

Would you like to express any stronger relationship between
the function call example and the occurrence of an if statement
by the discussed SmPL script?


> I made a specific rule because the specific problem is quite common.

Can it become also interesting to generalise this search pattern?

Regards,
Markus