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

[Rui Paulo]

On 23 Jan 2010, at 00:05, Giovanni Trematerra wrote:

> On Fri, Jan 22, 2010 at 05:40:41PM +0100, Attilio Rao wrote:
>> 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
> 
> In kthread_suspend_check you might have written
> panic("%s: curthread is not a valid kthread", __func__);
> 
>> 
>> Gianni, may you test this patch with the modules you made?
>> 
> 
> With your last patch, no deadlock happens now.
> It seems ok.
> 
> Hope this help
> 
> Just in case someone would make a review of the kernel test module
> here it is the code:

This is a great candidate for a regression add-on.

--
Rui Paulo