Skip site navigation (1)Skip section navigation (2)
Date:      Tue, 18 Sep 2012 10:54:56 +0000 (UTC)
From:      Gleb Smirnoff <glebius@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org
Subject:   svn commit: r240641 - in head/sys: net netpfil/pf
Message-ID:  <201209181054.q8IAsug5075250@svn.freebsd.org>

next in thread | raw e-mail | index | archive | help
Author: glebius
Date: Tue Sep 18 10:54:56 2012
New Revision: 240641
URL: http://svn.freebsd.org/changeset/base/240641

Log:
  Make ruleset anchors in pf(4) reentrant. We've got two problems here:
  
  1) Ruleset parser uses a global variable for anchor stack.
  2) When processing a wildcard anchor, matching anchors are marked.
  
  To fix the first one:
  
  o Allocate anchor processing stack on stack. To make this allocation
    as small as possible, following measures taken:
    - Maximum stack size reduced from 64 to 32.
    - The struct pf_anchor_stackframe trimmed by one pointer - parent.
      We can always obtain the parent via the rule pointer.
    - When pf_test_rule() calls pf_get_translation(), the former lends
      its stack to the latter, to avoid recursive allocation 32 entries.
  
  The second one appeared more tricky. The code, that marks anchors was
  added in OpenBSD rev. 1.516 of pf.c. According to commit log, the idea
  is to enable the "quick" keyword on an anchor rule. The feature isn't
  documented anywhere. The most obscure part of the 1.516 was that code
  examines the "match" mark on a just processed child, which couldn't be
  put here by current frame. Since this wasn't documented even in the
  commit message and functionality of this is not clear to me, I decided
  to drop this examination for now. The rest of 1.516 is redone in a
  thread safe manner - the mark isn't put on the anchor itself, but on
  current stack frame. To avoid growing stack frame, we utilize LSB
  from the rule pointer, relying on kernel malloc(9) returning pointer
  aligned addresses.
  
  Discussed with:		dhartmei

Modified:
  head/sys/net/pfvar.h
  head/sys/netpfil/pf/pf.c
  head/sys/netpfil/pf/pf_lb.c

Modified: head/sys/net/pfvar.h
==============================================================================
--- head/sys/net/pfvar.h	Tue Sep 18 10:52:46 2012	(r240640)
+++ head/sys/net/pfvar.h	Tue Sep 18 10:54:56 2012	(r240641)
@@ -989,7 +989,7 @@ struct pf_anchor {
 	char			 path[MAXPATHLEN];
 	struct pf_ruleset	 ruleset;
 	int			 refcnt;	/* anchor rules */
-	int			 match;
+	int			 match;	/* XXX: used for pfctl black magic */
 };
 RB_PROTOTYPE(pf_anchor_global, pf_anchor, entry_global, pf_anchor_compare);
 RB_PROTOTYPE(pf_anchor_node, pf_anchor, entry_node, pf_anchor_compare);
@@ -1019,6 +1019,8 @@ RB_PROTOTYPE(pf_anchor_node, pf_anchor, 
 				 PFR_TFLAG_REFDANCHOR	| \
 				 PFR_TFLAG_COUNTERS)
 
+struct pf_anchor_stackframe;
+
 struct pfr_table {
 	char			 pfrt_anchor[MAXPATHLEN];
 	char			 pfrt_name[PF_TABLE_NAME_SIZE];
@@ -1938,11 +1940,12 @@ int	pf_osfp_match(struct pf_osfp_enlist 
 #ifdef _KERNEL
 void			 pf_print_host(struct pf_addr *, u_int16_t, u_int8_t);
 
-void			 pf_step_into_anchor(int *, struct pf_ruleset **, int,
-			    struct pf_rule **, struct pf_rule **, int *);
-int			 pf_step_out_of_anchor(int *, struct pf_ruleset **,
-			    int, struct pf_rule **, struct pf_rule **,
-			    int *);
+void			 pf_step_into_anchor(struct pf_anchor_stackframe *, int *,
+			    struct pf_ruleset **, int, struct pf_rule **,
+			    struct pf_rule **, int *);
+int			 pf_step_out_of_anchor(struct pf_anchor_stackframe *, int *,
+			    struct pf_ruleset **, int, struct pf_rule **,
+			    struct pf_rule **, int *);
 
 int			 pf_map_addr(u_int8_t, struct pf_rule *,
 			    struct pf_addr *, struct pf_addr *,
@@ -1951,7 +1954,7 @@ struct pf_rule		*pf_get_translation(stru
 			    int, int, struct pfi_kif *, struct pf_src_node **,
 			    struct pf_state_key **, struct pf_state_key **,
 			    struct pf_addr *, struct pf_addr *,
-			    u_int16_t, u_int16_t);
+			    uint16_t, uint16_t, struct pf_anchor_stackframe *);
 
 struct pf_state_key	*pf_state_key_setup(struct pf_pdesc *, struct pf_addr *,
 			    struct pf_addr *, u_int16_t, u_int16_t);

Modified: head/sys/netpfil/pf/pf.c
==============================================================================
--- head/sys/netpfil/pf/pf.c	Tue Sep 18 10:52:46 2012	(r240640)
+++ head/sys/netpfil/pf/pf.c	Tue Sep 18 10:54:56 2012	(r240641)
@@ -127,15 +127,6 @@ VNET_DEFINE(int,			 pf_tcp_secret_init);
 VNET_DEFINE(int,			 pf_tcp_iss_off);
 #define	V_pf_tcp_iss_off		 VNET(pf_tcp_iss_off)
 
-struct pf_anchor_stackframe {
-	struct pf_ruleset		*rs;
-	struct pf_rule			*r;
-	struct pf_anchor_node		*parent;
-	struct pf_anchor		*child;
-};
-VNET_DEFINE(struct pf_anchor_stackframe, pf_anchor_stack[64]);
-#define	V_pf_anchor_stack		 VNET(pf_anchor_stack)
-
 /*
  * Queue for pf_intr() sends.
  */
@@ -2461,37 +2452,56 @@ pf_tag_packet(struct mbuf *m, struct pf_
 	return (0);
 }
 
+#define	PF_ANCHOR_STACKSIZE	32
+struct pf_anchor_stackframe {
+	struct pf_ruleset	*rs;
+	struct pf_rule		*r;	/* XXX: + match bit */
+	struct pf_anchor	*child;
+};
+
+/*
+ * XXX: We rely on malloc(9) returning pointer aligned addresses.
+ */
+#define	PF_ANCHORSTACK_MATCH	0x00000001
+#define	PF_ANCHORSTACK_MASK	(PF_ANCHORSTACK_MATCH)
+
+#define	PF_ANCHOR_MATCH(f)	((uintptr_t)(f)->r & PF_ANCHORSTACK_MATCH)
+#define	PF_ANCHOR_RULE(f)	(struct pf_rule *)			\
+				((uintptr_t)(f)->r & ~PF_ANCHORSTACK_MASK)
+#define	PF_ANCHOR_SET_MATCH(f)	do { (f)->r = (void *) 			\
+				((uintptr_t)(f)->r | PF_ANCHORSTACK_MATCH);  \
+} while (0)
+
 void
-pf_step_into_anchor(int *depth, struct pf_ruleset **rs, int n,
-    struct pf_rule **r, struct pf_rule **a, int *match)
+pf_step_into_anchor(struct pf_anchor_stackframe *stack, int *depth,
+    struct pf_ruleset **rs, int n, struct pf_rule **r, struct pf_rule **a,
+    int *match)
 {
 	struct pf_anchor_stackframe	*f;
 
 	PF_RULES_RASSERT();
 
-	(*r)->anchor->match = 0;
 	if (match)
 		*match = 0;
-	if (*depth >= sizeof(V_pf_anchor_stack) /
-	    sizeof(V_pf_anchor_stack[0])) {
-		printf("pf_step_into_anchor: stack overflow\n");
+	if (*depth >= PF_ANCHOR_STACKSIZE) {
+		printf("%s: anchor stack overflow on %s\n",
+		    __func__, (*r)->anchor->name);
 		*r = TAILQ_NEXT(*r, entries);
 		return;
 	} else if (*depth == 0 && a != NULL)
 		*a = *r;
-	f = V_pf_anchor_stack + (*depth)++;
+	f = stack + (*depth)++;
 	f->rs = *rs;
 	f->r = *r;
 	if ((*r)->anchor_wildcard) {
-		f->parent = &(*r)->anchor->children;
-		if ((f->child = RB_MIN(pf_anchor_node, f->parent)) ==
-		    NULL) {
+		struct pf_anchor_node *parent = &(*r)->anchor->children;
+
+		if ((f->child = RB_MIN(pf_anchor_node, parent)) == NULL) {
 			*r = NULL;
 			return;
 		}
 		*rs = &f->child->ruleset;
 	} else {
-		f->parent = NULL;
 		f->child = NULL;
 		*rs = &(*r)->anchor->ruleset;
 	}
@@ -2499,10 +2509,12 @@ pf_step_into_anchor(int *depth, struct p
 }
 
 int
-pf_step_out_of_anchor(int *depth, struct pf_ruleset **rs, int n,
-    struct pf_rule **r, struct pf_rule **a, int *match)
+pf_step_out_of_anchor(struct pf_anchor_stackframe *stack, int *depth,
+    struct pf_ruleset **rs, int n, struct pf_rule **r, struct pf_rule **a,
+    int *match)
 {
 	struct pf_anchor_stackframe	*f;
+	struct pf_rule *fr;
 	int quick = 0;
 
 	PF_RULES_RASSERT();
@@ -2510,14 +2522,26 @@ pf_step_out_of_anchor(int *depth, struct
 	do {
 		if (*depth <= 0)
 			break;
-		f = V_pf_anchor_stack + *depth - 1;
-		if (f->parent != NULL && f->child != NULL) {
-			if (f->child->match ||
-			    (match != NULL && *match)) {
-				f->r->anchor->match = 1;
+		f = stack + *depth - 1;
+		fr = PF_ANCHOR_RULE(f);
+		if (f->child != NULL) {
+			struct pf_anchor_node *parent;
+
+			/*
+			 * This block traverses through
+			 * a wildcard anchor.
+			 */
+			parent = &fr->anchor->children;
+			if (match != NULL && *match) {
+				/*
+				 * If any of "*" matched, then
+				 * "foo/ *" matched, mark frame
+				 * appropriately.
+				 */
+				PF_ANCHOR_SET_MATCH(f);
 				*match = 0;
 			}
-			f->child = RB_NEXT(pf_anchor_node, f->parent, f->child);
+			f->child = RB_NEXT(pf_anchor_node, parent, f->child);
 			if (f->child != NULL) {
 				*rs = &f->child->ruleset;
 				*r = TAILQ_FIRST((*rs)->rules[n].active.ptr);
@@ -2531,9 +2555,9 @@ pf_step_out_of_anchor(int *depth, struct
 		if (*depth == 0 && a != NULL)
 			*a = NULL;
 		*rs = f->rs;
-		if (f->r->anchor->match || (match != NULL && *match))
-			quick = f->r->quick;
-		*r = TAILQ_NEXT(f->r, entries);
+		if (PF_ANCHOR_MATCH(f) || (match != NULL && *match))
+			quick = fr->quick;
+		*r = TAILQ_NEXT(fr, entries);
 	} while (*r == NULL);
 
 	return (quick);
@@ -2887,6 +2911,7 @@ pf_test_rule(struct pf_rule **rm, struct
 	u_int16_t		 sport = 0, dport = 0;
 	u_int16_t		 bproto_sum = 0, bip_sum = 0;
 	u_int8_t		 icmptype = 0, icmpcode = 0;
+	struct pf_anchor_stackframe	anchor_stack[PF_ANCHOR_STACKSIZE];
 
 	PF_RULES_RASSERT();
 
@@ -2950,7 +2975,7 @@ pf_test_rule(struct pf_rule **rm, struct
 
 	/* check packet for BINAT/NAT/RDR */
 	if ((nr = pf_get_translation(pd, m, off, direction, kif, &nsn, &sk,
-	    &nk, saddr, daddr, sport, dport)) != NULL) {
+	    &nk, saddr, daddr, sport, dport, anchor_stack)) != NULL) {
 		KASSERT(sk != NULL, ("%s: null sk", __func__));
 		KASSERT(nk != NULL, ("%s: null nk", __func__));
 
@@ -3150,11 +3175,12 @@ pf_test_rule(struct pf_rule **rm, struct
 					break;
 				r = TAILQ_NEXT(r, entries);
 			} else
-				pf_step_into_anchor(&asd, &ruleset,
-				    PF_RULESET_FILTER, &r, &a, &match);
+				pf_step_into_anchor(anchor_stack, &asd,
+				    &ruleset, PF_RULESET_FILTER, &r, &a,
+				    &match);
 		}
-		if (r == NULL && pf_step_out_of_anchor(&asd, &ruleset,
-		    PF_RULESET_FILTER, &r, &a, &match))
+		if (r == NULL && pf_step_out_of_anchor(anchor_stack, &asd,
+		    &ruleset, PF_RULESET_FILTER, &r, &a, &match))
 			break;
 	}
 	r = *rm;
@@ -3527,6 +3553,7 @@ pf_test_fragment(struct pf_rule **rm, in
 	int			 tag = -1;
 	int			 asd = 0;
 	int			 match = 0;
+	struct pf_anchor_stackframe	anchor_stack[PF_ANCHOR_STACKSIZE];
 
 	PF_RULES_RASSERT();
 
@@ -3577,11 +3604,12 @@ pf_test_fragment(struct pf_rule **rm, in
 					break;
 				r = TAILQ_NEXT(r, entries);
 			} else
-				pf_step_into_anchor(&asd, &ruleset,
-				    PF_RULESET_FILTER, &r, &a, &match);
+				pf_step_into_anchor(anchor_stack, &asd,
+				    &ruleset, PF_RULESET_FILTER, &r, &a,
+				    &match);
 		}
-		if (r == NULL && pf_step_out_of_anchor(&asd, &ruleset,
-		    PF_RULESET_FILTER, &r, &a, &match))
+		if (r == NULL && pf_step_out_of_anchor(anchor_stack, &asd,
+		    &ruleset, PF_RULESET_FILTER, &r, &a, &match))
 			break;
 	}
 	r = *rm;

Modified: head/sys/netpfil/pf/pf_lb.c
==============================================================================
--- head/sys/netpfil/pf/pf_lb.c	Tue Sep 18 10:52:46 2012	(r240640)
+++ head/sys/netpfil/pf/pf_lb.c	Tue Sep 18 10:54:56 2012	(r240641)
@@ -58,7 +58,7 @@ static void		 pf_hash(struct pf_addr *, 
 static struct pf_rule	*pf_match_translation(struct pf_pdesc *, struct mbuf *,
 			    int, int, struct pfi_kif *,
 			    struct pf_addr *, u_int16_t, struct pf_addr *,
-			    u_int16_t, int);
+			    uint16_t, int, struct pf_anchor_stackframe *);
 static int		 pf_get_sport(sa_family_t, u_int8_t, struct pf_rule *,
 			    struct pf_addr *, struct pf_addr *, u_int16_t,
 			    struct pf_addr *, u_int16_t*, u_int16_t, u_int16_t,
@@ -124,7 +124,8 @@ pf_hash(struct pf_addr *inaddr, struct p
 static struct pf_rule *
 pf_match_translation(struct pf_pdesc *pd, struct mbuf *m, int off,
     int direction, struct pfi_kif *kif, struct pf_addr *saddr, u_int16_t sport,
-    struct pf_addr *daddr, u_int16_t dport, int rs_num)
+    struct pf_addr *daddr, uint16_t dport, int rs_num,
+    struct pf_anchor_stackframe *anchor_stack)
 {
 	struct pf_rule		*r, *rm = NULL;
 	struct pf_ruleset	*ruleset = NULL;
@@ -189,12 +190,12 @@ pf_match_translation(struct pf_pdesc *pd
 			if (r->anchor == NULL) {
 				rm = r;
 			} else
-				pf_step_into_anchor(&asd, &ruleset, rs_num,
-				    &r, NULL, NULL);
+				pf_step_into_anchor(anchor_stack, &asd,
+				    &ruleset, rs_num, &r, NULL, NULL);
 		}
 		if (r == NULL)
-			pf_step_out_of_anchor(&asd, &ruleset, rs_num, &r,
-			    NULL, NULL);
+			pf_step_out_of_anchor(anchor_stack, &asd, &ruleset,
+			    rs_num, &r, NULL, NULL);
 	}
 
 	if (tag > 0 && pf_tag_packet(m, pd, tag))
@@ -499,7 +500,7 @@ pf_get_translation(struct pf_pdesc *pd, 
     struct pfi_kif *kif, struct pf_src_node **sn,
     struct pf_state_key **skp, struct pf_state_key **nkp,
     struct pf_addr *saddr, struct pf_addr *daddr,
-    u_int16_t sport, u_int16_t dport)
+    uint16_t sport, uint16_t dport, struct pf_anchor_stackframe *anchor_stack)
 {
 	struct pf_rule	*r = NULL;
 	struct pf_addr	*naddr;
@@ -511,16 +512,18 @@ pf_get_translation(struct pf_pdesc *pd, 
 
 	if (direction == PF_OUT) {
 		r = pf_match_translation(pd, m, off, direction, kif, saddr,
-		    sport, daddr, dport, PF_RULESET_BINAT);
+		    sport, daddr, dport, PF_RULESET_BINAT, anchor_stack);
 		if (r == NULL)
 			r = pf_match_translation(pd, m, off, direction, kif,
-			    saddr, sport, daddr, dport, PF_RULESET_NAT);
+			    saddr, sport, daddr, dport, PF_RULESET_NAT,
+			    anchor_stack);
 	} else {
 		r = pf_match_translation(pd, m, off, direction, kif, saddr,
-		    sport, daddr, dport, PF_RULESET_RDR);
+		    sport, daddr, dport, PF_RULESET_RDR, anchor_stack);
 		if (r == NULL)
 			r = pf_match_translation(pd, m, off, direction, kif,
-			    saddr, sport, daddr, dport, PF_RULESET_BINAT);
+			    saddr, sport, daddr, dport, PF_RULESET_BINAT,
+			    anchor_stack);
 	}
 
 	if (r == NULL)



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201209181054.q8IAsug5075250>