[PATCH] kthread_{suspend, resume, suspend_check} locking bugs

[Attilio Rao]

2010/1/16 Jilles Tjoelker <jilles at stack.nl>:
> On Sun, Jan 10, 2010 at 02:09:43AM +0100, Attilio Rao wrote:
>> I think that routines kthread_suspend(), kthread_resume() and
>> kthread_suspend_check() are not adeguately SMP protected.
>> That is because, in particular, the critical path doesn't protect,
>> together, TDF_KTH_SUSP and sleeping activity. The right pattern should
>> be to use the thread lock spinlock as an interlock and use msleep.
>> Such bugs have not been revealed probabilly because there has been a
>> lack of testing of such primitives and there are not, currently,
>> consumers within our stock kernel.
>
>> Additively, kthread_suspend_check() seems to require to always pass
>> curthread, which is silly (as we don't have to conform to any
>> particular KPI), thus I think it is appropriate for the prototype to
>> change.
>> The following patch should fix the issue:
>> http://www.freebsd.org/~attilio/kthread.diff
>
> The analysis and patch look sensible.

After have digged more with jhb I made a new patch:
http://www.freebsd.org/~attilio/kthread_races.diff

The previous one has the problem that thread_lock is going to change
in ULE, and thus panic, if the awaken thread will be scheduled on a
different runqueue wrt the one where it got sleeping.
On this optic it would be good to panic if a td_lock lock is passed to
msleep_spin() but that is not an easy condition to check (I thought
about asserting the name of the locks to not be a threads container
one... sigh...).

On the original code, there is also another problem. The waitchannel
for suspender and suspending threads are different while they should
not be (and kproc_*() seems to agree with me). It might be fixed as
well.

Gianni, may you test this patch with the modules you made?

Thanks,
Attilio


-- 
Peace can only be achieved by understanding - A. Einstein