netdev - [NET] ROUTE: fix rcu_dereference() uses in /proc/net/rt

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: <47847A10.1020508@cosmosbay.com>
Date:	Wed, 09 Jan 2008 08:38:56 +0100
From:	Eric Dumazet <dada1@...mosbay.com>
To:	Herbert Xu <herbert@...dor.apana.org.au>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
CC:	davem@...emloft.net, dipankar@...ibm.com, netdev@...r.kernel.org
Subject: [NET] ROUTE: fix rcu_dereference() uses in /proc/net/rt_cache

Herbert Xu a écrit :
> Eric Dumazet <dada1@...mosbay.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@...mosbay.com>


View attachment "route_rcu.patch" of type "text/plain" (1012 bytes)