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

[Joe Perches <joe@perches.com>]

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.