linux-kernel - Re: [PATCH 1/3] wait-simple: Introduce the simple waitqueue implementation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 12 Dec 2013 09:42:27 -0500
From:	Steven Rostedt <rostedt@...dmis.org>
To:	Peter Zijlstra <peterz@...radead.org>
Cc:	Paul Gortmaker <paul.gortmaker@...driver.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...nel.org>,
	Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] wait-simple: Introduce the simple waitqueue
 implementation

On Thu, 12 Dec 2013 12:44:47 +0100
Peter Zijlstra <peterz@...radead.org> wrote:

> On Wed, Dec 11, 2013 at 08:06:37PM -0500, Paul Gortmaker wrote:
> > +/* Adds w to head->task_list. Must be called with head->lock locked. */
> > +static inline void __swait_enqueue(struct swait_queue_head *head,
> > +				   struct swaiter *w)
> > +{
> > +	list_add(&w->node, &head->task_list);
> > +	/* We can't let the condition leak before the setting of head */
> > +	smp_mb();
> > +}
> 
> > +unsigned int
> > +__swake_up_locked(struct swait_queue_head *head, unsigned int state,
> > +		  unsigned int num)
> > +{
> > +	struct swaiter *curr, *next;
> > +	int woken = 0;
> > +
> > +	list_for_each_entry_safe(curr, next, &head->task_list, node) {
> > +		if (wake_up_state(curr->task, state)) {
> > +			__swait_dequeue(curr);
> > +			/*
> > +			 * The waiting task can free the waiter as
> > +			 * soon as curr->task = NULL is written,
> > +			 * without taking any locks. A memory barrier
> > +			 * is required here to prevent the following
> > +			 * store to curr->task from getting ahead of
> > +			 * the dequeue operation.
> > +			 */
> > +			smp_wmb();
> > +			curr->task = NULL;
> > +			if (++woken == num)
> > +				break;
> > +		}
> > +	}
> > +	return woken;
> > +}
> 
> Are these two barriers matched or are they both unmatched and thus
> probabyl wrong?

Nope, the two are unrelated. The the smp_wmb() is to synchronize with
the swait_finish() code. When the task wakes up, it checks if w->task
is NULL, and if it is it does not grab the head->lock and does not
dequeue it, it simply exits, where the caller can then free the swaiter
structure.

Without the smp_wmb(), the curr->task can be set to NULL before we
dequeue it, and if the woken up process sees that NULL, it can jump
right to freeing the swaiter structure and cause havoc with this
__swait_dequeue().


The first smp_mb() is about the condition in:

+#define __swait_event(wq, condition)					\
+do {									\
+	DEFINE_SWAITER(__wait);						\
+									\
+	for (;;) {							\
+		swait_prepare(&wq, &__wait, TASK_UNINTERRUPTIBLE);	\
+		if (condition)						\
+			break;						\
+		schedule();						\
+	}								\
+	swait_finish(&wq, &__wait);					\
+} while (0)

without the smp_mb(), it is possible that the condition can leak into
the critical section of swait_prepare() and have the old condition seen
before the task is added to the wait list. My submission of this patch
described it in more details:

https://lkml.org/lkml/2013/8/19/275

> 
> In any case the comments need updating to be more explicit.

Yeah, I can add documentation about this as well. The smp_wmb() I think
is probably documented enough, but the two smp_mb()s need a little more
explanation.

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