[PATCH 3/9] staging: brcm80211: remove kernel_thread() for dhd_watchdog_thread.

[Jiri Slaby]

On 10/07/2010 11:42 AM, Jiri Slaby wrote:
> On 10/06/2010 11:40 PM, Jason Cooper wrote:
>> --- a/drivers/staging/brcm80211/brcmfmac/dhd_linux.c
>> +++ b/drivers/staging/brcm80211/brcmfmac/dhd_linux.c
...
>> +		if (IS_ERR(tsk)) {
>> +			printk(KERN_WARNING
>> +				"dhd_watchdog thread failed to start\n");
>> +			dhd->watchdog_pid = -1;
>> +		} else {
>> +			dhd->watchdog_pid = (long)get_pid(task_pid(tsk));
> 
> This looks very wrong:
> 1) you leak a pid reference,
> 2) you shouldn't need pid at all, you should use kthread_stop with
> kthread_should_stop instead

Actually, you have these checks in the code:
if (dhd->watchdog_pid >= 0) {
     kill_it(dhd->watchdog_pid)
}
But note that kernel pointers are mapped at the top of the VM. This
means, it's always negative when you cast pid pointer to long. Hence you
never kill it. "Fortunately", because you pass the long to
find_get_pid(), whereas it expects pid number, not struct pid pointer
casted to long.

So please redesign this to use task_struct properly. And get rid of
KILL_PROC too, because it is broken as well (it leaks a pid reference too).

regards,
-- 
js