Re: linux kernel 2.6.18-20 bug: rcu_read_unlock in __sock_create

Re: linux kernel 2.6.18-20 bug: rcu_read_unlock in __sock_create

[Herbert Xu]

oleg 123 <123.oleg@gmail.com> wrote:
>
> 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.

Well spotted! Please cc netdev@vger.kernel.org for networking
issues.

[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 <herbert@gondor.apana.org.au>

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
diff --git a/net/socket.c b/net/socket.c
index ec07703..7d44453 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -1168,7 +1168,7 @@ static int __sock_create(int family, int type, int protocol,
 	module_put(pf->owner);
 	err = security_socket_post_create(sock, family, type, protocol, kern);
 	if (err)
-		goto out_release;
+		goto out_sock_release;
 	*res = sock;
 
 	return 0;

Re: linux kernel 2.6.18-20 bug: rcu_read_unlock in __sock_create

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Wed, 15 Aug 2007 16:33:35 +0800

> [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 <herbert@gondor.apana.org.au>

Patch applied, thanks Herbert.

[PATCH] lockdep: annotate rcu_read_{,un}lock()

[Peter Zijlstra]

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

Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
---
 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
===================================================================
--- linux-2.6.orig/include/linux/lockdep.h
+++ linux-2.6/include/linux/lockdep.h
@@ -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
===================================================================
--- linux-2.6.orig/include/linux/rcupdate.h
+++ linux-2.6/include/linux/rcupdate.h
@@ -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
===================================================================
--- linux-2.6.orig/kernel/rcupdate.c
+++ linux-2.6/kernel/rcupdate.c
@@ -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,

Re: [PATCH] lockdep: annotate rcu_read_{,un}lock()

[Paul E. McKenney]

On Thu, Aug 16, 2007 at 04:25:07PM +0200, Peter Zijlstra wrote:
> 
> 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.

							Thanx, Paul

> Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
> ---
>  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
> ===================================================================
> --- linux-2.6.orig/include/linux/lockdep.h
> +++ linux-2.6/include/linux/lockdep.h
> @@ -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
> ===================================================================
> --- linux-2.6.orig/include/linux/rcupdate.h
> +++ linux-2.6/include/linux/rcupdate.h
> @@ -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
> ===================================================================
> --- linux-2.6.orig/kernel/rcupdate.c
> +++ linux-2.6/kernel/rcupdate.c
> @@ -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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] lockdep: annotate rcu_read_{,un}lock()

[Peter Zijlstra]

On Thu, 2007-08-16 at 09:01 -0700, Paul E. McKenney wrote:
> On Thu, Aug 16, 2007 at 04:25:07PM +0200, Peter Zijlstra wrote:
> > 
> > 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)

> 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? :-)

Re: [PATCH] lockdep: annotate rcu_read_{,un}lock()

[Paul E. McKenney]

On Fri, Aug 17, 2007 at 09:56:45AM +0200, Peter Zijlstra wrote:
> On Thu, 2007-08-16 at 09:01 -0700, Paul E. McKenney wrote:
> > On Thu, Aug 16, 2007 at 04:25:07PM +0200, Peter Zijlstra wrote:
> > > 
> > > 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)

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?

> > 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? :-)

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.

							Thanx, Paul

Re: [PATCH] lockdep: annotate rcu_read_{,un}lock()

[Corey Minyard]

Paul E. McKenney wrote:
> On Fri, Aug 17, 2007 at 09:56:45AM +0200, Peter Zijlstra wrote:
>   
>> On Thu, 2007-08-16 at 09:01 -0700, Paul E. McKenney wrote:
>>     
>>> On Thu, Aug 16, 2007 at 04:25:07PM +0200, Peter Zijlstra wrote:
>>>       
>>>> 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)
>>     
>
> 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?
>   
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 :(.

-corey

Re: [PATCH] lockdep: annotate rcu_read_{,un}lock()

[Paul E. McKenney]

On Fri, Aug 17, 2007 at 01:48:09PM -0500, Corey Minyard wrote:
> Paul E. McKenney wrote:
> >On Fri, Aug 17, 2007 at 09:56:45AM +0200, Peter Zijlstra wrote:
> >  
> >>On Thu, 2007-08-16 at 09:01 -0700, Paul E. McKenney wrote:
> >>    
> >>>On Thu, Aug 16, 2007 at 04:25:07PM +0200, Peter Zijlstra wrote:
> >>>      
> >>>>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)
> >>    
> >
> >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?
> >  
> 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 :).

So we would need to create an smi_enter() and smi_exit() an place them
appropriately.  Any preferences?

> 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 :(.

Can't say that I have much influence over them, but I must agree
that debuggability is a very good thing!

						Thanx, Paul

Re: [PATCH] lockdep: annotate rcu_read_{,un}lock()

[Paul E. McKenney]

On Fri, Aug 17, 2007 at 08:53:57AM -0700, Paul E. McKenney wrote:
> On Fri, Aug 17, 2007 at 09:56:45AM +0200, Peter Zijlstra wrote:
> > On Thu, 2007-08-16 at 09:01 -0700, Paul E. McKenney wrote:
> > > On Thu, Aug 16, 2007 at 04:25:07PM +0200, Peter Zijlstra wrote:
> > > > 
> > > > 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)
> 
> 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?
> 
> > > 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? :-)
> 
> 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.

4-CPU 1.8GHz Opteron 844 running 2.6.22 CONFIG_PREEMPT_NONE with Peter's
patch running rcu_read_lock()/rcu_read_unlock() in a tight loop:

1 CPU:  91.7ns
2 CPU: 335.2ns
3 CPU: 534.3ns  (But bimodal: two CPUs at 640.7ns, one at 321.6ns)
4 CPU: 784.0ns  (again bimodal: three CPUs at 895.9ns, one at 448.2ns)

Running without Peter's patch measures the loop overhead of about 1.2ns.

So not crippling, but definitely a debug-only option that is not enabled
by default in production.

In summary, seems like an eminently reasonable approach to me, assuming
that the NMI/SMI issues can be resolved across the platforms that have
them.  Good stuff, Peter!!!

						Thanx, Paul