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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
Don't match original implementations.
Signed-off-by: Denis Efremov <[email protected]>
---
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
> Match GFP_USER and optional __GFP_NOWARN allocations with
> memdup_user.cocci rule.
You suggest another interesting software extension.
…
> +++ 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);
But I find the proposed change for such SmPL code inappropriate.
It was specified by the means of the semantic patch language
to replace an assignment statement for which a function call provides a value.
I would interpret your adjustment in the way that an expression list
should be preserved.
Were two minus characters forgotten for the first SmPL rule?
Further concerns should be taken into account for the SmPL rule “r”.
I would appreciate if the clarification will be continued also for
the topic “Safer source code analysis by "memdup_user.cocci"”?
https://github.com/coccinelle/coccinelle/issues/78
Regards,
Markus
…
+++ b/scripts/coccinelle/api/memdup_user.cocci
@@ -39,6 +39,28 @@ …
…
> +@depends on patch@
> +expression from,to,size;
> +identifier l1,l2;
> +@@
> +
> +- to = \(kvmalloc\|kvzalloc\)(size,\(GFP_KERNEL\|GFP_USER\));
> ++ to = vmemdup_user(from,size);
I suggest to combine the desired adjustment with the previous SmPL rule
by using another disjunction.
> +@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
How does the SmPL asterisk functionality fit to the operation
modes “org” and “report”?
Regards,
Markus
On Mon, 8 Jun 2020, Markus Elfring wrote:
> …
> +++ b/scripts/coccinelle/api/memdup_user.cocci
> @@ -39,6 +39,28 @@ …
> …
> > +@depends on patch@
> > +expression from,to,size;
> > +identifier l1,l2;
> > +@@
> > +
> > +- to = \(kvmalloc\|kvzalloc\)(size,\(GFP_KERNEL\|GFP_USER\));
> > ++ to = vmemdup_user(from,size);
>
> I suggest to combine the desired adjustment with the previous SmPL rule
> by using another disjunction.
>
>
> > +@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
>
> How does the SmPL asterisk functionality fit to the operation
> modes “org” and “report”?
Make coccicheck uses --no-show-diff for org and report modes, so the * has
no effect in those cases.
julia
> Don't match original implementations.
I find such a change description insufficient.
Regards,
Markus
> 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
>>> +* to = \(kvmalloc@p\|kvzalloc@p\)(size,\(GFP_KERNEL\|GFP_USER\));
>>> + if (to==NULL || ...) S1
>>> + if (copy_from_user(to, from, size) != 0)
>>> + S2
>>
>> How does the SmPL asterisk functionality fit to the operation
>> modes “org” and “report”?
>
> Make coccicheck uses --no-show-diff for org and report modes, so the * has
> no effect in those cases.
* Can it be nicer to generate only required data for each operation mode?
* Would you like to compare the usage of this command parameter with other
design approaches?
Regards,
Markus
…
> +++ 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
…
> +++ b/scripts/coccinelle/api/memdup_user.cocci
> @@ -20,7 +20,9 @@ expression from,to,size;
…
> +- to = \(kmalloc\|kzalloc\)
> + (size,\(GFP_KERNEL\|GFP_USER\|
> + \(GFP_KERNEL\|GFP_USER\)|__GFP_NOWARN\));
I got the impression that this SmPL code needs another correction also
according to the proposed SmPL disjunction.
+-to = \( kmalloc \| kzalloc \) (size, \( GFP_KERNEL \| GFP_USER \) \( | __GFP_NOWARN \| \) );
Would you like to express by any other approach that a specific flag
is an optional source code transformation parameter?
Regards,
Markus
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 <[email protected]>
> ---
> 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
> [email protected]
> https://systeme.lip6.fr/mailman/listinfo/cocci
>
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
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(-)
>
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 <[email protected]>
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
> [email protected]
> https://systeme.lip6.fr/mailman/listinfo/cocci
>