diff mbox

PROBLEM: BUG (NULL ptr dereference in ipv4_dst_check)

Message ID 1311952306.2843.27.camel@edumazet-HP-Compaq-6005-Pro-SFF-PC
State RFC, archived
Delegated to: David Miller
Headers show

Commit Message

Eric Dumazet July 29, 2011, 3:11 p.m. UTC 
Le vendredi 29 juillet 2011 à 16:44 +0200, synapse a écrit :
> On 07/29/11 16:41, Eric Dumazet wrote:
> > Le vendredi 29 juillet 2011 à 07:39 -0700, David Miller a écrit :
> >> From: Eric Dumazet<eric.dumazet@gmail.com>
> >> Date: Fri, 29 Jul 2011 16:36:24 +0200
> >>
> >>> Hmm, I'll take a look, but check_peer_redir() seems suspicious at first
> >>> glance.
> >> I take full responsibility for any bugs you discover in it :-)
> >
> > ;)
> >
> > Bug origin is commit f39925dbde7788cfb96419c0f092b086aa325c0f
> > ipv4: Cache learned redirect information in inetpeer.
> >
> > I'll cook a patch ASAP
> >
> wow that was QUICK :)
> 
> When you're done I'll check it ASAP
> 
> Gergely Kalman

Thats tricky, because I am not sure we dont need RCU protection since we
can now exchange dst neighbour on the fly.

Following patch would only reduce the window of bug, not a complete
fix...

David, any opinion on this ?




--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

David Miller July 29, 2011, 3:19 p.m. UTC | #1 
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 29 Jul 2011 17:11:46 +0200

> Thats tricky, because I am not sure we dont need RCU protection since we
> can now exchange dst neighbour on the fly.
> 
> Following patch would only reduce the window of bug, not a complete
> fix...
> 
> David, any opinion on this ?

Indeed, old code worked because we invalidated entire route cache
entry, and we never before ran arp_bind_neighbour() except on new
route cache entires before they become globally visible.

I think when we change an existing neigh we will need to release old
neigh via RCU, at a minimum.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Dumazet July 29, 2011, 3:43 p.m. UTC | #2 
Le vendredi 29 juillet 2011 à 08:19 -0700, David Miller a écrit :
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Fri, 29 Jul 2011 17:11:46 +0200
> 
> > Thats tricky, because I am not sure we dont need RCU protection since we
> > can now exchange dst neighbour on the fly.
> > 
> > Following patch would only reduce the window of bug, not a complete
> > fix...
> > 
> > David, any opinion on this ?
> 
> Indeed, old code worked because we invalidated entire route cache
> entry, and we never before ran arp_bind_neighbour() except on new
> route cache entires before they become globally visible.
> 
> I think when we change an existing neigh we will need to release old
> neigh via RCU, at a minimum.


Oh well, we already use RCU in neigh_destroy(), so adding rcu would need
to change all dst_get_neighbour() callers to be in one rcu_read_lock()
section.

I'll take a look, I suspect its mostly already done.



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 1730689..6afc4eb 100644
--- 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);