[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 5/5] sunrpc/auth_gss: Call rcu_barrier() on module unload.

[Jesper Dangaard Brouer]

(trimmed Cc)

On Mon, 8 Jun 2009, Trond Myklebust wrote:

> 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

Well, Trond that was a hard question, as I don't know this code... but if 
it uses call_rcu() callbacks its most likely.

I have not done a complete sweep of the tree, I have only looked at the 
netdev parts, as this is where I usually snoop around.  So there is 
probably plenty of work for some kernel-janitors (Cc'ing the list ;-))

> net/sunrpc/sunrpc_syms.c:cleanup_sunrpc()

Looking at the code that end up in sunrpc.ko, I see that both 
net/sunrpc/auth_unix.c and net/sunrpc/auth_generic.c uses call_rcu() 
callbacks.

> and in fs/nfs/inode.c:exit_nfs_fs()?

Looking at the code which end up into nfs.ko (which includes 
fs/nfs/inode.c) I see that the fs/nfs/delegation.c uses call_rcu() 
callbacks.

But its hard for me to figure out how this code interacts.  Don't know if 
we need to document a code path that can occur fast enough, before we add 
this safe guard?  It should be Paul's saying...

Cheers,
   Jesper Brouer

--
-------------------------------------------------------------------
MSc. Master of Computer Science
Dept. of Computer Science, University of Copenhagen
Author of http://www.adsl-optimizer.dk
-------------------------------------------------------------------
--
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 5/5] sunrpc/auth_gss: Call rcu_barrier() on module unload.

[Paul E. McKenney]

On Mon, Jun 08, 2009 at 09:48:54PM +0200, Jesper Dangaard Brouer wrote:
>
> (trimmed Cc)
>
> On Mon, 8 Jun 2009, Trond Myklebust wrote:
>
>> 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
>
> Well, Trond that was a hard question, as I don't know this code... but if 
> it uses call_rcu() callbacks its most likely.
>
> I have not done a complete sweep of the tree, I have only looked at the 
> netdev parts, as this is where I usually snoop around.  So there is 
> probably plenty of work for some kernel-janitors (Cc'ing the list ;-))
>
>> net/sunrpc/sunrpc_syms.c:cleanup_sunrpc()
>
> Looking at the code that end up in sunrpc.ko, I see that both 
> net/sunrpc/auth_unix.c and net/sunrpc/auth_generic.c uses call_rcu() 
> callbacks.
>
>> and in fs/nfs/inode.c:exit_nfs_fs()?
>
> Looking at the code which end up into nfs.ko (which includes 
> fs/nfs/inode.c) I see that the fs/nfs/delegation.c uses call_rcu() 
> callbacks.
>
> But its hard for me to figure out how this code interacts.  Don't know if 
> we need to document a code path that can occur fast enough, before we add 
> this safe guard?  It should be Paul's saying...

Unless there is some other mechanism to ensure that all the RCU
callbacks have been invoked before the module exit, there needs to be
code in the module-exit function that does the following:

o	Prevent any new RCU callbacks from being posted.  In other
	words, make sure that no future call_rcu() invocations happen
	from this module -unless- those call_rcu() invocations touch
	only functions and data that outlive this module.

o	Invoke rcu_barrier().

o	Of course, if the module uses call_rcu_sched() instead of
	call_rcu(), then it should invoke rcu_barrier_sched() instead
	of rcu_barrier().  Similarly, if it uses call_rcu_bh() instead
	of call_rcu(), then it should invoke rcu_barrier_bh() instead of
	rcu_barrier().	If the module uses more than one of call_rcu(),
	call_rcu_sched(), and call_rcu_bh(), then it must invoke more than
	one of rcu_barrier(), rcu_barrier_sched(), and rcu_barrier_bh().

What other mechanism could be used?  I cannot think of one that it safe.
For example, a module that tried to count the number of RCU callbacks in
flight would be vulnerable to races as follows:

1.	CPU 0: RCU callback decrements the counter.

2.	CPU 1: module-exit function notices that the counter is zero,
	so removes the module.

3.	CPU 0: attempts to execute the code returning from the RCU
	callback, and dies horribly due to that code no longer being
	in memory.

If there was an easy solution (or even a hard solution) to this problem,
then I do not believe that Nikita Danilov would have asked Dipankar Sarma
for rcu_barrier().  Therefore, I do not expect anyone to be able to come
up with an alternative to rcu_barrier() and friends.  Always happy to
learn something by being proven wrong, of course!!!

So unless someone can show me some other safe mechanism, every unloadable
module that uses call_rcu(), call_rcu_sched(), or call_rcu_bh() must use
rcu_barrier(), rcu_barrier_sched(), and/or rcu_barrier_bh() in its
module-exit function.

							Thanx, Paul

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.

[Oliver Hartkopp]

David Miller 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".

Sorry.

> Otherwise
> patchwork doesn't pick them up automatically.

IMHO patchwork should be robust against these type of typos and convert them
for the users (== your) convenience into 'Acked-by:' ...

Regards,
Oliver

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

[David Miller]

From: Oliver Hartkopp <oliver@hartkopp.net>
Date: Wed, 10 Jun 2009 10:22:10 +0200

> David Miller wrote:
>> Otherwise
>> patchwork doesn't pick them up automatically.
> 
> IMHO patchwork should be robust against these type of typos and convert them
> for the users (== your) convenience into 'Acked-by:' ...

We really can't expect patchwork to look for every conceivable
malignment of the various reviewer tags.

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.)

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

[Jeremy Kerr]

David,

> We really can't expect patchwork to look for every conceivable
> malignment of the various reviewer tags.

No, but we could probably be more tolerant about capitalisation. Any 
objections about ignoring case completely? I have a patch ready to 
push...

Cheers,


Jeremy

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

[David Miller]

From: Jeremy Kerr <jk@ozlabs.org>
Date: Wed, 10 Jun 2009 22:41:47 +1000

> David,
> 
>> We really can't expect patchwork to look for every conceivable
>> malignment of the various reviewer tags.
> 
> No, but we could probably be more tolerant about capitalisation. Any 
> objections about ignoring case completely? I have a patch ready to 
> push...

Sure, sounds fine to me.