Coccinelle archive on lore.kernel.org
 help / color / Atom feed
* [Cocci] [PATCH] coccinelle: semantic patch to check for inappropriate do_div() calls
@ 2020-01-04  6:44 Wen Yang
  2020-01-04  7:00 ` Julia Lawall
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Wen Yang @ 2020-01-04  6:44 UTC (permalink / raw)
  To: Julia Lawall
  Cc: Michal Marek, Wen Yang, Gilles Muller, Greg Kroah-Hartman,
	Nicolas Palix, Matthias Maennich, linux-kernel, Thomas Gleixner,
	cocci

do_div() does a 64-by-32 division.
When the divisor is unsigned long, u64, or s64,
do_div() truncates it to 32 bits, this means it
can test non-zero and be truncated to zero for division.
This semantic patch is inspired by Mateusz Guzik's patch:
commit b0ab99e7736a ("sched: Fix possible divide by zero in avg_atom() calculation")

Signed-off-by: Wen Yang <wenyang@linux.alibaba.com>
Cc: Julia Lawall <Julia.Lawall@lip6.fr>
Cc: Gilles Muller <Gilles.Muller@lip6.fr>
Cc: Nicolas Palix <nicolas.palix@imag.fr>
Cc: Michal Marek <michal.lkml@markovi.net>
Cc: Matthias Maennich <maennich@google.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: cocci@systeme.lip6.fr
Cc: linux-kernel@vger.kernel.org
---
 scripts/coccinelle/misc/do_div.cocci | 66 ++++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)
 create mode 100644 scripts/coccinelle/misc/do_div.cocci

diff --git a/scripts/coccinelle/misc/do_div.cocci b/scripts/coccinelle/misc/do_div.cocci
new file mode 100644
index 0000000..f1b72d1
--- /dev/null
+++ b/scripts/coccinelle/misc/do_div.cocci
@@ -0,0 +1,66 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/// do_div() does a 64-by-32 division.
+/// When the divisor is unsigned long, u64, or s64,
+/// do_div() truncates it to 32 bits, this means it
+/// can test non-zero and be truncated to zero for division.
+///
+//# This makes an effort to find those inappropriate do_div () calls.
+//
+// Confidence: Moderate
+// Copyright: (C) 2020 Wen Yang, Alibaba.
+// Comments:
+// Options: --no-includes --include-headers
+
+virtual context
+virtual org
+virtual report
+
+@depends on context@
+expression f;
+long l;
+unsigned long ul;
+u64 ul64;
+s64 sl64;
+
+@@
+(
+* do_div(f, l);
+|
+* do_div(f, ul);
+|
+* do_div(f, ul64);
+|
+* do_div(f, sl64);
+)
+
+@r depends on (org || report)@
+expression f;
+long l;
+unsigned long ul;
+position p;
+u64 ul64;
+s64 sl64;
+@@
+(
+do_div@p(f, l);
+|
+do_div@p(f, ul);
+|
+do_div@p(f, ul64);
+|
+do_div@p(f, sl64);
+)
+
+@script:python depends on org@
+p << r.p;
+@@
+
+msg="WARNING: WARNING: do_div() does a 64-by-32 division, which may truncation the divisor to 32-bit"
+coccilib.org.print_todo(p[0], msg)
+
+@script:python depends on report@
+p << r.p;
+@@
+
+msg="WARNING: WARNING: do_div() does a 64-by-32 division, which may truncation the divisor to 32-bit"
+coccilib.report.print_report(p[0], msg)
-- 
1.8.3.1

_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Cocci] [PATCH] coccinelle: semantic patch to check for inappropriate do_div() calls
  2020-01-04  6:44 [Cocci] [PATCH] coccinelle: semantic patch to check for inappropriate do_div() calls Wen Yang
@ 2020-01-04  7:00 ` Julia Lawall
  2020-01-04  8:49   ` Wen Yang
  2020-01-04  7:16 ` Julia Lawall
  2020-01-05 10:33 ` Markus Elfring
  2 siblings, 1 reply; 10+ messages in thread
From: Julia Lawall @ 2020-01-04  7:00 UTC (permalink / raw)
  To: Wen Yang
  Cc: Michal Marek, Gilles Muller, Nicolas Palix, Matthias Maennich,
	linux-kernel, Julia Lawall, Greg Kroah-Hartman, Thomas Gleixner,
	cocci

On Sat, 4 Jan 2020, Wen Yang wrote:

> do_div() does a 64-by-32 division.
> When the divisor is unsigned long, u64, or s64,
> do_div() truncates it to 32 bits, this means it
> can test non-zero and be truncated to zero for division.
> This semantic patch is inspired by Mateusz Guzik's patch:
> commit b0ab99e7736a ("sched: Fix possible divide by zero in avg_atom() calculation")
>
> Signed-off-by: Wen Yang <wenyang@linux.alibaba.com>
> Cc: Julia Lawall <Julia.Lawall@lip6.fr>
> Cc: Gilles Muller <Gilles.Muller@lip6.fr>
> Cc: Nicolas Palix <nicolas.palix@imag.fr>
> Cc: Michal Marek <michal.lkml@markovi.net>
> Cc: Matthias Maennich <maennich@google.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: cocci@systeme.lip6.fr
> Cc: linux-kernel@vger.kernel.org
> ---
>  scripts/coccinelle/misc/do_div.cocci | 66 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 66 insertions(+)
>  create mode 100644 scripts/coccinelle/misc/do_div.cocci
>
> diff --git a/scripts/coccinelle/misc/do_div.cocci b/scripts/coccinelle/misc/do_div.cocci
> new file mode 100644
> index 0000000..f1b72d1
> --- /dev/null
> +++ b/scripts/coccinelle/misc/do_div.cocci
> @@ -0,0 +1,66 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/// do_div() does a 64-by-32 division.
> +/// When the divisor is unsigned long, u64, or s64,
> +/// do_div() truncates it to 32 bits, this means it
> +/// can test non-zero and be truncated to zero for division.
> +///
> +//# This makes an effort to find those inappropriate do_div () calls.
> +//
> +// Confidence: Moderate
> +// Copyright: (C) 2020 Wen Yang, Alibaba.
> +// Comments:
> +// Options: --no-includes --include-headers
> +
> +virtual context
> +virtual org
> +virtual report
> +
> +@depends on context@
> +expression f;
> +long l;
> +unsigned long ul;
> +u64 ul64;
> +s64 sl64;
> +
> +@@
> +(
> +* do_div(f, l);
> +|
> +* do_div(f, ul);
> +|
> +* do_div(f, ul64);
> +|
> +* do_div(f, sl64);
> +)
> +
> +@r depends on (org || report)@
> +expression f;
> +long l;
> +unsigned long ul;
> +position p;
> +u64 ul64;
> +s64 sl64;
> +@@
> +(
> +do_div@p(f, l);
> +|
> +do_div@p(f, ul);
> +|
> +do_div@p(f, ul64);
> +|
> +do_div@p(f, sl64);
> +)
> +
> +@script:python depends on org@
> +p << r.p;
> +@@
> +
> +msg="WARNING: WARNING: do_div() does a 64-by-32 division, which may truncation the divisor to 32-bit"
> +coccilib.org.print_todo(p[0], msg)
> +
> +@script:python depends on report@
> +p << r.p;
> +@@
> +
> +msg="WARNING: WARNING: do_div() does a 64-by-32 division, which may truncation the divisor to 32-bit"
> +coccilib.report.print_report(p[0], msg)

A few small issues: You have WARNING: twice in each case, and truncation
should be truncate.

Is there any generic strategy for fixing these issues?

julia
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Cocci] [PATCH] coccinelle: semantic patch to check for inappropriate do_div() calls
  2020-01-04  6:44 [Cocci] [PATCH] coccinelle: semantic patch to check for inappropriate do_div() calls Wen Yang
  2020-01-04  7:00 ` Julia Lawall
@ 2020-01-04  7:16 ` Julia Lawall
  2020-01-05 10:33 ` Markus Elfring
  2 siblings, 0 replies; 10+ messages in thread
From: Julia Lawall @ 2020-01-04  7:16 UTC (permalink / raw)
  To: Wen Yang
  Cc: Michal Marek, Gilles Muller, Nicolas Palix, Matthias Maennich,
	linux-kernel, Greg Kroah-Hartman, Thomas Gleixner, cocci



--- Please note the new email address ---


On Sat, 4 Jan 2020, Wen Yang wrote:

> do_div() does a 64-by-32 division.
> When the divisor is unsigned long, u64, or s64,
> do_div() truncates it to 32 bits, this means it
> can test non-zero and be truncated to zero for division.

Would it be worth having a specific warning message for the long/unsigned
long case?  If the target architecture has 32 bit longs then the warning
can be immediately ignored.

julia


> This semantic patch is inspired by Mateusz Guzik's patch:
> commit b0ab99e7736a ("sched: Fix possible divide by zero in avg_atom() calculation")
>
> Signed-off-by: Wen Yang <wenyang@linux.alibaba.com>
> Cc: Julia Lawall <Julia.Lawall@lip6.fr>
> Cc: Gilles Muller <Gilles.Muller@lip6.fr>
> Cc: Nicolas Palix <nicolas.palix@imag.fr>
> Cc: Michal Marek <michal.lkml@markovi.net>
> Cc: Matthias Maennich <maennich@google.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: cocci@systeme.lip6.fr
> Cc: linux-kernel@vger.kernel.org
> ---
>  scripts/coccinelle/misc/do_div.cocci | 66 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 66 insertions(+)
>  create mode 100644 scripts/coccinelle/misc/do_div.cocci
>
> diff --git a/scripts/coccinelle/misc/do_div.cocci b/scripts/coccinelle/misc/do_div.cocci
> new file mode 100644
> index 0000000..f1b72d1
> --- /dev/null
> +++ b/scripts/coccinelle/misc/do_div.cocci
> @@ -0,0 +1,66 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/// do_div() does a 64-by-32 division.
> +/// When the divisor is unsigned long, u64, or s64,
> +/// do_div() truncates it to 32 bits, this means it
> +/// can test non-zero and be truncated to zero for division.
> +///
> +//# This makes an effort to find those inappropriate do_div () calls.
> +//
> +// Confidence: Moderate
> +// Copyright: (C) 2020 Wen Yang, Alibaba.
> +// Comments:
> +// Options: --no-includes --include-headers
> +
> +virtual context
> +virtual org
> +virtual report
> +
> +@depends on context@
> +expression f;
> +long l;
> +unsigned long ul;
> +u64 ul64;
> +s64 sl64;
> +
> +@@
> +(
> +* do_div(f, l);
> +|
> +* do_div(f, ul);
> +|
> +* do_div(f, ul64);
> +|
> +* do_div(f, sl64);
> +)
> +
> +@r depends on (org || report)@
> +expression f;
> +long l;
> +unsigned long ul;
> +position p;
> +u64 ul64;
> +s64 sl64;
> +@@
> +(
> +do_div@p(f, l);
> +|
> +do_div@p(f, ul);
> +|
> +do_div@p(f, ul64);
> +|
> +do_div@p(f, sl64);
> +)
> +
> +@script:python depends on org@
> +p << r.p;
> +@@
> +
> +msg="WARNING: WARNING: do_div() does a 64-by-32 division, which may truncation the divisor to 32-bit"
> +coccilib.org.print_todo(p[0], msg)
> +
> +@script:python depends on report@
> +p << r.p;
> +@@
> +
> +msg="WARNING: WARNING: do_div() does a 64-by-32 division, which may truncation the divisor to 32-bit"
> +coccilib.report.print_report(p[0], msg)
> --
> 1.8.3.1
>
>
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Cocci] [PATCH] coccinelle: semantic patch to check for inappropriate do_div() calls
  2020-01-04  7:00 ` Julia Lawall
@ 2020-01-04  8:49   ` Wen Yang
  2020-01-04  8:55     ` Julia Lawall
  0 siblings, 1 reply; 10+ messages in thread
From: Wen Yang @ 2020-01-04  8:49 UTC (permalink / raw)
  To: Julia Lawall
  Cc: Michal Marek, Gilles Muller, Nicolas Palix, Matthias Maennich,
	linux-kernel, Julia Lawall, Greg Kroah-Hartman, Thomas Gleixner,
	cocci



On 2020/1/4 3:00 下午, Julia Lawall wrote:
> On Sat, 4 Jan 2020, Wen Yang wrote:
> 
>> do_div() does a 64-by-32 division.
>> When the divisor is unsigned long, u64, or s64,
>> do_div() truncates it to 32 bits, this means it
>> can test non-zero and be truncated to zero for division.
>> This semantic patch is inspired by Mateusz Guzik's patch:
>> commit b0ab99e7736a ("sched: Fix possible divide by zero in avg_atom() calculation")
>>
>> Signed-off-by: Wen Yang <wenyang@linux.alibaba.com>
>> Cc: Julia Lawall <Julia.Lawall@lip6.fr>
>> Cc: Gilles Muller <Gilles.Muller@lip6.fr>
>> Cc: Nicolas Palix <nicolas.palix@imag.fr>
>> Cc: Michal Marek <michal.lkml@markovi.net>
>> Cc: Matthias Maennich <maennich@google.com>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: cocci@systeme.lip6.fr
>> Cc: linux-kernel@vger.kernel.org
>> ---
>>   scripts/coccinelle/misc/do_div.cocci | 66 ++++++++++++++++++++++++++++++++++++
>>   1 file changed, 66 insertions(+)
>>   create mode 100644 scripts/coccinelle/misc/do_div.cocci
>>
>> diff --git a/scripts/coccinelle/misc/do_div.cocci b/scripts/coccinelle/misc/do_div.cocci
>> new file mode 100644
>> index 0000000..f1b72d1
>> --- /dev/null
>> +++ b/scripts/coccinelle/misc/do_div.cocci
>> @@ -0,0 +1,66 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/// do_div() does a 64-by-32 division.
>> +/// When the divisor is unsigned long, u64, or s64,
>> +/// do_div() truncates it to 32 bits, this means it
>> +/// can test non-zero and be truncated to zero for division.
>> +///
>> +//# This makes an effort to find those inappropriate do_div () calls.
>> +//
>> +// Confidence: Moderate
>> +// Copyright: (C) 2020 Wen Yang, Alibaba.
>> +// Comments:
>> +// Options: --no-includes --include-headers
>> +
>> +virtual context
>> +virtual org
>> +virtual report
>> +
>> +@depends on context@
>> +expression f;
>> +long l;
>> +unsigned long ul;
>> +u64 ul64;
>> +s64 sl64;
>> +
>> +@@
>> +(
>> +* do_div(f, l);
>> +|
>> +* do_div(f, ul);
>> +|
>> +* do_div(f, ul64);
>> +|
>> +* do_div(f, sl64);
>> +)
>> +
>> +@r depends on (org || report)@
>> +expression f;
>> +long l;
>> +unsigned long ul;
>> +position p;
>> +u64 ul64;
>> +s64 sl64;
>> +@@
>> +(
>> +do_div@p(f, l);
>> +|
>> +do_div@p(f, ul);
>> +|
>> +do_div@p(f, ul64);
>> +|
>> +do_div@p(f, sl64);
>> +)
>> +
>> +@script:python depends on org@
>> +p << r.p;
>> +@@
>> +
>> +msg="WARNING: WARNING: do_div() does a 64-by-32 division, which may truncation the divisor to 32-bit"
>> +coccilib.org.print_todo(p[0], msg)
>> +
>> +@script:python depends on report@
>> +p << r.p;
>> +@@
>> +
>> +msg="WARNING: WARNING: do_div() does a 64-by-32 division, which may truncation the divisor to 32-bit"
>> +coccilib.report.print_report(p[0], msg)
> 
> A few small issues: You have WARNING: twice in each case, and truncation
> should be truncate.
> 

Thanks for your comments, we will fix it soon.

> Is there any generic strategy for fixing these issues?
> 

We have done some experiments, such as:
https://lkml.org/lkml/2020/1/2/1354


-	avg = rec->time;
-	do_div(avg, rec->counter);
+	avg = div64_ul(rec->time, rec->counter);

--> Function replacement was performed here,
     and simple code cleanup was also performed.


-		do_div(stddev, rec->counter * (rec->counter - 1) * 1000);
+		stddev = div64_ul(stddev,
+				  rec->counter * (rec->counter - 1) * 1000);

--> Only the function replacement is performed here (because the 
variable ‘stddev’ corresponds to a more complicated equation, cleaning 
it will reduce readability).

In addition, there are some codes that do not need to be modified:
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/net/can/dev.c#n263

So we just print a warning.
As for how to fix it, we need to analyze the code carefully.

-- 
Best Wishes,
Wen


_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Cocci] [PATCH] coccinelle: semantic patch to check for inappropriate do_div() calls
  2020-01-04  8:49   ` Wen Yang
@ 2020-01-04  8:55     ` Julia Lawall
  2020-01-04 13:50       ` Wen Yang
  0 siblings, 1 reply; 10+ messages in thread
From: Julia Lawall @ 2020-01-04  8:55 UTC (permalink / raw)
  To: Wen Yang
  Cc: Michal Marek, Gilles Muller, Nicolas Palix, Matthias Maennich,
	linux-kernel, Julia Lawall, Greg Kroah-Hartman, Thomas Gleixner,
	cocci

[-- Attachment #1: Type: text/plain, Size: 4779 bytes --]

On Sat, 4 Jan 2020, Wen Yang wrote:

>
>
> On 2020/1/4 3:00 下午, Julia Lawall wrote:
> > On Sat, 4 Jan 2020, Wen Yang wrote:
> >
> > > do_div() does a 64-by-32 division.
> > > When the divisor is unsigned long, u64, or s64,
> > > do_div() truncates it to 32 bits, this means it
> > > can test non-zero and be truncated to zero for division.
> > > This semantic patch is inspired by Mateusz Guzik's patch:
> > > commit b0ab99e7736a ("sched: Fix possible divide by zero in avg_atom()
> > > calculation")
> > >
> > > Signed-off-by: Wen Yang <wenyang@linux.alibaba.com>
> > > Cc: Julia Lawall <Julia.Lawall@lip6.fr>
> > > Cc: Gilles Muller <Gilles.Muller@lip6.fr>
> > > Cc: Nicolas Palix <nicolas.palix@imag.fr>
> > > Cc: Michal Marek <michal.lkml@markovi.net>
> > > Cc: Matthias Maennich <maennich@google.com>
> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
> > > Cc: Thomas Gleixner <tglx@linutronix.de>
> > > Cc: cocci@systeme.lip6.fr
> > > Cc: linux-kernel@vger.kernel.org
> > > ---
> > >   scripts/coccinelle/misc/do_div.cocci | 66
> > > ++++++++++++++++++++++++++++++++++++
> > >   1 file changed, 66 insertions(+)
> > >   create mode 100644 scripts/coccinelle/misc/do_div.cocci
> > >
> > > diff --git a/scripts/coccinelle/misc/do_div.cocci
> > > b/scripts/coccinelle/misc/do_div.cocci
> > > new file mode 100644
> > > index 0000000..f1b72d1
> > > --- /dev/null
> > > +++ b/scripts/coccinelle/misc/do_div.cocci
> > > @@ -0,0 +1,66 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/// do_div() does a 64-by-32 division.
> > > +/// When the divisor is unsigned long, u64, or s64,
> > > +/// do_div() truncates it to 32 bits, this means it
> > > +/// can test non-zero and be truncated to zero for division.
> > > +///
> > > +//# This makes an effort to find those inappropriate do_div () calls.
> > > +//
> > > +// Confidence: Moderate
> > > +// Copyright: (C) 2020 Wen Yang, Alibaba.
> > > +// Comments:
> > > +// Options: --no-includes --include-headers
> > > +
> > > +virtual context
> > > +virtual org
> > > +virtual report
> > > +
> > > +@depends on context@
> > > +expression f;
> > > +long l;
> > > +unsigned long ul;
> > > +u64 ul64;
> > > +s64 sl64;
> > > +
> > > +@@
> > > +(
> > > +* do_div(f, l);
> > > +|
> > > +* do_div(f, ul);
> > > +|
> > > +* do_div(f, ul64);
> > > +|
> > > +* do_div(f, sl64);
> > > +)
> > > +
> > > +@r depends on (org || report)@
> > > +expression f;
> > > +long l;
> > > +unsigned long ul;
> > > +position p;
> > > +u64 ul64;
> > > +s64 sl64;
> > > +@@
> > > +(
> > > +do_div@p(f, l);
> > > +|
> > > +do_div@p(f, ul);
> > > +|
> > > +do_div@p(f, ul64);
> > > +|
> > > +do_div@p(f, sl64);
> > > +)
> > > +
> > > +@script:python depends on org@
> > > +p << r.p;
> > > +@@
> > > +
> > > +msg="WARNING: WARNING: do_div() does a 64-by-32 division, which may
> > > truncation the divisor to 32-bit"
> > > +coccilib.org.print_todo(p[0], msg)
> > > +
> > > +@script:python depends on report@
> > > +p << r.p;
> > > +@@
> > > +
> > > +msg="WARNING: WARNING: do_div() does a 64-by-32 division, which may
> > > truncation the divisor to 32-bit"
> > > +coccilib.report.print_report(p[0], msg)
> >
> > A few small issues: You have WARNING: twice in each case, and truncation
> > should be truncate.
> >
>
> Thanks for your comments, we will fix it soon.
>
> > Is there any generic strategy for fixing these issues?
> >
>
> We have done some experiments, such as:
> https://lkml.org/lkml/2020/1/2/1354

Thanks.  Actually, I would appreciate knowing about such experiments when
the semantic patch is submitted, since eg in this case I am not really an
expert in this issue.

>
> -	avg = rec->time;
> -	do_div(avg, rec->counter);
> +	avg = div64_ul(rec->time, rec->counter);
>
> --> Function replacement was performed here,
>     and simple code cleanup was also performed.
>
>
> -		do_div(stddev, rec->counter * (rec->counter - 1) * 1000);
> +		stddev = div64_ul(stddev,
> +				  rec->counter * (rec->counter - 1) * 1000);
>
> --> Only the function replacement is performed here (because the variable
> ‘stddev’ corresponds to a more complicated equation, cleaning it will reduce
> readability).

Would it be reasonable to extend the warning to say "consider using
div64_ul instead"?  Or do you think it is obvious to everyone?

> In addition, there are some codes that do not need to be modified:
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/net/can/dev.c#n263

Would it be worth having a special case for constants and checking whether
the value is obviously safe and no warning is needed?

thanks,
julia

> So we just print a warning.
> As for how to fix it, we need to analyze the code carefully.
>
> --
> Best Wishes,
> Wen
>
>
>

[-- Attachment #2: Type: text/plain, Size: 136 bytes --]

_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Cocci] [PATCH] coccinelle: semantic patch to check for inappropriate do_div() calls
  2020-01-04  8:55     ` Julia Lawall
@ 2020-01-04 13:50       ` Wen Yang
  2020-01-04 13:54         ` Julia Lawall
  0 siblings, 1 reply; 10+ messages in thread
From: Wen Yang @ 2020-01-04 13:50 UTC (permalink / raw)
  To: Julia Lawall
  Cc: Michal Marek, Gilles Muller, Nicolas Palix, Matthias Maennich,
	linux-kernel, Julia Lawall, Greg Kroah-Hartman, Thomas Gleixner,
	cocci



On 2020/1/4 4:55 下午, Julia Lawall wrote:
> On Sat, 4 Jan 2020, Wen Yang wrote:
> 
>>
>>
>> On 2020/1/4 3:00 下午, Julia Lawall wrote:
>>> On Sat, 4 Jan 2020, Wen Yang wrote:
>>>
>>>> do_div() does a 64-by-32 division.
>>>> When the divisor is unsigned long, u64, or s64,
>>>> do_div() truncates it to 32 bits, this means it
>>>> can test non-zero and be truncated to zero for division.
>>>> This semantic patch is inspired by Mateusz Guzik's patch:
>>>> commit b0ab99e7736a ("sched: Fix possible divide by zero in avg_atom()
>>>> calculation")
>>>>
>>>> Signed-off-by: Wen Yang <wenyang@linux.alibaba.com>
>>>> Cc: Julia Lawall <Julia.Lawall@lip6.fr>
>>>> Cc: Gilles Muller <Gilles.Muller@lip6.fr>
>>>> Cc: Nicolas Palix <nicolas.palix@imag.fr>
>>>> Cc: Michal Marek <michal.lkml@markovi.net>
>>>> Cc: Matthias Maennich <maennich@google.com>
>>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>>> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
>>>> Cc: Thomas Gleixner <tglx@linutronix.de>
>>>> Cc: cocci@systeme.lip6.fr
>>>> Cc: linux-kernel@vger.kernel.org
>>>> ---
>>>>    scripts/coccinelle/misc/do_div.cocci | 66
>>>> ++++++++++++++++++++++++++++++++++++
>>>>    1 file changed, 66 insertions(+)
>>>>    create mode 100644 scripts/coccinelle/misc/do_div.cocci
>>>>
>>>> diff --git a/scripts/coccinelle/misc/do_div.cocci
>>>> b/scripts/coccinelle/misc/do_div.cocci
>>>> new file mode 100644
>>>> index 0000000..f1b72d1
>>>> --- /dev/null
>>>> +++ b/scripts/coccinelle/misc/do_div.cocci
>>>> @@ -0,0 +1,66 @@
>>>> +// SPDX-License-Identifier: GPL-2.0-only
>>>> +/// do_div() does a 64-by-32 division.
>>>> +/// When the divisor is unsigned long, u64, or s64,
>>>> +/// do_div() truncates it to 32 bits, this means it
>>>> +/// can test non-zero and be truncated to zero for division.
>>>> +///
>>>> +//# This makes an effort to find those inappropriate do_div () calls.
>>>> +//
>>>> +// Confidence: Moderate
>>>> +// Copyright: (C) 2020 Wen Yang, Alibaba.
>>>> +// Comments:
>>>> +// Options: --no-includes --include-headers
>>>> +
>>>> +virtual context
>>>> +virtual org
>>>> +virtual report
>>>> +
>>>> +@depends on context@
>>>> +expression f;
>>>> +long l;
>>>> +unsigned long ul;
>>>> +u64 ul64;
>>>> +s64 sl64;
>>>> +
>>>> +@@
>>>> +(
>>>> +* do_div(f, l);
>>>> +|
>>>> +* do_div(f, ul);
>>>> +|
>>>> +* do_div(f, ul64);
>>>> +|
>>>> +* do_div(f, sl64);
>>>> +)
>>>> +
>>>> +@r depends on (org || report)@
>>>> +expression f;
>>>> +long l;
>>>> +unsigned long ul;
>>>> +position p;
>>>> +u64 ul64;
>>>> +s64 sl64;
>>>> +@@
>>>> +(
>>>> +do_div@p(f, l);
>>>> +|
>>>> +do_div@p(f, ul);
>>>> +|
>>>> +do_div@p(f, ul64);
>>>> +|
>>>> +do_div@p(f, sl64);
>>>> +)
>>>> +
>>>> +@script:python depends on org@
>>>> +p << r.p;
>>>> +@@
>>>> +
>>>> +msg="WARNING: WARNING: do_div() does a 64-by-32 division, which may
>>>> truncation the divisor to 32-bit"
>>>> +coccilib.org.print_todo(p[0], msg)
>>>> +
>>>> +@script:python depends on report@
>>>> +p << r.p;
>>>> +@@
>>>> +
>>>> +msg="WARNING: WARNING: do_div() does a 64-by-32 division, which may
>>>> truncation the divisor to 32-bit"
>>>> +coccilib.report.print_report(p[0], msg)
>>>
>>> A few small issues: You have WARNING: twice in each case, and truncation
>>> should be truncate.
>>>
>>
>> Thanks for your comments, we will fix it soon.
>>
>>> Is there any generic strategy for fixing these issues?
>>>
>>
>> We have done some experiments, such as:
>> https://lkml.org/lkml/2020/1/2/1354
> 
> Thanks.  Actually, I would appreciate knowing about such experiments when
> the semantic patch is submitted, since eg in this case I am not really an
> expert in this issue.
> 
>>
>> -	avg = rec->time;
>> -	do_div(avg, rec->counter);
>> +	avg = div64_ul(rec->time, rec->counter);
>>
>> --> Function replacement was performed here,
>>      and simple code cleanup was also performed.
>>
>>
>> -		do_div(stddev, rec->counter * (rec->counter - 1) * 1000);
>> +		stddev = div64_ul(stddev,
>> +				  rec->counter * (rec->counter - 1) * 1000);
>>
>> --> Only the function replacement is performed here (because the variable
>> ‘stddev’ corresponds to a more complicated equation, cleaning it will reduce
>> readability).
> 
> Would it be reasonable to extend the warning to say "consider using
> div64_ul instead"?  Or do you think it is obvious to everyone?
> 

Thank you for your comments.
We plan to modify it as follows:
msg="WARNING: do_div() does a 64-by-32 division, please consider using 
div64_ul, div64_long, div64_u64 or div64_s64 instead."

>> In addition, there are some codes that do not need to be modified:
>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/net/can/dev.c#n263
> 
> Would it be worth having a special case for constants and checking whether
> the value is obviously safe and no warning is needed?
>
Thanks.
This is very valuable in reducing false positives, and we'll try to 
implement it.

--
Best Wishes,
Wen

>> So we just print a warning.
>> As for how to fix it, we need to analyze the code carefully.
>>
>> --
>> Best Wishes,
>> Wen
>>
>>
>>
> 
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Cocci] [PATCH] coccinelle: semantic patch to check for inappropriate do_div() calls
  2020-01-04 13:50       ` Wen Yang
@ 2020-01-04 13:54         ` Julia Lawall
  0 siblings, 0 replies; 10+ messages in thread
From: Julia Lawall @ 2020-01-04 13:54 UTC (permalink / raw)
  To: Wen Yang
  Cc: Michal Marek, Gilles Muller, Nicolas Palix, Matthias Maennich,
	linux-kernel, Greg Kroah-Hartman, Thomas Gleixner, cocci

[-- Attachment #1: Type: text/plain, Size: 5914 bytes --]



--- Please note the new email address ---


On Sat, 4 Jan 2020, Wen Yang wrote:

>
>
> On 2020/1/4 4:55 下午, Julia Lawall wrote:
> > On Sat, 4 Jan 2020, Wen Yang wrote:
> >
> > >
> > >
> > > On 2020/1/4 3:00 下午, Julia Lawall wrote:
> > > > On Sat, 4 Jan 2020, Wen Yang wrote:
> > > >
> > > > > do_div() does a 64-by-32 division.
> > > > > When the divisor is unsigned long, u64, or s64,
> > > > > do_div() truncates it to 32 bits, this means it
> > > > > can test non-zero and be truncated to zero for division.
> > > > > This semantic patch is inspired by Mateusz Guzik's patch:
> > > > > commit b0ab99e7736a ("sched: Fix possible divide by zero in avg_atom()
> > > > > calculation")
> > > > >
> > > > > Signed-off-by: Wen Yang <wenyang@linux.alibaba.com>
> > > > > Cc: Julia Lawall <Julia.Lawall@lip6.fr>
> > > > > Cc: Gilles Muller <Gilles.Muller@lip6.fr>
> > > > > Cc: Nicolas Palix <nicolas.palix@imag.fr>
> > > > > Cc: Michal Marek <michal.lkml@markovi.net>
> > > > > Cc: Matthias Maennich <maennich@google.com>
> > > > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > > Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
> > > > > Cc: Thomas Gleixner <tglx@linutronix.de>
> > > > > Cc: cocci@systeme.lip6.fr
> > > > > Cc: linux-kernel@vger.kernel.org
> > > > > ---
> > > > >    scripts/coccinelle/misc/do_div.cocci | 66
> > > > > ++++++++++++++++++++++++++++++++++++
> > > > >    1 file changed, 66 insertions(+)
> > > > >    create mode 100644 scripts/coccinelle/misc/do_div.cocci
> > > > >
> > > > > diff --git a/scripts/coccinelle/misc/do_div.cocci
> > > > > b/scripts/coccinelle/misc/do_div.cocci
> > > > > new file mode 100644
> > > > > index 0000000..f1b72d1
> > > > > --- /dev/null
> > > > > +++ b/scripts/coccinelle/misc/do_div.cocci
> > > > > @@ -0,0 +1,66 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0-only
> > > > > +/// do_div() does a 64-by-32 division.
> > > > > +/// When the divisor is unsigned long, u64, or s64,
> > > > > +/// do_div() truncates it to 32 bits, this means it
> > > > > +/// can test non-zero and be truncated to zero for division.
> > > > > +///
> > > > > +//# This makes an effort to find those inappropriate do_div () calls.
> > > > > +//
> > > > > +// Confidence: Moderate
> > > > > +// Copyright: (C) 2020 Wen Yang, Alibaba.
> > > > > +// Comments:
> > > > > +// Options: --no-includes --include-headers
> > > > > +
> > > > > +virtual context
> > > > > +virtual org
> > > > > +virtual report
> > > > > +
> > > > > +@depends on context@
> > > > > +expression f;
> > > > > +long l;
> > > > > +unsigned long ul;
> > > > > +u64 ul64;
> > > > > +s64 sl64;
> > > > > +
> > > > > +@@
> > > > > +(
> > > > > +* do_div(f, l);
> > > > > +|
> > > > > +* do_div(f, ul);
> > > > > +|
> > > > > +* do_div(f, ul64);
> > > > > +|
> > > > > +* do_div(f, sl64);
> > > > > +)
> > > > > +
> > > > > +@r depends on (org || report)@
> > > > > +expression f;
> > > > > +long l;
> > > > > +unsigned long ul;
> > > > > +position p;
> > > > > +u64 ul64;
> > > > > +s64 sl64;
> > > > > +@@
> > > > > +(
> > > > > +do_div@p(f, l);
> > > > > +|
> > > > > +do_div@p(f, ul);
> > > > > +|
> > > > > +do_div@p(f, ul64);
> > > > > +|
> > > > > +do_div@p(f, sl64);
> > > > > +)
> > > > > +
> > > > > +@script:python depends on org@
> > > > > +p << r.p;
> > > > > +@@
> > > > > +
> > > > > +msg="WARNING: WARNING: do_div() does a 64-by-32 division, which may
> > > > > truncation the divisor to 32-bit"
> > > > > +coccilib.org.print_todo(p[0], msg)
> > > > > +
> > > > > +@script:python depends on report@
> > > > > +p << r.p;
> > > > > +@@
> > > > > +
> > > > > +msg="WARNING: WARNING: do_div() does a 64-by-32 division, which may
> > > > > truncation the divisor to 32-bit"
> > > > > +coccilib.report.print_report(p[0], msg)
> > > >
> > > > A few small issues: You have WARNING: twice in each case, and truncation
> > > > should be truncate.
> > > >
> > >
> > > Thanks for your comments, we will fix it soon.
> > >
> > > > Is there any generic strategy for fixing these issues?
> > > >
> > >
> > > We have done some experiments, such as:
> > > https://lkml.org/lkml/2020/1/2/1354
> >
> > Thanks.  Actually, I would appreciate knowing about such experiments when
> > the semantic patch is submitted, since eg in this case I am not really an
> > expert in this issue.
> >
> > >
> > > -	avg = rec->time;
> > > -	do_div(avg, rec->counter);
> > > +	avg = div64_ul(rec->time, rec->counter);
> > >
> > > --> Function replacement was performed here,
> > >      and simple code cleanup was also performed.
> > >
> > >
> > > -		do_div(stddev, rec->counter * (rec->counter - 1) * 1000);
> > > +		stddev = div64_ul(stddev,
> > > +				  rec->counter * (rec->counter - 1) * 1000);
> > >
> > > --> Only the function replacement is performed here (because the variable
> > > ‘stddev’ corresponds to a more complicated equation, cleaning it will
> > > reduce
> > > readability).
> >
> > Would it be reasonable to extend the warning to say "consider using
> > div64_ul instead"?  Or do you think it is obvious to everyone?
> >
>
> Thank you for your comments.
> We plan to modify it as follows:
> msg="WARNING: do_div() does a 64-by-32 division, please consider using
> div64_ul, div64_long, div64_u64 or div64_s64 instead."

This message seems helpful, thanks.

julia

>
> > > In addition, there are some codes that do not need to be modified:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/net/can/dev.c#n263
> >
> > Would it be worth having a special case for constants and checking whether
> > the value is obviously safe and no warning is needed?
> >
> Thanks.
> This is very valuable in reducing false positives, and we'll try to implement
> it.
>
> --
> Best Wishes,
> Wen
>
> > > So we just print a warning.
> > > As for how to fix it, we need to analyze the code carefully.
> > >
> > > --
> > > Best Wishes,
> > > Wen
> > >
> > >
> > >
> >
>

[-- Attachment #2: Type: text/plain, Size: 136 bytes --]

_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Cocci] [PATCH] coccinelle: semantic patch to check for inappropriate do_div() calls
  2020-01-04  6:44 [Cocci] [PATCH] coccinelle: semantic patch to check for inappropriate do_div() calls Wen Yang
  2020-01-04  7:00 ` Julia Lawall
  2020-01-04  7:16 ` Julia Lawall
@ 2020-01-05 10:33 ` Markus Elfring
  2020-01-05 10:41   ` Julia Lawall
  2 siblings, 1 reply; 10+ messages in thread
From: Markus Elfring @ 2020-01-05 10:33 UTC (permalink / raw)
  To: Wen Yang, cocci, kernel-janitors
  Cc: Michal Marek, Julia Lawall, Greg Kroah-Hartman, Nicolas Palix,
	Gilles Muller, Matthias Männich, linux-kernel,
	Thomas Gleixner

> +virtual context
> +virtual org
> +virtual report

The operation mode “patch” is not supported here.
Should the term “semantic code search” be used instead in the subject again?


> +@@
> +(
> +* do_div(f, l);
> +|
> +* do_div(f, ul);
> +|
> +* do_div(f, ul64);
> +|
> +* do_div(f, sl64);
> +)

I suggest to avoid the specification of duplicate SmPL code.

+@@
+*do_div(f, \( l \| ul \| ul64 \| sl64 \) );


Will any more case distinctions become helpful?


> +@script:python depends on report@
> +p << r.p;
> +@@
> +
> +msg="WARNING: WARNING: do_div() does a 64-by-32 division, which may truncation the divisor to 32-bit"
> +coccilib.report.print_report(p[0], msg)

Please improve the message construction.

Regards,
Markus
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Cocci] [PATCH] coccinelle: semantic patch to check for inappropriate do_div() calls
  2020-01-05 10:33 ` Markus Elfring
@ 2020-01-05 10:41   ` Julia Lawall
  2020-01-05 12:07     ` [Cocci] " Markus Elfring
  0 siblings, 1 reply; 10+ messages in thread
From: Julia Lawall @ 2020-01-05 10:41 UTC (permalink / raw)
  To: Markus Elfring
  Cc: Michal Marek, Wen Yang, Gilles Muller, Greg Kroah-Hartman,
	Nicolas Palix, kernel-janitors, linux-kernel,
	Matthias Männich, Julia Lawall, Thomas Gleixner, cocci

[-- Attachment #1: Type: text/plain, Size: 1153 bytes --]

On Sun, 5 Jan 2020, Markus Elfring wrote:

> > +virtual context
> > +virtual org
> > +virtual report
>
> The operation mode “patch” is not supported here.
> Should the term “semantic code search” be used instead in the subject again?

Doesn't matter,

>
>
> > +@@
> > +(
> > +* do_div(f, l);
> > +|
> > +* do_div(f, ul);
> > +|
> > +* do_div(f, ul64);
> > +|
> > +* do_div(f, sl64);
> > +)
>
> I suggest to avoid the specification of duplicate SmPL code.
>
> +@@
> +*do_div(f, \( l \| ul \| ul64 \| sl64 \) );

I don't se any point to this.  The code matched will be the same in both
cases.  The original code is quite readable, without the ugly \( etc.

>
> Will any more case distinctions become helpful?
>
>
> > +@script:python depends on report@
> > +p << r.p;
> > +@@
> > +
> > +msg="WARNING: WARNING: do_div() does a 64-by-32 division, which may truncation the divisor to 32-bit"
> > +coccilib.report.print_report(p[0], msg)
>
> Please improve the message construction.

Please make more precise comments (I already made some suggestions, so it
doesn't matter much here, but "please improve" does not provide any
concrete guidance).

julia

[-- Attachment #2: Type: text/plain, Size: 136 bytes --]

_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Cocci] coccinelle: semantic patch to check for inappropriate do_div() calls
  2020-01-05 10:41   ` Julia Lawall
@ 2020-01-05 12:07     ` " Markus Elfring
  0 siblings, 0 replies; 10+ messages in thread
From: Markus Elfring @ 2020-01-05 12:07 UTC (permalink / raw)
  To: Julia Lawall, Wen Yang, cocci
  Cc: Michal Marek, Julia Lawall, Greg Kroah-Hartman, Nicolas Palix,
	Gilles Muller, kernel-janitors, linux-kernel,
	Matthias Männich, Thomas Gleixner

>>> +(
>>> +* do_div(f, l);
>>> +|
>>> +* do_div(f, ul);
>>> +|
>>> +* do_div(f, ul64);
>>> +|
>>> +* do_div(f, sl64);
>>> +)
>>
>> I suggest to avoid the specification of duplicate SmPL code.
>>
>> +@@
>> +*do_div(f, \( l \| ul \| ul64 \| sl64 \) );
>
> I don't se any point to this.

Can such succinct SmPL code be occasionally desirable?


> The original code is quite readable,

Yes. - I dare to present a coding style alternative.


> without the ugly \( etc.

I wonder about this view.


>> Please improve the message construction.
>
> Please make more precise comments (I already made some suggestions,

Thus I omitted a repetition.


> so it doesn't matter much here, but "please improve" does not provide any
> concrete guidance).

I guess that Wen Yang can know corresponding software design possibilities
from previous development discussions.

Regards,
Markus
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, back to index

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-04  6:44 [Cocci] [PATCH] coccinelle: semantic patch to check for inappropriate do_div() calls Wen Yang
2020-01-04  7:00 ` Julia Lawall
2020-01-04  8:49   ` Wen Yang
2020-01-04  8:55     ` Julia Lawall
2020-01-04 13:50       ` Wen Yang
2020-01-04 13:54         ` Julia Lawall
2020-01-04  7:16 ` Julia Lawall
2020-01-05 10:33 ` Markus Elfring
2020-01-05 10:41   ` Julia Lawall
2020-01-05 12:07     ` [Cocci] " Markus Elfring

Coccinelle archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/cocci/0 cocci/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 cocci cocci/ https://lore.kernel.org/cocci \
		cocci@systeme.lip6.fr
	public-inbox-index cocci

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/fr.lip6.systeme.cocci


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git