smatch: module_init unwinding on errors
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
Quoted 13 lines · 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
* callrequest_irq()
* it possibly succeeded
* you don't callfree_irq()
* you're in a function that returns an int
* you are returning non-zero
Then complain.
Quoted 2 lines
One question that I had was for, "foo =
request_region(bar, size)" should we callrelease_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 ***
Quoted 6 lines · Randy Dunlap
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 torelease_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
Quoted 10 lines · On Thu, 29 Jul 2010 22:18:03 +0200 Dan Carpenter wrote:
Quoted 6 lines · Randy Dunlap
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 torelease_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 ***
Quoted 13 lines · Randy Dunlap
Quoted 10 lines · On Thu, 29 Jul 2010 22:18:03 +0200 Dan Carpenter wrote:
Quoted 6 lines · Randy Dunlap
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 torelease_region().That was another thing which was concerning me.
request_region()returns freshly allocated memory. Isn't it a leak to discard this?
Quoted 6 lines
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