mmotm 2010-04-28 - RCU whinges

mmotm 2010-04-28 - RCU whinges

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 1844 bytes --]

On Wed, 28 Apr 2010 16:53:32 PDT, akpm@linux-foundation.org said:
> The mm-of-the-moment snapshot 2010-04-28-16-53 has been uploaded to
> 
>    http://userweb.kernel.org/~akpm/mmotm/

I thought we swatted all these, hit another one...

[    9.131490] ctnetlink v0.93: registering with nfnetlink.
[    9.131535]
[    9.131535] ===================================================
[    9.131704] [ INFO: suspicious rcu_dereference_check() usage. ]
[    9.131794] ---------------------------------------------------
[    9.131883] net/netfilter/nf_conntrack_ecache.c:88 invoked rcu_dereference_check() without protection!
[    9.131977]
[    9.131977] other info that might help us debug this:
[    9.131978]
[    9.132218]
[    9.132219] rcu_scheduler_active = 1, debug_locks = 0
[    9.132434] 1 lock held by swapper/1:
[    9.132519]  #0:  (nf_ct_ecache_mutex){+.+...}, at: [<ffffffff8148922d>] nf_conntrack_register_notifier+0x1a/0x75
[    9.132938]
[    9.132939] stack backtrace:
[    9.133129] Pid: 1, comm: swapper Tainted: G        W   2.6.34-rc5-mmotm0428 #1
[    9.133220] Call Trace:
[    9.133319]  [<ffffffff81064832>] lockdep_rcu_dereference+0xaa/0xb2
[    9.133410]  [<ffffffff81489250>] nf_conntrack_register_notifier+0x3d/0x75
[    9.133521]  [<ffffffff81b5a157>] ctnetlink_init+0x71/0xd5
[    9.133627]  [<ffffffff81b5a0e6>] ? ctnetlink_init+0x0/0xd5
[    9.133735]  [<ffffffff810001ef>] do_one_initcall+0x59/0x14e
[    9.133843]  [<ffffffff81b2e68a>] kernel_init+0x144/0x1ce
[    9.133949]  [<ffffffff81003414>] kernel_thread_helper+0x4/0x10
[    9.134060]  [<ffffffff81598a40>] ? restore_args+0x0/0x30
[    9.134196]  [<ffffffff81b2e546>] ? kernel_init+0x0/0x1ce
[    9.134328]  [<ffffffff81003410>] ? kernel_thread_helper+0x0/0x10
[    9.134530] ip_tables: (C) 2000-2006 Netfilter Core Team
[    9.134655] TCP bic registered


[-- Attachment #2: Type: application/pgp-signature, Size: 227 bytes --]

Re: mmotm 2010-04-28 - RCU whinges

[Eric Dumazet]

Le dimanche 02 mai 2010 à 13:46 -0400, Valdis.Kletnieks@vt.edu a écrit :
> On Wed, 28 Apr 2010 16:53:32 PDT, akpm@linux-foundation.org said:
> > The mm-of-the-moment snapshot 2010-04-28-16-53 has been uploaded to
> > 
> >    http://userweb.kernel.org/~akpm/mmotm/
> 
> I thought we swatted all these, hit another one...
> 
> [    9.131490] ctnetlink v0.93: registering with nfnetlink.
> [    9.131535]
> [    9.131535] ===================================================
> [    9.131704] [ INFO: suspicious rcu_dereference_check() usage. ]
> [    9.131794] ---------------------------------------------------
> [    9.131883] net/netfilter/nf_conntrack_ecache.c:88 invoked rcu_dereference_check() without protection!
> [    9.131977]
> [    9.131977] other info that might help us debug this:
> [    9.131978]
> [    9.132218]
> [    9.132219] rcu_scheduler_active = 1, debug_locks = 0
> [    9.132434] 1 lock held by swapper/1:
> [    9.132519]  #0:  (nf_ct_ecache_mutex){+.+...}, at: [<ffffffff8148922d>] nf_conntrack_register_notifier+0x1a/0x75
> [    9.132938]
> [    9.132939] stack backtrace:
> [    9.133129] Pid: 1, comm: swapper Tainted: G        W   2.6.34-rc5-mmotm0428 #1
> [    9.133220] Call Trace:
> [    9.133319]  [<ffffffff81064832>] lockdep_rcu_dereference+0xaa/0xb2
> [    9.133410]  [<ffffffff81489250>] nf_conntrack_register_notifier+0x3d/0x75
> [    9.133521]  [<ffffffff81b5a157>] ctnetlink_init+0x71/0xd5
> [    9.133627]  [<ffffffff81b5a0e6>] ? ctnetlink_init+0x0/0xd5
> [    9.133735]  [<ffffffff810001ef>] do_one_initcall+0x59/0x14e
> [    9.133843]  [<ffffffff81b2e68a>] kernel_init+0x144/0x1ce
> [    9.133949]  [<ffffffff81003414>] kernel_thread_helper+0x4/0x10
> [    9.134060]  [<ffffffff81598a40>] ? restore_args+0x0/0x30
> [    9.134196]  [<ffffffff81b2e546>] ? kernel_init+0x0/0x1ce
> [    9.134328]  [<ffffffff81003410>] ? kernel_thread_helper+0x0/0x10
> [    9.134530] ip_tables: (C) 2000-2006 Netfilter Core Team
> [    9.134655] TCP bic registered
> 

Thanks for the report !

We can use rcu_dereference_protected() in those cases.

[PATCH] net: Use rcu_dereference_protected in nf_conntrack_ecache

Writers own nf_ct_ecache_mutex.

Reported-by: Valdis Kletnieks <Valdis.Kletnieks@vt.edu>
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
 net/netfilter/nf_conntrack_ecache.c |   12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/net/netfilter/nf_conntrack_ecache.c b/net/netfilter/nf_conntrack_ecache.c
index f516961..cdcc764 100644
--- a/net/netfilter/nf_conntrack_ecache.c
+++ b/net/netfilter/nf_conntrack_ecache.c
@@ -85,7 +85,8 @@ int nf_conntrack_register_notifier(struct nf_ct_event_notifier *new)
 	struct nf_ct_event_notifier *notify;
 
 	mutex_lock(&nf_ct_ecache_mutex);
-	notify = rcu_dereference(nf_conntrack_event_cb);
+	notify = rcu_dereference_protected(nf_conntrack_event_cb,
+					   lockdep_is_held(&nf_ct_ecache_mutex));
 	if (notify != NULL) {
 		ret = -EBUSY;
 		goto out_unlock;
@@ -105,7 +106,8 @@ void nf_conntrack_unregister_notifier(struct nf_ct_event_notifier *new)
 	struct nf_ct_event_notifier *notify;
 
 	mutex_lock(&nf_ct_ecache_mutex);
-	notify = rcu_dereference(nf_conntrack_event_cb);
+	notify = rcu_dereference_protected(nf_conntrack_event_cb,
+					   lockdep_is_held(&nf_ct_ecache_mutex));
 	BUG_ON(notify != new);
 	rcu_assign_pointer(nf_conntrack_event_cb, NULL);
 	mutex_unlock(&nf_ct_ecache_mutex);
@@ -118,7 +120,8 @@ int nf_ct_expect_register_notifier(struct nf_exp_event_notifier *new)
 	struct nf_exp_event_notifier *notify;
 
 	mutex_lock(&nf_ct_ecache_mutex);
-	notify = rcu_dereference(nf_expect_event_cb);
+	notify = rcu_dereference_protected(nf_expect_event_cb,
+					   lockdep_is_held(&nf_ct_ecache_mutex));
 	if (notify != NULL) {
 		ret = -EBUSY;
 		goto out_unlock;
@@ -138,7 +141,8 @@ void nf_ct_expect_unregister_notifier(struct nf_exp_event_notifier *new)
 	struct nf_exp_event_notifier *notify;
 
 	mutex_lock(&nf_ct_ecache_mutex);
-	notify = rcu_dereference(nf_expect_event_cb);
+	notify = rcu_dereference_protected(nf_expect_event_cb,
+					   lockdep_is_held(&nf_ct_ecache_mutex));
 	BUG_ON(notify != new);
 	rcu_assign_pointer(nf_expect_event_cb, NULL);
 	mutex_unlock(&nf_ct_ecache_mutex);

Re: mmotm 2010-04-28 - RCU whinges

[Eric Dumazet]

Le lundi 03 mai 2010 à 07:38 +0200, Eric Dumazet a écrit :
> Le dimanche 02 mai 2010 à 13:46 -0400, Valdis.Kletnieks@vt.edu a écrit :
> > On Wed, 28 Apr 2010 16:53:32 PDT, akpm@linux-foundation.org said:
> > > The mm-of-the-moment snapshot 2010-04-28-16-53 has been uploaded to
> > > 
> > >    http://userweb.kernel.org/~akpm/mmotm/
> > 
> > I thought we swatted all these, hit another one...
> > 
> > [    9.131490] ctnetlink v0.93: registering with nfnetlink.
> > [    9.131535]
> > [    9.131535] ===================================================
> > [    9.131704] [ INFO: suspicious rcu_dereference_check() usage. ]
> > [    9.131794] ---------------------------------------------------
> > [    9.131883] net/netfilter/nf_conntrack_ecache.c:88 invoked rcu_dereference_check() without protection!
> > [    9.131977]
> > [    9.131977] other info that might help us debug this:
> > [    9.131978]
> > [    9.132218]
> > [    9.132219] rcu_scheduler_active = 1, debug_locks = 0
> > [    9.132434] 1 lock held by swapper/1:
> > [    9.132519]  #0:  (nf_ct_ecache_mutex){+.+...}, at: [<ffffffff8148922d>] nf_conntrack_register_notifier+0x1a/0x75
> > [    9.132938]
> > [    9.132939] stack backtrace:
> > [    9.133129] Pid: 1, comm: swapper Tainted: G        W   2.6.34-rc5-mmotm0428 #1
> > [    9.133220] Call Trace:
> > [    9.133319]  [<ffffffff81064832>] lockdep_rcu_dereference+0xaa/0xb2
> > [    9.133410]  [<ffffffff81489250>] nf_conntrack_register_notifier+0x3d/0x75
> > [    9.133521]  [<ffffffff81b5a157>] ctnetlink_init+0x71/0xd5
> > [    9.133627]  [<ffffffff81b5a0e6>] ? ctnetlink_init+0x0/0xd5
> > [    9.133735]  [<ffffffff810001ef>] do_one_initcall+0x59/0x14e
> > [    9.133843]  [<ffffffff81b2e68a>] kernel_init+0x144/0x1ce
> > [    9.133949]  [<ffffffff81003414>] kernel_thread_helper+0x4/0x10
> > [    9.134060]  [<ffffffff81598a40>] ? restore_args+0x0/0x30
> > [    9.134196]  [<ffffffff81b2e546>] ? kernel_init+0x0/0x1ce
> > [    9.134328]  [<ffffffff81003410>] ? kernel_thread_helper+0x0/0x10
> > [    9.134530] ip_tables: (C) 2000-2006 Netfilter Core Team
> > [    9.134655] TCP bic registered
> > 
> 
> Thanks for the report !
> 
> We can use rcu_dereference_protected() in those cases.
> 
> [PATCH] net: Use rcu_dereference_protected in nf_conntrack_ecache
> 
> Writers own nf_ct_ecache_mutex.
> 
> Reported-by: Valdis Kletnieks <Valdis.Kletnieks@vt.edu>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> ---


Oops scratch that, I'll resend a correct version.



--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: mmotm 2010-04-28 - RCU whinges

[Eric Dumazet]

Le lundi 03 mai 2010 à 07:41 +0200, Eric Dumazet a écrit :

> Oops scratch that, I'll resend a correct version.
> 
> 

Sorry, patch _is_ fine, I had one brain collapse when re-reading it, I
thought a different mutex was in use in one of the functions.


--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: mmotm 2010-04-28 - RCU whinges

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 03 May 2010 07:43:56 +0200

> Le lundi 03 mai 2010 à 07:41 +0200, Eric Dumazet a écrit :
> 
>> Oops scratch that, I'll resend a correct version.
>> 
>> 
> 
> Sorry, patch _is_ fine, I had one brain collapse when re-reading it, I
> thought a different mutex was in use in one of the functions.

Ok, Patrick please review, thanks.

Re: mmotm 2010-04-28 - RCU whinges

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 897 bytes --]

David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Mon, 03 May 2010 07:43:56 +0200
> 
>> Le lundi 03 mai 2010 à 07:41 +0200, Eric Dumazet a écrit :
>>
>>> Oops scratch that, I'll resend a correct version.
>>>
>>>
>> Sorry, patch _is_ fine, I had one brain collapse when re-reading it, I
>> thought a different mutex was in use in one of the functions.
> 
> Ok, Patrick please review, thanks.

Actually we don't need the rcu_dereference() calls at all since
registration and unregistration are protected by the mutexes.

I queued this patch in nf-next, the only reason why I haven't
submitted it yet is that I was unable to get git to cleanly export
only the proper set of patches meant for -next due to a few merges,
it insists on including 5 patches already merged upstream. If you
don't mind ignoring the first 5 patches in the series, I'll send a
pull request tonight.



[-- Attachment #2: x --]
[-- Type: text/plain, Size: 3840 bytes --]

commit ed86308f6179d8fa6151c2d0f652aad0091548e2
Author: Patrick McHardy <kaber@trash.net>
Date:   Fri Apr 9 16:42:15 2010 +0200

    netfilter: remove invalid rcu_dereference() calls
    
    The CONFIG_PROVE_RCU option discovered a few invalid uses of
    rcu_dereference() in netfilter. In all these cases, the code code
    intends to check whether a pointer is already assigned when
    performing registration or whether the assigned pointer matches
    when performing unregistration. The entire registration/
    unregistration is protected by a mutex, so we don't need the
    rcu_dereference() calls.
    
    Reported-by: Valdis Kletnieks <Valdis.Kletnieks@vt.edu>
    Tested-by: Valdis Kletnieks <Valdis.Kletnieks@vt.edu>
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/net/netfilter/nf_conntrack_ecache.c b/net/netfilter/nf_conntrack_ecache.c
index d5a9bcd..849614a 100644
--- a/net/netfilter/nf_conntrack_ecache.c
+++ b/net/netfilter/nf_conntrack_ecache.c
@@ -81,11 +81,9 @@ EXPORT_SYMBOL_GPL(nf_ct_deliver_cached_events);
 int nf_conntrack_register_notifier(struct nf_ct_event_notifier *new)
 {
 	int ret = 0;
-	struct nf_ct_event_notifier *notify;
 
 	mutex_lock(&nf_ct_ecache_mutex);
-	notify = rcu_dereference(nf_conntrack_event_cb);
-	if (notify != NULL) {
+	if (nf_conntrack_event_cb != NULL) {
 		ret = -EBUSY;
 		goto out_unlock;
 	}
@@ -101,11 +99,8 @@ EXPORT_SYMBOL_GPL(nf_conntrack_register_notifier);
 
 void nf_conntrack_unregister_notifier(struct nf_ct_event_notifier *new)
 {
-	struct nf_ct_event_notifier *notify;
-
 	mutex_lock(&nf_ct_ecache_mutex);
-	notify = rcu_dereference(nf_conntrack_event_cb);
-	BUG_ON(notify != new);
+	BUG_ON(nf_conntrack_event_cb != new);
 	rcu_assign_pointer(nf_conntrack_event_cb, NULL);
 	mutex_unlock(&nf_ct_ecache_mutex);
 }
@@ -114,11 +109,9 @@ EXPORT_SYMBOL_GPL(nf_conntrack_unregister_notifier);
 int nf_ct_expect_register_notifier(struct nf_exp_event_notifier *new)
 {
 	int ret = 0;
-	struct nf_exp_event_notifier *notify;
 
 	mutex_lock(&nf_ct_ecache_mutex);
-	notify = rcu_dereference(nf_expect_event_cb);
-	if (notify != NULL) {
+	if (nf_expect_event_cb != NULL) {
 		ret = -EBUSY;
 		goto out_unlock;
 	}
@@ -134,11 +127,8 @@ EXPORT_SYMBOL_GPL(nf_ct_expect_register_notifier);
 
 void nf_ct_expect_unregister_notifier(struct nf_exp_event_notifier *new)
 {
-	struct nf_exp_event_notifier *notify;
-
 	mutex_lock(&nf_ct_ecache_mutex);
-	notify = rcu_dereference(nf_expect_event_cb);
-	BUG_ON(notify != new);
+	BUG_ON(nf_expect_event_cb != new);
 	rcu_assign_pointer(nf_expect_event_cb, NULL);
 	mutex_unlock(&nf_ct_ecache_mutex);
 }
diff --git a/net/netfilter/nf_log.c b/net/netfilter/nf_log.c
index 015725a..908f599 100644
--- a/net/netfilter/nf_log.c
+++ b/net/netfilter/nf_log.c
@@ -35,7 +35,6 @@ static struct nf_logger *__find_logger(int pf, const char *str_logger)
 /* return EEXIST if the same logger is registred, 0 on success. */
 int nf_log_register(u_int8_t pf, struct nf_logger *logger)
 {
-	const struct nf_logger *llog;
 	int i;
 
 	if (pf >= ARRAY_SIZE(nf_loggers))
@@ -52,8 +51,7 @@ int nf_log_register(u_int8_t pf, struct nf_logger *logger)
 	} else {
 		/* register at end of list to honor first register win */
 		list_add_tail(&logger->list[pf], &nf_loggers_l[pf]);
-		llog = rcu_dereference(nf_loggers[pf]);
-		if (llog == NULL)
+		if (nf_loggers[pf] == NULL)
 			rcu_assign_pointer(nf_loggers[pf], logger);
 	}
 
@@ -65,13 +63,11 @@ EXPORT_SYMBOL(nf_log_register);
 
 void nf_log_unregister(struct nf_logger *logger)
 {
-	const struct nf_logger *c_logger;
 	int i;
 
 	mutex_lock(&nf_log_mutex);
 	for (i = 0; i < ARRAY_SIZE(nf_loggers); i++) {
-		c_logger = rcu_dereference(nf_loggers[i]);
-		if (c_logger == logger)
+		if (nf_loggers[i] == logger)
 			rcu_assign_pointer(nf_loggers[i], NULL);
 		list_del(&logger->list[i]);
 	}

Re: mmotm 2010-04-28 - RCU whinges

[Eric Dumazet]

Le lundi 10 mai 2010 à 17:40 +0200, Patrick McHardy a écrit :
> David Miller wrote:
> > From: Eric Dumazet <eric.dumazet@gmail.com>
> > Date: Mon, 03 May 2010 07:43:56 +0200
> > 
> >> Le lundi 03 mai 2010 à 07:41 +0200, Eric Dumazet a écrit :
> >>
> >>> Oops scratch that, I'll resend a correct version.
> >>>
> >>>
> >> Sorry, patch _is_ fine, I had one brain collapse when re-reading it, I
> >> thought a different mutex was in use in one of the functions.
> > 
> > Ok, Patrick please review, thanks.
> 
> Actually we don't need the rcu_dereference() calls at all since
> registration and unregistration are protected by the mutexes.
> 
> I queued this patch in nf-next, the only reason why I haven't
> submitted it yet is that I was unable to get git to cleanly export
> only the proper set of patches meant for -next due to a few merges,
> it insists on including 5 patches already merged upstream. If you
> don't mind ignoring the first 5 patches in the series, I'll send a
> pull request tonight.
> 


This will clash with upcoming RCU patches, where rcu protected pointer
cannot be directly accessed without lockdep splats.

We will need one day or another a rcu_...(nf_conntrack_event_cb)

> 
> pièce jointe document texte brut (x)
> commit ed86308f6179d8fa6151c2d0f652aad0091548e2
> Author: Patrick McHardy <kaber@trash.net>
> Date:   Fri Apr 9 16:42:15 2010 +0200
> 
>     netfilter: remove invalid rcu_dereference() calls
>     
>     The CONFIG_PROVE_RCU option discovered a few invalid uses of
>     rcu_dereference() in netfilter. In all these cases, the code code
>     intends to check whether a pointer is already assigned when
>     performing registration or whether the assigned pointer matches
>     when performing unregistration. The entire registration/
>     unregistration is protected by a mutex, so we don't need the
>     rcu_dereference() calls.
>     
>     Reported-by: Valdis Kletnieks <Valdis.Kletnieks@vt.edu>
>     Tested-by: Valdis Kletnieks <Valdis.Kletnieks@vt.edu>
>     Signed-off-by: Patrick McHardy <kaber@trash.net>
> 
> diff --git a/net/netfilter/nf_conntrack_ecache.c b/net/netfilter/nf_conntrack_ecache.c
> index d5a9bcd..849614a 100644
> --- a/net/netfilter/nf_conntrack_ecache.c
> +++ b/net/netfilter/nf_conntrack_ecache.c
> @@ -81,11 +81,9 @@ EXPORT_SYMBOL_GPL(nf_ct_deliver_cached_events);
>  int nf_conntrack_register_notifier(struct nf_ct_event_notifier *new)
>  {
>  	int ret = 0;
> -	struct nf_ct_event_notifier *notify;
>  
>  	mutex_lock(&nf_ct_ecache_mutex);
> -	notify = rcu_dereference(nf_conntrack_event_cb);
> -	if (notify != NULL) {
> +	if (nf_conntrack_event_cb != NULL) {
>  		ret = -EBUSY;
>  		goto out_unlock;
>  	}
> @@ -101,11 +99,8 @@ EXPORT_SYMBOL_GPL(nf_conntrack_register_notifier);
>  
>  void nf_conntrack_unregister_notifier(struct nf_ct_event_notifier *new)
>  {
> -	struct nf_ct_event_notifier *notify;
> -
>  	mutex_lock(&nf_ct_ecache_mutex);
> -	notify = rcu_dereference(nf_conntrack_event_cb);
> -	BUG_ON(notify != new);
> +	BUG_ON(nf_conntrack_event_cb != new);
>  	rcu_assign_pointer(nf_conntrack_event_cb, NULL);
>  	mutex_unlock(&nf_ct_ecache_mutex);
>  }
> @@ -114,11 +109,9 @@ EXPORT_SYMBOL_GPL(nf_conntrack_unregister_notifier);
>  int nf_ct_expect_register_notifier(struct nf_exp_event_notifier *new)
>  {
>  	int ret = 0;
> -	struct nf_exp_event_notifier *notify;
>  
>  	mutex_lock(&nf_ct_ecache_mutex);
> -	notify = rcu_dereference(nf_expect_event_cb);
> -	if (notify != NULL) {
> +	if (nf_expect_event_cb != NULL) {
>  		ret = -EBUSY;
>  		goto out_unlock;
>  	}
> @@ -134,11 +127,8 @@ EXPORT_SYMBOL_GPL(nf_ct_expect_register_notifier);
>  
>  void nf_ct_expect_unregister_notifier(struct nf_exp_event_notifier *new)
>  {
> -	struct nf_exp_event_notifier *notify;
> -
>  	mutex_lock(&nf_ct_ecache_mutex);
> -	notify = rcu_dereference(nf_expect_event_cb);
> -	BUG_ON(notify != new);
> +	BUG_ON(nf_expect_event_cb != new);
>  	rcu_assign_pointer(nf_expect_event_cb, NULL);
>  	mutex_unlock(&nf_ct_ecache_mutex);
>  }
> diff --git a/net/netfilter/nf_log.c b/net/netfilter/nf_log.c
> index 015725a..908f599 100644
> --- a/net/netfilter/nf_log.c
> +++ b/net/netfilter/nf_log.c
> @@ -35,7 +35,6 @@ static struct nf_logger *__find_logger(int pf, const char *str_logger)
>  /* return EEXIST if the same logger is registred, 0 on success. */
>  int nf_log_register(u_int8_t pf, struct nf_logger *logger)
>  {
> -	const struct nf_logger *llog;
>  	int i;
>  
>  	if (pf >= ARRAY_SIZE(nf_loggers))
> @@ -52,8 +51,7 @@ int nf_log_register(u_int8_t pf, struct nf_logger *logger)
>  	} else {
>  		/* register at end of list to honor first register win */
>  		list_add_tail(&logger->list[pf], &nf_loggers_l[pf]);
> -		llog = rcu_dereference(nf_loggers[pf]);
> -		if (llog == NULL)
> +		if (nf_loggers[pf] == NULL)
>  			rcu_assign_pointer(nf_loggers[pf], logger);
>  	}
>  
> @@ -65,13 +63,11 @@ EXPORT_SYMBOL(nf_log_register);
>  
>  void nf_log_unregister(struct nf_logger *logger)
>  {
> -	const struct nf_logger *c_logger;
>  	int i;
>  
>  	mutex_lock(&nf_log_mutex);
>  	for (i = 0; i < ARRAY_SIZE(nf_loggers); i++) {
> -		c_logger = rcu_dereference(nf_loggers[i]);
> -		if (c_logger == logger)
> +		if (nf_loggers[i] == logger)
>  			rcu_assign_pointer(nf_loggers[i], NULL);
>  		list_del(&logger->list[i]);
>  	}

Re: mmotm 2010-04-28 - RCU whinges

[Eric Dumazet]

Le lundi 10 mai 2010 à 17:56 +0200, Eric Dumazet a écrit :
> Le lundi 10 mai 2010 à 17:40 +0200, Patrick McHardy a écrit :
> > David Miller wrote:
> > > From: Eric Dumazet <eric.dumazet@gmail.com>
> > > Date: Mon, 03 May 2010 07:43:56 +0200
> > > 
> > >> Le lundi 03 mai 2010 à 07:41 +0200, Eric Dumazet a écrit :
> > >>
> > >>> Oops scratch that, I'll resend a correct version.
> > >>>
> > >>>
> > >> Sorry, patch _is_ fine, I had one brain collapse when re-reading it, I
> > >> thought a different mutex was in use in one of the functions.
> > > 
> > > Ok, Patrick please review, thanks.
> > 
> > Actually we don't need the rcu_dereference() calls at all since
> > registration and unregistration are protected by the mutexes.
> > 
> > I queued this patch in nf-next, the only reason why I haven't
> > submitted it yet is that I was unable to get git to cleanly export
> > only the proper set of patches meant for -next due to a few merges,
> > it insists on including 5 patches already merged upstream. If you
> > don't mind ignoring the first 5 patches in the series, I'll send a
> > pull request tonight.
> > 
> 
> 
> This will clash with upcoming RCU patches, where rcu protected pointer
> cannot be directly accessed without lockdep splats.
> 

Sorry, I meant sparse here, not lockdep.

> We will need one day or another a rcu_...(nf_conntrack_event_cb)

Re: mmotm 2010-04-28 - RCU whinges

[Patrick McHardy]

Eric Dumazet wrote:
> Le lundi 10 mai 2010 à 17:40 +0200, Patrick McHardy a écrit :
>> David Miller wrote:
>>> From: Eric Dumazet <eric.dumazet@gmail.com>
>>> Date: Mon, 03 May 2010 07:43:56 +0200
>>>
>>>> Le lundi 03 mai 2010 à 07:41 +0200, Eric Dumazet a écrit :
>>>>
>>>>> Oops scratch that, I'll resend a correct version.
>>>>>
>>>>>
>>>> Sorry, patch _is_ fine, I had one brain collapse when re-reading it, I
>>>> thought a different mutex was in use in one of the functions.
>>> Ok, Patrick please review, thanks.
>> Actually we don't need the rcu_dereference() calls at all since
>> registration and unregistration are protected by the mutexes.
>>
>> I queued this patch in nf-next, the only reason why I haven't
>> submitted it yet is that I was unable to get git to cleanly export
>> only the proper set of patches meant for -next due to a few merges,
>> it insists on including 5 patches already merged upstream. If you
>> don't mind ignoring the first 5 patches in the series, I'll send a
>> pull request tonight.
>>
> 
> This will clash with upcoming RCU patches, where rcu protected pointer
> cannot be directly accessed without lockdep splats.
> 
> We will need one day or another a rcu_...(nf_conntrack_event_cb)

Thanks for the information, I didn't realize that when looking at
those patches. So I guess the correct fix once those patches are
merged would be to use rcu_assign_protected() and rcu_access_pointer().
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: mmotm 2010-04-28 - RCU whinges

[Patrick McHardy]

Patrick McHardy wrote:
> Eric Dumazet wrote:
>> Le lundi 10 mai 2010 à 17:40 +0200, Patrick McHardy a écrit :
>>> David Miller wrote:
>>>> From: Eric Dumazet <eric.dumazet@gmail.com>
>>>> Date: Mon, 03 May 2010 07:43:56 +0200
>>>>
>>>>> Le lundi 03 mai 2010 à 07:41 +0200, Eric Dumazet a écrit :
>>>>>
>>>>>> Oops scratch that, I'll resend a correct version.
>>>>>>
>>>>>>
>>>>> Sorry, patch _is_ fine, I had one brain collapse when re-reading it, I
>>>>> thought a different mutex was in use in one of the functions.
>>>> Ok, Patrick please review, thanks.
>>> Actually we don't need the rcu_dereference() calls at all since
>>> registration and unregistration are protected by the mutexes.
>>>
>>> I queued this patch in nf-next, the only reason why I haven't
>>> submitted it yet is that I was unable to get git to cleanly export
>>> only the proper set of patches meant for -next due to a few merges,
>>> it insists on including 5 patches already merged upstream. If you
>>> don't mind ignoring the first 5 patches in the series, I'll send a
>>> pull request tonight.
>>>
>> This will clash with upcoming RCU patches, where rcu protected pointer
>> cannot be directly accessed without lockdep splats.
>>
>> We will need one day or another a rcu_...(nf_conntrack_event_cb)
> 
> Thanks for the information, I didn't realize that when looking at
> those patches. So I guess the correct fix once those patches are
> merged would be to use rcu_assign_protected() and rcu_access_pointer().

Ah, and that's what you did. Sorry for the confusion :)
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: mmotm 2010-04-28 - RCU whinges

[Paul E. McKenney]

On Mon, May 10, 2010 at 05:40:58PM +0200, Patrick McHardy wrote:
> David Miller wrote:
> > From: Eric Dumazet <eric.dumazet@gmail.com>
> > Date: Mon, 03 May 2010 07:43:56 +0200
> > 
> >> Le lundi 03 mai 2010 à 07:41 +0200, Eric Dumazet a écrit :
> >>
> >>> Oops scratch that, I'll resend a correct version.
> >>>
> >>>
> >> Sorry, patch _is_ fine, I had one brain collapse when re-reading it, I
> >> thought a different mutex was in use in one of the functions.
> > 
> > Ok, Patrick please review, thanks.
> 
> Actually we don't need the rcu_dereference() calls at all since
> registration and unregistration are protected by the mutexes.

The best approach in that case is rcu_dereference_protected() listing
the lock that must be held.  Of course, your code, so your choice.

						Thanx, Paul

> I queued this patch in nf-next, the only reason why I haven't
> submitted it yet is that I was unable to get git to cleanly export
> only the proper set of patches meant for -next due to a few merges,
> it insists on including 5 patches already merged upstream. If you
> don't mind ignoring the first 5 patches in the series, I'll send a
> pull request tonight.
> 
> 

> commit ed86308f6179d8fa6151c2d0f652aad0091548e2
> Author: Patrick McHardy <kaber@trash.net>
> Date:   Fri Apr 9 16:42:15 2010 +0200
> 
>     netfilter: remove invalid rcu_dereference() calls
>     
>     The CONFIG_PROVE_RCU option discovered a few invalid uses of
>     rcu_dereference() in netfilter. In all these cases, the code code
>     intends to check whether a pointer is already assigned when
>     performing registration or whether the assigned pointer matches
>     when performing unregistration. The entire registration/
>     unregistration is protected by a mutex, so we don't need the
>     rcu_dereference() calls.
>     
>     Reported-by: Valdis Kletnieks <Valdis.Kletnieks@vt.edu>
>     Tested-by: Valdis Kletnieks <Valdis.Kletnieks@vt.edu>
>     Signed-off-by: Patrick McHardy <kaber@trash.net>
> 
> diff --git a/net/netfilter/nf_conntrack_ecache.c b/net/netfilter/nf_conntrack_ecache.c
> index d5a9bcd..849614a 100644
> --- a/net/netfilter/nf_conntrack_ecache.c
> +++ b/net/netfilter/nf_conntrack_ecache.c
> @@ -81,11 +81,9 @@ EXPORT_SYMBOL_GPL(nf_ct_deliver_cached_events);
>  int nf_conntrack_register_notifier(struct nf_ct_event_notifier *new)
>  {
>  	int ret = 0;
> -	struct nf_ct_event_notifier *notify;
> 
>  	mutex_lock(&nf_ct_ecache_mutex);
> -	notify = rcu_dereference(nf_conntrack_event_cb);
> -	if (notify != NULL) {
> +	if (nf_conntrack_event_cb != NULL) {
>  		ret = -EBUSY;
>  		goto out_unlock;
>  	}
> @@ -101,11 +99,8 @@ EXPORT_SYMBOL_GPL(nf_conntrack_register_notifier);
> 
>  void nf_conntrack_unregister_notifier(struct nf_ct_event_notifier *new)
>  {
> -	struct nf_ct_event_notifier *notify;
> -
>  	mutex_lock(&nf_ct_ecache_mutex);
> -	notify = rcu_dereference(nf_conntrack_event_cb);
> -	BUG_ON(notify != new);
> +	BUG_ON(nf_conntrack_event_cb != new);
>  	rcu_assign_pointer(nf_conntrack_event_cb, NULL);
>  	mutex_unlock(&nf_ct_ecache_mutex);
>  }
> @@ -114,11 +109,9 @@ EXPORT_SYMBOL_GPL(nf_conntrack_unregister_notifier);
>  int nf_ct_expect_register_notifier(struct nf_exp_event_notifier *new)
>  {
>  	int ret = 0;
> -	struct nf_exp_event_notifier *notify;
> 
>  	mutex_lock(&nf_ct_ecache_mutex);
> -	notify = rcu_dereference(nf_expect_event_cb);
> -	if (notify != NULL) {
> +	if (nf_expect_event_cb != NULL) {
>  		ret = -EBUSY;
>  		goto out_unlock;
>  	}
> @@ -134,11 +127,8 @@ EXPORT_SYMBOL_GPL(nf_ct_expect_register_notifier);
> 
>  void nf_ct_expect_unregister_notifier(struct nf_exp_event_notifier *new)
>  {
> -	struct nf_exp_event_notifier *notify;
> -
>  	mutex_lock(&nf_ct_ecache_mutex);
> -	notify = rcu_dereference(nf_expect_event_cb);
> -	BUG_ON(notify != new);
> +	BUG_ON(nf_expect_event_cb != new);
>  	rcu_assign_pointer(nf_expect_event_cb, NULL);
>  	mutex_unlock(&nf_ct_ecache_mutex);
>  }
> diff --git a/net/netfilter/nf_log.c b/net/netfilter/nf_log.c
> index 015725a..908f599 100644
> --- a/net/netfilter/nf_log.c
> +++ b/net/netfilter/nf_log.c
> @@ -35,7 +35,6 @@ static struct nf_logger *__find_logger(int pf, const char *str_logger)
>  /* return EEXIST if the same logger is registred, 0 on success. */
>  int nf_log_register(u_int8_t pf, struct nf_logger *logger)
>  {
> -	const struct nf_logger *llog;
>  	int i;
> 
>  	if (pf >= ARRAY_SIZE(nf_loggers))
> @@ -52,8 +51,7 @@ int nf_log_register(u_int8_t pf, struct nf_logger *logger)
>  	} else {
>  		/* register at end of list to honor first register win */
>  		list_add_tail(&logger->list[pf], &nf_loggers_l[pf]);
> -		llog = rcu_dereference(nf_loggers[pf]);
> -		if (llog == NULL)
> +		if (nf_loggers[pf] == NULL)
>  			rcu_assign_pointer(nf_loggers[pf], logger);
>  	}
> 
> @@ -65,13 +63,11 @@ EXPORT_SYMBOL(nf_log_register);
> 
>  void nf_log_unregister(struct nf_logger *logger)
>  {
> -	const struct nf_logger *c_logger;
>  	int i;
> 
>  	mutex_lock(&nf_log_mutex);
>  	for (i = 0; i < ARRAY_SIZE(nf_loggers); i++) {
> -		c_logger = rcu_dereference(nf_loggers[i]);
> -		if (c_logger == logger)
> +		if (nf_loggers[i] == logger)
>  			rcu_assign_pointer(nf_loggers[i], NULL);
>  		list_del(&logger->list[i]);
>  	}

Re: mmotm 2010-04-28 - RCU whinges

[David Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Mon, 10 May 2010 17:40:58 +0200

> I queued this patch in nf-next, the only reason why I haven't
> submitted it yet is that I was unable to get git to cleanly export
> only the proper set of patches meant for -next due to a few merges,
> it insists on including 5 patches already merged upstream. If you
> don't mind ignoring the first 5 patches in the series, I'll send a
> pull request tonight.

Something like "git format-patch origin" doesn't avoid those upstream
commits?  Weird...

Another trick is to specify a commit range using triple-dot "..."
notation, such as "origin...master"

Re: mmotm 2010-04-28 - RCU whinges

[Patrick McHardy]

David Miller wrote:
> From: Patrick McHardy <kaber@trash.net>
> Date: Mon, 10 May 2010 17:40:58 +0200
> 
>> I queued this patch in nf-next, the only reason why I haven't
>> submitted it yet is that I was unable to get git to cleanly export
>> only the proper set of patches meant for -next due to a few merges,
>> it insists on including 5 patches already merged upstream. If you
>> don't mind ignoring the first 5 patches in the series, I'll send a
>> pull request tonight.
> 
> Something like "git format-patch origin" doesn't avoid those upstream
> commits?  Weird...

Yeah, it seems to have something to do with me merging the nf-2.6.git
tree a few weeks ago since it had patches queued that were too late
for 2.6.34. Even the --ignore-if-in-upstream option doesn't help.

> Another trick is to specify a commit range using triple-dot "..."
> notation, such as "origin...master"

Thanks, I'll give it another try, the alternative is manually
renumbering the entire patchset.

Re: mmotm 2010-04-28 - RCU whinges

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 2704 bytes --]

On Mon, 03 May 2010 07:38:57 +0200, Eric Dumazet said:
> Le dimanche 02 mai 2010 à 13:46 -0400, Valdis.Kletnieks@vt.edu a écrit :
> > On Wed, 28 Apr 2010 16:53:32 PDT, akpm@linux-foundation.org said:
> > > The mm-of-the-moment snapshot 2010-04-28-16-53 has been uploaded to
> > > 
> > >    http://userweb.kernel.org/~akpm/mmotm/
> > 
> > I thought we swatted all these, hit another one...

> Thanks for the report !
> 
> We can use rcu_dereference_protected() in those cases.
> 
> [PATCH] net: Use rcu_dereference_protected in nf_conntrack_ecache
> 
> Writers own nf_ct_ecache_mutex.

I *really* thought we swatted a bunch of these - did the fixes not make it
into linux-next or -mm?  Your patch fixed that one, but then:

[    9.128899] Netfilter messages via NETLINK v0.30.
[    9.128919] nf_conntrack version 0.5.0 (16384 buckets, 65536 max)
[    9.129108] CONFIG_NF_CT_ACCT is deprecated and will be removed soon. Please use
[    9.129110] nf_conntrack.acct=1 kernel parameter, acct=1 nf_conntrack module option or
[    9.129113] sysctl net.netfilter.nf_conntrack_acct=1 to enable it.
[    9.129135] ctnetlink v0.93: registering with nfnetlink.
[    9.129452] ip_tables: (C) 2000-2006 Netfilter Core Team
[    9.129506] 
[    9.129507] ===================================================
[    9.129683] [ INFO: suspicious rcu_dereference_check() usage. ]
[    9.129777] ---------------------------------------------------
[    9.129872] net/netfilter/nf_log.c:55 invoked rcu_dereference_check() without protection!
[    9.129969] 
[    9.129969] other info that might help us debug this:
[    9.129970] 
[    9.130232] 
[    9.130232] rcu_scheduler_active = 1, debug_locks = 0
[    9.130407] 1 lock held by swapper/1:
[    9.130525]  #0:  (nf_log_mutex){+.+...}, at: [<ffffffff81481154>] nf_log_register+0x57/0x10f
[    9.130955] 
[    9.130956] stack backtrace:
[    9.131162] Pid: 1, comm: swapper Tainted: G        W   2.6.34-rc5-mmotm0428 #2
[    9.131259] Call Trace:
[    9.131370]  [<ffffffff81064832>] lockdep_rcu_dereference+0xaa/0xb2
[    9.131466]  [<ffffffff814811db>] nf_log_register+0xde/0x10f
[    9.131579]  [<ffffffff81b5ca28>] ? log_tg_init+0x0/0x29
[    9.131689]  [<ffffffff81b5ca4d>] log_tg_init+0x25/0x29
[    9.131800]  [<ffffffff810001ef>] do_one_initcall+0x59/0x14e
[    9.131912]  [<ffffffff81b2e68a>] kernel_init+0x144/0x1ce
[    9.132033]  [<ffffffff81003414>] kernel_thread_helper+0x4/0x10
[    9.132146]  [<ffffffff81598a40>] ? restore_args+0x0/0x30
[    9.132257]  [<ffffffff81b2e546>] ? kernel_init+0x0/0x1ce
[    9.132370]  [<ffffffff81003410>] ? kernel_thread_helper+0x0/0x10
[    9.132513] TCP bic registered


[-- Attachment #2: Type: application/pgp-signature, Size: 227 bytes --]

Re: mmotm 2010-04-28 - RCU whinges

[Eric Dumazet]

Le lundi 03 mai 2010 à 10:30 -0400, Valdis.Kletnieks@vt.edu a écrit :

> 
> I *really* thought we swatted a bunch of these - did the fixes not make it
> into linux-next or -mm?  Your patch fixed that one, but then:
> 
> [    9.128899] Netfilter messages via NETLINK v0.30.
> [    9.128919] nf_conntrack version 0.5.0 (16384 buckets, 65536 max)
> [    9.129108] CONFIG_NF_CT_ACCT is deprecated and will be removed soon. Please use
> [    9.129110] nf_conntrack.acct=1 kernel parameter, acct=1 nf_conntrack module option or
> [    9.129113] sysctl net.netfilter.nf_conntrack_acct=1 to enable it.
> [    9.129135] ctnetlink v0.93: registering with nfnetlink.
> [    9.129452] ip_tables: (C) 2000-2006 Netfilter Core Team
> [    9.129506] 
> [    9.129507] ===================================================
> [    9.129683] [ INFO: suspicious rcu_dereference_check() usage. ]
> [    9.129777] ---------------------------------------------------
> [    9.129872] net/netfilter/nf_log.c:55 invoked rcu_dereference_check() without protection!
> [    9.129969] 
> [    9.129969] other info that might help us debug this:
> [    9.129970] 
> [    9.130232] 
> [    9.130232] rcu_scheduler_active = 1, debug_locks = 0
> [    9.130407] 1 lock held by swapper/1:
> [    9.130525]  #0:  (nf_log_mutex){+.+...}, at: [<ffffffff81481154>] nf_log_register+0x57/0x10f
> [    9.130955] 
> [    9.130956] stack backtrace:
> [    9.131162] Pid: 1, comm: swapper Tainted: G        W   2.6.34-rc5-mmotm0428 #2
> [    9.131259] Call Trace:
> [    9.131370]  [<ffffffff81064832>] lockdep_rcu_dereference+0xaa/0xb2
> [    9.131466]  [<ffffffff814811db>] nf_log_register+0xde/0x10f
> [    9.131579]  [<ffffffff81b5ca28>] ? log_tg_init+0x0/0x29
> [    9.131689]  [<ffffffff81b5ca4d>] log_tg_init+0x25/0x29
> [    9.131800]  [<ffffffff810001ef>] do_one_initcall+0x59/0x14e
> [    9.131912]  [<ffffffff81b2e68a>] kernel_init+0x144/0x1ce
> [    9.132033]  [<ffffffff81003414>] kernel_thread_helper+0x4/0x10
> [    9.132146]  [<ffffffff81598a40>] ? restore_args+0x0/0x30
> [    9.132257]  [<ffffffff81b2e546>] ? kernel_init+0x0/0x1ce
> [    9.132370]  [<ffffffff81003410>] ? kernel_thread_helper+0x0/0x10
> [    9.132513] TCP bic registered
> 

You probably know this PROVE_RCU thing is new and reserved to
developpers ?

We yet have to change all spots were a rcu_dereference() was used
without rcu_read_lock(). Not a bug by itself, just lockdep is to be
instructed not to shout.

Maybe 30 patches already in, and maybe 30 other are still needed.

Re: mmotm 2010-04-28 - RCU whinges

[Eric Dumazet]

Le lundi 03 mai 2010 à 10:30 -0400, Valdis.Kletnieks@vt.edu a écrit :

> [    9.128899] Netfilter messages via NETLINK v0.30.
> [    9.128919] nf_conntrack version 0.5.0 (16384 buckets, 65536 max)
> [    9.129108] CONFIG_NF_CT_ACCT is deprecated and will be removed soon. Please use
> [    9.129110] nf_conntrack.acct=1 kernel parameter, acct=1 nf_conntrack module option or
> [    9.129113] sysctl net.netfilter.nf_conntrack_acct=1 to enable it.
> [    9.129135] ctnetlink v0.93: registering with nfnetlink.
> [    9.129452] ip_tables: (C) 2000-2006 Netfilter Core Team
> [    9.129506] 
> [    9.129507] ===================================================
> [    9.129683] [ INFO: suspicious rcu_dereference_check() usage. ]
> [    9.129777] ---------------------------------------------------
> [    9.129872] net/netfilter/nf_log.c:55 invoked rcu_dereference_check() without protection!
> [    9.129969] 
> [    9.129969] other info that might help us debug this:
> [    9.129970] 
> [    9.130232] 
> [    9.130232] rcu_scheduler_active = 1, debug_locks = 0
> [    9.130407] 1 lock held by swapper/1:
> [    9.130525]  #0:  (nf_log_mutex){+.+...}, at: [<ffffffff81481154>] nf_log_register+0x57/0x10f
> [    9.130955] 
> [    9.130956] stack backtrace:
> [    9.131162] Pid: 1, comm: swapper Tainted: G        W   2.6.34-rc5-mmotm0428 #2
> [    9.131259] Call Trace:
> [    9.131370]  [<ffffffff81064832>] lockdep_rcu_dereference+0xaa/0xb2
> [    9.131466]  [<ffffffff814811db>] nf_log_register+0xde/0x10f
> [    9.131579]  [<ffffffff81b5ca28>] ? log_tg_init+0x0/0x29
> [    9.131689]  [<ffffffff81b5ca4d>] log_tg_init+0x25/0x29
> [    9.131800]  [<ffffffff810001ef>] do_one_initcall+0x59/0x14e
> [    9.131912]  [<ffffffff81b2e68a>] kernel_init+0x144/0x1ce
> [    9.132033]  [<ffffffff81003414>] kernel_thread_helper+0x4/0x10
> [    9.132146]  [<ffffffff81598a40>] ? restore_args+0x0/0x30
> [    9.132257]  [<ffffffff81b2e546>] ? kernel_init+0x0/0x1ce
> [    9.132370]  [<ffffffff81003410>] ? kernel_thread_helper+0x0/0x10
> [    9.132513] TCP bic registered
> 

Thanks for the report !

[PATCH] net: nf_log RCU fixes

nf_log_register() and nf_log_unregister() use a mutex to have exclusive
access to nf_logers[]. Use appropriate rcu_dereference_protected()
lockdep annotation.

Reported-by: Valdis Kletnieks <Valdis.Kletnieks@vt.edu>
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
diff --git a/net/netfilter/nf_log.c b/net/netfilter/nf_log.c
index 015725a..7df37fd 100644
--- a/net/netfilter/nf_log.c
+++ b/net/netfilter/nf_log.c
@@ -52,7 +52,8 @@ int nf_log_register(u_int8_t pf, struct nf_logger *logger)
 	} else {
 		/* register at end of list to honor first register win */
 		list_add_tail(&logger->list[pf], &nf_loggers_l[pf]);
-		llog = rcu_dereference(nf_loggers[pf]);
+		llog = rcu_dereference_protected(nf_loggers[pf],
+						 lockdep_is_held(&nf_log_mutex));
 		if (llog == NULL)
 			rcu_assign_pointer(nf_loggers[pf], logger);
 	}
@@ -70,7 +71,8 @@ void nf_log_unregister(struct nf_logger *logger)
 
 	mutex_lock(&nf_log_mutex);
 	for (i = 0; i < ARRAY_SIZE(nf_loggers); i++) {
-		c_logger = rcu_dereference(nf_loggers[i]);
+		c_logger = rcu_dereference_protected(nf_loggers[i],
+						     lockdep_is_held(&nf_log_mutex));
 		if (c_logger == logger)
 			rcu_assign_pointer(nf_loggers[i], NULL);
 		list_del(&logger->list[i]);

Re: mmotm 2010-04-28 - RCU whinges

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 2085 bytes --]

On Mon, 03 May 2010 16:58:46 +0200, Eric Dumazet said:
> Le lundi 03 mai 2010 à 10:30 -0400, Valdis.Kletnieks@vt.edu a écrit :

> > [    9.129872] net/netfilter/nf_log.c:55 invoked rcu_dereference_check() without protection!

> Thanks for the report !
> 
> [PATCH] net: nf_log RCU fixes
> 
> nf_log_register() and nf_log_unregister() use a mutex to have exclusive
> access to nf_logers[]. Use appropriate rcu_dereference_protected()
> lockdep annotation.

Confirming that one fixed.  Now it lives a whole 36 seconds before whinging:

[   35.328729] ===================================================
[   35.328803] [ INFO: suspicious rcu_dereference_check() usage. ]
[   35.328837] ---------------------------------------------------
[   35.328872] net/ipv6/addrconf.c:2977 invoked rcu_dereference_check() without protection!
[   35.328926] 
[   35.328927] other info that might help us debug this:
[   35.328928] 
[   35.329016] 
[   35.329016] rcu_scheduler_active = 1, debug_locks = 0
[   35.329089] 2 locks held by ifconfig/2680:
[   35.329120]  #0:  (&p->lock){+.+.+.}, at: [<ffffffff810f5db8>] seq_read+0x3a/0x42d
[   35.329217]  #1:  (rcu_read_lock_bh){.+....}, at: [<ffffffff814eec68>] rcu_read_lock_bh+0x0/0x35
[   35.329322] 
[   35.329323] stack backtrace:
[   35.329380] Pid: 2680, comm: ifconfig Tainted: G        W   2.6.34-rc5-mmotm0428 #3
[   35.329439] Call Trace:
[   35.329471]  [<ffffffff81064832>] lockdep_rcu_dereference+0xaa/0xb2
[   35.329514]  [<ffffffff814ef3ae>] if6_get_next+0x34/0x6d
[   35.329554]  [<ffffffff814ef3f8>] if6_seq_next+0x11/0x18
[   35.329595]  [<ffffffff810f6083>] seq_read+0x305/0x42d
[   35.329635]  [<ffffffff810f5d7e>] ? seq_read+0x0/0x42d
[   35.329676]  [<ffffffff81129e8c>] proc_reg_read+0x8d/0xac
[   35.329717]  [<ffffffff810db7e0>] vfs_read+0xe0/0x140
[   35.329758]  [<ffffffff810db8f6>] sys_read+0x45/0x69
[   35.329799]  [<ffffffff810025eb>] system_call_fastpath+0x16/0x1b

Maybe I need to go and stick the "RCU whinge multiple times" patch on this
kernel and get it over with. :)


[-- Attachment #2: Type: application/pgp-signature, Size: 227 bytes --]

Re: mmotm 2010-04-28 - RCU whinges

[Paul E. McKenney]

On Mon, May 03, 2010 at 11:29:50AM -0400, Valdis.Kletnieks@vt.edu wrote:
> On Mon, 03 May 2010 16:58:46 +0200, Eric Dumazet said:
> > Le lundi 03 mai 2010 à 10:30 -0400, Valdis.Kletnieks@vt.edu a écrit :
> 
> > > [    9.129872] net/netfilter/nf_log.c:55 invoked rcu_dereference_check() without protection!
> 
> > Thanks for the report !
> > 
> > [PATCH] net: nf_log RCU fixes
> > 
> > nf_log_register() and nf_log_unregister() use a mutex to have exclusive
> > access to nf_logers[]. Use appropriate rcu_dereference_protected()
> > lockdep annotation.
> 
> Confirming that one fixed.  Now it lives a whole 36 seconds before whinging:
> 
> [   35.328729] ===================================================
> [   35.328803] [ INFO: suspicious rcu_dereference_check() usage. ]
> [   35.328837] ---------------------------------------------------
> [   35.328872] net/ipv6/addrconf.c:2977 invoked rcu_dereference_check() without protection!
> [   35.328926] 
> [   35.328927] other info that might help us debug this:
> [   35.328928] 
> [   35.329016] 
> [   35.329016] rcu_scheduler_active = 1, debug_locks = 0
> [   35.329089] 2 locks held by ifconfig/2680:
> [   35.329120]  #0:  (&p->lock){+.+.+.}, at: [<ffffffff810f5db8>] seq_read+0x3a/0x42d
> [   35.329217]  #1:  (rcu_read_lock_bh){.+....}, at: [<ffffffff814eec68>] rcu_read_lock_bh+0x0/0x35
> [   35.329322] 
> [   35.329323] stack backtrace:
> [   35.329380] Pid: 2680, comm: ifconfig Tainted: G        W   2.6.34-rc5-mmotm0428 #3
> [   35.329439] Call Trace:
> [   35.329471]  [<ffffffff81064832>] lockdep_rcu_dereference+0xaa/0xb2
> [   35.329514]  [<ffffffff814ef3ae>] if6_get_next+0x34/0x6d
> [   35.329554]  [<ffffffff814ef3f8>] if6_seq_next+0x11/0x18
> [   35.329595]  [<ffffffff810f6083>] seq_read+0x305/0x42d
> [   35.329635]  [<ffffffff810f5d7e>] ? seq_read+0x0/0x42d
> [   35.329676]  [<ffffffff81129e8c>] proc_reg_read+0x8d/0xac
> [   35.329717]  [<ffffffff810db7e0>] vfs_read+0xe0/0x140
> [   35.329758]  [<ffffffff810db8f6>] sys_read+0x45/0x69
> [   35.329799]  [<ffffffff810025eb>] system_call_fastpath+0x16/0x1b
> 
> Maybe I need to go and stick the "RCU whinge multiple times" patch on this
> kernel and get it over with. :)

Highly recommended.  ;-)

And thanks to you for your testing efforts and to Eric for the fixes!!!

							Thanx, Paul
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: mmotm 2010-04-28 - RCU whinges

[Eric Dumazet]

Le lundi 03 mai 2010 à 08:43 -0700, Paul E. McKenney a écrit :

> Highly recommended.  ;-)
> 
> And thanks to you for your testing efforts and to Eric for the fixes!!!
> 

For this last one, I think you should push following patch Paul

Followup of commit 3120438ad6
(rcu: Disable lockdep checking in RCU list-traversal primitives)

Or we might introduce a hlist_for_each_entry_continue_rcu_bh() macro...



diff --git a/include/linux/rculist.h b/include/linux/rculist.h
index 004908b..b0c7e24 100644
--- a/include/linux/rculist.h
+++ b/include/linux/rculist.h
@@ -435,10 +435,10 @@ static inline void hlist_add_after_rcu(struct hlist_node *prev,
  * @member:	the name of the hlist_node within the struct.
  */
 #define hlist_for_each_entry_continue_rcu(tpos, pos, member)		\
-	for (pos = rcu_dereference((pos)->next);			\
+	for (pos = rcu_dereference_raw((pos)->next);			\
 	     pos && ({ prefetch(pos->next); 1; }) &&			\
 	     ({ tpos = hlist_entry(pos, typeof(*tpos), member); 1; });  \
-	     pos = rcu_dereference(pos->next))
+	     pos = rcu_dereference_raw(pos->next))
 
 
 #endif	/* __KERNEL__ */

Re: mmotm 2010-04-28 - RCU whinges

[Paul E. McKenney]

On Mon, May 03, 2010 at 06:14:53PM +0200, Eric Dumazet wrote:
> Le lundi 03 mai 2010 à 08:43 -0700, Paul E. McKenney a écrit :
> 
> > Highly recommended.  ;-)
> > 
> > And thanks to you for your testing efforts and to Eric for the fixes!!!
> > 
> 
> For this last one, I think you should push following patch Paul

I would be happy to if I could find the commit creating
hlist_for_each_entry_continue_rcu()...

I do see a ca. 2008 patch from Stephen Hemminger:

	http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg264661.html

According to http://patchwork.ozlabs.org/patch/47997/, this is
going up the networking tree as of March 18, 2010.

So I would be happy to push the patch below, but to do so, I will need
to adopt the portion of Stephen's patch that created this primitive.

Please let me know how you would like to proceed!

							Thanx, Paul

> Followup of commit 3120438ad6
> (rcu: Disable lockdep checking in RCU list-traversal primitives)
> 
> Or we might introduce a hlist_for_each_entry_continue_rcu_bh() macro...
> 
> 
> 
> diff --git a/include/linux/rculist.h b/include/linux/rculist.h
> index 004908b..b0c7e24 100644
> --- a/include/linux/rculist.h
> +++ b/include/linux/rculist.h
> @@ -435,10 +435,10 @@ static inline void hlist_add_after_rcu(struct hlist_node *prev,
>   * @member:	the name of the hlist_node within the struct.
>   */
>  #define hlist_for_each_entry_continue_rcu(tpos, pos, member)		\
> -	for (pos = rcu_dereference((pos)->next);			\
> +	for (pos = rcu_dereference_raw((pos)->next);			\
>  	     pos && ({ prefetch(pos->next); 1; }) &&			\
>  	     ({ tpos = hlist_entry(pos, typeof(*tpos), member); 1; });  \
> -	     pos = rcu_dereference(pos->next))
> +	     pos = rcu_dereference_raw(pos->next))
> 
> 
>  #endif	/* __KERNEL__ */
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH net-next-2.6] net: if6_get_next() fix

[Eric Dumazet]

Le lundi 03 mai 2010 à 11:16 -0700, Paul E. McKenney a écrit :
> I would be happy to if I could find the commit creating
> hlist_for_each_entry_continue_rcu()...
> 
> I do see a ca. 2008 patch from Stephen Hemminger:
> 
> 	http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg264661.html
> 
> According to http://patchwork.ozlabs.org/patch/47997/, this is
> going up the networking tree as of March 18, 2010.
> 
> So I would be happy to push the patch below, but to do so, I will need
> to adopt the portion of Stephen's patch that created this primitive.
> 

Hmm,  I realize there is a true bug introduced by Stephen patch

Then, net-next-2.6 doesnt yet have your commit Paul to relax
hlist_for_each_entry_rcu(), so its a bit difficult to continue the work.

Thanks

[PATCH net-next-2.6] net: if6_get_next() fix

Must use rcu variant, we are in a rcu_read_lock_bh() section

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 34d2d64..16bb85c 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -2979,7 +2979,7 @@ static struct inet6_ifaddr *if6_get_next(struct seq_file *seq,
 			return ifa;
 
 	while (++state->bucket < IN6_ADDR_HSIZE) {
-		hlist_for_each_entry(ifa, n,
+		hlist_for_each_entry_rcu(ifa, n,
 				     &inet6_addr_lst[state->bucket], addr_lst) {
 			if (net_eq(dev_net(ifa->idev->dev), net))
 				return ifa;

Re: [PATCH net-next-2.6] net: if6_get_next() fix

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 03 May 2010 21:46:47 +0200

> Then, net-next-2.6 doesnt yet have your commit Paul to relax
> hlist_for_each_entry_rcu(), so its a bit difficult to continue the work.

Is that in Linus's tree yet?  If it propagates there I can make it
propagate to net-next-2.6, you just have to tell me you need it.

Re: [PATCH net-next-2.6] net: if6_get_next() fix

[Eric Dumazet]

Le lundi 03 mai 2010 à 13:09 -0700, David Miller a écrit :
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Mon, 03 May 2010 21:46:47 +0200
> 
> > Then, net-next-2.6 doesnt yet have your commit Paul to relax
> > hlist_for_each_entry_rcu(), so its a bit difficult to continue the work.
> 
> Is that in Linus's tree yet?  If it propagates there I can make it
> propagate to net-next-2.6, you just have to tell me you need it.


Hmm, it seems it's already in net-next-2.6, sorry for the confusion.

commit 3120438ad68601f341e61e7cb1323b0e1a6ca367
Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Date:   Mon Feb 22 17:04:48 2010 -0800

    rcu: Disable lockdep checking in RCU list-traversal primitives
    
    The theory is that use of bare rcu_dereference() is more prone
    to error than use of the RCU list-traversal primitives.
    Therefore, disable lockdep RCU read-side critical-section
    checking in these primitives for the time being.  Once all of
    the rcu_dereference() uses have been dealt with, it may be time
    to re-enable lockdep checking for the RCU list-traversal
    primitives.
    


--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next-2.6] net: if6_get_next() fix

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 03 May 2010 22:13:55 +0200

> Le lundi 03 mai 2010 à 13:09 -0700, David Miller a écrit :
>> From: Eric Dumazet <eric.dumazet@gmail.com>
>> Date: Mon, 03 May 2010 21:46:47 +0200
>> 
>> > Then, net-next-2.6 doesnt yet have your commit Paul to relax
>> > hlist_for_each_entry_rcu(), so its a bit difficult to continue the work.
>> 
>> Is that in Linus's tree yet?  If it propagates there I can make it
>> propagate to net-next-2.6, you just have to tell me you need it.
> 
> 
> Hmm, it seems it's already in net-next-2.6, sorry for the confusion.

No problem, just let me know in the future if something upstream needs
to propagate.

Re: [PATCH net-next-2.6] net: if6_get_next() fix

[Eric Dumazet]

Paul, David, here the patch I was thinking about :

Feel free to split it in two parts if you like, I am too tired and must
sleep now ;)

Thanks

[PATCH net-next-2.6] net: rcu fixes

Add hlist_for_each_entry_rcu_bh() and
hlist_for_each_entry_continue_rcu_bh() macros, and use them in
ipv6_get_ifaddr(), if6_get_first() and if6_get_next() to fix lockdeps
warnings.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
 include/linux/rculist.h |   29 +++++++++++++++++++++++++++++
 net/ipv6/addrconf.c     |   16 ++++++++--------
 2 files changed, 37 insertions(+), 8 deletions(-)

diff --git a/include/linux/rculist.h b/include/linux/rculist.h
index 004908b..4ec3b38 100644
--- a/include/linux/rculist.h
+++ b/include/linux/rculist.h
@@ -429,6 +429,23 @@ static inline void hlist_add_after_rcu(struct hlist_node *prev,
 		pos = rcu_dereference_raw(pos->next))
 
 /**
+ * hlist_for_each_entry_rcu_bh - iterate over rcu list of given type
+ * @tpos:	the type * to use as a loop cursor.
+ * @pos:	the &struct hlist_node to use as a loop cursor.
+ * @head:	the head for your list.
+ * @member:	the name of the hlist_node within the struct.
+ *
+ * This list-traversal primitive may safely run concurrently with
+ * the _rcu list-mutation primitives such as hlist_add_head_rcu()
+ * as long as the traversal is guarded by rcu_read_lock().
+ */
+#define hlist_for_each_entry_rcu_bh(tpos, pos, head, member)		 \
+	for (pos = rcu_dereference_bh((head)->first);			 \
+		pos && ({ prefetch(pos->next); 1; }) &&			 \
+		({ tpos = hlist_entry(pos, typeof(*tpos), member); 1; }); \
+		pos = rcu_dereference_bh(pos->next))
+
+/**
  * hlist_for_each_entry_continue_rcu - iterate over a hlist continuing after current point
  * @tpos:	the type * to use as a loop cursor.
  * @pos:	the &struct hlist_node to use as a loop cursor.
@@ -440,6 +457,18 @@ static inline void hlist_add_after_rcu(struct hlist_node *prev,
 	     ({ tpos = hlist_entry(pos, typeof(*tpos), member); 1; });  \
 	     pos = rcu_dereference(pos->next))
 
+/**
+ * hlist_for_each_entry_continue_rcu_bh - iterate over a hlist continuing after current point
+ * @tpos:	the type * to use as a loop cursor.
+ * @pos:	the &struct hlist_node to use as a loop cursor.
+ * @member:	the name of the hlist_node within the struct.
+ */
+#define hlist_for_each_entry_continue_rcu_bh(tpos, pos, member)		\
+	for (pos = rcu_dereference_bh((pos)->next);			\
+	     pos && ({ prefetch(pos->next); 1; }) &&			\
+	     ({ tpos = hlist_entry(pos, typeof(*tpos), member); 1; });  \
+	     pos = rcu_dereference_bh(pos->next))
+
 
 #endif	/* __KERNEL__ */
 #endif
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 34d2d64..3984f52 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -1346,7 +1346,7 @@ struct inet6_ifaddr *ipv6_get_ifaddr(struct net *net, const struct in6_addr *add
 	struct hlist_node *node;
 
 	rcu_read_lock_bh();
-	hlist_for_each_entry_rcu(ifp, node, &inet6_addr_lst[hash], addr_lst) {
+	hlist_for_each_entry_rcu_bh(ifp, node, &inet6_addr_lst[hash], addr_lst) {
 		if (!net_eq(dev_net(ifp->idev->dev), net))
 			continue;
 		if (ipv6_addr_equal(&ifp->addr, addr)) {
@@ -2959,7 +2959,7 @@ static struct inet6_ifaddr *if6_get_first(struct seq_file *seq)
 
 	for (state->bucket = 0; state->bucket < IN6_ADDR_HSIZE; ++state->bucket) {
 		struct hlist_node *n;
-		hlist_for_each_entry_rcu(ifa, n, &inet6_addr_lst[state->bucket],
+		hlist_for_each_entry_rcu_bh(ifa, n, &inet6_addr_lst[state->bucket],
 					 addr_lst)
 			if (net_eq(dev_net(ifa->idev->dev), net))
 				return ifa;
@@ -2974,12 +2974,12 @@ static struct inet6_ifaddr *if6_get_next(struct seq_file *seq,
 	struct net *net = seq_file_net(seq);
 	struct hlist_node *n = &ifa->addr_lst;
 
-	hlist_for_each_entry_continue_rcu(ifa, n, addr_lst)
+	hlist_for_each_entry_continue_rcu_bh(ifa, n, addr_lst)
 		if (net_eq(dev_net(ifa->idev->dev), net))
 			return ifa;
 
 	while (++state->bucket < IN6_ADDR_HSIZE) {
-		hlist_for_each_entry(ifa, n,
+		hlist_for_each_entry_rcu_bh(ifa, n,
 				     &inet6_addr_lst[state->bucket], addr_lst) {
 			if (net_eq(dev_net(ifa->idev->dev), net))
 				return ifa;
@@ -3000,7 +3000,7 @@ static struct inet6_ifaddr *if6_get_idx(struct seq_file *seq, loff_t pos)
 }
 
 static void *if6_seq_start(struct seq_file *seq, loff_t *pos)
-	__acquires(rcu)
+	__acquires(rcu_bh)
 {
 	rcu_read_lock_bh();
 	return if6_get_idx(seq, *pos);
@@ -3016,7 +3016,7 @@ static void *if6_seq_next(struct seq_file *seq, void *v, loff_t *pos)
 }
 
 static void if6_seq_stop(struct seq_file *seq, void *v)
-	__releases(rcu)
+	__releases(rcu_bh)
 {
 	rcu_read_unlock_bh();
 }
@@ -3093,7 +3093,7 @@ int ipv6_chk_home_addr(struct net *net, struct in6_addr *addr)
 	unsigned int hash = ipv6_addr_hash(addr);
 
 	rcu_read_lock_bh();
-	hlist_for_each_entry_rcu(ifp, n, &inet6_addr_lst[hash], addr_lst) {
+	hlist_for_each_entry_rcu_bh(ifp, n, &inet6_addr_lst[hash], addr_lst) {
 		if (!net_eq(dev_net(ifp->idev->dev), net))
 			continue;
 		if (ipv6_addr_equal(&ifp->addr, addr) &&
@@ -3127,7 +3127,7 @@ static void addrconf_verify(unsigned long foo)
 
 	for (i = 0; i < IN6_ADDR_HSIZE; i++) {
 restart:
-		hlist_for_each_entry_rcu(ifp, node,
+		hlist_for_each_entry_rcu_bh(ifp, node,
 					 &inet6_addr_lst[i], addr_lst) {
 			unsigned long age;
 

Re: [PATCH net-next-2.6] net: if6_get_next() fix

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 03 May 2010 22:50:14 +0200

> Paul, David, here the patch I was thinking about :
> 
> Feel free to split it in two parts if you like, I am too tired and must
> sleep now ;)
 ...
> [PATCH net-next-2.6] net: rcu fixes
> 
> Add hlist_for_each_entry_rcu_bh() and
> hlist_for_each_entry_continue_rcu_bh() macros, and use them in
> ipv6_get_ifaddr(), if6_get_first() and if6_get_next() to fix lockdeps
> warnings.
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>

Paul, let me know if you want to handle these seperately (one commit
in your tree for the rculist.h bit and one for the ipv6 change) or to
put it all at once into net-next-2.6, I'm happy either way.

Re: [PATCH net-next-2.6] net: if6_get_next() fix

[Paul E. McKenney]

On Mon, May 03, 2010 at 03:17:25PM -0700, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Mon, 03 May 2010 22:50:14 +0200
> 
> > Paul, David, here the patch I was thinking about :
> > 
> > Feel free to split it in two parts if you like, I am too tired and must
> > sleep now ;)
>  ...
> > [PATCH net-next-2.6] net: rcu fixes
> > 
> > Add hlist_for_each_entry_rcu_bh() and
> > hlist_for_each_entry_continue_rcu_bh() macros, and use them in
> > ipv6_get_ifaddr(), if6_get_first() and if6_get_next() to fix lockdeps
> > warnings.
> > 
> > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> 
> Paul, let me know if you want to handle these seperately (one commit
> in your tree for the rculist.h bit and one for the ipv6 change) or to
> put it all at once into net-next-2.6, I'm happy either way.

These changes look pretty closely integrated, so it is probably better
if they go up your tree with the related networking changes.  I will
take a look at them.

							Thanx, Paul

Re: [PATCH net-next-2.6] net: if6_get_next() fix

[Paul E. McKenney]

On Mon, May 03, 2010 at 10:50:14PM +0200, Eric Dumazet wrote:
> Paul, David, here the patch I was thinking about :
> 
> Feel free to split it in two parts if you like, I am too tired and must
> sleep now ;)
> 
> Thanks
> 
> [PATCH net-next-2.6] net: rcu fixes
> 
> Add hlist_for_each_entry_rcu_bh() and
> hlist_for_each_entry_continue_rcu_bh() macros, and use them in
> ipv6_get_ifaddr(), if6_get_first() and if6_get_next() to fix lockdeps
> warnings.

Looks good!!!

It will collide with Arnd's sparse-based changes, but that will be
easy to fix, so no problem.

Reviewed-by: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>

> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> ---
>  include/linux/rculist.h |   29 +++++++++++++++++++++++++++++
>  net/ipv6/addrconf.c     |   16 ++++++++--------
>  2 files changed, 37 insertions(+), 8 deletions(-)
> 
> diff --git a/include/linux/rculist.h b/include/linux/rculist.h
> index 004908b..4ec3b38 100644
> --- a/include/linux/rculist.h
> +++ b/include/linux/rculist.h
> @@ -429,6 +429,23 @@ static inline void hlist_add_after_rcu(struct hlist_node *prev,
>  		pos = rcu_dereference_raw(pos->next))
> 
>  /**
> + * hlist_for_each_entry_rcu_bh - iterate over rcu list of given type
> + * @tpos:	the type * to use as a loop cursor.
> + * @pos:	the &struct hlist_node to use as a loop cursor.
> + * @head:	the head for your list.
> + * @member:	the name of the hlist_node within the struct.
> + *
> + * This list-traversal primitive may safely run concurrently with
> + * the _rcu list-mutation primitives such as hlist_add_head_rcu()
> + * as long as the traversal is guarded by rcu_read_lock().
> + */
> +#define hlist_for_each_entry_rcu_bh(tpos, pos, head, member)		 \
> +	for (pos = rcu_dereference_bh((head)->first);			 \
> +		pos && ({ prefetch(pos->next); 1; }) &&			 \
> +		({ tpos = hlist_entry(pos, typeof(*tpos), member); 1; }); \
> +		pos = rcu_dereference_bh(pos->next))
> +
> +/**
>   * hlist_for_each_entry_continue_rcu - iterate over a hlist continuing after current point
>   * @tpos:	the type * to use as a loop cursor.
>   * @pos:	the &struct hlist_node to use as a loop cursor.
> @@ -440,6 +457,18 @@ static inline void hlist_add_after_rcu(struct hlist_node *prev,
>  	     ({ tpos = hlist_entry(pos, typeof(*tpos), member); 1; });  \
>  	     pos = rcu_dereference(pos->next))
> 
> +/**
> + * hlist_for_each_entry_continue_rcu_bh - iterate over a hlist continuing after current point
> + * @tpos:	the type * to use as a loop cursor.
> + * @pos:	the &struct hlist_node to use as a loop cursor.
> + * @member:	the name of the hlist_node within the struct.
> + */
> +#define hlist_for_each_entry_continue_rcu_bh(tpos, pos, member)		\
> +	for (pos = rcu_dereference_bh((pos)->next);			\
> +	     pos && ({ prefetch(pos->next); 1; }) &&			\
> +	     ({ tpos = hlist_entry(pos, typeof(*tpos), member); 1; });  \
> +	     pos = rcu_dereference_bh(pos->next))
> +
> 
>  #endif	/* __KERNEL__ */
>  #endif
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index 34d2d64..3984f52 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -1346,7 +1346,7 @@ struct inet6_ifaddr *ipv6_get_ifaddr(struct net *net, const struct in6_addr *add
>  	struct hlist_node *node;
> 
>  	rcu_read_lock_bh();
> -	hlist_for_each_entry_rcu(ifp, node, &inet6_addr_lst[hash], addr_lst) {
> +	hlist_for_each_entry_rcu_bh(ifp, node, &inet6_addr_lst[hash], addr_lst) {
>  		if (!net_eq(dev_net(ifp->idev->dev), net))
>  			continue;
>  		if (ipv6_addr_equal(&ifp->addr, addr)) {
> @@ -2959,7 +2959,7 @@ static struct inet6_ifaddr *if6_get_first(struct seq_file *seq)
> 
>  	for (state->bucket = 0; state->bucket < IN6_ADDR_HSIZE; ++state->bucket) {
>  		struct hlist_node *n;
> -		hlist_for_each_entry_rcu(ifa, n, &inet6_addr_lst[state->bucket],
> +		hlist_for_each_entry_rcu_bh(ifa, n, &inet6_addr_lst[state->bucket],
>  					 addr_lst)
>  			if (net_eq(dev_net(ifa->idev->dev), net))
>  				return ifa;
> @@ -2974,12 +2974,12 @@ static struct inet6_ifaddr *if6_get_next(struct seq_file *seq,
>  	struct net *net = seq_file_net(seq);
>  	struct hlist_node *n = &ifa->addr_lst;
> 
> -	hlist_for_each_entry_continue_rcu(ifa, n, addr_lst)
> +	hlist_for_each_entry_continue_rcu_bh(ifa, n, addr_lst)
>  		if (net_eq(dev_net(ifa->idev->dev), net))
>  			return ifa;
> 
>  	while (++state->bucket < IN6_ADDR_HSIZE) {
> -		hlist_for_each_entry(ifa, n,
> +		hlist_for_each_entry_rcu_bh(ifa, n,
>  				     &inet6_addr_lst[state->bucket], addr_lst) {
>  			if (net_eq(dev_net(ifa->idev->dev), net))
>  				return ifa;
> @@ -3000,7 +3000,7 @@ static struct inet6_ifaddr *if6_get_idx(struct seq_file *seq, loff_t pos)
>  }
> 
>  static void *if6_seq_start(struct seq_file *seq, loff_t *pos)
> -	__acquires(rcu)
> +	__acquires(rcu_bh)
>  {
>  	rcu_read_lock_bh();
>  	return if6_get_idx(seq, *pos);
> @@ -3016,7 +3016,7 @@ static void *if6_seq_next(struct seq_file *seq, void *v, loff_t *pos)
>  }
> 
>  static void if6_seq_stop(struct seq_file *seq, void *v)
> -	__releases(rcu)
> +	__releases(rcu_bh)
>  {
>  	rcu_read_unlock_bh();
>  }
> @@ -3093,7 +3093,7 @@ int ipv6_chk_home_addr(struct net *net, struct in6_addr *addr)
>  	unsigned int hash = ipv6_addr_hash(addr);
> 
>  	rcu_read_lock_bh();
> -	hlist_for_each_entry_rcu(ifp, n, &inet6_addr_lst[hash], addr_lst) {
> +	hlist_for_each_entry_rcu_bh(ifp, n, &inet6_addr_lst[hash], addr_lst) {
>  		if (!net_eq(dev_net(ifp->idev->dev), net))
>  			continue;
>  		if (ipv6_addr_equal(&ifp->addr, addr) &&
> @@ -3127,7 +3127,7 @@ static void addrconf_verify(unsigned long foo)
> 
>  	for (i = 0; i < IN6_ADDR_HSIZE; i++) {
>  restart:
> -		hlist_for_each_entry_rcu(ifp, node,
> +		hlist_for_each_entry_rcu_bh(ifp, node,
>  					 &inet6_addr_lst[i], addr_lst) {
>  			unsigned long age;
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next-2.6] net: if6_get_next() fix

[David Miller]

From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Date: Mon, 3 May 2010 15:52:29 -0700

> On Mon, May 03, 2010 at 10:50:14PM +0200, Eric Dumazet wrote:
>> Paul, David, here the patch I was thinking about :
>> 
>> Feel free to split it in two parts if you like, I am too tired and must
>> sleep now ;)
>> 
>> Thanks
>> 
>> [PATCH net-next-2.6] net: rcu fixes
>> 
>> Add hlist_for_each_entry_rcu_bh() and
>> hlist_for_each_entry_continue_rcu_bh() macros, and use them in
>> ipv6_get_ifaddr(), if6_get_first() and if6_get_next() to fix lockdeps
>> warnings.
> 
> Looks good!!!
> 
> It will collide with Arnd's sparse-based changes, but that will be
> easy to fix, so no problem.
> 
> Reviewed-by: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>

Applied, thanks!

Re: mmotm 2010-04-28 - RCU whinges

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 2484 bytes --]

Eric Dumazet wrote:
> Le dimanche 02 mai 2010 à 13:46 -0400, Valdis.Kletnieks@vt.edu a écrit :
>> On Wed, 28 Apr 2010 16:53:32 PDT, akpm@linux-foundation.org said:
>>> The mm-of-the-moment snapshot 2010-04-28-16-53 has been uploaded to
>>>
>>>    http://userweb.kernel.org/~akpm/mmotm/
>> I thought we swatted all these, hit another one...
>>
>> [    9.131490] ctnetlink v0.93: registering with nfnetlink.
>> [    9.131535]
>> [    9.131535] ===================================================
>> [    9.131704] [ INFO: suspicious rcu_dereference_check() usage. ]
>> [    9.131794] ---------------------------------------------------
>> [    9.131883] net/netfilter/nf_conntrack_ecache.c:88 invoked rcu_dereference_check() without protection!
>> [    9.131977]
>> [    9.131977] other info that might help us debug this:
>> [    9.131978]
>> [    9.132218]
>> [    9.132219] rcu_scheduler_active = 1, debug_locks = 0
>> [    9.132434] 1 lock held by swapper/1:
>> [    9.132519]  #0:  (nf_ct_ecache_mutex){+.+...}, at: [<ffffffff8148922d>] nf_conntrack_register_notifier+0x1a/0x75
>> [    9.132938]
>> [    9.132939] stack backtrace:
>> [    9.133129] Pid: 1, comm: swapper Tainted: G        W   2.6.34-rc5-mmotm0428 #1
>> [    9.133220] Call Trace:
>> [    9.133319]  [<ffffffff81064832>] lockdep_rcu_dereference+0xaa/0xb2
>> [    9.133410]  [<ffffffff81489250>] nf_conntrack_register_notifier+0x3d/0x75
>> [    9.133521]  [<ffffffff81b5a157>] ctnetlink_init+0x71/0xd5
>> [    9.133627]  [<ffffffff81b5a0e6>] ? ctnetlink_init+0x0/0xd5
>> [    9.133735]  [<ffffffff810001ef>] do_one_initcall+0x59/0x14e
>> [    9.133843]  [<ffffffff81b2e68a>] kernel_init+0x144/0x1ce
>> [    9.133949]  [<ffffffff81003414>] kernel_thread_helper+0x4/0x10
>> [    9.134060]  [<ffffffff81598a40>] ? restore_args+0x0/0x30
>> [    9.134196]  [<ffffffff81b2e546>] ? kernel_init+0x0/0x1ce
>> [    9.134328]  [<ffffffff81003410>] ? kernel_thread_helper+0x0/0x10
>> [    9.134530] ip_tables: (C) 2000-2006 Netfilter Core Team
>> [    9.134655] TCP bic registered
>>
> 
> Thanks for the report !
> 
> We can use rcu_dereference_protected() in those cases.
> 
> [PATCH] net: Use rcu_dereference_protected in nf_conntrack_ecache
> 
> Writers own nf_ct_ecache_mutex.

I've committed this patch to my tree, which also fixes up the nf_log
changes I already had queued.

I've also figured out how to prevent the false commits from showing
up using the '^' notation, I'll submit everything after some final
testing.



[-- Attachment #2: x --]
[-- Type: text/plain, Size: 3904 bytes --]

commit b56f2d55c6c22b0c5774b3b22e336fb6cc5f4094
Author: Patrick McHardy <kaber@trash.net>
Date:   Mon May 10 18:47:57 2010 +0200

    netfilter: use rcu_dereference_protected()
    
    Restore the rcu_dereference() calls in conntrack/expectation notifier
    and logger registration/unregistration, but use the _protected variant,
    which will be required by the upcoming __rcu annotations.
    
    Based on patch by Eric Dumazet <eric.dumazet@gmail.com>
    
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/net/netfilter/nf_conntrack_ecache.c b/net/netfilter/nf_conntrack_ecache.c
index a94ac3a..cdcc764 100644
--- a/net/netfilter/nf_conntrack_ecache.c
+++ b/net/netfilter/nf_conntrack_ecache.c
@@ -82,9 +82,12 @@ EXPORT_SYMBOL_GPL(nf_ct_deliver_cached_events);
 int nf_conntrack_register_notifier(struct nf_ct_event_notifier *new)
 {
 	int ret = 0;
+	struct nf_ct_event_notifier *notify;
 
 	mutex_lock(&nf_ct_ecache_mutex);
-	if (nf_conntrack_event_cb != NULL) {
+	notify = rcu_dereference_protected(nf_conntrack_event_cb,
+					   lockdep_is_held(&nf_ct_ecache_mutex));
+	if (notify != NULL) {
 		ret = -EBUSY;
 		goto out_unlock;
 	}
@@ -100,8 +103,12 @@ EXPORT_SYMBOL_GPL(nf_conntrack_register_notifier);
 
 void nf_conntrack_unregister_notifier(struct nf_ct_event_notifier *new)
 {
+	struct nf_ct_event_notifier *notify;
+
 	mutex_lock(&nf_ct_ecache_mutex);
-	BUG_ON(nf_conntrack_event_cb != new);
+	notify = rcu_dereference_protected(nf_conntrack_event_cb,
+					   lockdep_is_held(&nf_ct_ecache_mutex));
+	BUG_ON(notify != new);
 	rcu_assign_pointer(nf_conntrack_event_cb, NULL);
 	mutex_unlock(&nf_ct_ecache_mutex);
 }
@@ -110,9 +117,12 @@ EXPORT_SYMBOL_GPL(nf_conntrack_unregister_notifier);
 int nf_ct_expect_register_notifier(struct nf_exp_event_notifier *new)
 {
 	int ret = 0;
+	struct nf_exp_event_notifier *notify;
 
 	mutex_lock(&nf_ct_ecache_mutex);
-	if (nf_expect_event_cb != NULL) {
+	notify = rcu_dereference_protected(nf_expect_event_cb,
+					   lockdep_is_held(&nf_ct_ecache_mutex));
+	if (notify != NULL) {
 		ret = -EBUSY;
 		goto out_unlock;
 	}
@@ -128,8 +138,12 @@ EXPORT_SYMBOL_GPL(nf_ct_expect_register_notifier);
 
 void nf_ct_expect_unregister_notifier(struct nf_exp_event_notifier *new)
 {
+	struct nf_exp_event_notifier *notify;
+
 	mutex_lock(&nf_ct_ecache_mutex);
-	BUG_ON(nf_expect_event_cb != new);
+	notify = rcu_dereference_protected(nf_expect_event_cb,
+					   lockdep_is_held(&nf_ct_ecache_mutex));
+	BUG_ON(notify != new);
 	rcu_assign_pointer(nf_expect_event_cb, NULL);
 	mutex_unlock(&nf_ct_ecache_mutex);
 }
diff --git a/net/netfilter/nf_log.c b/net/netfilter/nf_log.c
index 908f599..7df37fd 100644
--- a/net/netfilter/nf_log.c
+++ b/net/netfilter/nf_log.c
@@ -35,6 +35,7 @@ static struct nf_logger *__find_logger(int pf, const char *str_logger)
 /* return EEXIST if the same logger is registred, 0 on success. */
 int nf_log_register(u_int8_t pf, struct nf_logger *logger)
 {
+	const struct nf_logger *llog;
 	int i;
 
 	if (pf >= ARRAY_SIZE(nf_loggers))
@@ -51,7 +52,9 @@ int nf_log_register(u_int8_t pf, struct nf_logger *logger)
 	} else {
 		/* register at end of list to honor first register win */
 		list_add_tail(&logger->list[pf], &nf_loggers_l[pf]);
-		if (nf_loggers[pf] == NULL)
+		llog = rcu_dereference_protected(nf_loggers[pf],
+						 lockdep_is_held(&nf_log_mutex));
+		if (llog == NULL)
 			rcu_assign_pointer(nf_loggers[pf], logger);
 	}
 
@@ -63,11 +66,14 @@ EXPORT_SYMBOL(nf_log_register);
 
 void nf_log_unregister(struct nf_logger *logger)
 {
+	const struct nf_logger *c_logger;
 	int i;
 
 	mutex_lock(&nf_log_mutex);
 	for (i = 0; i < ARRAY_SIZE(nf_loggers); i++) {
-		if (nf_loggers[i] == logger)
+		c_logger = rcu_dereference_protected(nf_loggers[i],
+						     lockdep_is_held(&nf_log_mutex));
+		if (c_logger == logger)
 			rcu_assign_pointer(nf_loggers[i], NULL);
 		list_del(&logger->list[i]);
 	}