Add GFP_USER to the allocation flags and handle vmemdup_user().
Changes in v2:
- memdup_user/vmemdup_user matching suppressed
- PoC for selfcheck virtual rule
Changes in v3:
- add missing '-' for patch rule in kmalloc/kzalloc call args
- selfcheck rule dropped from patchset
Denis Efremov (3):
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
scripts/coccinelle/api/memdup_user.cocci | 64 ++++++++++++++++++++++--
1 file changed, 61 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..0e29d41ecab8 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 0e29d41ecab8..60027e21c5e6 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
Don't match memdup_user/vmemdup_user.
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 60027e21c5e6..e01e95108405 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.
I suggest to clarify software design consequences according to such information
a bit more.
I find it helpful if you would have included also my email address directly
in the message field “To” or “Cc”.
Are there further reasons to consider for the extension of the recipient lists?
> +- to = \(kmalloc\|kzalloc\)
> +- (size,\(GFP_KERNEL\|GFP_USER\|
> +- \(GFP_KERNEL\|GFP_USER\)|__GFP_NOWARN\));
* Would you ever dare to specify such a source code search pattern
on a single line?
* I find the specification of SmPL disjunctions questionable
for the determination of relevant flags.
Could previous patch review trigger concerns and further considerations
for the proper handling of optional source code parts?
> +* to = \(kmalloc@p\|kzalloc@p\)
> + (size,\(GFP_KERNEL\|GFP_USER\|
> + \(GFP_KERNEL\|GFP_USER\)|__GFP_NOWARN\));
Would you like to use the SmPL asterisk really only for a single line?
How will the chances evolve to continue the clarification also for
the open issue “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 propose 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
* Can it be helpful to omit the SmPL asterisk functionality from
the operation modes “org” and “report”?
* Should the operation mode “context” work without an extra position metavariable?
Regards,
Markus
On Tue, 21 Jul 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 propose 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
>
> * Can it be helpful to omit the SmPL asterisk functionality from
> the operation modes “org” and “report”?
>
> * Should the operation mode “context” work without an extra position metavariable?
This is fine as is in all three aspects.
julia
> Don't match memdup_user/vmemdup_user.
I find such a change description insufficient.
Regards,
Markus
> This is fine as is in all three aspects.
I have tried again to point specific data processing consequences out
for operation modes of scripts in the semantic patch language.
Regards,
Markus
>>> +@depends on patch@
>>> +expression from,to,size;
>>> +identifier l1,l2;
>>> +@@
>>> +
>>> +- to = \(kvmalloc\|kvzalloc\)(size,\(GFP_KERNEL\|GFP_USER\));
>>> ++ to = vmemdup_user(from,size);
>>
>> I propose to combine the desired adjustment with the previous SmPL rule
>> by using another disjunction.
How do you think about to check run time characteristics for
the following SmPL script sketches?
A)
@R1@
@@
// Change something
@R2@
@@
// Change another thing
B)
@Replacement_with_disjunction@
@@
(
// R1: Change something
|
// R2: Change another thing
)
>>> +@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
>>
>> * Can it be helpful to omit the SmPL asterisk functionality from
>> the operation modes “org” and “report”?
>>
>> * Should the operation mode “context” work without an extra position metavariable?
>
> This is fine as is in all three aspects.
Is every technique from the Coccinelle software required for
each operation mode in such data processing approaches?
Regards,
Markus
On Wed, 22 Jul 2020, Markus Elfring wrote:
> >>> +@depends on patch@
> >>> +expression from,to,size;
> >>> +identifier l1,l2;
> >>> +@@
> >>> +
> >>> +- to = \(kvmalloc\|kvzalloc\)(size,\(GFP_KERNEL\|GFP_USER\));
> >>> ++ to = vmemdup_user(from,size);
> >>
> >> I propose to combine the desired adjustment with the previous SmPL rule
> >> by using another disjunction.
>
> How do you think about to check run time characteristics for
> the following SmPL script sketches?
>
> A)
> @R1@
> @@
> // Change something
>
> @R2@
> @@
> // Change another thing
>
>
> B)
> @Replacement_with_disjunction@
> @@
> (
> // R1: Change something
> |
> // R2: Change another thing
> )
Markus, you are welcome to try this since you are concerned about it.
But it doesn't matter.
julia
>
>
> >>> +@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
> >>
> >> * Can it be helpful to omit the SmPL asterisk functionality from
> >> the operation modes “org” and “report”?
> >>
> >> * Should the operation mode “context” work without an extra position metavariable?
> >
> > This is fine as is in all three aspects.
>
> Is every technique from the Coccinelle software required for
> each operation mode in such data processing approaches?
>
> Regards,
> Markus
>
> Markus, you are welcome to try this since you are concerned about it.
I dare to point software design variations for some reasons.
> But it doesn't matter.
Under which circumstances would you begin to care more for involved differences
in corresponding run time characteristics?
Regards,
Markus
On Wed, 22 Jul 2020, Markus Elfring wrote:
> > Markus, you are welcome to try this since you are concerned about it.
>
> I dare to point software design variations for some reasons.
>
>
> > But it doesn't matter.
>
> Under which circumstances would you begin to care more for involved differences
> in corresponding run time characteristics?
When the difference is hours vs seconds. In this case, there are
tradeoffs between the two options, andI don't know which would be faster,
but I think it is extremely unlikely that the difference is noticible.
But if you are concerned about it, you are welcome to try.
julia
>> Under which circumstances would you begin to care more for involved differences
>> in corresponding run time characteristics?
>
> When the difference is hours vs seconds.
Such a view can be influenced by execution environments in considerable ways.
> In this case, there are tradeoffs between the two options,
I am trying to improve the awareness for possible software design variations.
> andI don't know which would be faster,
I am curious if more contributors would get into the mood to compare
data processing approaches a bit more.
> but I think it is extremely unlikely that the difference is noticible.
But the following effects may be known for the evaluation of a SmPL rule
which was combined for two code parts.
* The SmPL dependencies are checked only once.
* Can the source code search efforts be reduced a bit?
> But if you are concerned about it, you are welcome to try.
Will anybody become interested to measure the software behaviour exactly?
Regards,
Markus
On Mon, 20 Jul 2020, Denis Efremov wrote:
> Add GFP_USER to the allocation flags and handle vmemdup_user().
>
> Changes in v2:
> - memdup_user/vmemdup_user matching suppressed
> - PoC for selfcheck virtual rule
> Changes in v3:
> - add missing '-' for patch rule in kmalloc/kzalloc call args
> - selfcheck rule dropped from patchset
>
> Denis Efremov (3):
> 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
All three applied. Thanks.
julia
>
> scripts/coccinelle/api/memdup_user.cocci | 64 ++++++++++++++++++++++--
> 1 file changed, 61 insertions(+), 3 deletions(-)
>
> --
> 2.26.2
>
>
…
> > Changes in v3:
> > - add missing '-' for patch rule in kmalloc/kzalloc call args
> > - selfcheck rule dropped from patchset
…
> All three applied. …
Will the software development discussion be continued by patches according to
previously mentioned ideas and remaining open issues?
Regards,
Markus