Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH] ipc/sem: Fix race between to-be-woken task and waker
To:     Waiman Long <longman@redhat.com>,
        Davidlohr Bueso <dave@stgolabs.net>,
        Peter Zijlstra <peterz@infradead.org>
References: <20190920155402.28996-1-longman () redhat ! com>
From:   Manfred Spraul <manfred@colorfullife.com>
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        1vier1@web.de
Message-ID: <d89b622a-2acf-b0a9-021d-c1c521a731f5@colorfullife.com>
Date:   Sun, 29 Sep 2019 12:24:31 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.1.0
MIME-Version: 1.0
In-Reply-To: <20190920155402.28996-1-longman () redhat ! com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.

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.

--

     Manfred