This patch adds a trivial script warning on
if(foo)
kfree(foo)
(based on checkpatch)
Cc: Julia Lawall <[email protected]>
Cc: Gilles Muller <[email protected]>
Cc: Joe Perches <[email protected]>
Signed-off-by: Fabian Frederick <[email protected]>
---
scripts/coccinelle/free/cond_kfree.cocci | 34 ++++++++++++++++++++++++++++++++
1 file changed, 34 insertions(+)
create mode 100644 scripts/coccinelle/free/cond_kfree.cocci
diff --git a/scripts/coccinelle/free/cond_kfree.cocci b/scripts/coccinelle/free/cond_kfree.cocci
new file mode 100644
index 0000000..0b8093e
--- /dev/null
+++ b/scripts/coccinelle/free/cond_kfree.cocci
@@ -0,0 +1,34 @@
+/// conditional kfree - NULL check before kfree is not needed.
+///
+/// Based on checkpatch warning
+/// "kfree(NULL) is safe this check is probably not required"
+/// and kfreeaddr.cocci by Julia Lawall.
+///
+/// Comments: -
+/// Options: --no-includes --include-headers
+
+virtual org
+virtual report
+virtual context
+
+@r depends on context || report || org @
+expression E;
+position p;
+@@
+
+* if (E)
+* kfree@p(E);
+
+@script:python depends on org@
+p << r.p;
+@@
+
+cocci.print_main("kfree", p)
+
+@script:python depends on report@
+p << r.p;
+@@
+
+msg = "WARNING: checking value to avoid kfree(NULL) is unnecessary."
+coccilib.report.print_report(p[0], msg)
+
--
1.8.4.5
On Tue, 2014-06-17 at 21:43 +0200, Fabian Frederick wrote:
> This patch adds a trivial script warning on
>
> if(foo)
> kfree(foo)
>
> (based on checkpatch)
[]
> diff --git a/scripts/coccinelle/free/cond_kfree.cocci b/scripts/coccinelle/free/cond_kfree.cocci
[]
> +* if (E)
> +* kfree@p(E);
You should probably add all of the unnecessary
conditional tests that checkpatch uses:
kfree
usb_free_urb
debugfs_remove
debugfs_remove_recursive
>> This patch adds a trivial script warning on
>>
>> if(foo)
>> kfree(foo)
[...]
> You should probably add all of the unnecessary
> conditional tests that checkpatch uses:
[...]
Would you like to look at my previous update suggestion "Deletion of unnecessary
checks before specific function calls" again?
https://systeme.lip6.fr/pipermail/cocci/2014-March/000675.html
https://lkml.org/lkml/2014/3/5/344
Regards,
Markus
On Tue, 17 Jun 2014, Joe Perches wrote:
> On Tue, 2014-06-17 at 21:43 +0200, Fabian Frederick wrote:
> > This patch adds a trivial script warning on
> >
> > if(foo)
> > kfree(foo)
> >
> > (based on checkpatch)
> []
> > diff --git a/scripts/coccinelle/free/cond_kfree.cocci b/scripts/coccinelle/free/cond_kfree.cocci
> []
> > +* if (E)
> > +* kfree@p(E);
>
> You should probably add all of the unnecessary
> conditional tests that checkpatch uses:
>
> kfree
> usb_free_urb
> debugfs_remove
> debugfs_remove_recursive
Personally, I would prefer that the message encourage the user to consider
whether it is necessary to call these functions with NULL as an argument
in any case.
julia
(adding Jesper Juhl)
On Tue, 2014-06-17 at 23:33 +0200, Julia Lawall wrote:
> On Tue, 17 Jun 2014, Joe Perches wrote:
> > On Tue, 2014-06-17 at 21:43 +0200, Fabian Frederick wrote:
> > > This patch adds a trivial script warning on
> > >
> > > if(foo)
> > > kfree(foo)
> > >
> > > (based on checkpatch)
> > []
> > > diff --git a/scripts/coccinelle/free/cond_kfree.cocci b/scripts/coccinelle/free/cond_kfree.cocci
> > []
> > > +* if (E)
> > > +* kfree@p(E);
> >
> > You should probably add all of the unnecessary
> > conditional tests that checkpatch uses:
> >
> > kfree
> > usb_free_urb
> > debugfs_remove
> > debugfs_remove_recursive
>
> Personally, I would prefer that the message encourage the user to consider
> whether it is necessary to call these functions with NULL as an argument
> in any case.
Jesper quite awhile ago wrote:
https://lkml.org/lkml/2005/10/13/81
- Since kfree always checks for a NULL argument there's no reason to have an
additional check prior to calling kfree. It's redundant.
- In many cases gcc produce significantly smaller code without the redundant
check before the call.
- It's been shown in the past (in discussions on LKML) that it's generally a
win performance wise to avoid the extra NULL check even though it might save
a function call. Only when the NULL check avoids the function call in the vast
majority of cases and the code is in a hot path does it make sense to have it.
- This patch removes quite a few more source lines than it adds, cutting down
on the overall number of source lines is generally a good thing.
- This patch reduces the indentation level, which is nice when the kfree call
is inside some deeply nested construct.
On Tue, 17 Jun 2014, Joe Perches wrote:
> (adding Jesper Juhl)
>
> On Tue, 2014-06-17 at 23:33 +0200, Julia Lawall wrote:
> > On Tue, 17 Jun 2014, Joe Perches wrote:
> > > On Tue, 2014-06-17 at 21:43 +0200, Fabian Frederick wrote:
> > > > This patch adds a trivial script warning on
> > > >
> > > > if(foo)
> > > > kfree(foo)
> > > >
> > > > (based on checkpatch)
> > > []
> > > > diff --git a/scripts/coccinelle/free/cond_kfree.cocci b/scripts/coccinelle/free/cond_kfree.cocci
> > > []
> > > > +* if (E)
> > > > +* kfree@p(E);
> > >
> > > You should probably add all of the unnecessary
> > > conditional tests that checkpatch uses:
> > >
> > > kfree
> > > usb_free_urb
> > > debugfs_remove
> > > debugfs_remove_recursive
> >
> > Personally, I would prefer that the message encourage the user to consider
> > whether it is necessary to call these functions with NULL as an argument
> > in any case.
>
> Jesper quite awhile ago wrote:
>
> https://lkml.org/lkml/2005/10/13/81
>
> - Since kfree always checks for a NULL argument there's no reason to have an
> additional check prior to calling kfree. It's redundant.
> - In many cases gcc produce significantly smaller code without the redundant
> check before the call.
> - It's been shown in the past (in discussions on LKML) that it's generally a
> win performance wise to avoid the extra NULL check even though it might save
> a function call. Only when the NULL check avoids the function call in the vast
> majority of cases and the code is in a hot path does it make sense to have it.
> - This patch removes quite a few more source lines than it adds, cutting down
> on the overall number of source lines is generally a good thing.
> - This patch reduces the indentation level, which is nice when the kfree call
> is inside some deeply nested construct.
What I don't like is:
a = kmalloc(...);
if (!a) goto out;
b = kmalloc(...);
if (!b) goto out;
c = kmalloc(...);
if (!c) goto out;
...
out:
kfree(a);
kfree(b);
kfree(c);
With a little thought, one could reorganize the code to not call kfree on
a null value. Someone who is not familiar with Linux programming style
could interpret the feedback as that the above code is perfectly fine.
(And perhaps some people do consider that it is perfectly fine).
On the other hand, in the case:
x = NULL;
if (complicated_condition)
x = kmalloc(...);
if (!x) return;
y = something(...);
if (!y)
goto out1;
...
out1: kfree(x);
I guess it's OK. Mildly unpleasant, but probably the best option given
the various tradeoff.
In looking at Jesper's patch, I see that another case is:
a = kmalloc(...);
b = kmalloc(...);
if (!a || !b) {
kfree(a);
kfree(b);
}
Personally, I would rather see each call have its own error handling code.
There is no point to make the second call if the first one fails.
When one tries to understand code, the main questions are why is this done
here, and why is this not done here. Doing things that are unnecessary
introduces confusion in this regard. Perhaps it doesn't matter for
kmalloc and kfree because everyone is familiar with them and they are
pretty innocuous. But for the more obscure functions, like in my
recollection of Markus's patch, I'm not convinced that simply blindly
removing all unneeded tests without thinking whether the code could be
written in a better way is a good idea.
julia
On Wed, 2014-06-18 at 07:25 +0200, Julia Lawall wrote:
>
> On Tue, 17 Jun 2014, Joe Perches wrote:
>
> > (adding Jesper Juhl)
> >
> > On Tue, 2014-06-17 at 23:33 +0200, Julia Lawall wrote:
> > > On Tue, 17 Jun 2014, Joe Perches wrote:
> > > > On Tue, 2014-06-17 at 21:43 +0200, Fabian Frederick wrote:
> > > > > This patch adds a trivial script warning on
> > > > >
> > > > > if(foo)
> > > > > kfree(foo)
> > > > >
> > > > > (based on checkpatch)
> > > > []
> > > > > diff --git a/scripts/coccinelle/free/cond_kfree.cocci b/scripts/coccinelle/free/cond_kfree.cocci
> > > > []
> > > > > +* if (E)
> > > > > +* kfree@p(E);
> > > >
> > > > You should probably add all of the unnecessary
> > > > conditional tests that checkpatch uses:
> > > >
> > > > kfree
> > > > usb_free_urb
> > > > debugfs_remove
> > > > debugfs_remove_recursive
> > >
> > > Personally, I would prefer that the message encourage the user to consider
> > > whether it is necessary to call these functions with NULL as an argument
> > > in any case.
> >
> > Jesper quite awhile ago wrote:
> >
> > https://lkml.org/lkml/2005/10/13/81
> >
> > - Since kfree always checks for a NULL argument there's no reason to have an
> > additional check prior to calling kfree. It's redundant.
> > - In many cases gcc produce significantly smaller code without the redundant
> > check before the call.
> > - It's been shown in the past (in discussions on LKML) that it's generally a
> > win performance wise to avoid the extra NULL check even though it might save
> > a function call. Only when the NULL check avoids the function call in the vast
> > majority of cases and the code is in a hot path does it make sense to have it.
> > - This patch removes quite a few more source lines than it adds, cutting down
> > on the overall number of source lines is generally a good thing.
> > - This patch reduces the indentation level, which is nice when the kfree call
> > is inside some deeply nested construct.
>
> What I don't like is:
>
> a = kmalloc(...);
> if (!a) goto out;
> b = kmalloc(...);
> if (!b) goto out;
> c = kmalloc(...);
> if (!c) goto out;
> ...
> out:
> kfree(a);
> kfree(b);
> kfree(c);
>
> With a little thought, one could reorganize the code to not call kfree on
> a null value.
And I think most kernel malloc failures are written like:
a = kmalloc(...);
if (!a) goto out1;
b = kmalloc(...);
if (!b) goto out2;
c = kmalloc(...)
if (!c) goto out3;
...
out3: kfree(b);
out2: kfree(a);
out1: ...
> Someone who is not familiar with Linux programming style
> could interpret the feedback as that the above code is perfectly fine.
> (And perhaps some people do consider that it is perfectly fine).
maybe so.
> On the other hand, in the case:
>
> x = NULL;
> if (complicated_condition)
> x = kmalloc(...);
> if (!x) return;
> y = something(...);
> if (!y)
> goto out1;
> ...
> out1: kfree(x);
>
> I guess it's OK. Mildly unpleasant, but probably the best option given
> the various tradeoff.
>
> In looking at Jesper's patch, I see that another case is:
>
> a = kmalloc(...);
> b = kmalloc(...);
> if (!a || !b) {
> kfree(a);
> kfree(b);
> }
>
> Personally, I would rather see each call have its own error handling code.
> There is no point to make the second call if the first one fails.
>
> When one tries to understand code, the main questions are why is this done
> here, and why is this not done here. Doing things that are unnecessary
> introduces confusion in this regard. Perhaps it doesn't matter for
> kmalloc and kfree because everyone is familiar with them and they are
> pretty innocuous. But for the more obscure functions, like in my
> recollection of Markus's patch, I'm not convinced that simply blindly
> removing all unneeded tests without thinking whether the code could be
> written in a better way is a good idea.
Blindly applying patches, even those produced by coccinelle,
let alone mine, is rarely good practice.
On Tue, 17 Jun 2014, Joe Perches wrote:
> On Wed, 2014-06-18 at 07:25 +0200, Julia Lawall wrote:
> >
> > On Tue, 17 Jun 2014, Joe Perches wrote:
> >
> > > (adding Jesper Juhl)
> > >
> > > On Tue, 2014-06-17 at 23:33 +0200, Julia Lawall wrote:
> > > > On Tue, 17 Jun 2014, Joe Perches wrote:
> > > > > On Tue, 2014-06-17 at 21:43 +0200, Fabian Frederick wrote:
> > > > > > This patch adds a trivial script warning on
> > > > > >
> > > > > > if(foo)
> > > > > > kfree(foo)
> > > > > >
> > > > > > (based on checkpatch)
> > > > > []
> > > > > > diff --git a/scripts/coccinelle/free/cond_kfree.cocci b/scripts/coccinelle/free/cond_kfree.cocci
> > > > > []
> > > > > > +* if (E)
> > > > > > +* kfree@p(E);
> > > > >
> > > > > You should probably add all of the unnecessary
> > > > > conditional tests that checkpatch uses:
> > > > >
> > > > > kfree
> > > > > usb_free_urb
> > > > > debugfs_remove
> > > > > debugfs_remove_recursive
> > > >
> > > > Personally, I would prefer that the message encourage the user to consider
> > > > whether it is necessary to call these functions with NULL as an argument
> > > > in any case.
> > >
> > > Jesper quite awhile ago wrote:
> > >
> > > https://lkml.org/lkml/2005/10/13/81
> > >
> > > - Since kfree always checks for a NULL argument there's no reason to have an
> > > additional check prior to calling kfree. It's redundant.
> > > - In many cases gcc produce significantly smaller code without the redundant
> > > check before the call.
> > > - It's been shown in the past (in discussions on LKML) that it's generally a
> > > win performance wise to avoid the extra NULL check even though it might save
> > > a function call. Only when the NULL check avoids the function call in the vast
> > > majority of cases and the code is in a hot path does it make sense to have it.
> > > - This patch removes quite a few more source lines than it adds, cutting down
> > > on the overall number of source lines is generally a good thing.
> > > - This patch reduces the indentation level, which is nice when the kfree call
> > > is inside some deeply nested construct.
> >
> > What I don't like is:
> >
> > a = kmalloc(...);
> > if (!a) goto out;
> > b = kmalloc(...);
> > if (!b) goto out;
> > c = kmalloc(...);
> > if (!c) goto out;
> > ...
> > out:
> > kfree(a);
> > kfree(b);
> > kfree(c);
> >
> > With a little thought, one could reorganize the code to not call kfree on
> > a null value.
>
> And I think most kernel malloc failures are written like:
>
> a = kmalloc(...);
> if (!a) goto out1;
> b = kmalloc(...);
> if (!b) goto out2;
> c = kmalloc(...)
> if (!c) goto out3;
> ...
> out3: kfree(b);
> out2: kfree(a);
> out1: ...
This is the case for the good code. But not necessarily in staging.
> > Someone who is not familiar with Linux programming style
> > could interpret the feedback as that the above code is perfectly fine.
> > (And perhaps some people do consider that it is perfectly fine).
>
> maybe so.
>
> > On the other hand, in the case:
> >
> > x = NULL;
> > if (complicated_condition)
> > x = kmalloc(...);
> > if (!x) return;
> > y = something(...);
> > if (!y)
> > goto out1;
> > ...
> > out1: kfree(x);
> >
> > I guess it's OK. Mildly unpleasant, but probably the best option given
> > the various tradeoff.
> >
> > In looking at Jesper's patch, I see that another case is:
> >
> > a = kmalloc(...);
> > b = kmalloc(...);
> > if (!a || !b) {
> > kfree(a);
> > kfree(b);
> > }
> >
> > Personally, I would rather see each call have its own error handling code.
> > There is no point to make the second call if the first one fails.
> >
> > When one tries to understand code, the main questions are why is this done
> > here, and why is this not done here. Doing things that are unnecessary
> > introduces confusion in this regard. Perhaps it doesn't matter for
> > kmalloc and kfree because everyone is familiar with them and they are
> > pretty innocuous. But for the more obscure functions, like in my
> > recollection of Markus's patch, I'm not convinced that simply blindly
> > removing all unneeded tests without thinking whether the code could be
> > written in a better way is a good idea.
>
> Blindly applying patches, even those produced by coccinelle,
> let alone mine, is rarely good practice.
Sure. I would just argue for changing the text associated with the
semantic patch a little bit, to suggest considering whether the code can
be reorganized to eliminate the possibility of passing NULL to these
functions in the first place.
julia