[NET] ROUTE: fix rcu_dereference() uses in /proc/net/rt_cache

[Eric Dumazet <dada1@cosmosbay.com>]

[-- Attachment #1: Type: text/plain, Size: 2290 bytes --]

Herbert Xu a écrit :
> Eric Dumazet <dada1@cosmosbay.com> wrote:
>> Very good question, but honestly I really dont see why it was there at the 
>> first place :
> 
> It was there because someone went through this file and robotically
> replaced all conditional read barriers with rcu_dereference, even when
> it made zero sense.
> 
> Basically you can add a conditional barrier either at the point where
> the pointer gets read, or where it gets derferenced.  Previously we
> did the latter (except that the show function didn't have a barrier
> at all which is technically a bug though harmless in pratice).  This
> patch moves it to the spot where it gets read which is also OK.
> 
>> static struct rtable *rt_cache_get_next(struct seq_file *seq, struct rtable *r)
>> {
>> -       struct rt_cache_iter_state *st = rcu_dereference(seq->private);
>> +       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 r;
> 
> Slight optimisation: please move both barriers onto the return statement,
> i.e.,
> 
> 	return rcu_dereference(r);

I am not sure this is valid, since it will do this :

r = rt_hash_table[st->bucket].chain;
if (r)
     return rcu_dereference(r);

So compiler might be dumb enough do dereference 
&rt_hash_table[st->bucket].chain two times.


It seems Dipankar is busy at this moment, so I will post again the patch and 
ask a comment from Paul :)

Thank you

[NET] ROUTE: fix rcu_dereference() uses in /proc/net/rt_cache

In rt_cache_get_next(), no need to guard seq->private by a rcu_dereference()
since seq is private to the thread running this function. Reading seq.private
once (as guaranted bu rcu_dereference()) or several time if compiler really is 
dumb enough wont change the result.

But we miss real spots where rcu_dereference() are needed, both in 
rt_cache_get_first() and rt_cache_get_next()

Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>


[-- Attachment #2: route_rcu.patch --]
[-- Type: text/plain, Size: 1011 bytes --]

diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index d337706..3b7562f 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -278,7 +278,7 @@ static struct rtable *rt_cache_get_first(struct seq_file *seq)
 
 	for (st->bucket = rt_hash_mask; st->bucket >= 0; --st->bucket) {
 		rcu_read_lock_bh();
-		r = rt_hash_table[st->bucket].chain;
+		r = rcu_dereference(rt_hash_table[st->bucket].chain);
 		if (r)
 			break;
 		rcu_read_unlock_bh();
@@ -288,15 +288,15 @@ static struct rtable *rt_cache_get_first(struct seq_file *seq)
 
 static struct rtable *rt_cache_get_next(struct seq_file *seq, struct rtable *r)
 {
-	struct rt_cache_iter_state *st = rcu_dereference(seq->private);
+	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 r;
 }