[PATCH net-next] neigh: Add missing rcu_assign_pointer

[PATCH net-next] neigh: Add missing rcu_assign_pointer

[Ying Xue]

Commit e4c4e448cf55 ("neigh: Convert garbage collection from softirq
to workqueue") misses to use rcu_assign_pointer() macro to assign a
RCU-protected pointer.

Signed-off-by: Ying Xue <ying.xue@windriver.com>
---
 net/core/neighbour.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index 3a74df7..aaad3a5 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -783,7 +783,8 @@ static void neigh_periodic_work(struct work_struct *work)
 			if (atomic_read(&n->refcnt) == 1 &&
 			    (state == NUD_FAILED ||
 			     time_after(jiffies, n->used + NEIGH_VAR(n->parms, GC_STALETIME)))) {
-				*np = n->next;
+				rcu_assign_pointer(*np, rcu_dereference_protected(n->next,
+								lockdep_is_held(&tbl->lock)));
 				n->dead = 1;
 				write_unlock(&n->lock);
 				neigh_cleanup_and_release(n);
-- 
1.7.9.5

Re: [PATCH net-next] neigh: Add missing rcu_assign_pointer

[Eric Dumazet]

On Thu, 2015-05-28 at 16:28 +0800, Ying Xue wrote:
> Commit e4c4e448cf55 ("neigh: Convert garbage collection from softirq
> to workqueue") misses to use rcu_assign_pointer() macro to assign a
> RCU-protected pointer.
> 
> Signed-off-by: Ying Xue <ying.xue@windriver.com>
> ---
>  net/core/neighbour.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> index 3a74df7..aaad3a5 100644
> --- a/net/core/neighbour.c
> +++ b/net/core/neighbour.c
> @@ -783,7 +783,8 @@ static void neigh_periodic_work(struct work_struct *work)
>  			if (atomic_read(&n->refcnt) == 1 &&
>  			    (state == NUD_FAILED ||
>  			     time_after(jiffies, n->used + NEIGH_VAR(n->parms, GC_STALETIME)))) {
> -				*np = n->next;
> +				rcu_assign_pointer(*np, rcu_dereference_protected(n->next,
> +								lockdep_is_held(&tbl->lock)));
>  				n->dead = 1;
>  				write_unlock(&n->lock);
>  				neigh_cleanup_and_release(n);


This patch is not needed.

You really should read Documentation/RCU , because it looks like you are
quite confused.

When we remove an element from a RCU protected list, all the objects in
the chain are already ready to be caught by rcu readers.

Therefore, no additional memory barrier is needed before doing *np =
n->next;

Please do not add spurious memory barriers. Like atomic operations, we
want all of them being required and possibly documented.

Re: [PATCH net-next] neigh: Add missing rcu_assign_pointer

[Herbert Xu]

Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
> This patch is not needed.
> 
> You really should read Documentation/RCU , because it looks like you are
> quite confused.
> 
> When we remove an element from a RCU protected list, all the objects in
> the chain are already ready to be caught by rcu readers.
> 
> Therefore, no additional memory barrier is needed before doing *np =
> n->next;
> 
> Please do not add spurious memory barriers. Like atomic operations, we
> want all of them being required and possibly documented.

This patch is indeed bogus but accessing an RCU-protected like
this will trigger sparse warnings.  So better make it an
RCU_INIT_POINTER.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH net-next] neigh: Add missing rcu_assign_pointer

[Eric Dumazet]

On Thu, 2015-05-28 at 21:50 +0800, Herbert Xu wrote:

> This patch is indeed bogus but accessing an RCU-protected like
> this will trigger sparse warnings.  So better make it an
> RCU_INIT_POINTER.


A = B;  is perfectly fine since both A and B have the same __rcu
attribute.

Sparse has no warning and should not.

root@edumazet-glaptop2:/usr/src/net# grep CONFIG_SPARSE_RCU_POINTER .config
CONFIG_SPARSE_RCU_POINTER=y
root@edumazet-glaptop2:/usr/src/net# make C=2 CF=-D__CHECK_ENDIAN__ net/core/neighbour.o
...
  CHECK   net/core/neighbour.c

Re: [PATCH net-next] neigh: Add missing rcu_assign_pointer

[Herbert Xu]

On Thu, May 28, 2015 at 07:38:21AM -0700, Eric Dumazet wrote:
>
> A = B;  is perfectly fine since both A and B have the same __rcu
> attribute.
> 
> Sparse has no warning and should not.
> 
> root@edumazet-glaptop2:/usr/src/net# grep CONFIG_SPARSE_RCU_POINTER .config
> CONFIG_SPARSE_RCU_POINTER=y
> root@edumazet-glaptop2:/usr/src/net# make C=2 CF=-D__CHECK_ENDIAN__ net/core/neighbour.o
> ...
>   CHECK   net/core/neighbour.c

Indeed.  Thanks for pointing this out.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH net-next] neigh: Add missing rcu_assign_pointer

[Ying Xue]

On 05/28/2015 06:13 PM, Eric Dumazet wrote:
> This patch is not needed.
> 
> You really should read Documentation/RCU , because it looks like you are
> quite confused.
> 
> When we remove an element from a RCU protected list, all the objects in
> the chain are already ready to be caught by rcu readers.
> 
> Therefore, no additional memory barrier is needed before doing *np =
> n->next;
> 
> Please do not add spurious memory barriers. Like atomic operations, we
> want all of them being required and possibly documented.


Yes, you are right, thanks for your clear explanation :)
However, there are still three places where we use rcu_assign_pointer() to
remove a neigh entry from a RCU-protected list, and the three places are
neigh_forced_gc(), neigh_flush_dev(), and __neigh_for_each_release()
respectively. This means it's redundant for us to use rcu_assign_pointer() in
the three places, right?

Regards,
Ying

Re: [PATCH net-next] neigh: Add missing rcu_assign_pointer

[Eric Dumazet]

On Fri, 2015-05-29 at 09:21 +0800, Ying Xue wrote:
> On 05/28/2015 06:13 PM, Eric Dumazet wrote:
> > This patch is not needed.
> > 
> > You really should read Documentation/RCU , because it looks like you are
> > quite confused.
> > 
> > When we remove an element from a RCU protected list, all the objects in
> > the chain are already ready to be caught by rcu readers.
> > 
> > Therefore, no additional memory barrier is needed before doing *np =
> > n->next;
> > 
> > Please do not add spurious memory barriers. Like atomic operations, we
> > want all of them being required and possibly documented.
> 
> 
> Yes, you are right, thanks for your clear explanation :)
> However, there are still three places where we use rcu_assign_pointer() to
> remove a neigh entry from a RCU-protected list, and the three places are
> neigh_forced_gc(), neigh_flush_dev(), and __neigh_for_each_release()
> respectively. This means it's redundant for us to use rcu_assign_pointer() in
> the three places, right?

I count 5 places of redundancy. 

diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index 3a74df750af4044eba0e7d88ae01ca9b4dac0e72..ac3b69183cc982e722d9683d6de7a39f66b50b64 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -141,9 +141,7 @@ static int neigh_forced_gc(struct neigh_table *tbl)
 			write_lock(&n->lock);
 			if (atomic_read(&n->refcnt) == 1 &&
 			    !(n->nud_state & NUD_PERMANENT)) {
-				rcu_assign_pointer(*np,
-					rcu_dereference_protected(n->next,
-						  lockdep_is_held(&tbl->lock)));
+				*np = n->next;
 				n->dead = 1;
 				shrunk	= 1;
 				write_unlock(&n->lock);
@@ -210,9 +208,7 @@ static void neigh_flush_dev(struct neigh_table *tbl, struct net_device *dev)
 				np = &n->next;
 				continue;
 			}
-			rcu_assign_pointer(*np,
-				   rcu_dereference_protected(n->next,
-						lockdep_is_held(&tbl->lock)));
+			*np = n->next;
 			write_lock(&n->lock);
 			neigh_del_timer(n);
 			n->dead = 1;
@@ -380,10 +376,8 @@ static struct neigh_hash_table *neigh_hash_grow(struct neigh_table *tbl,
 			next = rcu_dereference_protected(n->next,
 						lockdep_is_held(&tbl->lock));
 
-			rcu_assign_pointer(n->next,
-					   rcu_dereference_protected(
-						new_nht->hash_buckets[hash],
-						lockdep_is_held(&tbl->lock)));
+			n->next = new_nht->hash_buckets[hash];
+
 			rcu_assign_pointer(new_nht->hash_buckets[hash], n);
 		}
 	}
@@ -515,9 +509,7 @@ struct neighbour *__neigh_create(struct neigh_table *tbl, const void *pkey,
 	n->dead = 0;
 	if (want_ref)
 		neigh_hold(n);
-	rcu_assign_pointer(n->next,
-			   rcu_dereference_protected(nht->hash_buckets[hash_val],
-						     lockdep_is_held(&tbl->lock)));
+	n->next = nht->hash_buckets[hash_val];
 	rcu_assign_pointer(nht->hash_buckets[hash_val], n);
 	write_unlock_bh(&tbl->lock);
 	neigh_dbg(2, "neigh %p is created\n", n);
@@ -2381,9 +2373,7 @@ void __neigh_for_each_release(struct neigh_table *tbl,
 			write_lock(&n->lock);
 			release = cb(n);
 			if (release) {
-				rcu_assign_pointer(*np,
-					rcu_dereference_protected(n->next,
-						lockdep_is_held(&tbl->lock)));
+				*np = n->next;
 				n->dead = 1;
 			} else
 				np = &n->next;

Re: [PATCH net-next] neigh: Add missing rcu_assign_pointer

[Ying Xue]

On 05/29/2015 09:50 AM, Eric Dumazet wrote:
> I count 5 places of redundancy. 
> 

Another two places you found are necessary indeed!

Acked-by: Ying Xue <ying.xue@windriver.com>

> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> index 3a74df750af4044eba0e7d88ae01ca9b4dac0e72..ac3b69183cc982e722d9683d6de7a39f66b50b64 100644
> --- a/net/core/neighbour.c
> +++ b/net/core/neighbour.c
> @@ -141,9 +141,7 @@ static int neigh_forced_gc(struct neigh_table *tbl)
>  			write_lock(&n->lock);
>  			if (atomic_read(&n->refcnt) == 1 &&
>  			    !(n->nud_state & NUD_PERMANENT)) {
> -				rcu_assign_pointer(*np,
> -					rcu_dereference_protected(n->next,
> -						  lockdep_is_held(&tbl->lock)));
> +				*np = n->next;
>  				n->dead = 1;
>  				shrunk	= 1;
>  				write_unlock(&n->lock);
> @@ -210,9 +208,7 @@ static void neigh_flush_dev(struct neigh_table *tbl, struct net_device *dev)
>  				np = &n->next;
>  				continue;
>  			}
> -			rcu_assign_pointer(*np,
> -				   rcu_dereference_protected(n->next,
> -						lockdep_is_held(&tbl->lock)));
> +			*np = n->next;
>  			write_lock(&n->lock);
>  			neigh_del_timer(n);
>  			n->dead = 1;
> @@ -380,10 +376,8 @@ static struct neigh_hash_table *neigh_hash_grow(struct neigh_table *tbl,
>  			next = rcu_dereference_protected(n->next,
>  						lockdep_is_held(&tbl->lock));
>  
> -			rcu_assign_pointer(n->next,
> -					   rcu_dereference_protected(
> -						new_nht->hash_buckets[hash],
> -						lockdep_is_held(&tbl->lock)));
> +			n->next = new_nht->hash_buckets[hash];
> +
>  			rcu_assign_pointer(new_nht->hash_buckets[hash], n);
>  		}
>  	}
> @@ -515,9 +509,7 @@ struct neighbour *__neigh_create(struct neigh_table *tbl, const void *pkey,
>  	n->dead = 0;
>  	if (want_ref)
>  		neigh_hold(n);
> -	rcu_assign_pointer(n->next,
> -			   rcu_dereference_protected(nht->hash_buckets[hash_val],
> -						     lockdep_is_held(&tbl->lock)));
> +	n->next = nht->hash_buckets[hash_val];
>  	rcu_assign_pointer(nht->hash_buckets[hash_val], n);
>  	write_unlock_bh(&tbl->lock);
>  	neigh_dbg(2, "neigh %p is created\n", n);
> @@ -2381,9 +2373,7 @@ void __neigh_for_each_release(struct neigh_table *tbl,
>  			write_lock(&n->lock);
>  			release = cb(n);
>  			if (release) {
> -				rcu_assign_pointer(*np,
> -					rcu_dereference_protected(n->next,
> -						lockdep_is_held(&tbl->lock)));
> +				*np = n->next;
>  				n->dead = 1;
>  			} else
>  				np = &n->next;