[Cocci] [PATCH v2 0/4] Update memdup_user.cocci

[Cocci] [PATCH v2 0/4] Update memdup_user.cocci

[Denis Efremov]

Add GFP_USER to the allocation flags and handle vmemdup_user().
The third patch supresses memdup_user(), vmemdup_user() functions
detection. Last patch is a proof of concept for the rule selfchecking.
Gives the ability to detect that an open-coded pattern in a function
definition that we search for in the kernel sources changed.

Denis Efremov (4):
  coccinelle: api: extend memdup_user transformation with GFP_USER
  coccinelle: api: extend memdup_user rule with vmemdup_user()
  coccinelle: api: filter out memdup_user definitions
  coccinelle: api: add selfcheck for memdup_user rule

 scripts/coccinelle/api/memdup_user.cocci | 106 ++++++++++++++++++++++-
 1 file changed, 103 insertions(+), 3 deletions(-)

-- 
2.26.2

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

[Cocci] [PATCH v2 1/4] coccinelle: api: extend memdup_user transformation with GFP_USER

[Denis Efremov]

Match GFP_USER and optional __GFP_NOWARN allocations with
memdup_user.cocci rule.
Commit 6c2c97a24f09 ("memdup_user(): switch to GFP_USER") switched
memdup_user() from GFP_KERNEL to GFP_USER. In almost all cases it
is still a good idea to recommend memdup_user() for GFP_KERNEL
allocations. The motivation behind altering memdup_user() to GFP_USER:
https://lkml.org/lkml/2018/1/6/333

Signed-off-by: Denis Efremov <efremov@linux.com>
---
 scripts/coccinelle/api/memdup_user.cocci | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/scripts/coccinelle/api/memdup_user.cocci b/scripts/coccinelle/api/memdup_user.cocci
index c809ab10bbce..cadcc2e87881 100644
--- a/scripts/coccinelle/api/memdup_user.cocci
+++ b/scripts/coccinelle/api/memdup_user.cocci
@@ -20,7 +20,9 @@ expression from,to,size;
 identifier l1,l2;
 @@
 
--  to = \(kmalloc\|kzalloc\)(size,GFP_KERNEL);
+-  to = \(kmalloc\|kzalloc\)
+		(size,\(GFP_KERNEL\|GFP_USER\|
+		      \(GFP_KERNEL\|GFP_USER\)|__GFP_NOWARN\));
 +  to = memdup_user(from,size);
    if (
 -      to==NULL
@@ -43,7 +45,9 @@ position p;
 statement S1,S2;
 @@
 
-*  to = \(kmalloc@p\|kzalloc@p\)(size,GFP_KERNEL);
+*  to = \(kmalloc@p\|kzalloc@p\)
+		(size,\(GFP_KERNEL\|GFP_USER\|
+		      \(GFP_KERNEL\|GFP_USER\)|__GFP_NOWARN\));
    if (to==NULL || ...) S1
    if (copy_from_user(to, from, size) != 0)
    S2
-- 
2.26.2

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

[Cocci] [PATCH v2 2/4] coccinelle: api: extend memdup_user rule with vmemdup_user()

[Denis Efremov]

Add vmemdup_user() transformations to the memdup_user.cocci rule.
Commit 50fd2f298bef ("new primitive: vmemdup_user()") introduced
vmemdup_user(). The function uses kvmalloc with GPF_USER flag.

Signed-off-by: Denis Efremov <efremov@linux.com>
---
 scripts/coccinelle/api/memdup_user.cocci | 45 ++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/scripts/coccinelle/api/memdup_user.cocci b/scripts/coccinelle/api/memdup_user.cocci
index cadcc2e87881..d15c061a34ab 100644
--- a/scripts/coccinelle/api/memdup_user.cocci
+++ b/scripts/coccinelle/api/memdup_user.cocci
@@ -39,6 +39,28 @@ identifier l1,l2;
 -    ...+>
 -  }
 
+@depends on patch@
+expression from,to,size;
+identifier l1,l2;
+@@
+
+-  to = \(kvmalloc\|kvzalloc\)(size,\(GFP_KERNEL\|GFP_USER\));
++  to = vmemdup_user(from,size);
+   if (
+-      to==NULL
++      IS_ERR(to)
+                 || ...) {
+   <+... when != goto l1;
+-  -ENOMEM
++  PTR_ERR(to)
+   ...+>
+   }
+-  if (copy_from_user(to, from, size) != 0) {
+-    <+... when != goto l2;
+-    -EFAULT
+-    ...+>
+-  }
+
 @r depends on !patch@
 expression from,to,size;
 position p;
@@ -52,6 +74,17 @@ statement S1,S2;
    if (copy_from_user(to, from, size) != 0)
    S2
 
+@rv depends on !patch@
+expression from,to,size;
+position p;
+statement S1,S2;
+@@
+
+*  to = \(kvmalloc@p\|kvzalloc@p\)(size,\(GFP_KERNEL\|GFP_USER\));
+   if (to==NULL || ...) S1
+   if (copy_from_user(to, from, size) != 0)
+   S2
+
 @script:python depends on org@
 p << r.p;
 @@
@@ -63,3 +96,15 @@ p << r.p;
 @@
 
 coccilib.report.print_report(p[0], "WARNING opportunity for memdup_user")
+
+@script:python depends on org@
+p << rv.p;
+@@
+
+coccilib.org.print_todo(p[0], "WARNING opportunity for vmemdup_user")
+
+@script:python depends on report@
+p << rv.p;
+@@
+
+coccilib.report.print_report(p[0], "WARNING opportunity for vmemdup_user")
-- 
2.26.2

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

[Cocci] [PATCH v2 3/4] coccinelle: api: filter out memdup_user definitions

[Denis Efremov]

Don't match original implementations.

Signed-off-by: Denis Efremov <efremov@linux.com>
---
 scripts/coccinelle/api/memdup_user.cocci | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/scripts/coccinelle/api/memdup_user.cocci b/scripts/coccinelle/api/memdup_user.cocci
index d15c061a34ab..8621bd98be1e 100644
--- a/scripts/coccinelle/api/memdup_user.cocci
+++ b/scripts/coccinelle/api/memdup_user.cocci
@@ -15,12 +15,20 @@ virtual context
 virtual org
 virtual report
 
+@initialize:python@
+@@
+filter = frozenset(['memdup_user', 'vmemdup_user'])
+
+def relevant(p):
+    return not (filter & {el.current_element for el in p})
+
 @depends on patch@
 expression from,to,size;
 identifier l1,l2;
+position p : script:python() { relevant(p) };
 @@
 
--  to = \(kmalloc\|kzalloc\)
+-  to = \(kmalloc@p\|kzalloc@p\)
 		(size,\(GFP_KERNEL\|GFP_USER\|
 		      \(GFP_KERNEL\|GFP_USER\)|__GFP_NOWARN\));
 +  to = memdup_user(from,size);
@@ -42,9 +50,10 @@ identifier l1,l2;
 @depends on patch@
 expression from,to,size;
 identifier l1,l2;
+position p : script:python() { relevant(p) };
 @@
 
--  to = \(kvmalloc\|kvzalloc\)(size,\(GFP_KERNEL\|GFP_USER\));
+-  to = \(kvmalloc@p\|kvzalloc@p\)(size,\(GFP_KERNEL\|GFP_USER\));
 +  to = vmemdup_user(from,size);
    if (
 -      to==NULL
@@ -63,7 +72,7 @@ identifier l1,l2;
 
 @r depends on !patch@
 expression from,to,size;
-position p;
+position p : script:python() { relevant(p) };
 statement S1,S2;
 @@
 
@@ -76,7 +85,7 @@ statement S1,S2;
 
 @rv depends on !patch@
 expression from,to,size;
-position p;
+position p : script:python() { relevant(p) };
 statement S1,S2;
 @@
 
-- 
2.26.2

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

[Cocci] [PATCH v2 4/4] coccinelle: api: add selfcheck for memdup_user rule

[Denis Efremov]

Check that the rule matches vmemdup_user implementation.
memdup_user is out of scope because we are not matching
kmalloc_track_caller() function.

Signed-off-by: Denis Efremov <efremov@linux.com>
---
 scripts/coccinelle/api/memdup_user.cocci | 46 ++++++++++++++++++++++--
 1 file changed, 44 insertions(+), 2 deletions(-)

diff --git a/scripts/coccinelle/api/memdup_user.cocci b/scripts/coccinelle/api/memdup_user.cocci
index 8621bd98be1e..78fded83b197 100644
--- a/scripts/coccinelle/api/memdup_user.cocci
+++ b/scripts/coccinelle/api/memdup_user.cocci
@@ -14,13 +14,24 @@ virtual patch
 virtual context
 virtual org
 virtual report
+virtual selfcheck
 
 @initialize:python@
 @@
-filter = frozenset(['memdup_user', 'vmemdup_user'])
+
+definitions = {
+    'memdup_user': 'mm/util.c',
+    'vmemdup_user': 'mm/util.c',
+}
+
+filter = frozenset(definitions.keys())
+coccinelle.filtered = set()
+coccinelle.checked_files = set()
 
 def relevant(p):
-    return not (filter & {el.current_element for el in p})
+    found = filter & {el.current_element for el in p}
+    coccinelle.filtered |= found
+    return not found
 
 @depends on patch@
 expression from,to,size;
@@ -117,3 +128,34 @@ p << rv.p;
 @@
 
 coccilib.report.print_report(p[0], "WARNING opportunity for vmemdup_user")
+
+@script:python depends on selfcheck@
+@@
+coccinelle.checked_files |= set(definitions.values()) & set(cocci.files())
+
+@finalize:python depends on selfcheck@
+filtered << merge.filtered;
+checked_files << merge.checked_files;
+@@
+
+# Don't check memdup_user because the pattern is not capturing
+# kmalloc_track_caller() calls
+del definitions['memdup_user']
+
+# mapping between checked files and filtered definitions
+found_defns = {}
+for files, funcs in zip(checked_files, filtered):
+   for file in files:
+      found_defns[file] = funcs
+
+# reverse mapping of definitions
+expected_defns = {v : set() for v in definitions.values()}
+for k, v in definitions.items():
+    expected_defns[v] |= {k}
+
+for efile, efuncs in expected_defns.items():
+    if efile in found_defns:
+        not_found = efuncs - found_defns[efile]
+        if not_found:
+            print('SELF-CHECK: the pattern no longer matches ' \
+		  'definitions {} in file {}'.format(not_found, efile))
-- 
2.26.2

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

Re: [Cocci] [PATCH v2 4/4] coccinelle: api: add selfcheck for memdup_user rule

[Julia Lawall]

On Mon, 8 Jun 2020, Denis Efremov wrote:

> Check that the rule matches vmemdup_user implementation.
> memdup_user is out of scope because we are not matching
> kmalloc_track_caller() function.

Is this a bit over-enginered?  More precisely, even if it is nice to check
that the API definition has the expected behavior, does it make sense to
do it in one case but not the other?

julia

>
> Signed-off-by: Denis Efremov <efremov@linux.com>
> ---
>  scripts/coccinelle/api/memdup_user.cocci | 46 ++++++++++++++++++++++--
>  1 file changed, 44 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/coccinelle/api/memdup_user.cocci b/scripts/coccinelle/api/memdup_user.cocci
> index 8621bd98be1e..78fded83b197 100644
> --- a/scripts/coccinelle/api/memdup_user.cocci
> +++ b/scripts/coccinelle/api/memdup_user.cocci
> @@ -14,13 +14,24 @@ virtual patch
>  virtual context
>  virtual org
>  virtual report
> +virtual selfcheck
>
>  @initialize:python@
>  @@
> -filter = frozenset(['memdup_user', 'vmemdup_user'])
> +
> +definitions = {
> +    'memdup_user': 'mm/util.c',
> +    'vmemdup_user': 'mm/util.c',
> +}
> +
> +filter = frozenset(definitions.keys())
> +coccinelle.filtered = set()
> +coccinelle.checked_files = set()
>
>  def relevant(p):
> -    return not (filter & {el.current_element for el in p})
> +    found = filter & {el.current_element for el in p}
> +    coccinelle.filtered |= found
> +    return not found
>
>  @depends on patch@
>  expression from,to,size;
> @@ -117,3 +128,34 @@ p << rv.p;
>  @@
>
>  coccilib.report.print_report(p[0], "WARNING opportunity for vmemdup_user")
> +
> +@script:python depends on selfcheck@
> +@@
> +coccinelle.checked_files |= set(definitions.values()) & set(cocci.files())
> +
> +@finalize:python depends on selfcheck@
> +filtered << merge.filtered;
> +checked_files << merge.checked_files;
> +@@
> +
> +# Don't check memdup_user because the pattern is not capturing
> +# kmalloc_track_caller() calls
> +del definitions['memdup_user']
> +
> +# mapping between checked files and filtered definitions
> +found_defns = {}
> +for files, funcs in zip(checked_files, filtered):
> +   for file in files:
> +      found_defns[file] = funcs
> +
> +# reverse mapping of definitions
> +expected_defns = {v : set() for v in definitions.values()}
> +for k, v in definitions.items():
> +    expected_defns[v] |= {k}
> +
> +for efile, efuncs in expected_defns.items():
> +    if efile in found_defns:
> +        not_found = efuncs - found_defns[efile]
> +        if not_found:
> +            print('SELF-CHECK: the pattern no longer matches ' \
> +		  'definitions {} in file {}'.format(not_found, efile))
> --
> 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

Re: [Cocci] [PATCH v2 4/4] coccinelle: api: add selfcheck for memdup_user rule

[Denis Efremov]

On 6/9/20 7:22 PM, Julia Lawall wrote:
> 
> 
> On Mon, 8 Jun 2020, Denis Efremov wrote:
> 
>> Check that the rule matches vmemdup_user implementation.
>> memdup_user is out of scope because we are not matching
>> kmalloc_track_caller() function.
> 
> Is this a bit over-enginered?

Last patch it's just a PoC. Patches 1-3 are independent from 4.

> More precisely, even if it is nice to check
> that the API definition has the expected behavior, does it make sense to
> do it in one case but not the other?

Yes, I also don't like it. However, I doubt that we need to match
kmalloc_track_caller.

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

Re: [Cocci] [PATCH v2 0/4] Update memdup_user.cocci

[Denis Efremov]

Ping?

On 6/8/20 6:00 PM, Denis Efremov wrote:
> Add GFP_USER to the allocation flags and handle vmemdup_user().
> The third patch supresses memdup_user(), vmemdup_user() functions
> detection. Last patch is a proof of concept for the rule selfchecking.
> Gives the ability to detect that an open-coded pattern in a function
> definition that we search for in the kernel sources changed.
> 
> Denis Efremov (4):
>   coccinelle: api: extend memdup_user transformation with GFP_USER
>   coccinelle: api: extend memdup_user rule with vmemdup_user()
>   coccinelle: api: filter out memdup_user definitions
>   coccinelle: api: add selfcheck for memdup_user rule
> 
>  scripts/coccinelle/api/memdup_user.cocci | 106 ++++++++++++++++++++++-
>  1 file changed, 103 insertions(+), 3 deletions(-)
> 
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH v2 1/4] coccinelle: api: extend memdup_user transformation with GFP_USER

[Julia Lawall]

On Mon, 8 Jun 2020, Denis Efremov wrote:

> Match GFP_USER and optional __GFP_NOWARN allocations with
> memdup_user.cocci rule.
> Commit 6c2c97a24f09 ("memdup_user(): switch to GFP_USER") switched
> memdup_user() from GFP_KERNEL to GFP_USER. In almost all cases it
> is still a good idea to recommend memdup_user() for GFP_KERNEL
> allocations. The motivation behind altering memdup_user() to GFP_USER:
> https://lkml.org/lkml/2018/1/6/333
>
> Signed-off-by: Denis Efremov <efremov@linux.com>

Applied.

> ---
>  scripts/coccinelle/api/memdup_user.cocci | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/coccinelle/api/memdup_user.cocci b/scripts/coccinelle/api/memdup_user.cocci
> index c809ab10bbce..cadcc2e87881 100644
> --- a/scripts/coccinelle/api/memdup_user.cocci
> +++ b/scripts/coccinelle/api/memdup_user.cocci
> @@ -20,7 +20,9 @@ expression from,to,size;
>  identifier l1,l2;
>  @@
>
> --  to = \(kmalloc\|kzalloc\)(size,GFP_KERNEL);
> +-  to = \(kmalloc\|kzalloc\)
> +		(size,\(GFP_KERNEL\|GFP_USER\|
> +		      \(GFP_KERNEL\|GFP_USER\)|__GFP_NOWARN\));
>  +  to = memdup_user(from,size);
>     if (
>  -      to==NULL
> @@ -43,7 +45,9 @@ position p;
>  statement S1,S2;
>  @@
>
> -*  to = \(kmalloc@p\|kzalloc@p\)(size,GFP_KERNEL);
> +*  to = \(kmalloc@p\|kzalloc@p\)
> +		(size,\(GFP_KERNEL\|GFP_USER\|
> +		      \(GFP_KERNEL\|GFP_USER\)|__GFP_NOWARN\));
>     if (to==NULL || ...) S1
>     if (copy_from_user(to, from, size) != 0)
>     S2
> --
> 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

Re: [Cocci] [PATCH v2 4/4] coccinelle: api: add selfcheck for memdup_user rule

[Markus Elfring]

…
> +++ b/scripts/coccinelle/api/memdup_user.cocci
> @@ -14,13 +14,24 @@ virtual patch
>  virtual context
>  virtual org
>  virtual report
> +virtual selfcheck

Would you like to avoid the repetition of a SmPL key word here?

+virtual patch, context, org, report, selfcheck


> @@ -117,3 +128,34 @@ p << rv.p;
>  @@
>
>  coccilib.report.print_report(p[0], "WARNING opportunity for vmemdup_user")
> +
> +@script:python depends on selfcheck@
> +@@
> +coccinelle.checked_files |= set(definitions.values()) & set(cocci.files())

I suggest to reconsider the usage of the function “cocci.files()”.
Can such a script rule determine for which file it should perform data processing?


> +            print('SELF-CHECK: the pattern no longer matches ' \
> +		  'definitions {} in file {}'.format(not_found, efile))

Can the following code variant be a bit nicer?

+            sys.stdout.write('SELF-CHECK: The pattern does not match definitions {} in file {} any more.\n' \
+                             .format(not_found, efile))


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

Re: [Cocci] [PATCH v2 4/4] coccinelle: api: add selfcheck for memdup_user rule

[Markus Elfring]

> Check that the rule matches vmemdup_user implementation.
> memdup_user is out of scope because we are not matching
> kmalloc_track_caller() function.

I find this change description improvable.
Will it become helpful (for example) to mention that you would like to
add another operation mode?


> +@finalize:python depends on selfcheck@
> +filtered << merge.filtered;
> +checked_files << merge.checked_files;
> +@@

Are we looking for better software documentation for such functionality?

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