[PATCH 1/1] scripts/coccinelle/free: add conditional kfree test

[PATCH 1/1] scripts/coccinelle/free: add conditional kfree test

[Fabian Frederick]

This patch adds a trivial script warning on

if(foo)
	kfree(foo)

(based on checkpatch)

Cc: Julia Lawall <Julia.Lawall@lip6.fr>
Cc: Gilles Muller <Gilles.Muller@lip6.fr>
Cc: Joe Perches <joe@perches.com>
Signed-off-by: Fabian Frederick <fabf@skynet.be>
---
 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

Re: [PATCH 1/1] scripts/coccinelle/free: add conditional kfree test

[Joe Perches]

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

Re: [PATCH 1/1] scripts/coccinelle/free: add conditional kfree test

[SF Markus Elfring]

>> 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

Re: [PATCH 1/1] scripts/coccinelle/free: add conditional kfree test

[Julia Lawall]

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

Re: [PATCH 1/1] scripts/coccinelle/free: add conditional kfree test

[Joe Perches]

(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.

Re: [PATCH 1/1] scripts/coccinelle/free: add conditional kfree test

[Julia Lawall]

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

Re: [PATCH 1/1] scripts/coccinelle/free: add conditional kfree test

[Joe Perches]

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.

Re: [PATCH 1/1] scripts/coccinelle/free: add conditional kfree test

[Julia Lawall]

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