Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1298255 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH] coccinelle: api: check for propagation of error from platform_get_irq Julia Lawall <Julia.Lawall@lip6.fr> - 2015-12-26 20:40 +0100
    Re: [PATCH] coccinelle: api: check for propagation of error from  platform_get_irq Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - 2015-12-26 21:00 +0100
      Re: [PATCH] coccinelle: api: check for propagation of error from  platform_get_irq Julia Lawall <julia.lawall@lip6.fr> - 2015-12-26 21:10 +0100
        Re: [PATCH] coccinelle: api: check for propagation of error from  platform_get_irq Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - 2015-12-26 21:30 +0100
          Re: [PATCH] coccinelle: api: check for propagation of error from  platform_get_irq Julia Lawall <julia.lawall@lip6.fr> - 2015-12-26 21:40 +0100
          [PATCH v2] coccinelle: api: check for propagation of error from  platform_get_irq Julia Lawall <julia.lawall@lip6.fr> - 2015-12-26 22:00 +0100
            Re: [PATCH v2] coccinelle: api: check for propagation of error from  platform_get_irq Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - 2015-12-26 23:30 +0100
              Re: [PATCH v2] coccinelle: api: check for propagation of error from  platform_get_irq Julia Lawall <julia.lawall@lip6.fr> - 2015-12-26 23:40 +0100
                Re: [PATCH v2] coccinelle: api: check for propagation of error from  platform_get_irq Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - 2015-12-26 23:40 +0100
                  Re: [PATCH v2] coccinelle: api: check for propagation of error from  platform_get_irq Julia Lawall <julia.lawall@lip6.fr> - 2015-12-27 07:20 +0100
                    Re: [PATCH v2] coccinelle: api: check for propagation of error from  platform_get_irq Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - 2015-12-27 12:20 +0100
            Re: [Cocci] [PATCH v2] coccinelle: api: check for propagation of  error from platform_get_irq SF Markus Elfring <elfring@users.sourceforge.net> - 2015-12-27 09:00 +0100
              Re: [Cocci] [PATCH v2] coccinelle: api: check for propagation of  error from platform_get_irq Julia Lawall <julia.lawall@lip6.fr> - 2015-12-27 12:50 +0100
                Re: [Cocci] [PATCH v2] coccinelle: api: check for propagation of  error from platform_get_irq SF Markus Elfring <elfring@users.sourceforge.net> - 2015-12-28 03:50 +0100
        Re: [PATCH] coccinelle: api: check for propagation of error from  platform_get_irq Julia Lawall <julia.lawall@lip6.fr> - 2015-12-26 21:30 +0100

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

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

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

[toc] | [next] | [standalone]

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

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web