linux-kernel - Re: [tip:core/urgent] WARN_ON_SMP(): Add comment to explain ({0;})

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>] [day] [month] [year] [list]

Date:	Tue, 29 Mar 2011 10:47:28 -0400
From:	Steven Rostedt <rostedt@...dmis.org>
To:	"H. Peter Anvin" <hpa@...or.com>
Cc:	Ingo Molnar <mingo@...e.hu>, Alexey Dobriyan <adobriyan@...il.com>,
	richard -rw- weinberger <richard.weinberger@...il.com>,
	linux-kernel@...r.kernel.org, mingo@...hat.com,
	torvalds@...ux-foundation.org, srostedt@...hat.com,
	tglx@...utronix.de, linux-tip-commits@...r.kernel.org
Subject: Re: [tip:core/urgent] WARN_ON_SMP(): Add comment to explain ({0;})

On Tue, 2011-03-29 at 07:32 -0700, H. Peter Anvin wrote:
> I think the question is what are the expected semantics if WARN_ON_SMP
> is used in an if... especially for UP.

Well, actually this whole patchset was created because we added the
semantic if (WARN_ON_SMP(x)).

This patch: https://lkml.org/lkml/2011/3/11/463


Added the following code:


+static void __unqueue_futex(struct futex_q *q)
+{
+	struct futex_hash_bucket *hb;
+
+	if (WARN_ON(!q->lock_ptr || !spin_is_locked(q->lock_ptr)
+			|| plist_node_empty(&q->list)))
+		return;
+
+	hb = container_of(q->lock_ptr, struct futex_hash_bucket, lock);
+	plist_del(&q->list, &hb->chain);
+}


Which on !CONFIG_SMP broke, because that "|| !spin_is_locked()" always
return true and we trigger the WARN_ON() as well as returning early,
breaking the code, and triggering bugs, as we never do the
"plist_del()".

To fix this, I added: https://lkml.org/lkml/2011/3/17/312

Which does this change:

@@ -785,8 +785,8 @@ static void __unqueue_futex(struct futex_q *q)
 {
 	struct futex_hash_bucket *hb;
 
-	if (WARN_ON(!q->lock_ptr || !spin_is_locked(q->lock_ptr)
-			|| plist_node_empty(&q->list)))
+	if (WARN_ON_SMP(!q->lock_ptr || !spin_is_locked(q->lock_ptr))
+	    || WARN_ON(plist_node_empty(&q->list)))
 		return;
 
 	hb = container_of(q->lock_ptr, struct futex_hash_bucket, lock);


But as WARN_ON_SMP() currently had no if () context, I needed to first
make the change of: https://lkml.org/lkml/2011/3/17/313

#ifdef CONFIG_SMP
 # define WARN_ON_SMP(x)			WARN_ON(x)
 #else
-# define WARN_ON_SMP(x)			do { } while (0)
+# define WARN_ON_SMP(x)			({0;})
 #endif

But if this is such a problem, we can revert all this and do:

@@ -785,8 +785,11 @@ static void __unqueue_futex(struct futex_q *q)
 {
 	struct futex_hash_bucket *hb;
 
-	if (WARN_ON(!q->lock_ptr || !spin_is_locked(q->lock_ptr)
-			|| plist_node_empty(&q->list)))
+#ifdef CONFIG_SMP
+	if (WARN_ON(!q->lock_ptr || !spin_is_locked(q->lock_ptr))
+		return;
+#endif
+	if (WARN_ON(plist_node_empty(&q->list)))
 		return;
 
 	hb = container_of(q->lock_ptr, struct futex_hash_bucket, lock);


And leave the WARN_ON_SMP() alone.

-- 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/