Re: [Patch net] ipv4: restore rt->fi for reference counting

[Eric Dumazet]

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.

Like :

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');
        }