Improve error handling and recovery during state insertion

Reshuffle the code around a bit and greatly improve error handling
fixing a few bugs along the way.

Problem reported by and fix was written with Alexandr Nedvedicky.
OK henning

2 files changed, 45 insertions, 33 deletions

diff --git a/sys/net/pf.c b/sys/net/pf.c
index 34b107727ed..7f5f1aeec10 100644
--- a/sys/net/pf.c
+++ b/sys/net/pf.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: pf.c,v 1.916 2015/05/26 16:17:51 mikeb Exp $ */
+/*	$OpenBSD: pf.c,v 1.917 2015/06/05 13:22:34 mikeb Exp $ */
 
 /*
  * Copyright (c) 2001 Daniel Hartmeier
@@ -203,9 +203,9 @@ int			 pf_test_state_icmp(struct pf_pdesc *,
 u_int8_t		 pf_get_wscale(struct pf_pdesc *);
 u_int16_t		 pf_get_mss(struct pf_pdesc *);
 u_int16_t		 pf_calc_mss(struct pf_addr *, sa_family_t, int,
-				u_int16_t);
-void			 pf_set_rt_ifp(struct pf_state *,
-			    struct pf_addr *);
+			    u_int16_t);
+static __inline int	 pf_set_rt_ifp(struct pf_state *, struct pf_addr *,
+			    sa_family_t);
 struct pf_divert	*pf_get_divert(struct mbuf *);
 int			 pf_walk_option6(struct pf_pdesc *, struct ip6_hdr *,
 			    int, int, u_short *);
@@ -2958,32 +2958,39 @@ pf_calc_mss(struct pf_addr *addr, sa_family_t af, int rtableid, u_int16_t offer)
 	return (mss);
 }
 
-void
-pf_set_rt_ifp(struct pf_state *s, struct pf_addr *saddr)
+static __inline int
+pf_set_rt_ifp(struct pf_state *s, struct pf_addr *saddr, sa_family_t af)
 {
 	struct pf_rule *r = s->rule.ptr;
 	struct pf_src_node *sns[PF_SN_MAX];
+	int	rv;
 
 	s->rt_kif = NULL;
 	if (!r->rt)
-		return;
+		return (0);
+
 	bzero(sns, sizeof(sns));
-	switch (s->key[PF_SK_WIRE]->af) {
+	switch (af) {
 	case AF_INET:
-		pf_map_addr(AF_INET, r, saddr, &s->rt_addr, NULL, sns,
+		rv = pf_map_addr(AF_INET, r, saddr, &s->rt_addr, NULL, sns,
 		    &r->route, PF_SN_ROUTE);
-		s->rt_kif = r->route.kif;
-		s->natrule.ptr = r;
 		break;
 #ifdef INET6
 	case AF_INET6:
-		pf_map_addr(AF_INET6, r, saddr, &s->rt_addr, NULL, sns,
+		rv = pf_map_addr(AF_INET6, r, saddr, &s->rt_addr, NULL, sns,
 		    &r->route, PF_SN_ROUTE);
-		s->rt_kif = r->route.kif;
-		s->natrule.ptr = r;
 		break;
 #endif /* INET6 */
+	default:
+		rv = 1;
 	}
+
+	if (rv == 0) {
+		s->rt_kif = r->route.kif;
+		s->natrule.ptr = r;
+	}
+
+	return (rv);
 }
 
 u_int32_t
@@ -3557,16 +3564,6 @@ pf_create_state(struct pf_pdesc *pd, struct pf_rule *r, struct pf_rule *a,
 		goto csfailed;
 	}
 
-	if (pf_state_insert(BOUND_IFACE(r, pd->kif), skw, sks, s)) {
-		pf_state_key_detach(s, PF_SK_STACK);
-		pf_state_key_detach(s, PF_SK_WIRE);
-		*sks = *skw = NULL;
-		REASON_SET(&reason, PFRES_STATEINS);
-		goto csfailed;
-	} else
-		*sm = s;
-
-	/* attach src nodes late, otherwise cleanup on error nontrivial */
 	for (i = 0; i < PF_SN_MAX; i++)
 		if (sns[i] != NULL) {
 			struct pf_sn_item	*sni;
@@ -3574,17 +3571,26 @@ pf_create_state(struct pf_pdesc *pd, struct pf_rule *r, struct pf_rule *a,
 			sni = pool_get(&pf_sn_item_pl, PR_NOWAIT);
 			if (sni == NULL) {
 				REASON_SET(&reason, PFRES_MEMORY);
-				pf_src_tree_remove_state(s);
-				STATE_DEC_COUNTERS(s);
-				pool_put(&pf_state_pl, s);
-				return (PF_DROP);
+				goto csfailed;
 			}
 			sni->sn = sns[i];
 			SLIST_INSERT_HEAD(&s->src_nodes, sni, next);
 			sni->sn->states++;
 		}
 
-	pf_set_rt_ifp(s, pd->src);	/* needs s->state_key set */
+	if (pf_set_rt_ifp(s, pd->src, (*skw)->af) != 0) {
+		REASON_SET(&reason, PFRES_NOROUTE);
+		goto csfailed;
+	}
+
+	if (pf_state_insert(BOUND_IFACE(r, pd->kif), skw, sks, s)) {
+		pf_detach_state(s);
+		*sks = *skw = NULL;
+		REASON_SET(&reason, PFRES_STATEINS);
+		goto csfailed;
+	} else
+		*sm = s;
+
 	if (tag > 0) {
 		pf_tag_ref(tag);
 		s->tag = tag;
@@ -3612,12 +3618,16 @@ pf_create_state(struct pf_pdesc *pd, struct pf_rule *r, struct pf_rule *a,
 	return (PF_PASS);
 
 csfailed:
+	if (s) {
+		pf_normalize_tcp_cleanup(s);	/* safe even w/o init */
+		pf_src_tree_remove_state(s);
+	}
+
 	for (i = 0; i < PF_SN_MAX; i++)
 		if (sns[i] != NULL)
 			pf_remove_src_node(sns[i]);
+
 	if (s) {
-		pf_normalize_tcp_cleanup(s);	/* safe even w/o init */
-		pf_src_tree_remove_state(s);
 		STATE_DEC_COUNTERS(s);
 		pool_put(&pf_state_pl, s);
 	}
diff --git a/sys/net/pfvar.h b/sys/net/pfvar.h
index 2a26374066b..69541b70a72 100644
--- a/sys/net/pfvar.h
+++ b/sys/net/pfvar.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: pfvar.h,v 1.414 2015/04/11 13:00:12 dlg Exp $ */
+/*	$OpenBSD: pfvar.h,v 1.415 2015/06/05 13:22:34 mikeb Exp $ */
 
 /*
  * Copyright (c) 2001 Daniel Hartmeier
@@ -1316,7 +1316,8 @@ struct pf_pdesc {
 #define PFRES_SRCLIMIT	13		/* Source node/conn limit */
 #define PFRES_SYNPROXY	14		/* SYN proxy */
 #define PFRES_TRANSLATE	15		/* No translation address available */
-#define PFRES_MAX	16		/* total+1 */
+#define PFRES_NOROUTE	16		/* No route found for PBR action */
+#define PFRES_MAX	17		/* total+1 */
 
 #define PFRES_NAMES { \
 	"match", \
@@ -1335,6 +1336,7 @@ struct pf_pdesc {
 	"src-limit", \
 	"synproxy", \
 	"translate", \
+	"no-route", \
 	NULL \
 }