Patchwork nfnetlink: use rcu_assign_pointer in nfnetlink_subsys_unregister

login
register
mail settings
Submitter Eric Dumazet
Date Oct. 8, 2013, 3:23 p.m.
Message ID <1381245834.12191.37.camel@edumazet-glaptop.roam.corp.google.com>
Download mbox | patch
Permalink /patch/281496/
State Deferred
Headers show

Comments

Eric Dumazet - Oct. 8, 2013, 3:23 p.m.
On Tue, 2013-10-08 at 18:04 +0800, Gao feng wrote:
> Though I don't face an oops, but it is more safer to
> set table's subsys through rcu_assign_pointer.
> 
> Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com>
> ---
>  net/netfilter/nfnetlink.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/netfilter/nfnetlink.c b/net/netfilter/nfnetlink.c
> index 572d87d..3cd2fe6 100644
> --- a/net/netfilter/nfnetlink.c
> +++ b/net/netfilter/nfnetlink.c
> @@ -78,7 +78,7 @@ EXPORT_SYMBOL_GPL(nfnetlink_subsys_register);
>  int nfnetlink_subsys_unregister(const struct nfnetlink_subsystem *n)
>  {
>  	nfnl_lock(n->subsys_id);
> -	table[n->subsys_id].subsys = NULL;
> +	rcu_assign_pointer(table[n->subsys_id].subsys, NULL);
>  	nfnl_unlock(n->subsys_id);
>  	synchronize_rcu();
>  	return 0;

Certainly not.

Assigning a NULL pointer do not require rcu_assign_pointer() but this,
if you want to be really really clean.





--
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
Gao feng - Oct. 9, 2013, 3:28 a.m.
On 10/08/2013 11:23 PM, Eric Dumazet wrote:
> On Tue, 2013-10-08 at 18:04 +0800, Gao feng wrote:
>> Though I don't face an oops, but it is more safer to
>> set table's subsys through rcu_assign_pointer.
>>
>> Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com>
>> ---
>>  net/netfilter/nfnetlink.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/netfilter/nfnetlink.c b/net/netfilter/nfnetlink.c
>> index 572d87d..3cd2fe6 100644
>> --- a/net/netfilter/nfnetlink.c
>> +++ b/net/netfilter/nfnetlink.c
>> @@ -78,7 +78,7 @@ EXPORT_SYMBOL_GPL(nfnetlink_subsys_register);
>>  int nfnetlink_subsys_unregister(const struct nfnetlink_subsystem *n)
>>  {
>>  	nfnl_lock(n->subsys_id);
>> -	table[n->subsys_id].subsys = NULL;
>> +	rcu_assign_pointer(table[n->subsys_id].subsys, NULL);
>>  	nfnl_unlock(n->subsys_id);
>>  	synchronize_rcu();
>>  	return 0;
> 
> Certainly not.
> 
> Assigning a NULL pointer do not require rcu_assign_pointer() but this,
> if you want to be really really clean.
> 

The reason assigning a NULL pointer do not require rcu_assign_pointer
is it's impossible for a reader to access to uninitialized content?
So we don't need write barrier here to make sure the resource being
initialized before reader accessing it. Right?

> diff --git a/net/netfilter/nfnetlink.c b/net/netfilter/nfnetlink.c
> index 572d87d..649958b 100644
> --- a/net/netfilter/nfnetlink.c
> +++ b/net/netfilter/nfnetlink.c
> @@ -78,7 +78,7 @@ EXPORT_SYMBOL_GPL(nfnetlink_subsys_register);
>  int nfnetlink_subsys_unregister(const struct nfnetlink_subsystem *n)
>  {
>  	nfnl_lock(n->subsys_id);
> -	table[n->subsys_id].subsys = NULL;
> +	RCU_INIT_POINTER(table[n->subsys_id].subsys, NULL);
>  	nfnl_unlock(n->subsys_id);
>  	synchronize_rcu();
>  	return 0;
> 
> 
> 
> 
> 

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

diff --git a/net/netfilter/nfnetlink.c b/net/netfilter/nfnetlink.c
index 572d87d..649958b 100644
--- a/net/netfilter/nfnetlink.c
+++ b/net/netfilter/nfnetlink.c
@@ -78,7 +78,7 @@  EXPORT_SYMBOL_GPL(nfnetlink_subsys_register);
 int nfnetlink_subsys_unregister(const struct nfnetlink_subsystem *n)
 {
 	nfnl_lock(n->subsys_id);
-	table[n->subsys_id].subsys = NULL;
+	RCU_INIT_POINTER(table[n->subsys_id].subsys, NULL);
 	nfnl_unlock(n->subsys_id);
 	synchronize_rcu();
 	return 0;