[PATCH RFC] coccinelle: flag constants being passed for jiffies

[PATCH RFC] coccinelle: flag constants being passed for jiffies

[Nicholas Mc Guire]

schedule_timeout_* takes a jiffies value as timeout - passing in a constant
makes the timeout HZ dependent which is wrong. Checking for contstants only
yields many false positives so they are filtered for digits only. A numeric
value of 1 is though commonly in use for "shortest possible delay" so those
cases are treated as false positives as well and not reported.

Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
---

The cases reported all look like they are actual API inconsistencies but
not reporting does not mean there is no bug with respect to jiffies. This
still can miss some values e.g. like in:
 drivers/staging/rts5208/rtsx_chip.h:66 #define POLLING_INTERVAL 30
 drivers/staging/rts5208/rtsx.c:540 schedule_timeout(POLLING_INTERVAL);

For schedule_timeout_*() it reports 23 cases - all of which look like 
they are correct findings. 

Further the list of functions taking jiffies as arguments is much longer
but they can be added once the basic script is sound - which it probably
is not at this point.

 scripts/coccinelle/api/timeout_HZ_dependent.cocci |   62 +++++++++++++++++++++
 1 file changed, 62 insertions(+)
 create mode 100644 scripts/coccinelle/api/timeout_HZ_dependent.cocci

diff --git a/scripts/coccinelle/api/timeout_HZ_dependent.cocci b/scripts/coccinelle/api/timeout_HZ_dependent.cocci
new file mode 100644
index 0000000..6e98da1
--- /dev/null
+++ b/scripts/coccinelle/api/timeout_HZ_dependent.cocci
@@ -0,0 +1,62 @@
+/* check for hardcoded timeout values which are thus HZ dependent
+ * only report findings if the value is digits and != 1 as hardcoded
+ * 1 seems to be in use for short delays.
+ *
+ * No patch mode for this as converting the value from C to
+ * msecs_to_jiffies(C) could be changing the effective timeout by
+ * more than a factor of 10 so this always needs manual inspection
+ *
+ * Options:  --no-includes --include-headers
+ */
+virtual context
+virtual patch
+virtual org
+virtual report
+
+@ep depends on !patch && (org || report)@
+identifier f;
+constant C;
+position p1,p2;
+@@
+
+f@p1(...) {
+ <+...
+(
+* schedule_timeout@p2(C)
+|
+* schedule_timeout_interruptible@p2(C)
+|
+* schedule_timeout_killable@p2(C)
+|
+* schedule_timeout_uninterruptible@p2(C)
+)
+ ...+>
+}
+
+@script:python depends on org@
+p1 << ep.p1;
+p2 << ep.p2;
+timeout << ep.C;
+@@
+
+# schedule_timeout(1) for a "short" delay is not HZ dependent
+# as it always would be converted to 1 by msecs_to_jiffies as well
+# so count this as false positive
+if str.isdigit(timeout):
+   if (int(timeout) != 1):
+      msg="WARNING: timeout is HZ dependent"
+      coccilib.org.print_todo(p2[0], msg)
+
+@script:python depends on report@
+p1 << ep.p1;
+p2 << ep.p2;
+timeout << ep.C;
+@@
+
+# schedule_timeout(1) for a "short" delay is not HZ dependent
+# as it always would be converted to 1 by msecs_to_jiffies as well
+# so count this as false positive
+if str.isdigit(timeout):
+   if (int(timeout) != 1):
+      msg="WARNING: timeout (%s) seems HZ dependent" % (timeout)
+      coccilib.report.print_report(p2[0], msg)
-- 
1.7.10.4

Re: [PATCH RFC] coccinelle: flag constants being passed for jiffies

[Julia Lawall]

On Wed, 27 May 2015, Nicholas Mc Guire wrote:

> schedule_timeout_* takes a jiffies value as timeout - passing in a constant
> makes the timeout HZ dependent which is wrong. Checking for contstants only
> yields many false positives so they are filtered for digits only. A numeric
> value of 1 is though commonly in use for "shortest possible delay" so those
> cases are treated as false positives as well and not reported.
> 
> Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> ---
> 
> The cases reported all look like they are actual API inconsistencies but
> not reporting does not mean there is no bug with respect to jiffies. This
> still can miss some values e.g. like in:
>  drivers/staging/rts5208/rtsx_chip.h:66 #define POLLING_INTERVAL 30
>  drivers/staging/rts5208/rtsx.c:540 schedule_timeout(POLLING_INTERVAL);
> 
> For schedule_timeout_*() it reports 23 cases - all of which look like 
> they are correct findings. 
> 
> Further the list of functions taking jiffies as arguments is much longer
> but they can be added once the basic script is sound - which it probably
> is not at this point.
> 
>  scripts/coccinelle/api/timeout_HZ_dependent.cocci |   62 +++++++++++++++++++++
>  1 file changed, 62 insertions(+)
>  create mode 100644 scripts/coccinelle/api/timeout_HZ_dependent.cocci
> 
> diff --git a/scripts/coccinelle/api/timeout_HZ_dependent.cocci b/scripts/coccinelle/api/timeout_HZ_dependent.cocci
> new file mode 100644
> index 0000000..6e98da1
> --- /dev/null
> +++ b/scripts/coccinelle/api/timeout_HZ_dependent.cocci
> @@ -0,0 +1,62 @@
> +/* check for hardcoded timeout values which are thus HZ dependent
> + * only report findings if the value is digits and != 1 as hardcoded
> + * 1 seems to be in use for short delays.
> + *
> + * No patch mode for this as converting the value from C to
> + * msecs_to_jiffies(C) could be changing the effective timeout by
> + * more than a factor of 10 so this always needs manual inspection
> + *
> + * Options:  --no-includes --include-headers
> + */
> +virtual context
> +virtual patch
> +virtual org
> +virtual report
> +
> +@ep depends on !patch && (org || report)@
> +identifier f;
> +constant C;
> +position p1,p2;
> +@@
> +
> +f@p1(...) {

Do you need the function for anything?  This would be more efficient with 
just the calls.


> + <+...
> +(
> +* schedule_timeout@p2(C)

If you put in the argument list \(i\|1\|C@p2\) then you can get rid of the 
python code (i would be an identifier metavariable).

What kind of false positives do you get for the macro case?  Would it be 
sufficient to do:

@r@
expression e != 1;
identifier i,
@@

#define i e

@@
identifier r.i,
@@

*schedule_timeout(i)

(and all the other cases?)

julia

> +|
> +* schedule_timeout_interruptible@p2(C)
> +|
> +* schedule_timeout_killable@p2(C)
> +|
> +* schedule_timeout_uninterruptible@p2(C)
> +)
> + ...+>
> +}
> +
> +@script:python depends on org@
> +p1 << ep.p1;
> +p2 << ep.p2;
> +timeout << ep.C;
> +@@
> +
> +# schedule_timeout(1) for a "short" delay is not HZ dependent
> +# as it always would be converted to 1 by msecs_to_jiffies as well
> +# so count this as false positive
> +if str.isdigit(timeout):
> +   if (int(timeout) != 1):
> +      msg="WARNING: timeout is HZ dependent"
> +      coccilib.org.print_todo(p2[0], msg)
> +
> +@script:python depends on report@
> +p1 << ep.p1;
> +p2 << ep.p2;
> +timeout << ep.C;
> +@@
> +
> +# schedule_timeout(1) for a "short" delay is not HZ dependent
> +# as it always would be converted to 1 by msecs_to_jiffies as well
> +# so count this as false positive
> +if str.isdigit(timeout):
> +   if (int(timeout) != 1):
> +      msg="WARNING: timeout (%s) seems HZ dependent" % (timeout)
> +      coccilib.report.print_report(p2[0], msg)
> -- 
> 1.7.10.4
> 
> 

Re: [Cocci] [PATCH RFC] coccinelle: flag constants being passed for jiffies

[Nicholas Mc Guire]

On Thu, 28 May 2015, Julia Lawall wrote:

> 
> 
> On Wed, 27 May 2015, Nicholas Mc Guire wrote:
> 
> > schedule_timeout_* takes a jiffies value as timeout - passing in a constant
> > makes the timeout HZ dependent which is wrong. Checking for contstants only
> > yields many false positives so they are filtered for digits only. A numeric
> > value of 1 is though commonly in use for "shortest possible delay" so those
> > cases are treated as false positives as well and not reported.
> > 
> > Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> > ---
> > 
> > The cases reported all look like they are actual API inconsistencies but
> > not reporting does not mean there is no bug with respect to jiffies. This
> > still can miss some values e.g. like in:
> >  drivers/staging/rts5208/rtsx_chip.h:66 #define POLLING_INTERVAL 30
> >  drivers/staging/rts5208/rtsx.c:540 schedule_timeout(POLLING_INTERVAL);
> > 
> > For schedule_timeout_*() it reports 23 cases - all of which look like 
> > they are correct findings. 
> > 
> > Further the list of functions taking jiffies as arguments is much longer
> > but they can be added once the basic script is sound - which it probably
> > is not at this point.
> > 
> >  scripts/coccinelle/api/timeout_HZ_dependent.cocci |   62 +++++++++++++++++++++
> >  1 file changed, 62 insertions(+)
> >  create mode 100644 scripts/coccinelle/api/timeout_HZ_dependent.cocci
> > 
> > diff --git a/scripts/coccinelle/api/timeout_HZ_dependent.cocci b/scripts/coccinelle/api/timeout_HZ_dependent.cocci
> > new file mode 100644
> > index 0000000..6e98da1
> > --- /dev/null
> > +++ b/scripts/coccinelle/api/timeout_HZ_dependent.cocci
> > @@ -0,0 +1,62 @@
> > +/* check for hardcoded timeout values which are thus HZ dependent
> > + * only report findings if the value is digits and != 1 as hardcoded
> > + * 1 seems to be in use for short delays.
> > + *
> > + * No patch mode for this as converting the value from C to
> > + * msecs_to_jiffies(C) could be changing the effective timeout by
> > + * more than a factor of 10 so this always needs manual inspection
> > + *
> > + * Options:  --no-includes --include-headers
> > + */
> > +virtual context
> > +virtual patch
> > +virtual org
> > +virtual report
> > +
> > +@ep depends on !patch && (org || report)@
> > +identifier f;
> > +constant C;
> > +position p1,p2;
> > +@@
> > +
> > +f@p1(...) {
> 
> Do you need the function for anything?  This would be more efficient with 
> just the calls.
>

nop - just forgot to remove it - thanks !
 
> 
> > + <+...
> > +(
> > +* schedule_timeout@p2(C)
> 
> If you put in the argument list \(i\|1\|C@p2\) then you can get rid of the 
> python code (i would be an identifier metavariable).

*schedule_timeout(\(i\|1\|C@p\))
works but it is a lot slower than handling it with those 2 lines of
python script - about 75min vs. 1min 30sec ?

> 
> What kind of false positives do you get for the macro case?  Would it be 
> sufficient to do:
> 
> @r@
> expression e != 1;
> identifier i,
> @@
> 
> #define i e
> 
> @@
> identifier r.i,
> @@
> 
> *schedule_timeout(i)
> 
> (and all the other cases?)
>
this will work but has the same problem as (C@p) it will report
false positives like
  ./arch/x86/kernel/apm_32.c:1440:19-36: WARNING: timeout (APM_CHECK_TIMEOUT) is HZ dependent

with
  #define APM_CHECK_TIMEOUT       (HZ)
this is not a bug.

and its also a lot slower than the brute force version with str.isdigit

thx!
hofrat