Prevent a MP race in rtable_lookup().

If an ART node is linked to multiple route entries, in the MPATH case,
it is not safe to dereference ``an_dst''.  This non-refcounted pointer
can be changed at any time by another CPU.

So get rid of the pointer and use the first destination of a route entry
when comparing sockaddrs.

This allows us so remove a pointer from 'struct art_node' and save 5Mb of
memory in an IPv4 fullfeed.

ok jmatthew@, claudio@, dlg@

3 files changed, 32 insertions, 19 deletions

diff --git a/sys/net/art.c b/sys/net/art.c
index 027b043ca3e..82a8b9b1b45 100644
--- a/sys/net/art.c
+++ b/sys/net/art.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: art.c,v 1.26 2017/01/24 10:08:30 krw Exp $ */
+/*	$OpenBSD: art.c,v 1.27 2017/02/28 09:50:13 mpi Exp $ */
 
 /*
  * Copyright (c) 2015 Martin Pieuchot
@@ -703,7 +703,6 @@ art_walk_apply(struct art_root *ar,
 	int error = 0;
 
 	if ((an != NULL) && (an != next)) {
-		/* this assumes an->an_dst is not used by f */
 		rw_exit_write(&ar->ar_lock);
 		error = (*f)(an, arg);
 		rw_enter_write(&ar->ar_lock);
@@ -930,7 +929,6 @@ art_get(void *dst, uint8_t plen)
 	if (an == NULL)
 		return (NULL);
 
-	an->an_dst = dst;
 	an->an_plen = plen;
 	SRPL_INIT(&an->an_rtlist);
 
diff --git a/sys/net/art.h b/sys/net/art.h
index 87b1e67a626..b13e2394e54 100644
--- a/sys/net/art.h
+++ b/sys/net/art.h
@@ -1,4 +1,4 @@
-/* $OpenBSD: art.h,v 1.16 2017/01/23 01:02:11 claudio Exp $ */
+/* $OpenBSD: art.h,v 1.17 2017/02/28 09:50:13 mpi Exp $ */
 
 /*
  * Copyright (c) 2015 Martin Pieuchot
@@ -46,15 +46,14 @@ struct rtentry;
  * A node is the internal representation of a route entry.
  */
 struct art_node {
-	SRPL_HEAD(, rtentry)	 an_rtlist;	/* Route related to this node */
 	union {
-		void		*an__dst;	/* Destination address (key) */
-		struct art_node	*an__gc;	/* Entry on GC list */
+	    SRPL_HEAD(, rtentry) an__rtlist;	/* Route related to this node */
+	    struct art_node	*an__gc;	/* Entry on GC list */
 	}			 an_pointer;
 	uint8_t			 an_plen;	/* Prefix length */
 };
-#define an_dst	an_pointer.an__dst
-#define an_gc	an_pointer.an__gc
+#define an_rtlist	an_pointer.an__rtlist
+#define an_gc		an_pointer.an__gc
 
 void		 art_init(void);
 struct art_root	*art_alloc(unsigned int, unsigned int, unsigned int);
diff --git a/sys/net/rtable.c b/sys/net/rtable.c
index 74025a58487..0536c25696f 100644
--- a/sys/net/rtable.c
+++ b/sys/net/rtable.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: rtable.c,v 1.57 2017/01/24 10:08:30 krw Exp $ */
+/*	$OpenBSD: rtable.c,v 1.58 2017/02/28 09:50:13 mpi Exp $ */
 
 /*
  * Copyright (c) 2014-2016 Martin Pieuchot
@@ -479,6 +479,7 @@ rtable_mpath_reprio(unsigned int rtableid, struct sockaddr *dst,
 
 static inline uint8_t	*satoaddr(struct art_root *, struct sockaddr *);
 
+int	an_match(struct art_node *, struct sockaddr *, int);
 void	rtentry_ref(void *, void *);
 void	rtentry_unref(void *, void *);
 
@@ -530,8 +531,7 @@ rtable_lookup(unsigned int rtableid, struct sockaddr *dst,
 		an = art_lookup(ar, addr, plen, &nsr);
 
 		/* Make sure we've got a perfect match. */
-		if (an == NULL || an->an_plen != plen ||
-		    memcmp(an->an_dst, dst, dst->sa_len))
+		if (!an_match(an, dst, plen))
 			goto out;
 	}
 
@@ -666,8 +666,7 @@ rtable_insert(unsigned int rtableid, struct sockaddr *dst,
 	/* Do not permit exactly the same dst/mask/gw pair. */
 	an = art_lookup(ar, addr, plen, &sr);
 	srp_leave(&sr); /* an can't go away while we have the lock */
-	if (an != NULL && an->an_plen == plen &&
-	    !memcmp(an->an_dst, dst, dst->sa_len)) {
+	if (an_match(an, dst, plen)) {
 		struct rtentry  *mrt;
 		int		 mpathok = ISSET(rt->rt_flags, RTF_MPATH);
 
@@ -776,8 +775,7 @@ rtable_delete(unsigned int rtableid, struct sockaddr *dst,
 	srp_leave(&sr); /* an can't go away while we have the lock */
 
 	/* Make sure we've got a perfect match. */
-	if (an == NULL || an->an_plen != plen ||
-	    memcmp(an->an_dst, dst, dst->sa_len)) {
+	if (!an_match(an, dst, plen)) {
 		error = ESRCH;
 		goto leave;
 	}
@@ -796,7 +794,6 @@ rtable_delete(unsigned int rtableid, struct sockaddr *dst,
 		    rt_next);
 
 		mrt = SRPL_FIRST_LOCKED(&an->an_rtlist);
-		an->an_dst = mrt->rt_dest;
 		if (npaths == 2)
 			mrt->rt_flags &= ~RTF_MPATH;
 
@@ -912,8 +909,7 @@ rtable_mpath_reprio(unsigned int rtableid, struct sockaddr *dst,
 	srp_leave(&sr); /* an can't go away while we have the lock */
 
 	/* Make sure we've got a perfect match. */
-	if (an == NULL || an->an_plen != plen ||
-	    memcmp(an->an_dst, dst, dst->sa_len))
+	if (!an_match(an, dst, plen))
 		error = ESRCH;
 	else {
 		rtref(rt); /* keep rt alive in between remove and insert */
@@ -966,6 +962,26 @@ rtable_mpath_insert(struct art_node *an, struct rtentry *rt)
 }
 #endif /* SMALL_KERNEL */
 
+/*
+ * Returns 1 if ``an'' perfectly matches (``dst'', ``plen''), 0 otherwise.
+ */
+int
+an_match(struct art_node *an, struct sockaddr *dst, int plen)
+{
+	struct rtentry			*rt;
+	struct srp_ref			 sr;
+	int				 match;
+
+	if (an == NULL || an->an_plen != plen)
+		return (0);
+
+	rt = SRPL_FIRST(&sr, &an->an_rtlist);
+	match = (memcmp(rt->rt_dest, dst, dst->sa_len) == 0);
+	SRPL_LEAVE(&sr);
+
+	return (match);
+}
+
 void
 rtentry_ref(void *null, void *xrt)
 {