Message-ID: <1430239010.2004.6.camel@stgolabs.net>
Subject: Re: [PATCH v2] ipc/mqueue: remove STATE_PENDING
From: Davidlohr Bueso <dave@stgolabs.net>
To: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>,
        Sebastian Andrzej Siewior <bigeasy@linutronix.de>,
        Manfred Spraul <manfred@colorfullife.com>,
        LKML <linux-kernel@vger.kernel.org>, Ingo Molnar <mingo@redhat.com>,
        Darren Hart <darren@dvhart.com>, Steven Rostedt <rostedt@goodmis.org>,
        fredrik.markstrom@windriver.com,
        "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Date: Tue, 28 Apr 2015 09:36:50 -0700
In-Reply-To: <20150428123738.GY23123@twins.programming.kicks-ass.net>
References: <1428419030-20030-1-git-send-email-bigeasy@linutronix.de>
	 <1428419030-20030-4-git-send-email-bigeasy@linutronix.de>
	 <55241851.7060704@colorfullife.com> <20150410143726.GD3057@linutronix.de>
	 <alpine.DEB.2.11.1504221404350.13914@nanos>
	 <1430191493.2050.3.camel@stgolabs.net>
	 <20150428123738.GY23123@twins.programming.kicks-ass.net>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1734
Lines: 47

On Tue, 2015-04-28 at 14:37 +0200, Peter Zijlstra wrote:
> On Mon, Apr 27, 2015 at 08:24:53PM -0700, Davidlohr Bueso wrote:
> > +static inline void pipelined_send(struct wake_q_head *wake_q,
> > +				  struct mqueue_inode_info *info,
> >  				  struct msg_msg *message,
> >  				  struct ext_wait_queue *receiver)
> >  {
> >  	receiver->msg = message;
> >  	list_del(&receiver->list);
> > +	wake_q_add(wake_q, receiver->task);
> > +	/*
> > +	 * Ensure that updating receiver->state is the last
> > +	 * write operation: As once set, the receiver can continue,
> > +	 * and if we don't have the reference count from the wake_q,
> > +	 * yet, at that point we can later have a use-after-free
> > +	 * condition and bogus wakeup.
> > +	 */
> > +	smp_wmb(); /* pairs with smp_rmb() in wq_sleep */
> 
> You have this barrier because we cannot rely on a failed cmpxchg()
> actually being a full barrier, right?

Failed cmpxchg() calls implies that the task is never added to the queue
(duplicate, which I cannot see occurring in this patch), so nothing
wrong with the bogus wakeups mentioned in the comment.

This barrier is not added by this patch though. Currently we have it
serializing with the wake_up_process() with STATE_READY, for similar
reasons. Because there is no task refcounting going on, the task can
easily disappear underneath us if the state is set before the wakeup. I
applied the same judgment here.

Thanks,
Davidlohr

> 
> >  	receiver->state = STATE_READY;
> >  }
> 
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/