From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1751955AbcJILU1 (ORCPT <rfc822;w@1wt.eu>);
        Sun, 9 Oct 2016 07:20:27 -0400
Received: from Galois.linutronix.de ([146.0.238.70]:47476 "EHLO
        Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1751647AbcJILU0 (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Sun, 9 Oct 2016 07:20:26 -0400
Date: Sun, 9 Oct 2016 13:17:50 +0200 (CEST)
From: Thomas Gleixner <tglx@linutronix.de>
To: Peter Zijlstra <peterz@infradead.org>
cc: mingo@kernel.org, juri.lelli@arm.com, rostedt@goodmis.org,
        xlpang@redhat.com, bigeasy@linutronix.de, linux-kernel@vger.kernel.org,
        mathieu.desnoyers@efficios.com, jdesfossez@efficios.com,
        bristot@redhat.com
Subject: Re: [RFC][PATCH 4/4] futex: Rewrite FUTEX_UNLOCK_PI
In-Reply-To: <20161007112143.GJ3117@twins.programming.kicks-ass.net>
Message-ID: <alpine.DEB.2.20.1610091309360.5222@nanos>
References: <20161003091234.879763059@infradead.org> <20161003091847.704255067@infradead.org> <20161007112143.GJ3117@twins.programming.kicks-ass.net>
User-Agent: Alpine 2.20 (DEB 67 2015-01-07)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Fri, 7 Oct 2016, Peter Zijlstra wrote:
>  	top_waiter = futex_top_waiter(hb, &key);
>  	if (top_waiter) {
> -		ret = wake_futex_pi(uaddr, uval, top_waiter, hb);
> +		struct futex_pi_state *pi_state = top_waiter->pi_state;
> +
> +		ret = -EINVAL;
> +		if (!pi_state)
> +			goto out_unlock;
> +
> +		/*
> +		 * If current does not own the pi_state then the futex is
> +		 * inconsistent and user space fiddled with the futex value.
> +		 */
> +		if (pi_state->owner != current)
> +			goto out_unlock;
> +
> +		/*
> +		 * Grab a reference on the pi_state and drop hb->lock.
> +		 *
> +		 * The reference ensures pi_state lives, dropping the hb->lock
> +		 * is tricky.. wake_futex_pi() will take rt_mutex::wait_lock to
> +		 * close the races against futex_lock_pi(), but in case of
> +		 * _any_ fail we'll abort and retry the whole deal.
> +		 */
> +		WARN_ON_ONCE(!atomic_inc_not_zero(&pi_state->refcount));
> +		spin_unlock(&hb->lock);
> +
> +		ret = wake_futex_pi(uaddr, uval, pi_state);
> +
> +		put_pi_state(pi_state);

put_pi_state() requires hb->lock protection AFAICT.

CPU0	       			 CPU1

    wake_futex_pi()		 attach_to_pi_state()
    put_pi_state()
	refcount--;	
	if (!refcount)		
	    free_state();	
	    			WARN_ON(!pi_state->refcount);

we might not see the warning, but in any case the following access to
pi_state on cpu1 is borked.

Thanks,

	tglx