| Message ID | 1494373805.7796.98.camel@edumazet-glaptop3.roam.corp.google.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On Tue, May 9, 2017 at 4:50 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > On Tue, 2017-05-09 at 16:35 -0700, Cong Wang wrote: > >> All of them take RCU read lock, so, as I explained in the code comment, >> they all should be fine because of synchronize_net() on unregister path. >> Do you see anything otherwise? > > They might take rcu lock, but compiler is still allowed to read > fi->fib_dev multiple times, and crashes might happen. > > You will need to audit all code and fix it, using proper > rcu_dereference() or similar code ensuring compiler wont do stupid > things. > Point taken. But without my patch, nh_dev is supposed to be protected by RCU too, it is freed in a rcu callback and dereferenced like: struct in_device *in_dev = __in_dev_get_rcu(nh->nh_dev);
diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c index 1201409ba1dcb18ee028003b065410b87bf4a602..ab69517befbb5f300af785fbb20071a3d1086593 100644 --- a/net/ipv4/fib_trie.c +++ b/net/ipv4/fib_trie.c @@ -2666,11 +2666,13 @@ static int fib_route_seq_show(struct seq_file *seq, void *v) seq_setwidth(seq, 127); - if (fi) + if (fi) { + struct net_device *dev = rcu_dereference(fi->fib_dev); + seq_printf(seq, "%s\t%08X\t%08X\t%04X\t%d\t%u\t" "%d\t%08X\t%d\t%u\t%u", - fi->fib_dev ? fi->fib_dev->name : "*", + dev ? dev->name : "*", prefix, fi->fib_nh->nh_gw, flags, 0, 0, fi->fib_priority, @@ -2679,13 +2681,13 @@ static int fib_route_seq_show(struct seq_file *seq, void *v) fi->fib_advmss + 40 : 0), fi->fib_window, fi->fib_rtt >> 3); - else + } else { seq_printf(seq, "*\t%08X\t%08X\t%04X\t%d\t%u\t" "%d\t%08X\t%d\t%u\t%u", prefix, 0, flags, 0, 0, 0, mask, 0, 0, 0); - + } seq_pad(seq, '\n'); }