[Cocci] [PATCH] coccinelle: semantic patch to check for inappropriate do_div() calls

[Cocci] [PATCH] coccinelle: semantic patch to check for inappropriate do_div() calls

[Wen Yang]

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

Re: [Cocci] [PATCH] coccinelle: semantic patch to check for inappropriate do_div() calls

[Julia Lawall]

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

Re: [Cocci] [PATCH] coccinelle: semantic patch to check for inappropriate do_div() calls

[Wen Yang]

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

Re: [Cocci] [PATCH] coccinelle: semantic patch to check for inappropriate do_div() calls

[Julia Lawall]

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

Re: [Cocci] [PATCH] coccinelle: semantic patch to check for inappropriate do_div() calls

[Wen Yang]

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

Re: [Cocci] [PATCH] coccinelle: semantic patch to check for inappropriate do_div() calls

[Julia Lawall]

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

Re: [Cocci] [PATCH] coccinelle: semantic patch to check for inappropriate do_div() calls

[Julia Lawall]

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

Re: [Cocci] [PATCH] coccinelle: semantic patch to check for inappropriate do_div() calls

[Markus Elfring]

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

Re: [Cocci] [PATCH] coccinelle: semantic patch to check for inappropriate do_div() calls

[Julia Lawall]

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

Re: [Cocci] coccinelle: semantic patch to check for inappropriate do_div() calls

[Markus Elfring]

>>> +(
>>> +* 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