Re: [PATCH] ipc/sem: Fix race between to-be-woken task and waker

[Waiman Long]

On 9/29/19 6:24 AM, Manfred Spraul wrote:
> Hi Waiman,
>
> I have now written the mail 3 times:
> Twice I thought that I found a race, but during further analysis, it
> always turns out that the spin_lock() is sufficient.
>
> First, to avoid any obvious things: Until the series with e.g.
> 27d7be1801a4824e, there was a race inside sem_lock().
>
> Thus it was possible that multiple threads were operating on the same
> semaphore array, with obviously arbitrary impact.
>
I was saying that there was a race. It was just that my initial analysis
of the code seems to indicate a race was possible.


> On 9/20/19 5:54 PM, Waiman Long wrote:
>
>> Â +ÂÂÂÂÂÂÂ /*
>> +ÂÂÂÂÂÂÂÂ * A spurious wakeup at the right moment can cause race
>> +ÂÂÂÂÂÂÂÂ * between the to-be-woken task and the waker leading to
>> +ÂÂÂÂÂÂÂÂ * missed wakeup. Setting state back to TASK_INTERRUPTIBLE
>> +ÂÂÂÂÂÂÂÂ * before checking queue.status will ensure that the race
>> +ÂÂÂÂÂÂÂÂ * won't happen.
>> +ÂÂÂÂÂÂÂÂ *
>> +ÂÂÂÂÂÂÂÂ *ÂÂÂ CPU0ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ CPU1
>> +ÂÂÂÂÂÂÂÂ *
>> +ÂÂÂÂÂÂÂÂ *Â <spurious wakeup>ÂÂÂÂÂÂÂ wake_up_sem_queue_prepare():
>> +ÂÂÂÂÂÂÂÂ *Â state = TASK_INTERRUPTIBLEÂÂÂ status = error
>> +ÂÂÂÂÂÂÂÂ *ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ try_to_wake_up():
>> +ÂÂÂÂÂÂÂÂ *Â smp_mb()ÂÂÂÂÂÂÂÂÂÂÂÂÂ smp_mb()
>> +ÂÂÂÂÂÂÂÂ *Â if (status == -EINTR)ÂÂÂÂÂ if (!(p->state & state))
>> +ÂÂÂÂÂÂÂÂ *ÂÂÂ schedule()ÂÂÂÂÂÂÂÂÂÂÂ goto out
>> +ÂÂÂÂÂÂÂÂ */
>> +ÂÂÂÂÂÂÂ set_current_state(TASK_INTERRUPTIBLE);
>> +
>
> So the the hypothesis is that we have a race due to the optimization
> within try_to_wake_up():
> If the status is already TASK_RUNNING, then the wakeup is a nop.
>
> Correct?
>
> The waker wants to use:
>
> ÂÂÂ lock();
> ÂÂÂ set_conditions();
> ÂÂÂ unlock();
>
> as the wake_q is a shared list, completely asynchroneously this will
> happen:
>
> ÂÂÂ smp_mb(); //// ***1
> ÂÂÂ if (current->state = TASK_INTERRUPTIBLE) current->state=TASK_RUNNING;
>
> The only guarantee is that this will happen after lock(), it may
> happen before set_conditions().
>
> The task that goes to sleep uses:
>
> ÂÂÂ lock();
> ÂÂÂ check_conditions();
> ÂÂÂ __set_current_state();
> ÂÂÂ unlock(); //// ***2
> ÂÂÂ schedule();
>
> You propose to change that to:
>
> ÂÂÂ lock();
> ÂÂÂ set_current_state();
> ÂÂÂ check_conditions();
> ÂÂÂ unlock();
> ÂÂÂ schedule();
>
> I don't see a race anymore, and I don't see how the proposed change
> will help.
> e.g.: __set_current_state() and smp_mb() have paired memory barriers
> ***1 and ***2 above. 

Now that I had taken a second look at it. I agreed that the current code
should be fine. My only comment is that we should probably add extra
comments to clarify the situation so that it won't get messed up in the
future.

Cheers,
Longman