netdev - Re: [PATCH net-next] neigh: Add missing rcu_assign

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1432864234.7456.56.camel@edumazet-glaptop2.roam.corp.google.com>
Date:	Thu, 28 May 2015 18:50:34 -0700
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Ying Xue <ying.xue@...driver.com>
Cc:	netdev@...r.kernel.org, herbert@...dor.apana.org.au
Subject: Re: [PATCH net-next] neigh: Add missing rcu_assign_pointer

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;



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