net: fix NULL dereferences in check_peer_redir()

Gergely Kalman reported crashes in check_peer_redir().

It appears commit f39925dbde778 (ipv4: Cache learned redirect
information in inetpeer.) added a race, leading to possible NULL ptr
dereference.

Since we can now change dst neighbour, we should make sure a reader can
safely use a neighbour.

Add RCU protection to dst neighbour, and make sure check_peer_redir()
can be called safely by different cpus in parallel.

As neighbours are already freed after one RCU grace period, this patch
should not add typical RCU penalty (cache cold effects)

Many thanks to Gergely for providing a pretty report pointing to the
bug.

Reported-by: Gergely Kalman <synapse@hippy.csoma.elte.hu>
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/include/net/dst.h b/include/net/dst.h
index 29e255796ce1fdb0875f4da4a652054b48fb72ce..13d507d69ddbcfb01bd717ba1fd2659cc74e53d6 100644 (file)
--- a/include/net/dst.h
+++ b/include/net/dst.h
@@ -37,7 +37,7 @@ struct dst_entry {
        unsigned long           _metrics;
        unsigned long           expires;
        struct dst_entry        *path;
-       struct neighbour        *_neighbour;
+       struct neighbour __rcu  *_neighbour;
 #ifdef CONFIG_XFRM
        struct xfrm_state       *xfrm;
 #else
@@ -88,12 +88,17 @@ struct dst_entry {
 
 static inline struct neighbour *dst_get_neighbour(struct dst_entry *dst)
 {
-       return dst->_neighbour;
+       return rcu_dereference(dst->_neighbour);
+}
+
+static inline struct neighbour *dst_get_neighbour_raw(struct dst_entry *dst)
+{
+       return rcu_dereference_raw(dst->_neighbour);
 }
 
 static inline void dst_set_neighbour(struct dst_entry *dst, struct neighbour *neigh)
 {
-       dst->_neighbour = neigh;
+       rcu_assign_pointer(dst->_neighbour, neigh);
 }
 
 extern u32 *dst_cow_metrics_generic(struct dst_entry *dst, unsigned long old);
@@ -382,8 +387,12 @@ static inline void dst_rcu_free(struct rcu_head *head)
 static inline void dst_confirm(struct dst_entry *dst)
 {
        if (dst) {
-               struct neighbour *n = dst_get_neighbour(dst);
+               struct neighbour *n;
+
+               rcu_read_lock();
+               n = dst_get_neighbour(dst);
                neigh_confirm(n);
+               rcu_read_unlock();
        }
 }
 

diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index ccaaa851ab429ee48b0aa803fd5bf8edd5ee34ce..77d3eded665ad2e30c03f8e62ea30b29ad1b4bb9 100644 (file)
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c
@@ -204,9 +204,15 @@ static inline int ip_finish_output2(struct sk_buff *skb)
                skb = skb2;
        }
 
+       rcu_read_lock();
        neigh = dst_get_neighbour(dst);
-       if (neigh)
-               return neigh_output(neigh, skb);
+       if (neigh) {
+               int res = neigh_output(neigh, skb);
+
+               rcu_read_unlock();
+               return res;
+       }
+       rcu_read_unlock();
 
        if (net_ratelimit())
                printk(KERN_DEBUG "ip_finish_output2: No header cache and no neighbour!\n");

diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 1730689f560e67cd3e2142834d805216134202c9..6afc4eb50591ae00e1d456b98e97ca073b9a8ca3 100644 (file)
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -1628,16 +1628,18 @@ static int check_peer_redir(struct dst_entry *dst, struct inet_peer *peer)
 {
        struct rtable *rt = (struct rtable *) dst;
        __be32 orig_gw = rt->rt_gateway;
-       struct neighbour *n;
+       struct neighbour *n, *old_n;
 
        dst_confirm(&rt->dst);
 
-       neigh_release(dst_get_neighbour(&rt->dst));
-       dst_set_neighbour(&rt->dst, NULL);
-
        rt->rt_gateway = peer->redirect_learned.a4;
-       rt_bind_neighbour(rt);
-       n = dst_get_neighbour(&rt->dst);
+
+       n = ipv4_neigh_lookup(&rt->dst, &rt->rt_gateway);
+       if (IS_ERR(n))
+               return PTR_ERR(n);
+       old_n = xchg(&rt->dst._neighbour, n);
+       if (old_n)
+               neigh_release(old_n);
        if (!n || !(n->nud_state & NUD_VALID)) {
                if (n)
                        neigh_event_send(n, NULL);

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index a55500cc0b29ab710e012d42f0862a95f4750669..f012ebd87b4338181f76d3154165eb1f41cd0ba4 100644 (file)
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -656,7 +656,7 @@ ipv6_add_addr(struct inet6_dev *idev, const struct in6_addr *addr, int pfxlen,
         * layer address of our nexhop router
         */
 
-       if (dst_get_neighbour(&rt->dst) == NULL)
+       if (dst_get_neighbour_raw(&rt->dst) == NULL)
                ifa->flags &= ~IFA_F_OPTIMISTIC;
 
        ifa->idev = idev;

diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
index 54a4678955bf57b850a7a3a9fff70d0fed0eaf14..320d91d20ad7ddfd981cee92161482f218c225b4 100644 (file)
--- a/net/ipv6/ip6_fib.c
+++ b/net/ipv6/ip6_fib.c
@@ -1455,7 +1455,7 @@ static int fib6_age(struct rt6_info *rt, void *arg)
                        RT6_TRACE("aging clone %p\n", rt);
                        return -1;
                } else if ((rt->rt6i_flags & RTF_GATEWAY) &&
-                          (!(dst_get_neighbour(&rt->dst)->flags & NTF_ROUTER))) {
+                          (!(dst_get_neighbour_raw(&rt->dst)->flags & NTF_ROUTER))) {
                        RT6_TRACE("purging route %p via non-router but gateway\n",
                                  rt);
                        return -1;

diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index 32e5339db0c899928b2b805e537ab06b8d0e498b..4c882cf4e8a14f21514e053fd0d125926e75584e 100644 (file)
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -135,10 +135,15 @@ static int ip6_finish_output2(struct sk_buff *skb)
                                skb->len);
        }
 
+       rcu_read_lock();
        neigh = dst_get_neighbour(dst);
-       if (neigh)
-               return neigh_output(neigh, skb);
+       if (neigh) {
+               int res = neigh_output(neigh, skb);
 
+               rcu_read_unlock();
+               return res;
+       }
+       rcu_read_unlock();
        IP6_INC_STATS_BH(dev_net(dst->dev),
                         ip6_dst_idev(dst), IPSTATS_MIB_OUTNOROUTES);
        kfree_skb(skb);
@@ -975,12 +980,14 @@ static int ip6_dst_lookup_tail(struct sock *sk,
         * dst entry and replace it instead with the
         * dst entry of the nexthop router
         */
+       rcu_read_lock();
        n = dst_get_neighbour(*dst);
        if (n && !(n->nud_state & NUD_VALID)) {
                struct inet6_ifaddr *ifp;
                struct flowi6 fl_gw6;
                int redirect;
 
+               rcu_read_unlock();
                ifp = ipv6_get_ifaddr(net, &fl6->saddr,
                                      (*dst)->dev, 1);
 
@@ -1000,6 +1007,8 @@ static int ip6_dst_lookup_tail(struct sock *sk,
                        if ((err = (*dst)->error))
                                goto out_err_release;
                }
+       } else {
+               rcu_read_unlock();
        }
 #endif
 

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index e8987da06667225e44dae0cbfaf7fc51490788d7..9e69eb0ec6dd48a96d92bcf59c2cd89ad5e8412a 100644 (file)
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -364,7 +364,7 @@ out:
 #ifdef CONFIG_IPV6_ROUTER_PREF
 static void rt6_probe(struct rt6_info *rt)
 {
-       struct neighbour *neigh = rt ? dst_get_neighbour(&rt->dst) : NULL;
+       struct neighbour *neigh;
        /*
         * Okay, this does not seem to be appropriate
         * for now, however, we need to check if it
@@ -373,8 +373,10 @@ static void rt6_probe(struct rt6_info *rt)
         * Router Reachability Probe MUST be rate-limited
         * to no more than one per minute.
         */
+       rcu_read_lock();
+       neigh = rt ? dst_get_neighbour(&rt->dst) : NULL;
        if (!neigh || (neigh->nud_state & NUD_VALID))
-               return;
+               goto out;
        read_lock_bh(&neigh->lock);
        if (!(neigh->nud_state & NUD_VALID) &&
            time_after(jiffies, neigh->updated + rt->rt6i_idev->cnf.rtr_probe_interval)) {
@@ -387,8 +389,11 @@ static void rt6_probe(struct rt6_info *rt)
                target = (struct in6_addr *)&neigh->primary_key;
                addrconf_addr_solict_mult(target, &mcaddr);
                ndisc_send_ns(rt->rt6i_dev, NULL, target, &mcaddr, NULL);
-       } else
+       } else {
                read_unlock_bh(&neigh->lock);
+       }
+out:
+       rcu_read_unlock();
 }
 #else
 static inline void rt6_probe(struct rt6_info *rt)
@@ -412,8 +417,11 @@ static inline int rt6_check_dev(struct rt6_info *rt, int oif)
 
 static inline int rt6_check_neigh(struct rt6_info *rt)
 {
-       struct neighbour *neigh = dst_get_neighbour(&rt->dst);
+       struct neighbour *neigh;
        int m;
+
+       rcu_read_lock();
+       neigh = dst_get_neighbour(&rt->dst);
        if (rt->rt6i_flags & RTF_NONEXTHOP ||
            !(rt->rt6i_flags & RTF_GATEWAY))
                m = 1;
@@ -430,6 +438,7 @@ static inline int rt6_check_neigh(struct rt6_info *rt)
                read_unlock_bh(&neigh->lock);
        } else
                m = 0;
+       rcu_read_unlock();
        return m;
 }
 
@@ -769,7 +778,7 @@ static struct rt6_info *rt6_alloc_clone(struct rt6_info *ort,
                rt->rt6i_dst.plen = 128;
                rt->rt6i_flags |= RTF_CACHE;
                rt->dst.flags |= DST_HOST;
-               dst_set_neighbour(&rt->dst, neigh_clone(dst_get_neighbour(&ort->dst)));
+               dst_set_neighbour(&rt->dst, neigh_clone(dst_get_neighbour_raw(&ort->dst)));
        }
        return rt;
 }
@@ -803,7 +812,7 @@ restart:
        dst_hold(&rt->dst);
        read_unlock_bh(&table->tb6_lock);
 
-       if (!dst_get_neighbour(&rt->dst) && !(rt->rt6i_flags & RTF_NONEXTHOP))
+       if (!dst_get_neighbour_raw(&rt->dst) && !(rt->rt6i_flags & RTF_NONEXTHOP))
                nrt = rt6_alloc_cow(rt, &fl6->daddr, &fl6->saddr);
        else if (!(rt->dst.flags & DST_HOST))
                nrt = rt6_alloc_clone(rt, &fl6->daddr);
@@ -1587,7 +1596,7 @@ void rt6_redirect(const struct in6_addr *dest, const struct in6_addr *src,
        dst_confirm(&rt->dst);
 
        /* Duplicate redirect: silently ignore. */
-       if (neigh == dst_get_neighbour(&rt->dst))
+       if (neigh == dst_get_neighbour_raw(&rt->dst))
                goto out;
 
        nrt = ip6_rt_copy(rt, dest);
@@ -1682,7 +1691,7 @@ again:
           1. It is connected route. Action: COW
           2. It is gatewayed route or NONEXTHOP route. Action: clone it.
         */
-       if (!dst_get_neighbour(&rt->dst) && !(rt->rt6i_flags & RTF_NONEXTHOP))
+       if (!dst_get_neighbour_raw(&rt->dst) && !(rt->rt6i_flags & RTF_NONEXTHOP))
                nrt = rt6_alloc_cow(rt, daddr, saddr);
        else
                nrt = rt6_alloc_clone(rt, daddr);
@@ -2326,6 +2335,7 @@ static int rt6_fill_node(struct net *net,
        struct nlmsghdr *nlh;
        long expires;
        u32 table;
+       struct neighbour *n;
 
        if (prefix) {   /* user wants prefix routes only */
                if (!(rt->rt6i_flags & RTF_PREFIX_RT)) {
@@ -2414,8 +2424,11 @@ static int rt6_fill_node(struct net *net,
        if (rtnetlink_put_metrics(skb, dst_metrics_ptr(&rt->dst)) < 0)
                goto nla_put_failure;
 
-       if (dst_get_neighbour(&rt->dst))
-               NLA_PUT(skb, RTA_GATEWAY, 16, &dst_get_neighbour(&rt->dst)->primary_key);
+       rcu_read_lock();
+       n = dst_get_neighbour(&rt->dst);
+       if (n)
+               NLA_PUT(skb, RTA_GATEWAY, 16, &n->primary_key);
+       rcu_read_unlock();
 
        if (rt->dst.dev)
                NLA_PUT_U32(skb, RTA_OIF, rt->rt6i_dev->ifindex);
@@ -2608,12 +2621,14 @@ static int rt6_info_route(struct rt6_info *rt, void *p_arg)
 #else
        seq_puts(m, "00000000000000000000000000000000 00 ");
 #endif
+       rcu_read_lock();
        n = dst_get_neighbour(&rt->dst);
        if (n) {
                seq_printf(m, "%pi6", n->primary_key);
        } else {
                seq_puts(m, "00000000000000000000000000000000");
        }
+       rcu_read_unlock();
        seq_printf(m, " %08x %08x %08x %08x %8s\n",
                   rt->rt6i_metric, atomic_read(&rt->dst.__refcnt),
                   rt->dst.__use, rt->rt6i_flags,