[PATCH 0/5] We must use rcu_barrier() on module unload

[PATCH 0/5] We must use rcu_barrier() on module unload

[Jesper Dangaard Brouer]

If an unloadable module uses RCU callbacks, it need to use
rcu_barrier() so that the module may be safely unloaded.

While hacking on a netfilter module of my own, I learned the
importance of calling rcu_barrier() instead of only a
synchronize_rcu() on module unload (iif using any call_rcu()
callbacks).  synchronize_rcu() does wait for a grace period to
elapse, but it does not wait for the callbacks to complete.

For documentation see:
 http://lwn.net/Articles/217484/
 Documentation/RCU/rcubarrier.txt


Looking through the net/ directory for call_rcu() users and unloadable
modules I found 5 modules that didn't behave correctly:

 net/8021q/vlan.c

 net/can/af_can.c

 net/netfilter/nfnetlink_queue.c

 net/sctp/protocol.c

 net/sunrpc/auth_gss/auth_gss.c

I have made a patch for each individual module, so objections can be
made on a per module basis.  I have Cc'ed all of the patches to the
maintainers of each module (according to the MAINTAINERS file).

The patchset is made on top of DaveM's net-next-2.6 tree (starting on
top of commit a1c1db392090). As this is my usual development tree,
even though this patchset is bugfixes. (Just tested it applied cleanly
on Linus'es tree ...)

---
Jesper Dangaard Brouer (5):
      sunrpc/auth_gss: Call rcu_barrier() on module unload.
      sctp: protocol.c call rcu_barrier() on unload.
      can: af_can.c use rcu_barrier() on module unload.
      nfnetlink_queue: Use rcu_barrier() on module unload.
      8021q: Vlan driver should use rcu_barrier() on unload instead of syncronize_net()


 net/8021q/vlan.c                |    2 +-
 net/can/af_can.c                |    2 ++
 net/netfilter/nfnetlink_queue.c |    4 +++-
 net/sctp/protocol.c             |    2 ++
 net/sunrpc/auth_gss/auth_gss.c  |    1 +
 5 files changed, 9 insertions(+), 2 deletions(-)

--
Best regards,
  Jesper Brouer
  ComX Networks A/S
  Linux Network developer
  Cand. Scient Datalog / MSc.
  Author of http://adsl-optimizer.dk
  LinkedIn: http://www.linkedin.com/in/brouer

[PATCH 1/5] 8021q: Vlan driver should use rcu_barrier() on unload instead of syncronize_net()

[Jesper Dangaard Brouer]

The VLAN 8021q driver needs to call rcu_barrier() when unloading the module,
instead of syncronize_net().  This is needed to make sure that outstanding
call_rcu() callbacks have completed, before the callback function code is
removed on module unload.

Signed-off-by: Jesper Dangaard Brouer <hawk@comx.dk>
---

 net/8021q/vlan.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c
index 714e1c3..fe64908 100644
--- a/net/8021q/vlan.c
+++ b/net/8021q/vlan.c
@@ -758,7 +758,7 @@ static void __exit vlan_cleanup_module(void)
 		BUG_ON(!hlist_empty(&vlan_group_hash[i]));
 
 	unregister_pernet_gen_device(vlan_net_id, &vlan_net_ops);
-	synchronize_net();
+	rcu_barrier(); /* Wait for completion of call_rcu()'s */
 
 	vlan_gvrp_uninit();
 }

[PATCH 2/5] nfnetlink_queue: Use rcu_barrier() on module unload.

[Jesper Dangaard Brouer]

This module uses rcu_call() thus it should use rcu_barrier() on module unload.

Also fixed a trivial typo 'nfetlink' -> 'nfnetlink' in comment.

Signed-off-by: Jesper Dangaard Brouer <hawk@comx.dk>
---

 net/netfilter/nfnetlink_queue.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
index 8c86011..71daa09 100644
--- a/net/netfilter/nfnetlink_queue.c
+++ b/net/netfilter/nfnetlink_queue.c
@@ -1,6 +1,6 @@
 /*
  * This is a module which is used for queueing packets and communicating with
- * userspace via nfetlink.
+ * userspace via nfnetlink.
  *
  * (C) 2005 by Harald Welte <laforge@netfilter.org>
  * (C) 2007 by Patrick McHardy <kaber@trash.net>
@@ -932,6 +932,8 @@ static void __exit nfnetlink_queue_fini(void)
 #endif
 	nfnetlink_subsys_unregister(&nfqnl_subsys);
 	netlink_unregister_notifier(&nfqnl_rtnl_notifier);
+
+	rcu_barrier(); /* Wait for completion of call_rcu()'s */
 }
 
 MODULE_DESCRIPTION("netfilter packet queue handler");

[PATCH 3/5] can: af_can.c use rcu_barrier() on module unload.

[Jesper Dangaard Brouer]

This module uses rcu_call() thus it should use rcu_barrier()
on module unload.

Signed-off-by: Jesper Dangaard Brouer <hawk@comx.dk>
---

 net/can/af_can.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/net/can/af_can.c b/net/can/af_can.c
index 10f0528..e733725 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -903,6 +903,8 @@ static __exit void can_exit(void)
 	}
 	spin_unlock(&can_rcvlists_lock);
 
+	rcu_barrier(); /* Wait for completion of call_rcu()'s */
+
 	kmem_cache_destroy(rcv_cache);
 }
 

[PATCH 4/5] sctp: protocol.c call rcu_barrier() on unload.

[Jesper Dangaard Brouer]

On module unload call rcu_barrier(), this is needed as synchronize_rcu()
is not strong enough.  The kmem_cache_destroy() does invoke
synchronize_rcu() but it does not provide same protection.

Signed-off-by: Jesper Dangaard Brouer <hawk@comx.dk>
---

 net/sctp/protocol.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
index cb2c50d..79cbd47 100644
--- a/net/sctp/protocol.c
+++ b/net/sctp/protocol.c
@@ -1370,6 +1370,8 @@ SCTP_STATIC __exit void sctp_exit(void)
 	sctp_proc_exit();
 	cleanup_sctp_mibs();
 
+	rcu_barrier(); /* Wait for completion of call_rcu()'s */
+
 	kmem_cache_destroy(sctp_chunk_cachep);
 	kmem_cache_destroy(sctp_bucket_cachep);
 }

[PATCH 5/5] sunrpc/auth_gss: Call rcu_barrier() on module unload.

[Jesper Dangaard Brouer]

As the module uses rcu_call() we should make sure that all
rcu callback has been completed before removing the code.

Signed-off-by: Jesper Dangaard Brouer <hawk@comx.dk>
---

 net/sunrpc/auth_gss/auth_gss.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c
index e630b38..66d458f 100644
--- a/net/sunrpc/auth_gss/auth_gss.c
+++ b/net/sunrpc/auth_gss/auth_gss.c
@@ -1548,6 +1548,7 @@ static void __exit exit_rpcsec_gss(void)
 {
 	gss_svc_shutdown();
 	rpcauth_unregister(&authgss_ops);
+	rcu_barrier(); /* Wait for completion of call_rcu()'s */
 }
 
 MODULE_LICENSE("GPL");

Re: [PATCH 3/5] can: af_can.c use rcu_barrier() on module unload.

[Oliver Hartkopp]

Jesper Dangaard Brouer wrote:
> This module uses rcu_call() thus it should use rcu_barrier()
> on module unload.
> 
> Signed-off-by: Jesper Dangaard Brouer <hawk@comx.dk>

Thanks Jesper for pointing at this issue!

Acked-By: Oliver Hartkopp <oliver@hartkopp.net>

Btw. i do agree with theses patches to be a bug fix that should go into
2.6.30-rc8 as well as into the stable series.

Best regards,
Oliver


> ---
> 
>  net/can/af_can.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/net/can/af_can.c b/net/can/af_can.c
> index 10f0528..e733725 100644
> --- a/net/can/af_can.c
> +++ b/net/can/af_can.c
> @@ -903,6 +903,8 @@ static __exit void can_exit(void)
>  	}
>  	spin_unlock(&can_rcvlists_lock);
>  
> +	rcu_barrier(); /* Wait for completion of call_rcu()'s */
> +
>  	kmem_cache_destroy(rcv_cache);
>  }
>  
> 
> --
> 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 0/5] We must use rcu_barrier() on module unload

[Patrick McHardy]

Jesper Dangaard Brouer wrote:
> If an unloadable module uses RCU callbacks, it need to use
> rcu_barrier() so that the module may be safely unloaded.
> 
> While hacking on a netfilter module of my own, I learned the
> importance of calling rcu_barrier() instead of only a
> synchronize_rcu() on module unload (iif using any call_rcu()
> callbacks).  synchronize_rcu() does wait for a grace period to
> elapse, but it does not wait for the callbacks to complete.
> 
> ...
> I have made a patch for each individual module, so objections can be
> made on a per module basis.  I have Cc'ed all of the patches to the
> maintainers of each module (according to the MAINTAINERS file).

Acked-by: Patrick McHardy <kaber@trash.net> for patches 1 and 2, good
catch.

Re: [PATCH 1/5] 8021q: Vlan driver should use rcu_barrier() on unload instead of syncronize_net()

[Paul E. McKenney]

On Mon, Jun 08, 2009 at 03:11:28PM +0200, Jesper Dangaard Brouer wrote:
> The VLAN 8021q driver needs to call rcu_barrier() when unloading the module,
> instead of syncronize_net().  This is needed to make sure that outstanding
> call_rcu() callbacks have completed, before the callback function code is
> removed on module unload.

Looks good!  And thank you for checking up on this!!!

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

> Signed-off-by: Jesper Dangaard Brouer <hawk@comx.dk>
> ---
> 
>  net/8021q/vlan.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c
> index 714e1c3..fe64908 100644
> --- a/net/8021q/vlan.c
> +++ b/net/8021q/vlan.c
> @@ -758,7 +758,7 @@ static void __exit vlan_cleanup_module(void)
>  		BUG_ON(!hlist_empty(&vlan_group_hash[i]));
> 
>  	unregister_pernet_gen_device(vlan_net_id, &vlan_net_ops);
> -	synchronize_net();
> +	rcu_barrier(); /* Wait for completion of call_rcu()'s */
> 
>  	vlan_gvrp_uninit();
>  }
> 

Re: [PATCH 2/5] nfnetlink_queue: Use rcu_barrier() on module unload.

[Paul E. McKenney]

On Mon, Jun 08, 2009 at 03:11:33PM +0200, Jesper Dangaard Brouer wrote:
> This module uses rcu_call() thus it should use rcu_barrier() on module unload.
> 
> Also fixed a trivial typo 'nfetlink' -> 'nfnetlink' in comment.

Assuming that netlink_unregister_notifier(), nfnetlink_subsys_unregister(),
and so on prevent any subsequent calls to call_rcu(), looks good!!!

Acked-by: Paul E. McKenney <paulmck-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>

> Signed-off-by: Jesper Dangaard Brouer <hawk-4UpuNZONu4c@public.gmane.org>
> ---
> 
>  net/netfilter/nfnetlink_queue.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
> index 8c86011..71daa09 100644
> --- a/net/netfilter/nfnetlink_queue.c
> +++ b/net/netfilter/nfnetlink_queue.c
> @@ -1,6 +1,6 @@
>  /*
>   * This is a module which is used for queueing packets and communicating with
> - * userspace via nfetlink.
> + * userspace via nfnetlink.
>   *
>   * (C) 2005 by Harald Welte <laforge-Cap9r6Oaw4JrovVCs/uTlw@public.gmane.org>
>   * (C) 2007 by Patrick McHardy <kaber-dcUjhNyLwpNeoWH0uzbU5w@public.gmane.org>
> @@ -932,6 +932,8 @@ static void __exit nfnetlink_queue_fini(void)
>  #endif
>  	nfnetlink_subsys_unregister(&nfqnl_subsys);
>  	netlink_unregister_notifier(&nfqnl_rtnl_notifier);
> +
> +	rcu_barrier(); /* Wait for completion of call_rcu()'s */
>  }
> 
>  MODULE_DESCRIPTION("netfilter packet queue handler");
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/5] can: af_can.c use rcu_barrier() on module unload.

[Paul E. McKenney]

On Mon, Jun 08, 2009 at 03:11:38PM +0200, Jesper Dangaard Brouer wrote:
> This module uses rcu_call() thus it should use rcu_barrier()
> on module unload.

This does appear to make things better!!!

However, I don't understand why it is safe to do the following in
can_exit():

	hlist_for_each_entry_safe(d, n, next, &can_rx_dev_list, list) {
		hlist_del(&d->list);
		kfree(d);
	}

Given that this list is scanned by RCU readers, shouldn't this kfree()
be something like "call_rcu(&d->rcu, can_rx_delete_device);"?

Also, what frees up the "struct receiver" structures?

							Thanx, Paul

> Signed-off-by: Jesper Dangaard Brouer <hawk@comx.dk>
> ---
> 
>  net/can/af_can.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/net/can/af_can.c b/net/can/af_can.c
> index 10f0528..e733725 100644
> --- a/net/can/af_can.c
> +++ b/net/can/af_can.c
> @@ -903,6 +903,8 @@ static __exit void can_exit(void)
>  	}
>  	spin_unlock(&can_rcvlists_lock);
> 
> +	rcu_barrier(); /* Wait for completion of call_rcu()'s */
> +
>  	kmem_cache_destroy(rcv_cache);
>  }
> 
> 

Re: [PATCH 4/5] sctp: protocol.c call rcu_barrier() on unload.

[Paul E. McKenney]

On Mon, Jun 08, 2009 at 03:11:43PM +0200, Jesper Dangaard Brouer wrote:
> On module unload call rcu_barrier(), this is needed as synchronize_rcu()
> is not strong enough.  The kmem_cache_destroy() does invoke
> synchronize_rcu() but it does not provide same protection.

Good, looks like sctp_v4_del_protocol() invokes call_rcu(), which the
rcu_barrier() would then wait for.  And it looks like sctp_v6_del_protocol()
does the same for IPv6.

Reviewed_by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

> Signed-off-by: Jesper Dangaard Brouer <hawk@comx.dk>
> ---
> 
>  net/sctp/protocol.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
> index cb2c50d..79cbd47 100644
> --- a/net/sctp/protocol.c
> +++ b/net/sctp/protocol.c
> @@ -1370,6 +1370,8 @@ SCTP_STATIC __exit void sctp_exit(void)
>  	sctp_proc_exit();
>  	cleanup_sctp_mibs();
> 
> +	rcu_barrier(); /* Wait for completion of call_rcu()'s */
> +
>  	kmem_cache_destroy(sctp_chunk_cachep);
>  	kmem_cache_destroy(sctp_bucket_cachep);
>  }
> 

Re: [PATCH 5/5] sunrpc/auth_gss: Call rcu_barrier() on module unload.

[Paul E. McKenney]

On Mon, Jun 08, 2009 at 03:11:48PM +0200, Jesper Dangaard Brouer wrote:
> As the module uses rcu_call() we should make sure that all
> rcu callback has been completed before removing the code.

Good improvement!!!  Assuming that gss_svc_shutdown() and
rpcauth_unregister() prevent any future RCU callbacks to be registered,
this patch will fix things up.

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

> Signed-off-by: Jesper Dangaard Brouer <hawk@comx.dk>
> ---
> 
>  net/sunrpc/auth_gss/auth_gss.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c
> index e630b38..66d458f 100644
> --- a/net/sunrpc/auth_gss/auth_gss.c
> +++ b/net/sunrpc/auth_gss/auth_gss.c
> @@ -1548,6 +1548,7 @@ static void __exit exit_rpcsec_gss(void)
>  {
>  	gss_svc_shutdown();
>  	rpcauth_unregister(&authgss_ops);
> +	rcu_barrier(); /* Wait for completion of call_rcu()'s */
>  }
> 
>  MODULE_LICENSE("GPL");
> 

Re: [PATCH 3/5] can: af_can.c use rcu_barrier() on module unload.

[Oliver Hartkopp]

Paul E. McKenney wrote:
> On Mon, Jun 08, 2009 at 03:11:38PM +0200, Jesper Dangaard Brouer wrote:
>> This module uses rcu_call() thus it should use rcu_barrier()
>> on module unload.
> 
> This does appear to make things better!!!
> 
> However, I don't understand why it is safe to do the following in
> can_exit():
> 
> 	hlist_for_each_entry_safe(d, n, next, &can_rx_dev_list, list) {
> 		hlist_del(&d->list);
> 		kfree(d);
> 	}
> 
> Given that this list is scanned by RCU readers, shouldn't this kfree()
> be something like "call_rcu(&d->rcu, can_rx_delete_device);"?
> 
> Also, what frees up the "struct receiver" structures?

Hi Paul,

af_can.c only provides an infrastructure for PF_CAN modules like can-raw.ko,
can-bcm.ko or can-isotp.ko.

Please take a look into can_notifier() in net/can/af_can.c and raw_notifier()
in net/can/raw.c:

The receivers are removed when the appropriate socket is closed that created
the belonging receivers. And you can not remove can.ko (af_can.c) when another
PF_CAN protocol like can-raw.ko is using it.

So when a netdev notifier removes the interface both the PF_CAN protocol (e.g.
can-raw.ko) and the PF_CAN core (af_can.c) cleans up all receivers and finally
removes the per-interface structure dev_rcv_lists (e.g. for can0).

In can_exit() all the dev_rcv_lists for ARPHRD_CAN interfaces are removed that
had been created by NETDEV_REGISTER notifier and are unused by any of the
PF_CAN protocols and therefore without any receivers attached to them.

The list is protected by spin_lock(&can_rcvlists_lock) - which is probably not
even needed in this particular case - and there is no PF_CAN protocol
registered at this time. So it's really save to remove the empty dev_rcv_lists
structs here that do not link to any receivers.

Puh - much text. But i hope it clarifies it.

Thinking about the rcu stuff again, rcu_barrier() still makes sense when you
are unloading the module chain of can-raw.ko and can.ko very fast.

Regards,
Oliver


>> Signed-off-by: Jesper Dangaard Brouer <hawk@comx.dk>
>> ---
>>
>>  net/can/af_can.c |    2 ++
>>  1 files changed, 2 insertions(+), 0 deletions(-)
>>
>> diff --git a/net/can/af_can.c b/net/can/af_can.c
>> index 10f0528..e733725 100644
>> --- a/net/can/af_can.c
>> +++ b/net/can/af_can.c
>> @@ -903,6 +903,8 @@ static __exit void can_exit(void)
>>  	}
>>  	spin_unlock(&can_rcvlists_lock);
>>
>> +	rcu_barrier(); /* Wait for completion of call_rcu()'s */
>> +
>>  	kmem_cache_destroy(rcv_cache);
>>  }

Re: [PATCH 5/5] sunrpc/auth_gss: Call rcu_barrier() on module unload.

[Trond Myklebust]

On Mon, 2009-06-08 at 09:26 -0700, Paul E. McKenney wrote:
> On Mon, Jun 08, 2009 at 03:11:48PM +0200, Jesper Dangaard Brouer wrote:
> > As the module uses rcu_call() we should make sure that all
> > rcu callback has been completed before removing the code.
> 
> Good improvement!!!  Assuming that gss_svc_shutdown() and
> rpcauth_unregister() prevent any future RCU callbacks to be registered,
> this patch will fix things up.
> 
> Acked-by: Paul E. McKenney <paulmck-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> 
> > Signed-off-by: Jesper Dangaard Brouer <hawk-4UpuNZONu4c@public.gmane.org>
> > ---
> > 
> >  net/sunrpc/auth_gss/auth_gss.c |    1 +
> >  1 files changed, 1 insertions(+), 0 deletions(-)
> > 
> > diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c
> > index e630b38..66d458f 100644
> > --- a/net/sunrpc/auth_gss/auth_gss.c
> > +++ b/net/sunrpc/auth_gss/auth_gss.c
> > @@ -1548,6 +1548,7 @@ static void __exit exit_rpcsec_gss(void)
> >  {
> >  	gss_svc_shutdown();
> >  	rpcauth_unregister(&authgss_ops);
> > +	rcu_barrier(); /* Wait for completion of call_rcu()'s */
> >  }
> > 
> >  MODULE_LICENSE("GPL");
> > 

Hmm... If this is about ensuring that all the call_rcu() callbacks
complete before we remove the module, then don't we also need similar
barriers in net/sunrpc/sunrpc_syms.c:cleanup_sunrpc() and in
fs/nfs/inode.c:exit_nfs_fs()?

Cheers
  Trond
-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org
www.netapp.com
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/5] sctp: protocol.c call rcu_barrier() on unload.

[Vlad Yasevich]

Paul E. McKenney wrote:
> On Mon, Jun 08, 2009 at 03:11:43PM +0200, Jesper Dangaard Brouer wrote:
>> On module unload call rcu_barrier(), this is needed as synchronize_rcu()
>> is not strong enough.  The kmem_cache_destroy() does invoke
>> synchronize_rcu() but it does not provide same protection.
> 
> Good, looks like sctp_v4_del_protocol() invokes call_rcu(), which the
> rcu_barrier() would then wait for.  And it looks like sctp_v6_del_protocol()
> does the same for IPv6.
> 
> Reviewed_by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> 
>> Signed-off-by: Jesper Dangaard Brouer <hawk@comx.dk>
>> ---
>>
>>  net/sctp/protocol.c |    2 ++
>>  1 files changed, 2 insertions(+), 0 deletions(-)
>>
>> diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
>> index cb2c50d..79cbd47 100644
>> --- a/net/sctp/protocol.c
>> +++ b/net/sctp/protocol.c
>> @@ -1370,6 +1370,8 @@ SCTP_STATIC __exit void sctp_exit(void)
>>  	sctp_proc_exit();
>>  	cleanup_sctp_mibs();
>>
>> +	rcu_barrier(); /* Wait for completion of call_rcu()'s */
>> +
>>  	kmem_cache_destroy(sctp_chunk_cachep);
>>  	kmem_cache_destroy(sctp_bucket_cachep);
>>  }
>>

Shouldn't the rcu_barrier call be before sctp_free_local_addr_list()?

-vlad

Re: [PATCH 4/5] sctp: protocol.c call rcu_barrier() on unload.

[Paul E. McKenney]

On Tue, Jun 09, 2009 at 11:44:23AM -0400, Vlad Yasevich wrote:
> Paul E. McKenney wrote:
> > On Mon, Jun 08, 2009 at 03:11:43PM +0200, Jesper Dangaard Brouer wrote:
> >> On module unload call rcu_barrier(), this is needed as synchronize_rcu()
> >> is not strong enough.  The kmem_cache_destroy() does invoke
> >> synchronize_rcu() but it does not provide same protection.
> > 
> > Good, looks like sctp_v4_del_protocol() invokes call_rcu(), which the
> > rcu_barrier() would then wait for.  And it looks like sctp_v6_del_protocol()
> > does the same for IPv6.
> > 
> > Reviewed_by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > 
> >> Signed-off-by: Jesper Dangaard Brouer <hawk@comx.dk>
> >> ---
> >>
> >>  net/sctp/protocol.c |    2 ++
> >>  1 files changed, 2 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
> >> index cb2c50d..79cbd47 100644
> >> --- a/net/sctp/protocol.c
> >> +++ b/net/sctp/protocol.c
> >> @@ -1370,6 +1370,8 @@ SCTP_STATIC __exit void sctp_exit(void)
> >>  	sctp_proc_exit();
> >>  	cleanup_sctp_mibs();
> >>
> >> +	rcu_barrier(); /* Wait for completion of call_rcu()'s */
> >> +
> >>  	kmem_cache_destroy(sctp_chunk_cachep);
> >>  	kmem_cache_destroy(sctp_bucket_cachep);
> >>  }
> 
> Shouldn't the rcu_barrier call be before sctp_free_local_addr_list()?

Hmmm...  What sequence of events would lead to a failure if
rcu_barrier() is after sctp_free_local_addr_list()?

							Thanx, Paul

Re: [PATCH 4/5] sctp: protocol.c call rcu_barrier() on unload.

[Vlad Yasevich]

Paul E. McKenney wrote:
> On Tue, Jun 09, 2009 at 11:44:23AM -0400, Vlad Yasevich wrote:
>> Paul E. McKenney wrote:
>>> On Mon, Jun 08, 2009 at 03:11:43PM +0200, Jesper Dangaard Brouer wrote:
>>>> On module unload call rcu_barrier(), this is needed as synchronize_rcu()
>>>> is not strong enough.  The kmem_cache_destroy() does invoke
>>>> synchronize_rcu() but it does not provide same protection.
>>> Good, looks like sctp_v4_del_protocol() invokes call_rcu(), which the
>>> rcu_barrier() would then wait for.  And it looks like sctp_v6_del_protocol()
>>> does the same for IPv6.
>>>
>>> Reviewed_by: Paul E. McKenney <paulmck-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
>>>
>>>> Signed-off-by: Jesper Dangaard Brouer <hawk-4UpuNZONu4c@public.gmane.org>
>>>> ---
>>>>
>>>>  net/sctp/protocol.c |    2 ++
>>>>  1 files changed, 2 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
>>>> index cb2c50d..79cbd47 100644
>>>> --- a/net/sctp/protocol.c
>>>> +++ b/net/sctp/protocol.c
>>>> @@ -1370,6 +1370,8 @@ SCTP_STATIC __exit void sctp_exit(void)
>>>>  	sctp_proc_exit();
>>>>  	cleanup_sctp_mibs();
>>>>
>>>> +	rcu_barrier(); /* Wait for completion of call_rcu()'s */
>>>> +
>>>>  	kmem_cache_destroy(sctp_chunk_cachep);
>>>>  	kmem_cache_destroy(sctp_bucket_cachep);
>>>>  }
>> Shouldn't the rcu_barrier call be before sctp_free_local_addr_list()?
> 
> Hmmm...  What sequence of events would lead to a failure if
> rcu_barrier() is after sctp_free_local_addr_list()?
> 
> 							Thanx, Paul
> 

I thought that the notifier could could potentially execute at the
same time as the unregister(), but I see that's protected.  So, I guess
it doesn't really matter then where the barrier is.

Acked-by: Vlad Yasevich <vladislav.yasevich-VXdhtT5mjnY@public.gmane.org>

-vlad
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5] nfnetlink_queue: Use rcu_barrier() on module unload.

[Andrew Morton]

On Mon, 08 Jun 2009 15:11:33 +0200 Jesper Dangaard Brouer <hawk@comx.dk> wrote:

> This module uses rcu_call() thus it should use rcu_barrier() on module unload.
> 
> Also fixed a trivial typo 'nfetlink' -> 'nfnetlink' in comment.
> 
> Signed-off-by: Jesper Dangaard Brouer <hawk@comx.dk>
> ---
> 
>  net/netfilter/nfnetlink_queue.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
> index 8c86011..71daa09 100644
> --- a/net/netfilter/nfnetlink_queue.c
> +++ b/net/netfilter/nfnetlink_queue.c
> @@ -1,6 +1,6 @@
>  /*
>   * This is a module which is used for queueing packets and communicating with
> - * userspace via nfetlink.
> + * userspace via nfnetlink.
>   *
>   * (C) 2005 by Harald Welte <laforge@netfilter.org>
>   * (C) 2007 by Patrick McHardy <kaber@trash.net>
> @@ -932,6 +932,8 @@ static void __exit nfnetlink_queue_fini(void)
>  #endif
>  	nfnetlink_subsys_unregister(&nfqnl_subsys);
>  	netlink_unregister_notifier(&nfqnl_rtnl_notifier);
> +
> +	rcu_barrier(); /* Wait for completion of call_rcu()'s */
>  }
>  
>  MODULE_DESCRIPTION("netfilter packet queue handler");

Possibly you've fixed the bug which the module_put(THIS_MODULE) in
instance_destroy_rcu() is addressing.

Do we still need to take a ref against the module for each instance
once the above fix is in place?

<goes git mining>

Nope, the THIS_MODULE games have been there since day one, and I can't
work out why they're there.  net/netfilter/nfnetlink_log.c has them
too.

Re: [PATCH 3/5] can: af_can.c use rcu_barrier() on module unload.

[David Miller]

From: Oliver Hartkopp <oliver@hartkopp.net>
Date: Mon, 08 Jun 2009 15:24:58 +0200

> Acked-By: Oliver Hartkopp <oliver@hartkopp.net>

Please don't capitalize the "By" in "Acked-By".  Otherwise
patchwork doesn't pick them up automatically.

It's just like "Signed-off-by", only the first word is capitalized.

Thank you.

Re: [PATCH 0/5] We must use rcu_barrier() on module unload

[David Miller]

From: Jesper Dangaard Brouer <hawk-4UpuNZONu4c@public.gmane.org>
Date: Mon, 08 Jun 2009 15:11:23 +0200

> If an unloadable module uses RCU callbacks, it need to use
> rcu_barrier() so that the module may be safely unloaded.
 ...
> The patchset is made on top of DaveM's net-next-2.6 tree (starting on
> top of commit a1c1db392090). As this is my usual development tree,
> even though this patchset is bugfixes. (Just tested it applied cleanly
> on Linus'es tree ...)

All 5 patches applied, thanks!
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/5] can: af_can.c use rcu_barrier() on module unload.

[Alan Cox]

On Wed, 10 Jun 2009 01:10:27 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: Oliver Hartkopp <oliver@hartkopp.net>
> Date: Mon, 08 Jun 2009 15:24:58 +0200
> 
> > Acked-By: Oliver Hartkopp <oliver@hartkopp.net>
> 
> Please don't capitalize the "By" in "Acked-By".  Otherwise
> patchwork doesn't pick them up automatically.

Dave. We have computers to do all the silly tedious pointless jobs in
life, please just fix the scripts.

Alan

Re: [PATCH 3/5] can: af_can.c use rcu_barrier() on module unload.

[Jan Engelhardt]

On Wednesday 2009-06-10 10:10, David Miller wrote:
>> Acked-By: Oliver Hartkopp <oliver@hartkopp.net>
>
>Please don't capitalize the "By" in "Acked-By".  Otherwise
>patchwork doesn't pick them up automatically.

Is not that a patchwork bug then that should be reported? Or a
missing feature to checkpatch? Do we need a CodingStyle for
SOBs now too, just for that?

(Personally I prefer lowercase after the dash too, but I wanted to
illuminate all sides of the box.)