smatch: module_init unwinding on errors

smatch: module_init unwinding on errors

[Dan Carpenter]

Sort of inspired by Vasiliy's coccinelle work, I wrote a Smatch script to
verify that modules unwind on errors.

http://repo.or.cz/w/smatch.git/blob/HEAD:/check_unwind.c 

The heuristic is if you
	* call request_irq() 
	* it possibly succeeded
	* you don't call free_irq() 
	* you're in a function that returns an int
	* you are returning non-zero
Then complain.

One question that I had was for, "foo = request_region(bar, size)"
should we call release_region() on "foo" or on "bar"?

regards,
dan carpenter

Re: smatch: module_init unwinding on errors

[Randy Dunlap]

On Thu, 29 Jul 2010 19:43:29 +0200 Dan Carpenter wrote:

> Sort of inspired by Vasiliy's coccinelle work, I wrote a Smatch script to
> verify that modules unwind on errors.
> 
> http://repo.or.cz/w/smatch.git/blob/HEAD:/check_unwind.c 
> 
> The heuristic is if you
> 	* call request_irq() 
> 	* it possibly succeeded
> 	* you don't call free_irq() 
> 	* you're in a function that returns an int
> 	* you are returning non-zero
> Then complain.
> 
> One question that I had was for, "foo = request_region(bar, size)"
> should we call release_region() on "foo" or on "bar"?

Hi Dan,

Most drivers just use the result of request_region() as a pass/fail indicator.
Only a very few of them actually save the result pointer;  mostly they just pass
bar to release_region().

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: smatch: module_init unwinding on errors

[Dan Carpenter]

On Thu, Jul 29, 2010 at 11:00:07AM -0700, Randy Dunlap wrote:
> Hi Dan,
> 
> Most drivers just use the result of request_region() as a pass/fail indicator.
> Only a very few of them actually save the result pointer;  mostly they just pass
> bar to release_region().
> 

That was another thing which was concerning me.  request_region()
returns freshly allocated memory.  Isn't it a leak to discard this?

regards,
dan carpenter

Re: smatch: module_init unwinding on errors

[Randy Dunlap]

On Thu, 29 Jul 2010 22:18:03 +0200 Dan Carpenter wrote:

> On Thu, Jul 29, 2010 at 11:00:07AM -0700, Randy Dunlap wrote:
> > Hi Dan,
> > 
> > Most drivers just use the result of request_region() as a pass/fail indicator.
> > Only a very few of them actually save the result pointer;  mostly they just pass
> > bar to release_region().
> > 
> 
> That was another thing which was concerning me.  request_region()
> returns freshly allocated memory.  Isn't it a leak to discard this?

If I knew the answer, I would have already replied...
I was hoping that someone else could help out here.

I expect that if there was a leak, someone would have already noticed
and pointed this out to us.  OTOH, this could be the last class of kernel
leaks that needs to be fixed...  I dunno.

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: smatch: module_init unwinding on errors

[Julia Lawall]

On Thu, 5 Aug 2010, Randy Dunlap wrote:

> On Thu, 29 Jul 2010 22:18:03 +0200 Dan Carpenter wrote:
> 
> > On Thu, Jul 29, 2010 at 11:00:07AM -0700, Randy Dunlap wrote:
> > > Hi Dan,
> > > 
> > > Most drivers just use the result of request_region() as a pass/fail indicator.
> > > Only a very few of them actually save the result pointer;  mostly they just pass
> > > bar to release_region().
> > > 
> > 
> > That was another thing which was concerning me.  request_region()
> > returns freshly allocated memory.  Isn't it a leak to discard this?
> 
> If I knew the answer, I would have already replied...
> I was hoping that someone else could help out here.
> 
> I expect that if there was a leak, someone would have already noticed
> and pointed this out to us.  OTOH, this could be the last class of kernel
> leaks that needs to be fixed...  I dunno.

request_region expands to a call to __request_region, which creates the 
new memory and calls __request_resource.  __request_resource fortunately 
puts it into a tree that release_region subsequently frees.  So there is 
no problem with discarding the return value.

julia