Re: [Cocci] [PATCH] coccinelle: api: add kvfree script

Re: [Cocci] [PATCH] coccinelle: api: add kvfree script

[Markus Elfring]

> +@choice@
> +expression E, E1;
> +position kok, vok;
> +@@
> +
> +(
> +  if (...) {
> +    ...
> +    E = \(kmalloc@kok\|…\)(...)

Further implementation details from this SmPL script caught my software
development attention.

* Is there a need to add the specification “when any” to the SmPL ellipses
  before such assignment statements?

* A limited search approach was expressed. Will additional source code variations
  become relevant?
  + switch statement
  + if branches with single statements
  + conditional operator


> +@opportunity depends on !patch …@
…
> +  E = \(kmalloc\|…\)(..., size, ...)
> +  ... when != E = E1
> +      when != size = E1

I wonder that two assignments should be excluded here according to
the same expression metavariable.


+@pkfree depends on patch exists@
…
+- \(kfree\|kvfree\)(E)
++ vfree(E)

Would you like to use a SmPL code variant like the following
at any more places?
(Is it occasionally helpful to increase the change precision?)

+-\(kfree\|kvfree\)
++vfree
+      (E)


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

Re: [Cocci] [PATCH] coccinelle: api: add kvfree script

[Julia Lawall]

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



On Sat, 6 Jun 2020, Markus Elfring wrote:

> > +@choice@
> > +expression E, E1;
> > +position kok, vok;
> > +@@
> > +
> > +(
> > +  if (...) {
> > +    ...
> > +    E = \(kmalloc@kok\|…\)(...)
>
> Further implementation details from this SmPL script caught my software
> development attention.
>
> * Is there a need to add the specification “when any” to the SmPL ellipses
>   before such assignment statements?

Having multiple assignments to kmalloc in one if seems unlikely, and
perhaps one would want to think about such a case differently, so it seems
ok as is.

>
> * A limited search approach was expressed. Will additional source code variations
>   become relevant?
>   + switch statement
>   + if branches with single statements
>   + conditional operator

The point is that there is a kmalloc in one branch and a vmalloc in
another branch, so a if with a single branch doesn't seem relevant.

The other cases sem highly improbable.

>
> > +@opportunity depends on !patch …@
> …
> > +  E = \(kmalloc\|…\)(..., size, ...)
> > +  ... when != E = E1
> > +      when != size = E1
>
> I wonder that two assignments should be excluded here according to
> the same expression metavariable.

Doesn't matter.  The metavariables are considered separately in the
different whens.

>
> +@pkfree depends on patch exists@
> …
> +- \(kfree\|kvfree\)(E)
> ++ vfree(E)
>
> Would you like to use a SmPL code variant like the following
> at any more places?
> (Is it occasionally helpful to increase the change precision?)
>
> +-\(kfree\|kvfree\)
> ++vfree
> +      (E)

"increase the change precision" seems to be an obscure way to say "improve
the formatting".  Indeed, leaving (E) as is would have the effect of not
changing the formatting.  But the problem seems unlikely for a functoin
with such a short name.  And this presentation will likely run afoul of
the fact that you can't attach + code to a disjunction.  So the original
presentation was more concise, and should be fine in practice.

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: api: add kvfree script

[Markus Elfring]

>> * A limited search approach was expressed. Will additional source code variations
>>   become relevant?
>>   + switch statement
>>   + if branches with single statements
>>   + conditional operator
>
> The point is that there is a kmalloc in one branch and a vmalloc in
> another branch, so a if with a single branch doesn't seem relevant.

Is an other wording more appropriate to handle if/else statements
without curly brackets?


> The other cases sem highly improbable.

This can be.

But how much do such details influence the confidence level
for such a SmPL script?


>>> +@opportunity depends on !patch …@
>> …
>>> +  E = \(kmalloc\|…\)(..., size, ...)
>>> +  ... when != E = E1
>>> +      when != size = E1
>>
>> I wonder that two assignments should be excluded here according to
>> the same expression metavariable.
>
> Doesn't matter.

Would different variable names reduce the potential for confusion?


> The metavariables are considered separately in the different whens.

Is this information relevant for a better software documentation?


>>> +@pkfree depends on patch exists@
>> …
>>> +- \(kfree\|kvfree\)(E)
>>> ++ vfree(E)
>>
>> Would you like to use a SmPL code variant like the following
>> at any more places?
>> (Is it occasionally helpful to increase the change precision?)
>>
>> +-\(kfree\|kvfree\)
>> ++vfree
>> +      (E)
>
> "increase the change precision" seems to be an obscure way to say "improve
> the formatting".

We come along a different understanding of such a transformation approach
once more.


> Indeed, leaving (E) as is would have the effect of not changing the formatting.

I just propose to leave source code unmodified as much as possible here.


> But the problem seems unlikely for a functoin with such a short name.

This can be.


> And this presentation will likely run afoul of the fact
> that you can't attach + code to a disjunction.

There is a minus character before such SmPL disjunctions.


> So the original presentation was more concise, and should be fine in practice.

Is less duplicated SmPL code useful?

I point a design alternative out.
Would you like to integrate it anyhow?

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

Re: [Cocci] [PATCH] coccinelle: api: add kvfree script

[Denis Efremov]

On 6/14/20 12:17 PM, Julia Lawall wrote:
> 
> 
> On Sun, 14 Jun 2020, Denis Efremov wrote:
> 
>>
>>
>> On 6/5/20 11:51 PM, Julia Lawall wrote:
>>> Also, there is no need to exceed 80 characters here.  You can put a
>>> newline in the middle of a \( ... \)
>>
>> It's required. Looks like it's impossible to break "when" lines.
> 
> That's true. Sorry for the noise.
> 

Anyway, I will send v2 with other lines fixed.

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

Re: [Cocci] [PATCH] coccinelle: api: add kvfree script

[Julia Lawall]

On Sun, 14 Jun 2020, Denis Efremov wrote:

>
>
> On 6/5/20 11:51 PM, Julia Lawall wrote:
> > Also, there is no need to exceed 80 characters here.  You can put a
> > newline in the middle of a \( ... \)
>
> It's required. Looks like it's impossible to break "when" lines.

That's true. Sorry for the noise.

julia

>
> ... when != if (...) { ... E = \(vmalloc\|vzalloc\|vmalloc_user\|vmalloc_node\|vzalloc_node\|vmalloc_exec\|vmalloc_32\|vmalloc_32_user\|__vmalloc\|__vmalloc_node_range\|__vmalloc_node\|kvmalloc\|kvzalloc\|kvcalloc\|kvzalloc_node\|kvmalloc_node\|kvmalloc_array\)(...); ... }
>
> Thanks,
> Denis
>
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH] coccinelle: api: add kvfree script

[Denis Efremov]

On 6/5/20 11:51 PM, Julia Lawall wrote:
> Also, there is no need to exceed 80 characters here.  You can put a
> newline in the middle of a \( ... \)

It's required. Looks like it's impossible to break "when" lines.

... when != if (...) { ... E = \(vmalloc\|vzalloc\|vmalloc_user\|vmalloc_node\|vzalloc_node\|vmalloc_exec\|vmalloc_32\|vmalloc_32_user\|__vmalloc\|__vmalloc_node_range\|__vmalloc_node\|kvmalloc\|kvzalloc\|kvcalloc\|kvzalloc_node\|kvmalloc_node\|kvmalloc_array\)(...); ... }

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

Re: [Cocci] [PATCH] coccinelle: api: add kvfree script

[Julia Lawall]

> > +    E = \(kmalloc@kok\|kzalloc@kok\|krealloc@kok\|kcalloc@kok\|kmalloc_node@kok\|kzalloc_node@kok\|kmalloc_array@kok\|kmalloc_array_node@kok\|kcalloc_node@kok\)(...)
>
> I would prefer an other coding style here.
>
> * Items for such SmPL disjunctions can be specified also on multiple lines.
>
> * The semantic patch language supports further means to handle function name lists
>   in more convenient ways.
>   Would you like to work with customised constraints?

Please don't follow this advice.  Coccinelle is not able to optimize its
search process according to the information in constraints.  It will
needlessly parse many files.

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

Re: [Cocci] [PATCH] coccinelle: api: add kvfree script

[Markus Elfring]

> Check that alloc and free types of functions match each other.

Further software development challenges are interesting also for such an use case.


> +/// Check that kvmalloc'ed memory is freed by kfree functions,
> +/// vmalloc'ed by vfree functions and kvmalloc'ed by kvfree
> +/// functions.

* How do you think about to offer a wording suggestion for subjects of
  generated patches?

* Will the presented case distinction trigger further improvements for
  the desired matching?

* Would you like to generalise the safe handling of allocations
  and corresponding release of system resources?


> +// Confidence: High

I suggest to reconsider this information once more.


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

+virtual patch, report, org, context

Is such a SmPL code variant more succinct?


> +@choice@

* Can it be that this SmPL rule is not relevant for all operation modes?

* Will additional dependencies matter?


> +    E = \(kmalloc@kok\|kzalloc@kok\|krealloc@kok\|kcalloc@kok\|kmalloc_node@kok\|kzalloc_node@kok\|kmalloc_array@kok\|kmalloc_array_node@kok\|kcalloc_node@kok\)(...)

I would prefer an other coding style here.

* Items for such SmPL disjunctions can be specified also on multiple lines.

* The semantic patch language supports further means to handle function name lists
  in more convenient ways.
  Would you like to work with customised constraints?


> +msg = "WARNING: kmalloc is used to allocate this memory at line %s" % (k[0].line)
> +coccilib.report.print_report(p[0], msg)

* I propose once more omit the extra variable “msg” at similar places.
  The desired message object can be directly passed as a function parameter.

* I find the diagnostic text insufficient.

* Can the corresponding function category be dynamically determined?


Are you looking for opportunities to avoid unwanted code duplication?

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

Re: [Cocci] [PATCH] coccinelle: api: add kvfree script

[Julia Lawall]

On Sat, 6 Jun 2020, Denis Efremov wrote:

> On 6/5/20 11:51 PM, Julia Lawall wrote:
> > Is there a strong reason for putting the choice rule first?  It may make
> > things somewhat slower than necessary, if it matches in many places,
> > because the opportunity rule will have to detect that it doesn't care
> > about all of those places.
>
> No, I didn't know that order of rules matters. I just checked it, my PC
> shows no difference in exec time if I swap these rules.

OK, probably choice doesn't match in very many places, so there is not
much impact.

julia

>
> "choice" doesn't check the size. "opportunity" is more strict.
> The motivation for adding 2 rules is that we could recommend to use
> kvmalloc* only when there is a size condition. At the same time, we
> should skip all if (...) {kmalloc()} else {vmalloc()},
> res = kmalloc() if (!res) {vmalloc()} cases as false positives.
>
> It seems that it's not possible to use subexpression rule
> "expression size <= choice.E" in this case.
>
> > Also, there is no need to exceed 80 characters here.  You can put a
> > newline in the middle of a \( ... \)
>
> Ok, I will fix it in v2 after all comments/suggestions.
>
> Thanks,
> Denis
>
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH] coccinelle: api: add kvfree script

[Denis Efremov]

On 6/5/20 11:51 PM, Julia Lawall wrote:
> Is there a strong reason for putting the choice rule first?  It may make
> things somewhat slower than necessary, if it matches in many places,
> because the opportunity rule will have to detect that it doesn't care
> about all of those places.

No, I didn't know that order of rules matters. I just checked it, my PC
shows no difference in exec time if I swap these rules.

"choice" doesn't check the size. "opportunity" is more strict.
The motivation for adding 2 rules is that we could recommend to use
kvmalloc* only when there is a size condition. At the same time, we
should skip all if (...) {kmalloc()} else {vmalloc()},
res = kmalloc() if (!res) {vmalloc()} cases as false positives.

It seems that it's not possible to use subexpression rule
"expression size <= choice.E" in this case.

> Also, there is no need to exceed 80 characters here.  You can put a
> newline in the middle of a \( ... \)

Ok, I will fix it in v2 after all comments/suggestions.

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

Re: [Cocci] [PATCH] coccinelle: api: add kvfree script

[Julia Lawall]

On Fri, 5 Jun 2020, Denis Efremov wrote:

> Check that alloc and free types of functions match each other.

Is there a strong reason for putting the choice rule first?  It may make
things somewhat slower than necessary, if it matches in many places,
because the opportunity rule will have to detect that it doesn't care
about all of those places.

Also, there is no need to exceed 80 characters here.  You can put a
newline in the middle of a \( ... \)

julia

>
> Signed-off-by: Denis Efremov <efremov@linux.com>
> ---
> List of patches to stable:
> - https://lkml.org/lkml/2020/6/1/713
> - https://lkml.org/lkml/2020/6/5/200
> - https://lkml.org/lkml/2020/6/5/838
> - https://lkml.org/lkml/2020/6/5/887
>
> Other patches:
> - https://lkml.org/lkml/2020/6/1/701
> - https://lkml.org/lkml/2020/6/5/839
> - https://lkml.org/lkml/2020/6/5/864
> - https://lkml.org/lkml/2020/6/5/865
> - https://lkml.org/lkml/2020/6/5/895
> - https://lkml.org/lkml/2020/6/5/901
>
> There is a false positive that I can't beat:
> fs/btrfs/send.c:1119:11-12: WARNING: kmalloc is used to allocate
> this memory at line 1036
>
>  scripts/coccinelle/api/kvfree.cocci | 196 ++++++++++++++++++++++++++++
>  1 file changed, 196 insertions(+)
>  create mode 100644 scripts/coccinelle/api/kvfree.cocci
>
> diff --git a/scripts/coccinelle/api/kvfree.cocci b/scripts/coccinelle/api/kvfree.cocci
> new file mode 100644
> index 000000000000..e3fa3d0fd2fd
> --- /dev/null
> +++ b/scripts/coccinelle/api/kvfree.cocci
> @@ -0,0 +1,196 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +///
> +/// Check that kvmalloc'ed memory is freed by kfree functions,
> +/// vmalloc'ed by vfree functions and kvmalloc'ed by kvfree
> +/// functions.
> +///
> +// Confidence: High
> +// Copyright: (C) 2020 Denis Efremov ISPRAS
> +// Options: --no-includes --include-headers
> +//
> +
> +virtual patch
> +virtual report
> +virtual org
> +virtual context
> +
> +
> +@choice@
> +expression E, E1;
> +position kok, vok;
> +@@
> +
> +(
> +  if (...) {
> +    ...
> +    E = \(kmalloc@kok\|kzalloc@kok\|krealloc@kok\|kcalloc@kok\|kmalloc_node@kok\|kzalloc_node@kok\|kmalloc_array@kok\|kmalloc_array_node@kok\|kcalloc_node@kok\)(...)
> +    ...
> +  } else {
> +    ...
> +    E = \(vmalloc@vok\|vzalloc@vok\|vmalloc_user@vok\|vmalloc_node@vok\|vzalloc_node@vok\|vmalloc_exec@vok\|vmalloc_32@vok\|vmalloc_32_user@vok\|__vmalloc@vok\|__vmalloc_node_range@vok\|__vmalloc_node@vok\)(...)
> +    ...
> +  }
> +|
> +  E = \(kmalloc\|kzalloc\|krealloc\|kcalloc\|kmalloc_node\|kzalloc_node\|kmalloc_array\|kmalloc_array_node\|kcalloc_node\)(...)
> +  ... when != E = E1
> +      when any
> +  if (\(!E\|E == NULL\)) {
> +    ...
> +    E = \(vmalloc@vok\|vzalloc@vok\|vmalloc_user@vok\|vmalloc_node@vok\|vzalloc_node@vok\|vmalloc_exec@vok\|vmalloc_32@vok\|vmalloc_32_user@vok\|__vmalloc@vok\|__vmalloc_node_range@vok\|__vmalloc_node@vok\)(...)
> +    ...
> +  }
> +)
> +
> +// exclude mm/vmalloc.c because of kvmalloc* definitions
> +@opportunity depends on !patch && !(file in "mm/vmalloc.c")@
> +expression E, E1, size;
> +position p;
> +@@
> +
> +(
> +* if (\(size <= E1\|size < E1\|size = E1\|size > E1\) || ...)@p {
> +    ...
> +    E = \(kmalloc\|kzalloc\|krealloc\|kcalloc\|kmalloc_node\|kzalloc_node\|kmalloc_array\|kmalloc_array_node\|kcalloc_node\)(..., size, ...)
> +    ...
> +  } else {
> +    ...
> +    E = \(vmalloc\|vzalloc\|vmalloc_user\|vmalloc_node\|vzalloc_node\|vmalloc_exec\|vmalloc_32\|vmalloc_32_user\|__vmalloc\|__vmalloc_node_range\|__vmalloc_node\)(..., size, ...)
> +    ...
> +  }
> +|
> +  E = \(kmalloc\|kzalloc\|krealloc\|kcalloc\|kmalloc_node\|kzalloc_node\|kmalloc_array\|kmalloc_array_node\|kcalloc_node\)(..., size, ...)
> +  ... when != E = E1
> +      when != size = E1
> +      when any
> +* if (\(!E\|E == NULL\))@p {
> +    ...
> +    E = \(vmalloc\|vzalloc\|vmalloc_user\|vmalloc_node\|vzalloc_node\|vmalloc_exec\|vmalloc_32\|vmalloc_32_user\|__vmalloc\|__vmalloc_node_range\|__vmalloc_node\)(..., size, ...)
> +    ...
> +  }
> +)
> +
> +@vfree depends on !patch@
> +expression E;
> +position k != choice.kok;
> +position p;
> +@@
> +
> +* E = \(kmalloc@k\|kzalloc@k\|krealloc@k\|kcalloc@k\|kmalloc_node@k\|kzalloc_node@k\|kmalloc_array@k\|kmalloc_array_node@k\|kcalloc_node@k\)(...)
> +  ... when != if (...) { ... E = \(vmalloc\|vzalloc\|vmalloc_user\|vmalloc_node\|vzalloc_node\|vmalloc_exec\|vmalloc_32\|vmalloc_32_user\|__vmalloc\|__vmalloc_node_range\|__vmalloc_node\|kvmalloc\|kvzalloc\|kvcalloc\|kvzalloc_node\|kvmalloc_node\|kvmalloc_array\)(...); ... }
> +      when != is_vmalloc_addr(E)
> +      when any
> +* \(vfree\|vfree_atomic\|kvfree\)(E)@p
> +
> +@pvfree depends on patch exists@
> +expression E;
> +position k != choice.kok;
> +@@
> +
> +  E = \(kmalloc@k\|kzalloc@k\|krealloc@k\|kcalloc@k\|kmalloc_node@k\|kzalloc_node@k\|kmalloc_array@k\|kmalloc_array_node@k\|kcalloc_node@k\)(...)
> +  ... when != if (...) { ... E = \(vmalloc\|vzalloc\|vmalloc_user\|vmalloc_node\|vzalloc_node\|vmalloc_exec\|vmalloc_32\|vmalloc_32_user\|__vmalloc\|__vmalloc_node_range\|__vmalloc_node\|kvmalloc\|kvzalloc\|kvcalloc\|kvzalloc_node\|kvmalloc_node\|kvmalloc_array\)(...); ... }
> +      when != is_vmalloc_addr(E)
> +      when any
> +- \(vfree\|vfree_atomic\|kvfree\)(E)
> ++ kfree(E)
> +
> +@kfree depends on !patch@
> +expression E;
> +position v != choice.vok;
> +position p;
> +@@
> +
> +* E = \(vmalloc@v\|vzalloc@v\|vmalloc_user@v\|vmalloc_node@v\|vzalloc_node@v\|vmalloc_exec@v\|vmalloc_32@v\|vmalloc_32_user@v\|__vmalloc@v\|__vmalloc_node_range@v\|__vmalloc_node@v\)(...)
> +  ... when != !is_vmalloc_addr(E)
> +      when any
> +* \(kfree\|kzfree\|kvfree\)(E)
> +
> +@pkfree depends on patch exists@
> +expression E;
> +position v != choice.vok;
> +@@
> +
> +  E = \(vmalloc@v\|vzalloc@v\|vmalloc_user@v\|vmalloc_node@v\|vzalloc_node@v\|vmalloc_exec@v\|vmalloc_32@v\|vmalloc_32_user@v\|__vmalloc@v\|__vmalloc_node_range@v\|__vmalloc_node@v\)(...)
> +  ... when != !is_vmalloc_addr(E)
> +      when any
> +- \(kfree\|kvfree\)(E)
> ++ vfree(E)
> +
> +@kvfree depends on !patch@
> +expression E;
> +position p, k;
> +@@
> +
> +* E = \(kvmalloc\|kvzalloc\|kvcalloc\|kvzalloc_node\|kvmalloc_node\|kvmalloc_array\)(...)@k
> +  ... when != is_vmalloc_addr(E)
> +      when any
> +* \(kfree\|kzfree\|vfree\|vfree_atomic\)(E)@p
> +
> +@pkvfree depends on patch exists@
> +expression E;
> +@@
> +
> +  E = \(kvmalloc\|kvzalloc\|kvcalloc\|kvzalloc_node\|kvmalloc_node\|kvmalloc_array\)(...)
> +  ... when != is_vmalloc_addr(E)
> +      when any
> +- \(kfree\|vfree\)(E)
> ++ kvfree(E)
> +
> +@script: python depends on report@
> +k << vfree.k;
> +p << vfree.p;
> +@@
> +
> +msg = "WARNING: kmalloc is used to allocate this memory at line %s" % (k[0].line)
> +coccilib.report.print_report(p[0], msg)
> +
> +@script: python depends on org@
> +k << vfree.k;
> +p << vfree.p;
> +@@
> +
> +msg = "WARNING: kmalloc is used to allocate this memory at line %s" % (k[0].line)
> +coccilib.org.print_todo(p[0], msg)
> +
> +@script: python depends on report@
> +v << kfree.v;
> +p << kfree.p;
> +@@
> +
> +msg = "WARNING: vmalloc is used to allocate this memory at line %s" % (v[0].line)
> +coccilib.report.print_report(p[0], msg)
> +
> +@script: python depends on org@
> +v << kfree.v;
> +p << kfree.p;
> +@@
> +
> +msg = "WARNING: vmalloc is used to allocate this memory at line %s" % (v[0].line)
> +coccilib.org.print_todo(p[0], msg)
> +
> +@script: python depends on report@
> +k << kvfree.k;
> +p << kvfree.p;
> +@@
> +
> +msg = "WARNING: kvmalloc is used to allocate this memory at line %s" % (k[0].line)
> +coccilib.report.print_report(p[0], msg)
> +
> +@script: python depends on org@
> +k << kvfree.k;
> +p << kvfree.p;
> +@@
> +
> +msg = "WARNING: kvmalloc is used to allocate this memory at line %s" % (k[0].line)
> +coccilib.org.print_todo(p[0], msg)
> +
> +@script: python depends on report@
> +p << opportunity.p;
> +@@
> +
> +coccilib.report.print_report(p[0], "WARNING: opportunity for kvmalloc")
> +
> +@script: python depends on org@
> +p << opportunity.p;
> +@@
> +
> +coccilib.org.print_todo(p[0], "WARNING: opportunity for kvmalloc")
> --
> 2.26.2
>
> _______________________________________________
> Cocci mailing list
> Cocci@systeme.lip6.fr
> https://systeme.lip6.fr/mailman/listinfo/cocci
>
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

[Cocci] [PATCH] coccinelle: api: add kvfree script

[Denis Efremov]

Check that alloc and free types of functions match each other.

Signed-off-by: Denis Efremov <efremov@linux.com>
---
List of patches to stable:
- https://lkml.org/lkml/2020/6/1/713
- https://lkml.org/lkml/2020/6/5/200
- https://lkml.org/lkml/2020/6/5/838
- https://lkml.org/lkml/2020/6/5/887

Other patches:
- https://lkml.org/lkml/2020/6/1/701
- https://lkml.org/lkml/2020/6/5/839
- https://lkml.org/lkml/2020/6/5/864
- https://lkml.org/lkml/2020/6/5/865
- https://lkml.org/lkml/2020/6/5/895
- https://lkml.org/lkml/2020/6/5/901

There is a false positive that I can't beat:
fs/btrfs/send.c:1119:11-12: WARNING: kmalloc is used to allocate
this memory at line 1036

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

diff --git a/scripts/coccinelle/api/kvfree.cocci b/scripts/coccinelle/api/kvfree.cocci
new file mode 100644
index 000000000000..e3fa3d0fd2fd
--- /dev/null
+++ b/scripts/coccinelle/api/kvfree.cocci
@@ -0,0 +1,196 @@
+// SPDX-License-Identifier: GPL-2.0-only
+///
+/// Check that kvmalloc'ed memory is freed by kfree functions,
+/// vmalloc'ed by vfree functions and kvmalloc'ed by kvfree
+/// functions.
+///
+// Confidence: High
+// Copyright: (C) 2020 Denis Efremov ISPRAS
+// Options: --no-includes --include-headers
+//
+
+virtual patch
+virtual report
+virtual org
+virtual context
+
+
+@choice@
+expression E, E1;
+position kok, vok;
+@@
+
+(
+  if (...) {
+    ...
+    E = \(kmalloc@kok\|kzalloc@kok\|krealloc@kok\|kcalloc@kok\|kmalloc_node@kok\|kzalloc_node@kok\|kmalloc_array@kok\|kmalloc_array_node@kok\|kcalloc_node@kok\)(...)
+    ...
+  } else {
+    ...
+    E = \(vmalloc@vok\|vzalloc@vok\|vmalloc_user@vok\|vmalloc_node@vok\|vzalloc_node@vok\|vmalloc_exec@vok\|vmalloc_32@vok\|vmalloc_32_user@vok\|__vmalloc@vok\|__vmalloc_node_range@vok\|__vmalloc_node@vok\)(...)
+    ...
+  }
+|
+  E = \(kmalloc\|kzalloc\|krealloc\|kcalloc\|kmalloc_node\|kzalloc_node\|kmalloc_array\|kmalloc_array_node\|kcalloc_node\)(...)
+  ... when != E = E1
+      when any
+  if (\(!E\|E == NULL\)) {
+    ...
+    E = \(vmalloc@vok\|vzalloc@vok\|vmalloc_user@vok\|vmalloc_node@vok\|vzalloc_node@vok\|vmalloc_exec@vok\|vmalloc_32@vok\|vmalloc_32_user@vok\|__vmalloc@vok\|__vmalloc_node_range@vok\|__vmalloc_node@vok\)(...)
+    ...
+  }
+)
+
+// exclude mm/vmalloc.c because of kvmalloc* definitions
+@opportunity depends on !patch && !(file in "mm/vmalloc.c")@
+expression E, E1, size;
+position p;
+@@
+
+(
+* if (\(size <= E1\|size < E1\|size = E1\|size > E1\) || ...)@p {
+    ...
+    E = \(kmalloc\|kzalloc\|krealloc\|kcalloc\|kmalloc_node\|kzalloc_node\|kmalloc_array\|kmalloc_array_node\|kcalloc_node\)(..., size, ...)
+    ...
+  } else {
+    ...
+    E = \(vmalloc\|vzalloc\|vmalloc_user\|vmalloc_node\|vzalloc_node\|vmalloc_exec\|vmalloc_32\|vmalloc_32_user\|__vmalloc\|__vmalloc_node_range\|__vmalloc_node\)(..., size, ...)
+    ...
+  }
+|
+  E = \(kmalloc\|kzalloc\|krealloc\|kcalloc\|kmalloc_node\|kzalloc_node\|kmalloc_array\|kmalloc_array_node\|kcalloc_node\)(..., size, ...)
+  ... when != E = E1
+      when != size = E1
+      when any
+* if (\(!E\|E == NULL\))@p {
+    ...
+    E = \(vmalloc\|vzalloc\|vmalloc_user\|vmalloc_node\|vzalloc_node\|vmalloc_exec\|vmalloc_32\|vmalloc_32_user\|__vmalloc\|__vmalloc_node_range\|__vmalloc_node\)(..., size, ...)
+    ...
+  }
+)
+
+@vfree depends on !patch@
+expression E;
+position k != choice.kok;
+position p;
+@@
+
+* E = \(kmalloc@k\|kzalloc@k\|krealloc@k\|kcalloc@k\|kmalloc_node@k\|kzalloc_node@k\|kmalloc_array@k\|kmalloc_array_node@k\|kcalloc_node@k\)(...)
+  ... when != if (...) { ... E = \(vmalloc\|vzalloc\|vmalloc_user\|vmalloc_node\|vzalloc_node\|vmalloc_exec\|vmalloc_32\|vmalloc_32_user\|__vmalloc\|__vmalloc_node_range\|__vmalloc_node\|kvmalloc\|kvzalloc\|kvcalloc\|kvzalloc_node\|kvmalloc_node\|kvmalloc_array\)(...); ... }
+      when != is_vmalloc_addr(E)
+      when any
+* \(vfree\|vfree_atomic\|kvfree\)(E)@p
+
+@pvfree depends on patch exists@
+expression E;
+position k != choice.kok;
+@@
+
+  E = \(kmalloc@k\|kzalloc@k\|krealloc@k\|kcalloc@k\|kmalloc_node@k\|kzalloc_node@k\|kmalloc_array@k\|kmalloc_array_node@k\|kcalloc_node@k\)(...)
+  ... when != if (...) { ... E = \(vmalloc\|vzalloc\|vmalloc_user\|vmalloc_node\|vzalloc_node\|vmalloc_exec\|vmalloc_32\|vmalloc_32_user\|__vmalloc\|__vmalloc_node_range\|__vmalloc_node\|kvmalloc\|kvzalloc\|kvcalloc\|kvzalloc_node\|kvmalloc_node\|kvmalloc_array\)(...); ... }
+      when != is_vmalloc_addr(E)
+      when any
+- \(vfree\|vfree_atomic\|kvfree\)(E)
++ kfree(E)
+
+@kfree depends on !patch@
+expression E;
+position v != choice.vok;
+position p;
+@@
+
+* E = \(vmalloc@v\|vzalloc@v\|vmalloc_user@v\|vmalloc_node@v\|vzalloc_node@v\|vmalloc_exec@v\|vmalloc_32@v\|vmalloc_32_user@v\|__vmalloc@v\|__vmalloc_node_range@v\|__vmalloc_node@v\)(...)
+  ... when != !is_vmalloc_addr(E)
+      when any
+* \(kfree\|kzfree\|kvfree\)(E)
+
+@pkfree depends on patch exists@
+expression E;
+position v != choice.vok;
+@@
+
+  E = \(vmalloc@v\|vzalloc@v\|vmalloc_user@v\|vmalloc_node@v\|vzalloc_node@v\|vmalloc_exec@v\|vmalloc_32@v\|vmalloc_32_user@v\|__vmalloc@v\|__vmalloc_node_range@v\|__vmalloc_node@v\)(...)
+  ... when != !is_vmalloc_addr(E)
+      when any
+- \(kfree\|kvfree\)(E)
++ vfree(E)
+
+@kvfree depends on !patch@
+expression E;
+position p, k;
+@@
+
+* E = \(kvmalloc\|kvzalloc\|kvcalloc\|kvzalloc_node\|kvmalloc_node\|kvmalloc_array\)(...)@k
+  ... when != is_vmalloc_addr(E)
+      when any
+* \(kfree\|kzfree\|vfree\|vfree_atomic\)(E)@p
+
+@pkvfree depends on patch exists@
+expression E;
+@@
+
+  E = \(kvmalloc\|kvzalloc\|kvcalloc\|kvzalloc_node\|kvmalloc_node\|kvmalloc_array\)(...)
+  ... when != is_vmalloc_addr(E)
+      when any
+- \(kfree\|vfree\)(E)
++ kvfree(E)
+
+@script: python depends on report@
+k << vfree.k;
+p << vfree.p;
+@@
+
+msg = "WARNING: kmalloc is used to allocate this memory at line %s" % (k[0].line)
+coccilib.report.print_report(p[0], msg)
+
+@script: python depends on org@
+k << vfree.k;
+p << vfree.p;
+@@
+
+msg = "WARNING: kmalloc is used to allocate this memory at line %s" % (k[0].line)
+coccilib.org.print_todo(p[0], msg)
+
+@script: python depends on report@
+v << kfree.v;
+p << kfree.p;
+@@
+
+msg = "WARNING: vmalloc is used to allocate this memory at line %s" % (v[0].line)
+coccilib.report.print_report(p[0], msg)
+
+@script: python depends on org@
+v << kfree.v;
+p << kfree.p;
+@@
+
+msg = "WARNING: vmalloc is used to allocate this memory at line %s" % (v[0].line)
+coccilib.org.print_todo(p[0], msg)
+
+@script: python depends on report@
+k << kvfree.k;
+p << kvfree.p;
+@@
+
+msg = "WARNING: kvmalloc is used to allocate this memory at line %s" % (k[0].line)
+coccilib.report.print_report(p[0], msg)
+
+@script: python depends on org@
+k << kvfree.k;
+p << kvfree.p;
+@@
+
+msg = "WARNING: kvmalloc is used to allocate this memory at line %s" % (k[0].line)
+coccilib.org.print_todo(p[0], msg)
+
+@script: python depends on report@
+p << opportunity.p;
+@@
+
+coccilib.report.print_report(p[0], "WARNING: opportunity for kvmalloc")
+
+@script: python depends on org@
+p << opportunity.p;
+@@
+
+coccilib.org.print_todo(p[0], "WARNING: opportunity for kvmalloc")
-- 
2.26.2

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