linux-kernel - Re: [PATCH]: Use cmpxchg() in WARN_*

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

Message-ID: <4D971BC4.7010404@redhat.com>
Date:	Sat, 02 Apr 2011 08:51:16 -0400
From:	Prarit Bhargava <prarit@...hat.com>
To:	linux-kernel@...r.kernel.org
CC:	Steven Rostedt <rostedt@...dmis.org>, dzickus@...hat.com
Subject: Re: [PATCH]: Use cmpxchg() in WARN_*_ONCE() functions

Missed deleting a chunk at the end of the patch...

An issue popped up where WARN_ON_ONCE() was used in a callback function
in smp_call_function().  This resulted in the WARN_ON executing multiple times
when it should have only executed once.

We cannot use a lock in the WARN_*_ONCE functions because it is far too heavy
for the code in question.  Don Zickus suggested using cmpxchg(), which resolves
the problem of the multiple outputs.

The question surrounding cmpxchg(), of course, is one of performance.  It
appears that, at least on x86, it isn't a concern.

I ran the following test in a simple module:

void prarit_callback(void *info)
{
        WARN_ON_ONCE(1);
}

        for (j = 0; j < 10; j++)
                for (i = 0; i < 1000000; i++)
                        prarit_callback(NULL);

With the current code in place an average of 10 runs was .219s.  Doing just
a (!cmpxchg(&__warned, 0, 1)) the time jumps to .402s.  Adding in a check
for the __warned flag, (!__warned && !cmpxchg(&__warned, 0, 1)), results in
an average of .220s.

I then did

        for (i = 0; i < 1000000; i++)
                on_each_cpu(prarit_callback, NULL, 0);

The current code, of course, explodes :).  That's the bug I'm trying to fix.
What is interesting in this test, however, is the impact that checking the
!__warned flag has [Aside: Checking the !__warned flag is an enhancement
and is not explicitly required for this code].

A run with just (!cmpxchg(&__warned, 0, 1)) results in an average of 21.323s,
and a run with  (!__warned && !cmpxchg(&__warned, 0, 1)) results in an
average of 20.233s.  Of course, the !__warned is not necessary for the code
to work properly but it seems to be a significant impact to the time to run
this code.

P.

WARN_ON_ONCE() is used in callback functions in smp_call_function().  This
results in races through the WARN_ON() code.

Use cmxchg() in the WARN_*_ON functions in order to guarantee single
execution of the WARN_ONs.

Signed-off-by: Prarit Bhargava <prarit@...hat.com>

diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index f2d2faf..1825fd7 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -133,32 +133,32 @@ extern void warn_slowpath_null(const char *file, const int line);
 #endif

 #define WARN_ON_ONCE(condition)	({				\
-	static bool __warned;					\
+	static int __warned;					\
 	int __ret_warn_once = !!(condition);			\
 								\
 	if (unlikely(__ret_warn_once))				\
-		if (WARN_ON(!__warned)) 			\
-			__warned = true;			\
+		if (!__warned && !cmpxchg(&__warned, 0, 1))	\
+			WARN_ON(1);				\
 	unlikely(__ret_warn_once);				\
 })

 #define WARN_ONCE(condition, format...)	({			\
-	static bool __warned;					\
+	static int __warned;					\
 	int __ret_warn_once = !!(condition);			\
 								\
 	if (unlikely(__ret_warn_once))				\
-		if (WARN(!__warned, format)) 			\
-			__warned = true;			\
+		if (!__warned && !cmpxchg(&__warned, 0, 1))	\
+			WARN(1, format);			\
 	unlikely(__ret_warn_once);				\
 })

 #define WARN_TAINT_ONCE(condition, taint, format...)	({	\
-	static bool __warned;					\
+	static int __warned;					\
 	int __ret_warn_once = !!(condition);			\
 								\
 	if (unlikely(__ret_warn_once))				\
-		if (WARN_TAINT(!__warned, taint, format))	\
-			__warned = true;			\
+		if (!__warned && !cmpxchg(&__warned, 0, 1))	\
+			WARN_TAINT(1, taint, format);		\
 	unlikely(__ret_warn_once);				\
 })

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