Re: [RFC][2.6.8-rc1-mm1] perfctr inheritance locking issue

[Manfred Spraul <manfred@colorfullife.com>]

Andrew Morton wrote:

>Mikael Pettersson <mikpe@csd.uu.se> wrote:
>  
>
>>Andrew,
>>
>>There is another locking problem with the per-process
>>performance counter inheritance changes I sent you.
>>
>>I currently use task_lock(tsk) to synchronise accesses
>>to tsk->thread.perfctr, when that pointer could change.
>>
>>The write_lock_irq(&tasklist_lock) in release_task() is
>>needed to prevent ->parent from changing while releasing the
>>child, but the parent's ->thread.perfctr must also be locked.
>>However, sched.h explicitly forbids holding task_lock()
>>simultaneously with write_lock_irq(&tasklist_lock). Ouch.
>>    
>>
>
>That's ghastly.
>
> * Nests both inside and outside of read_lock(&tasklist_lock).
> * It must not be nested with write_lock_irq(&tasklist_lock),
> * neither inside nor outside.
>
>Manfred, where did you discover the offending code?
>
>  
>
Think about interrupts: they are permitted to acquire the tasklist_lock 
for read.

Someone does
    read_lock(&tasklist_lock);
    task_lock(tsk);

One example is __do_SAK in tty_io.c, but I think there are further examples.

Now add a softirq that tries to deliver a signal: kill_something_info() 
contains a
    read_lock(&tasklist_lock);

This sequence doesn't deadlock - rw spinlocks starve writers.
But it means that both
    task_lock();
    write_lock_irq(&tasklist_lock);
and
    write_lock_irq(&tasklist_lock);
    task_lock();

can deadlock with the read_lock()/task_lock()/read_lock() sequence.

>Would be better to just sort out the locking, then take task_lock() inside
>tasklist_lock.  That was allegedly the rule in 2.4.
>  
>
It probably works by chance in 2.4.

--  
    Manfred