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 3/6] ipc/mqueue.c: Update/document memory barriers
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        Davidlohr Bueso <dave@stgolabs.net>,
        Waiman Long <longman@redhat.com>, 1vier1@web.de,
        Andrew Morton <akpm@linux-foundation.org>,
        Jonathan Corbet <corbet@lwn.net>
References: <20191012054958.3624-1-manfred@colorfullife.com>
 <20191012054958.3624-4-manfred@colorfullife.com>
 <20191014125911.GF2328@hirez.programming.kicks-ass.net>
 <20191014135832.GO2359@hirez.programming.kicks-ass.net>
From:   Manfred Spraul <manfred@colorfullife.com>
Message-ID: <575dbddb-355c-f667-92ca-d39b893c5ab1@colorfullife.com>
Date:   Mon, 14 Oct 2019 20:06:43 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.1.1
MIME-Version: 1.0
In-Reply-To: <20191014135832.GO2359@hirez.programming.kicks-ass.net>
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 Peter,

On 10/14/19 3:58 PM, Peter Zijlstra wrote:
> On Mon, Oct 14, 2019 at 02:59:11PM +0200, Peter Zijlstra wrote:
>> On Sat, Oct 12, 2019 at 07:49:55AM +0200, Manfred Spraul wrote:
>>
>>>   	for (;;) {
>>> +		/* memory barrier not required, we hold info->lock */
>>>   		__set_current_state(TASK_INTERRUPTIBLE);
>>>   
>>>   		spin_unlock(&info->lock);
>>>   		time = schedule_hrtimeout_range_clock(timeout, 0,
>>>   			HRTIMER_MODE_ABS, CLOCK_REALTIME);
>>>   
>>> +		if (READ_ONCE(ewp->state) == STATE_READY) {
>>> +			/*
>>> +			 * Pairs, together with READ_ONCE(), with
>>> +			 * the barrier in __pipelined_op().
>>> +			 */
>>> +			smp_acquire__after_ctrl_dep();
>>>   			retval = 0;
>>>   			goto out;
>>>   		}
>>>   		spin_lock(&info->lock);
>>> +
>>> +		/* we hold info->lock, so no memory barrier required */
>>> +		if (READ_ONCE(ewp->state) == STATE_READY) {
>>>   			retval = 0;
>>>   			goto out_unlock;
>>>   		}
>>> @@ -925,14 +933,12 @@ static inline void __pipelined_op(struct wake_q_head *wake_q,
>>>   	list_del(&this->list);
>>>   	wake_q_add(wake_q, this->task);
>>>   	/*
>>> +	 * The barrier is required to ensure that the refcount increase
>>> +	 * inside wake_q_add() is completed before the state is updated.
>> fails to explain *why* this is important.
>>
>>> +	 *
>>> +	 * The barrier pairs with READ_ONCE()+smp_mb__after_ctrl_dep().
>>>   	 */
>>> +        smp_store_release(&this->state, STATE_READY);
>> You retained the whitespace damage.
>>
>> And I'm terribly confused by this code, probably due to the lack of
>> 'why' as per the above. What is this trying to do?
>>
>> Are we worried about something like:
>>
>> 	A			B				C
>>
>>
>> 				wq_sleep()
>> 				  schedule_...();
>>
>> 								/* spuriuos wakeup */
>> 								wake_up_process(B)
>>
>> 	wake_q_add(A)
>> 	  if (cmpxchg()) // success
>>
>> 	->state = STATE_READY (reordered)
>>
>> 				  if (READ_ONCE() == STATE_READY)
>> 				    goto out;
>>
>> 				exit();
>>
>>
>> 	    get_task_struct() // UaF
>>
>>
>> Can we put the exact and full race in the comment please?

Yes, I'll do that. Actually, two threads are sufficient:

A                    B

WRITE_ONCE(wait.state, STATE_NONE);
schedule_hrtimeout()

                       wake_q_add(A)
                       if (cmpxchg()) // success
                       ->state = STATE_READY (reordered)

<timeout returns>
if (wait.state == STATE_READY) return;
sysret to user space
sys_exit()

                       get_task_struct() // UaF


> Like Davidlohr already suggested, elsewhere we write it like so:
>
>
> --- a/ipc/mqueue.c
> +++ b/ipc/mqueue.c
> @@ -930,15 +930,10 @@ static inline void __pipelined_op(struct
>   				  struct mqueue_inode_info *info,
>   				  struct ext_wait_queue *this)
>   {
> +	get_task_struct(this->task);
>   	list_del(&this->list);
> -	wake_q_add(wake_q, this->task);
> -	/*
> -	 * The barrier is required to ensure that the refcount increase
> -	 * inside wake_q_add() is completed before the state is updated.
> -	 *
> -	 * The barrier pairs with READ_ONCE()+smp_mb__after_ctrl_dep().
> -	 */
> -        smp_store_release(&this->state, STATE_READY);
> +	smp_store_release(&this->state, STATE_READY);
> +	wake_q_add_safe(wake_q, this->task);
>   }
>   
>   /* pipelined_send() - send a message directly to the task waiting in

Much better, I'll rewrite it and then resend the series.

--

     Manfred