rcu,inet,fib_trie,route,radix-tree,DECnet,mac80211: fix meaningless rcu_dereference(local_var)

PATCH In review Lai Jiangshan 4 participants 5 messages Jun 21, 2008 – Jun 23, 2008

Subsystems (5)

Networking Network drivers Library code IPv4/IPv6 +1 more subsystems

lore

Share & tools

lore

Lai Jiangshan

author

2008-06-21 09:56 UTC #0 lore raw

PATCH 1/1 rcu,inet,fib_trie,route,radix-tree,DECnet,mac80211: fix meaningless rcu_dereference(local_var)

rcu_dereference is provided for fetching an RCU-protected pointer.

And rcu_dereference(local_var) is meaningless and may causes bugs.

Signed-off-by Lai Jiangshan <[email protected]>

---

include/linux/inetdevice.h +1 -4

index c6f51ad..da7aada 100644
@@ -166,10 +166,7 @@ static __inline__ int bad_mask(__be32 mask, __be32 addr)
 
 static inline struct in_device *__in_dev_get_rcu(const struct net_device *dev)
 {
-	struct in_device *in_dev = dev->ip_ptr;
-	if (in_dev)
-		in_dev = rcu_dereference(in_dev);
-	return in_dev;
+	return rcu_dereference(dev->ip_ptr);
 }
 
 static __inline__ struct in_device *

lib/radix-tree.c +3 -4

index 169a2f8..bfae4e2 100644
@@ -703,9 +703,9 @@ __lookup(struct radix_tree_node *slot, void **results, unsigned long index,
 	for (i = index & RADIX_TREE_MAP_MASK; i < RADIX_TREE_MAP_SIZE; i++) {
 		struct radix_tree_node *node;
 		index++;
-		node = slot->slots[i];
+		node = rcu_dereference(slot->slots[i]);
 		if (node) {
-			results[nr_found++] = rcu_dereference(node);
+			results[nr_found++] = node;
 			if (nr_found == max_items)
 				goto out;
 		}

@@ -815,7 +815,7 @@ __lookup_tag(struct radix_tree_node *slot, void **results, unsigned long index,
 				index++;
 				if (!tag_get(slot, tag, j))
 					continue;
-				node = slot->slots[j];
+				node = rcu_dereference(slot->slots[j]);
 				/*
 				 * Even though the tag was found set, we need to
 				 * recheck that we have a non-NULL node, because

@@ -827,7 +827,6 @@ __lookup_tag(struct radix_tree_node *slot, void **results, unsigned long index,
 				 * rely on its value remaining the same).
 				 */
 				if (node) {
-					node = rcu_dereference(node);
 					results[nr_found++] = node;
 					if (nr_found == max_items)
 						goto out;

net/decnet/dn_route.c +5 -5

index f50e88b..d0b61f6 100644
@@ -1658,27 +1658,27 @@ static struct dn_route *dn_rt_cache_get_first(struct seq_file *seq)
 
 	for(s->bucket = dn_rt_hash_mask; s->bucket >= 0; --s->bucket) {
 		rcu_read_lock_bh();
-		rt = dn_rt_hash_table[s->bucket].chain;
+		rt = rcu_dereference(dn_rt_hash_table[s->bucket].chain);
 		if (rt)
 			break;
 		rcu_read_unlock_bh();
 	}
-	return rcu_dereference(rt);
+	return rt;
 }
 
 static struct dn_route *dn_rt_cache_get_next(struct seq_file *seq, struct dn_route *rt)
 {
 	struct dn_rt_cache_iter_state *s = seq->private;
 
-	rt = rt->u.dst.dn_next;
+	rt = rcu_dereference(rt->u.dst.dn_next);
 	while(!rt) {
 		rcu_read_unlock_bh();
 		if (--s->bucket < 0)
 			break;
 		rcu_read_lock_bh();
-		rt = dn_rt_hash_table[s->bucket].chain;
+		rt = rcu_dereference(dn_rt_hash_table[s->bucket].chain);
 	}
-	return rcu_dereference(rt);
+	return rt;
 }
 
 static void *dn_rt_cache_seq_start(struct seq_file *seq, loff_t *pos)

net/ipv4/fib_trie.c +6 -4

index 4b02d14..3815328 100644
@@ -174,9 +174,11 @@ static inline struct tnode *node_parent(struct node *node)
 
 static inline struct tnode *node_parent_rcu(struct node *node)
 {
-	struct tnode *ret = node_parent(node);
+	struct tnode *ret = (struct tnode *)(ACCESS_ONCE(node->parent)
+		& ~NODE_TYPE_MASK);
 
-	return rcu_dereference(ret);
+	smp_read_barrier_depends();
+	return ret;
 }
 
 /* Same as rcu_assign_pointer

@@ -197,9 +199,9 @@ static inline struct node *tnode_get_child(struct tnode *tn, unsigned int i)
 
 static inline struct node *tnode_get_child_rcu(struct tnode *tn, unsigned int i)
 {
-	struct node *ret = tnode_get_child(tn, i);
+	BUG_ON(i >= 1U << tn->bits);
 
-	return rcu_dereference(ret);
+	return rcu_dereference(tn->child[i]);
 }
 
 static inline int tnode_child_length(const struct tnode *tn)

net/ipv4/route.c +3 -3

index 96be336..d02346e 100644
@@ -297,15 +297,15 @@ static struct rtable *__rt_cache_get_next(struct seq_file *seq,
 					  struct rtable *r)
 {
 	struct rt_cache_iter_state *st = seq->private;
-	r = r->u.dst.rt_next;
+	r = rcu_dereference(r->u.dst.rt_next);
 	while (!r) {
 		rcu_read_unlock_bh();
 		if (--st->bucket < 0)
 			break;
 		rcu_read_lock_bh();
-		r = rt_hash_table[st->bucket].chain;
+		r = rcu_dereference(rt_hash_table[st->bucket].chain);
 	}
-	return rcu_dereference(r);
+	return r;
 }
 
 static struct rtable *rt_cache_get_next(struct seq_file *seq,

net/mac80211/mesh_hwmp.c +0 -1

index af0cd1e..16e545b 100644
@@ -830,7 +830,6 @@ void mesh_path_timer(unsigned long data)
 
 	rcu_read_lock();
 	mpath = (struct mesh_path *) data;
-	mpath = rcu_dereference(mpath);
 	if (!mpath)
 		goto endmpathtimer;
 	spin_lock_bh(&mpath->state_lock);

Nick Piggin

2008-06-23 02:37 UTC #1 lore raw

Quoted 26 lines · Lai Jiangshan

lib/radix-tree.c +0 -0

index 169a2f8..bfae4e2 100644
@@ -703,9 +703,9 @@ __lookup(struct radix_tree_node *slot, void **results,

unsigned long index, for (i = index & RADIX_TREE_MAP_MASK; i < RADIX_TREE_MAP_SIZE; i++) { struct radix_tree_node *node;

lib/radix-tree.c +3 -3

 		index++;
-		node = slot->slots[i];
+		node = rcu_dereference(slot->slots[i]);
 		if (node) {
-			results[nr_found++] = rcu_dereference(node);
+			results[nr_found++] = node;
 			if (nr_found == max_items)
 				goto out;
 		}

@@ -815,7 +815,7 @@ __lookup_tag(struct radix_tree_node *slot, void
 **results, unsigned long index, index++;
 				if (!tag_get(slot, tag, j))
 					continue;
-				node = slot->slots[j];
+				node = rcu_dereference(slot->slots[j]);
 				/*
 				 * Even though the tag was found set, we need to
 				 * recheck that we have a non-NULL node, because

lib/radix-tree.c __lookup_tag(struct radix_tree_node *slot, void · line 832

Quoted 7 lines

lib/radix-tree.c +0 -1

@@ -827,7 +827,6 @@ __lookup_tag(struct radix_tree_node *slot, void
 **results, unsigned long index, * rely on its value remaining the same).
 				 */
 				if (node) {
-					node = rcu_dereference(node);
 					results[nr_found++] = node;
 					if (nr_found == max_items)
 						goto out;

This was done like this IIRC to avoid the barrier when possible.

Lai Jiangshan

author

2008-06-23 05:37 UTC #2 lore raw

Add CC: Linus Torvalds

Quoted 29 lines · Nick Piggin

Quoted 27 lines · Lai Jiangshan

lib/radix-tree.c +0 -0

index 169a2f8..bfae4e2 100644
@@ -703,9 +703,9 @@ __lookup(struct radix_tree_node *slot, void **results,

unsigned long index, for (i = index & RADIX_TREE_MAP_MASK; i < RADIX_TREE_MAP_SIZE; i++) { struct radix_tree_node *node;

lib/radix-tree.c +3 -3

 		index++;
-		node = slot->slots[i];
+		node = rcu_dereference(slot->slots[i]);
 		if (node) {
-			results[nr_found++] = rcu_dereference(node);
+			results[nr_found++] = node;
 			if (nr_found == max_items)
 				goto out;
 		}

@@ -815,7 +815,7 @@ __lookup_tag(struct radix_tree_node *slot, void
 **results, unsigned long index, index++;
 				if (!tag_get(slot, tag, j))
 					continue;
-				node = slot->slots[j];
+				node = rcu_dereference(slot->slots[j]);
 				/*
 				 * Even though the tag was found set, we need to
 				 * recheck that we have a non-NULL node, because

@@ -827,7 +827,6 @@ __lookup_tag(struct radix_tree_node *slot, void
 **results, unsigned long index, * rely on its value remaining the same).

Quoted 11 lines

lib/radix-tree.c __lookup_tag(struct radix_tree_node *slot, void **results, unsigned long index, · line 832
Quoted 6 lines
lib/radix-tree.c +0 -1
 				 */
 				if (node) {
-					node = rcu_dereference(node);
 					results[nr_found++] = node;
 					if (nr_found == max_items)
 						goto out;
This was done like this IIRC to avoid the barrier when possible.

This(http://lkml.org/lkml/2008/4/20/217) shows why rcu_dereference(local_var) is meaningless. And why not use smp_read_barrier_depends() here?

I guessed somebody use rcu_dereference(local_var) in if-statements to avoid the barrier when possible, and I made this patch(http://lkml.org/lkml/2008/6/21/29), but it is incorrect.

Nick Piggin

2008-06-23 08:49 UTC #3 lore raw

Quoted 43 lines · Lai Jiangshan

Add CC: Linus Torvalds

Quoted 29 lines · Nick Piggin

Quoted 27 lines · Lai Jiangshan

lib/radix-tree.c +0 -0

index 169a2f8..bfae4e2 100644
@@ -703,9 +703,9 @@ __lookup(struct radix_tree_node *slot, void **results,

unsigned long index, for (i = index & RADIX_TREE_MAP_MASK; i < RADIX_TREE_MAP_SIZE; i++) { struct radix_tree_node *node;

lib/radix-tree.c +3 -3

 		index++;
-		node = slot->slots[i];
+		node = rcu_dereference(slot->slots[i]);
 		if (node) {
-			results[nr_found++] = rcu_dereference(node);
+			results[nr_found++] = node;
 			if (nr_found == max_items)
 				goto out;
 		}

@@ -815,7 +815,7 @@ __lookup_tag(struct radix_tree_node *slot, void
 **results, unsigned long index, index++;
 				if (!tag_get(slot, tag, j))
 					continue;
-				node = slot->slots[j];
+				node = rcu_dereference(slot->slots[j]);
 				/*
 				 * Even though the tag was found set, we need to
 				 * recheck that we have a non-NULL node, because

@@ -827,7 +827,6 @@ __lookup_tag(struct radix_tree_node *slot, void
 **results, unsigned long index, * rely on its value remaining the same).

Quoted 11 lines

lib/radix-tree.c __lookup_tag(struct radix_tree_node *slot, void **results, unsigned long index, · line 832
Quoted 6 lines
lib/radix-tree.c +0 -1
 				 */
 				if (node) {
-					node = rcu_dereference(node);
 					results[nr_found++] = node;
 					if (nr_found == max_items)
 						goto out;
This was done like this IIRC to avoid the barrier when possible.

Quoted 2 lines

This(http://lkml.org/lkml/2008/4/20/217) shows why rcu_dereference(local_var) is meaningless. And why not use smp_read_barrier_depends() here?

It is "meaningless" in that it isn't being applied as the API is supposed to, however it does provide the barrier that's required. I guess read barrier depends could just be used instead, although I like the self commenting nature of the rcu_dereference, even if it is not quite applied correctly, the reader can easily see the intention.

I *think* it should even do the right thing WRT the access_once macro here, and cause node not to be reloaded from source, but I could be wrong on that.

Quoted 3 lines

I guessed somebody use rcu_dereference(local_var) in if-statements to avoid the barrier when possible, and I made this patch(http://lkml.org/lkml/2008/6/21/29), but it is incorrect.

So it doesn't help me ;)

I'm not sure what the best way to go is, but I would ask Paul for ideas if he's not too busy.

In reality, the barriers probably don't matter much (but I'd really love to have an Alpha to test it on :)), but I still try to avoid them as much as possible. For the radix-tree as used by pagecache, it is a completely usual operation to lookup non existing elements, so we are talking about a fastpath of sorts...

Paul E. McKenney

2008-06-23 10:01 UTC #4 lore raw

In reply to Lai Jiangshan

Quoted 43 lines · Lai Jiangshan

Add CC: Linus Torvalds

Quoted 29 lines · Nick Piggin

Quoted 27 lines · Lai Jiangshan

lib/radix-tree.c +0 -0

index 169a2f8..bfae4e2 100644
@@ -703,9 +703,9 @@ __lookup(struct radix_tree_node *slot, void **results,

unsigned long index, for (i = index & RADIX_TREE_MAP_MASK; i < RADIX_TREE_MAP_SIZE; i++) { struct radix_tree_node *node;

lib/radix-tree.c +3 -3

 		index++;
-		node = slot->slots[i];
+		node = rcu_dereference(slot->slots[i]);
 		if (node) {
-			results[nr_found++] = rcu_dereference(node);
+			results[nr_found++] = node;
 			if (nr_found == max_items)
 				goto out;
 		}

@@ -815,7 +815,7 @@ __lookup_tag(struct radix_tree_node *slot, void
 **results, unsigned long index, index++;
 				if (!tag_get(slot, tag, j))
 					continue;
-				node = slot->slots[j];
+				node = rcu_dereference(slot->slots[j]);
 				/*
 				 * Even though the tag was found set, we need to
 				 * recheck that we have a non-NULL node, because

@@ -827,7 +827,6 @@ __lookup_tag(struct radix_tree_node *slot, void
 **results, unsigned long index, * rely on its value remaining the same).

Quoted 11 lines

lib/radix-tree.c __lookup_tag(struct radix_tree_node *slot, void **results, unsigned long index, · line 832
Quoted 6 lines
lib/radix-tree.c +0 -1
 				 */
 				if (node) {
-					node = rcu_dereference(node);
 					results[nr_found++] = node;
 					if (nr_found == max_items)
 						goto out;
This was done like this IIRC to avoid the barrier when possible.

Quoted 2 lines

This(http://lkml.org/lkml/2008/4/20/217) shows why rcu_dereference(local_var) is meaningless. And why not use smp_read_barrier_depends() here?

One caution... smp_read_barrier_depends() makes for difficult-to-read code. After a few years and patches to surrounding code, it becomes quite difficult to figure out what the smp_read_barrier_depends() is constraining.

I very strongly recommend using rcu_dereference() instead. Especially since rcu_dereference() is extremely lightweight on almost all machines available today -- there aren't that many DEC Alphas running Linux, from what I understand.

Quoted 3 lines

I guessed somebody use rcu_dereference(local_var) in if-statements to avoid the barrier when possible, and I made this patch(http://lkml.org/lkml/2008/6/21/29), but it is incorrect.

On all but Alpha, it is quite possible that rcu_dereference() is cheaper than a conditional branch. ;-)

Thanx, Paul