linux-kernel - Re: [PATCH v3] timer: implement lockdep deadlock detection

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]

Message-Id: <1233166017.4811.9.camel@johannes.local>
Date:	Wed, 28 Jan 2009 19:06:57 +0100
From:	Johannes Berg <johannes@...solutions.net>
To:	Peter Zijlstra <peterz@...radead.org>
Cc:	Ingo Molnar <mingo@...e.hu>, Thomas Gleixner <tglx@...utronix.de>,
	Linux Kernel list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3] timer: implement lockdep deadlock detection

On Wed, 2009-01-28 at 11:59 +0100, Johannes Berg wrote:

> > > I actually got "trying to register non-static key" on my powerpc64
> > > machine. Is there a possibility that functions are not static??
> > 
> > Hmm, weird, afaict static_obj() includes both text and data, for the
> > core kernel as well as modules.
> 
> Yeah, I'd think it should. I'll run it by the powerpc list when I get it
> again, it only seems to happen very rarely.

It's actually a generic bug.

delayed work structs are initialised like this:

#define __DELAYED_WORK_INITIALIZER(n, f) {                      \
        .work = __WORK_INITIALIZER((n).work, (f)),              \
        .timer = TIMER_INITIALIZER(NULL, 0, 0),                 \
        }

#define DECLARE_DELAYED_WORK(n, f)                              \
        struct delayed_work n = __DELAYED_WORK_INITIALIZER(n, f)


Note the NULL function, which I used for the key of timers. Thus, the
key is NULL, and the name is "NULL".

Now, this means that my code for the run timer:

				struct lockdep_map lockdep_map =
					timer->lockdep_map;

will actually have lockdep_map here with a NULL key. Once it gets into

			lock_map_acquire(&lockdep_map);

it'll try to register &lockdep_map as the key.

Interestingly, that doesn't seem to be a problem on x86_64, which would
appear to be a bug, the stack certainly isn't a static location.

The patch below fixes it by using the file/lineno of the static
definition as both the name and the key -- using it as the name means
you have a good chance of finding it if something goes wrong, and using
it as the key means we have a good key for it. The patch looks horrible
though. Any better ideas? If not, I think we should roll this into the
original patch.

johannes
---
 include/linux/timer.h |   26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

--- wireless-testing.orig/include/linux/timer.h	2009-01-28 18:55:03.270071595 +0100
+++ wireless-testing/include/linux/timer.h	2009-01-28 19:00:41.326947619 +0100
@@ -32,23 +32,35 @@ extern struct tvec_base boot_tvec_bases;
 /*
  * NB: because we have to copy the lockdep_map, setting _key
  * here is required, otherwise it could get initialised to the
- * copy of the lockdep_map! We use the function pointer as the
- * lockdep key here.
+ * copy of the lockdep_map! We use the pointer to and the string
+ * "<file>:<line>" as the key resp. the name of the lockdep_map.
  */
-#define __TIMER_LOCKDEP_MAP_INITIALIZER(fn, name)		\
-	.lockdep_map = STATIC_LOCKDEP_MAP_INIT(name, fn),
+#define __TIMER_LOCKDEP_MAP_INITIALIZER(name, key)			\
+	.lockdep_map = STATIC_LOCKDEP_MAP_INIT(name, key),
 #else
-#define __TIMER_LOCKDEP_MAP_INITIALIZER(fn, name)
+#define __TIMER_LOCKDEP_MAP_INITIALIZER(name, key)
 #endif
 
-#define TIMER_INITIALIZER(_function, _expires, _data) {			\
+#define ____TIMER_INITIALIZER(_function, _expires, _data, _name, _key) {\
 		.entry = { .prev = TIMER_ENTRY_STATIC },		\
 		.function = (_function),				\
 		.expires = (_expires),					\
 		.data = (_data),					\
 		.base = &boot_tvec_bases,				\
-		__TIMER_LOCKDEP_MAP_INITIALIZER((_function), #_function)\
+		__TIMER_LOCKDEP_MAP_INITIALIZER((_name), (_key))	\
 	}
+/*
+ * All the indirection here is required to build the
+ * "<file>:<line>" string and the pointer to it.
+ */
+#define ___TIMER_INITIALIZER(_fn, _exp, _dat, _kn)			\
+	____TIMER_INITIALIZER(_fn, _exp, _dat, (_kn), &(_kn))
+#define __TIMER_INITIALIZER(_fn, _exp, _dat, _f, _c, _l)		\
+	___TIMER_INITIALIZER(_fn, _exp, _dat, _f # _c # _l)
+#define _TIMER_INITIALIZER(_fn, _exp, _dat, _f, _l)			\
+	__TIMER_INITIALIZER(_fn, _exp, _dat, _f, :, _l)
+#define TIMER_INITIALIZER(_function, _expires, _data)			\
+	_TIMER_INITIALIZER(_function, _expires, _data, __FILE__, __LINE__)
 
 #define DEFINE_TIMER(_name, _function, _expires, _data)		\
 	struct timer_list _name =				\


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