linux kernel 2.6.18-20 bug: rcu_read_unlock in __sock_create

There is a bug in __sock_create() function (net/socket.c). If an error occur in LSM hook security_socket_post_create, then unneeded rcu_read_unlock() is made in __sock_create.

[NET]: Fix unbalanced rcu_read_unlock in __sock_create

The recent RCU work created an unbalanced rcu_read_unlock in __sock_create. This patch fixes that. Reported by oleg 123.

Signed-off-by Herbert Xu <[email protected]>

There seem to be some unbalanced rcu_read_{,un}lock() issues of late, how about doing something like this:

Signed-off-by Peter Zijlstra <[email protected]>

---
 include/linux/lockdep.h  |    7 +++++++
 include/linux/rcupdate.h |   12 ++++++++++++
 kernel/rcupdate.c        |    8 ++++++++
 3 files changed, 27 insertions(+)

Index: linux-2.6/include/linux/lockdep.h
===================================================================

include/linux/lockdep.h +7 -0

@@ -252,6 +252,13 @@ extern void lockdep_init_map(struct lock
 			     struct lock_class_key *key, int subclass);
 
 /*
+ * To initialize a lockdep_map statically use this macro.
+ * Note that _name must not be NULL.
+ */
+#define STATIC_LOCKDEP_MAP_INIT(_name, _key) \
+	{ .name = (_name), .key = (void *)(_key), }
+
+/*
  * Reinitialize a lock key - for cases where there is special locking or
  * special initialization of locks so that the validator gets the scope
  * of dependencies wrong: they are either too broad (they need a class-split)

Index: linux-2.6/include/linux/rcupdate.h
===================================================================

include/linux/rcupdate.h +12 -0

@@ -41,6 +41,7 @@
 #include <linux/percpu.h>
 #include <linux/cpumask.h>
 #include <linux/seqlock.h>
+#include <linux/lockdep.h>
 
 /**
  * struct rcu_head - callback structure for use with RCU

@@ -133,6 +134,15 @@ static inline void rcu_bh_qsctr_inc(int 
 extern int rcu_pending(int cpu);
 extern int rcu_needs_cpu(int cpu);
 
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+extern struct lockdep_map rcu_lock_map;
+# define rcu_read_acquire()	lock_acquire(&rcu_lock_map, 0, 0, 2, 1, _THIS_IP_)
+# define rcu_read_release()	lock_release(&rcu_lock_map, 1, _THIS_IP_)
+#else
+# define rcu_read_acquire()	do { } while (0)
+# define rcu_read_release()	do { } while (0)
+#endif
+
 /**
  * rcu_read_lock - mark the beginning of an RCU read-side critical section.
  *

@@ -166,6 +176,7 @@ extern int rcu_needs_cpu(int cpu);
 	do { \
 		preempt_disable(); \
 		__acquire(RCU); \
+		rcu_read_acquire(); \
 	} while(0)
 
 /**

@@ -175,6 +186,7 @@ extern int rcu_needs_cpu(int cpu);
  */
 #define rcu_read_unlock() \
 	do { \
+		rcu_read_release(); \
 		__release(RCU); \
 		preempt_enable(); \
 	} while(0)

Index: linux-2.6/kernel/rcupdate.c
===================================================================

kernel/rcupdate.c +8 -1

@@ -49,6 +49,14 @@
 #include <linux/cpu.h>
 #include <linux/mutex.h>
 
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+static struct lock_class_key rcu_lock_key;
+struct lockdep_map rcu_lock_map =
+	STATIC_LOCKDEP_MAP_INIT("rcu_read_lock", &rcu_lock_key);
+
+EXPORT_SYMBOL_GPL(rcu_lock_map);
+#endif
+
 /* Definition for rcupdate control block. */
 static struct rcu_ctrlblk rcu_ctrlblk = {
 	.cur = -300,
 
 
-

To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 3 lines · Peter Zijlstra

There seem to be some unbalanced rcu_read_{,un}lock() issues of late, how about doing something like this:

This will break when rcu_read_lock() and rcu_read_unlock() are invoked from NMI/SMI handlers -- the raw_local_irq_save() in lock_acquire() will not mask NMIs or SMIs.

One approach would be to check for being in an NMI/SMI handler, and to avoid calling lock_acquire() and lock_release() in those cases.

Another approach would be to use sparse, which has checks for rcu_read_lock()/rcu_read_unlock() nesting.

Quoted 11 lines · Paul E. McKenney

Quoted 3 lines · Peter Zijlstra

There seem to be some unbalanced rcu_read_{,un}lock() issues of late, how about doing something like this:

This will break when rcu_read_lock() and rcu_read_unlock() are invoked from NMI/SMI handlers -- the raw_local_irq_save() in lock_acquire() will not mask NMIs or SMIs.

One approach would be to check for being in an NMI/SMI handler, and to avoid calling lock_acquire() and lock_release() in those cases.

It seems:

#define nmi_enter() do { lockdep_off(); __irq_enter(); } while (0)
#define nmi_exit() do { __irq_exit(); lockdep_on(); } while (0)

Should make it all work out just fine. (for NMIs at least, /me fully ignorant of the workings of SMIs)

Quoted 2 lines

Another approach would be to use sparse, which has checks for rcu_read_lock()/rcu_read_unlock() nesting.

Yeah, but one more method can never hurt, no? :-)

Quoted 14 lines · Peter Zijlstra

Quoted 5 lines · Paul E. McKenney

Quoted 3 lines · Peter Zijlstra

There seem to be some unbalanced rcu_read_{,un}lock() issues of late, how about doing something like this:

Quoted 7 lines

This will break when rcu_read_lock() and rcu_read_unlock() are invoked from NMI/SMI handlers -- the raw_local_irq_save() in lock_acquire() will not mask NMIs or SMIs.

One approach would be to check for being in an NMI/SMI handler, and to avoid calling lock_acquire() and lock_release() in those cases.

Quoted 8 lines

It seems:

#define nmi_enter() do { lockdep_off(); __irq_enter(); } while (0)
#define nmi_exit() do { __irq_exit(); lockdep_on(); } while (0)

Should make it all work out just fine. (for NMIs at least, /me fully ignorant of the workings of SMIs)

Very good point, at least for NMIs on i386 and x86_64. Can't say that I know much about SMIs myself. Or about whatever equivalents to NMIs and SMIs might exist on other platforms. :-/ Of course, the other platforms could be handled by making the RCU lockdep operate only on i386 and x86_64 if required.

Corey, any advice on SMI handlers? Is there something like nmi_enter() and nmi_exit() that allows disabing lockdep?

Quoted 32 lines · Paul E. McKenney

Quoted 14 lines · Peter Zijlstra

Quoted 5 lines · Paul E. McKenney

Quoted 3 lines · Peter Zijlstra

There seem to be some unbalanced rcu_read_{,un}lock() issues of late, how about doing something like this:

Quoted 7 lines

This will break when rcu_read_lock() and rcu_read_unlock() are invoked from NMI/SMI handlers -- the raw_local_irq_save() in lock_acquire() will not mask NMIs or SMIs.

One approach would be to check for being in an NMI/SMI handler, and to avoid calling lock_acquire() and lock_release() in those cases.

Quoted 9 lines

It seems:

#define nmi_enter() do { lockdep_off(); __irq_enter(); } while
(0)
#define nmi_exit() do { __irq_exit(); lockdep_on(); } while (0)

Should make it all work out just fine. (for NMIs at least, /me fully ignorant of the workings of SMIs)

Very good point, at least for NMIs on i386 and x86_64. Can't say that I know much about SMIs myself. Or about whatever equivalents to NMIs and SMIs might exist on other platforms. :-/ Of course, the other platforms could be handled by making the RCU lockdep operate only on i386 and x86_64 if required.

Quoted 3 lines

Corey, any advice on SMI handlers? Is there something like nmi_enter() and nmi_exit() that allows disabing lockdep?

You will certainly need something like nmi_enter() and nmi_exit() for SMIs, since they can occur at any time like NMIs. As far as anything else, you just have to be extremely careful and remember that it can occur anyplace. But you already know that :).

It would be nice if the PowerPC board vendors would tie watchdog pretimeouts and some type of timer into the SMI input. It would make debugging certain problems much easier. And all those Marvell bridge chips have a watchdog pretimeout and I haven't seen any board vendor wire it up :(.

Quoted 13 lines · Peter Zijlstra

Quoted 11 lines · Paul E. McKenney

Quoted 3 lines · Peter Zijlstra

There seem to be some unbalanced rcu_read_{,un}lock() issues of late, how about doing something like this:

This will break when rcu_read_lock() and rcu_read_unlock() are invoked from NMI/SMI handlers -- the raw_local_irq_save() in lock_acquire() will not mask NMIs or SMIs.

One approach would be to check for being in an NMI/SMI handler, and to avoid calling lock_acquire() and lock_release() in those cases.

Quoted 7 lines

It seems:

#define nmi_enter() do { lockdep_off(); __irq_enter(); } while (0)
#define nmi_exit() do { __irq_exit(); lockdep_on(); } while (0)

Should make it all work out just fine. (for NMIs at least, /me fully ignorant of the workings of SMIs)

Very good point, at least for NMIs on i386 and x86_64. Can't say that I know much about SMIs myself. Or about whatever equivalents to NMIs and SMIs might exist on other platforms. :-/ Of course, the other platforms could be handled by making the RCU lockdep operate only on i386 and x86_64 if required.

Corey, any advice on SMI handlers? Is there something like nmi_enter() and nmi_exit() that allows disabing lockdep?

Quoted 3 lines

Quoted 2 lines

Another approach would be to use sparse, which has checks for rcu_read_lock()/rcu_read_unlock() nesting.

Quoted 1 line

Yeah, but one more method can never hurt, no? :-)

Excellent point!

I guess the next thing for me is to do a performance check. Looks like CONFIG_DEBUG_LOCK_ALLOC is not on by default, so not violently worried about performance, but would be good to know.