Skip site navigation (1)Skip section navigation (2)
Date:      Mon, 18 Feb 2019 10:51:27 +0000 (UTC)
From:      "Andrey V. Elsukov" <ae@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-12@freebsd.org
Subject:   svn commit: r344251 - stable/12/sys/netpfil/ipfw
Message-ID:  <201902181051.x1IApRCD016366@repo.freebsd.org>

next in thread | raw e-mail | index | archive | help
Author: ae
Date: Mon Feb 18 10:51:27 2019
New Revision: 344251
URL: https://svnweb.freebsd.org/changeset/base/344251

Log:
  MFC r344018:
    Remove `set' field from state structure and use set from parent rule.
  
    Initially it was introduced because parent rule pointer could be freed,
    and rule's information could become inaccessible. In r341471 this was
    changed. And now we don't need this information, and also it can become
    stale. E.g. rule can be moved from one set to another. This can lead
    to parent's set and state's set will not match. In this case it is
    possible that static rule will be freed, but dynamic state will not.
    This can happen when `ipfw delete set N` command is used to delete
    rules, that were moved to another set.
    To fix the problem we will use the set number from parent rule.
  
    Obtained from:	Yandex LLC
    Sponsored by:	Yandex LLC

Modified:
  stable/12/sys/netpfil/ipfw/ip_fw_dynamic.c
Directory Properties:
  stable/12/   (props changed)

Modified: stable/12/sys/netpfil/ipfw/ip_fw_dynamic.c
==============================================================================
--- stable/12/sys/netpfil/ipfw/ip_fw_dynamic.c	Mon Feb 18 10:13:52 2019	(r344250)
+++ stable/12/sys/netpfil/ipfw/ip_fw_dynamic.c	Mon Feb 18 10:51:27 2019	(r344251)
@@ -135,9 +135,8 @@ struct dyn_data {
 
 	uint32_t	hashval;	/* hash value used for hash resize */
 	uint16_t	fibnum;		/* fib used to send keepalives */
-	uint8_t		_pad[2];
+	uint8_t		_pad[3];
 	uint8_t		flags;		/* internal flags */
-	uint8_t		set;		/* parent rule set number */
 	uint16_t	rulenum;	/* parent rule number */
 	uint32_t	ruleid;		/* parent rule id */
 
@@ -163,8 +162,7 @@ struct dyn_data {
 struct dyn_parent {
 	void		*parent;	/* pointer to parent rule */
 	uint32_t	count;		/* number of linked states */
-	uint8_t		_pad;
-	uint8_t		set;		/* parent rule set number */
+	uint8_t		_pad[2];
 	uint16_t	rulenum;	/* parent rule number */
 	uint32_t	ruleid;		/* parent rule id */
 	uint32_t	hashval;	/* hash value used for hash resize */
@@ -507,7 +505,7 @@ static int dyn_lookup_ipv6_state_locked(const struct i
     uint32_t, const void *, int, uint32_t, uint16_t);
 static struct dyn_ipv6_state *dyn_alloc_ipv6_state(
     const struct ipfw_flow_id *, uint32_t, uint16_t, uint8_t);
-static int dyn_add_ipv6_state(void *, uint32_t, uint16_t, uint8_t,
+static int dyn_add_ipv6_state(void *, uint32_t, uint16_t,
     const struct ipfw_flow_id *, uint32_t, const void *, int, uint32_t,
     struct ipfw_dyn_info *, uint16_t, uint16_t, uint8_t);
 static void dyn_export_ipv6_state(const struct dyn_ipv6_state *,
@@ -528,8 +526,7 @@ static struct dyn_ipv6_state *dyn_lookup_ipv6_parent_l
     const struct ipfw_flow_id *, uint32_t, const void *, uint32_t, uint16_t,
     uint32_t);
 static struct dyn_ipv6_state *dyn_add_ipv6_parent(void *, uint32_t, uint16_t,
-    uint8_t, const struct ipfw_flow_id *, uint32_t, uint32_t, uint32_t,
-    uint16_t);
+    const struct ipfw_flow_id *, uint32_t, uint32_t, uint32_t, uint16_t);
 #endif /* INET6 */
 
 /* Functions to work with limit states */
@@ -540,17 +537,17 @@ static struct dyn_ipv4_state *dyn_lookup_ipv4_parent(
 static struct dyn_ipv4_state *dyn_lookup_ipv4_parent_locked(
     const struct ipfw_flow_id *, const void *, uint32_t, uint16_t, uint32_t);
 static struct dyn_parent *dyn_alloc_parent(void *, uint32_t, uint16_t,
-    uint8_t, uint32_t);
+    uint32_t);
 static struct dyn_ipv4_state *dyn_add_ipv4_parent(void *, uint32_t, uint16_t,
-    uint8_t, const struct ipfw_flow_id *, uint32_t, uint32_t, uint16_t);
+    const struct ipfw_flow_id *, uint32_t, uint32_t, uint16_t);
 
 static void dyn_tick(void *);
 static void dyn_expire_states(struct ip_fw_chain *, ipfw_range_tlv *);
 static void dyn_free_states(struct ip_fw_chain *);
-static void dyn_export_parent(const struct dyn_parent *, uint16_t,
+static void dyn_export_parent(const struct dyn_parent *, uint16_t, uint8_t,
     ipfw_dyn_rule *);
 static void dyn_export_data(const struct dyn_data *, uint16_t, uint8_t,
-    ipfw_dyn_rule *);
+    uint8_t, ipfw_dyn_rule *);
 static uint32_t dyn_update_tcp_state(struct dyn_data *,
     const struct ipfw_flow_id *, const struct tcphdr *, int);
 static void dyn_update_proto_state(struct dyn_data *,
@@ -563,7 +560,7 @@ static int dyn_lookup_ipv4_state_locked(const struct i
     const void *, int, uint32_t, uint16_t);
 static struct dyn_ipv4_state *dyn_alloc_ipv4_state(
     const struct ipfw_flow_id *, uint16_t, uint8_t);
-static int dyn_add_ipv4_state(void *, uint32_t, uint16_t, uint8_t,
+static int dyn_add_ipv4_state(void *, uint32_t, uint16_t,
     const struct ipfw_flow_id *, const void *, int, uint32_t,
     struct ipfw_dyn_info *, uint16_t, uint16_t, uint8_t);
 static void dyn_export_ipv4_state(const struct dyn_ipv4_state *,
@@ -1460,7 +1457,7 @@ ipfw_dyn_lookup_state(const struct ip_fw_args *args, c
 
 static struct dyn_parent *
 dyn_alloc_parent(void *parent, uint32_t ruleid, uint16_t rulenum,
-    uint8_t set, uint32_t hashval)
+    uint32_t hashval)
 {
 	struct dyn_parent *limit;
 
@@ -1479,7 +1476,6 @@ dyn_alloc_parent(void *parent, uint32_t ruleid, uint16
 	limit->parent = parent;
 	limit->ruleid = ruleid;
 	limit->rulenum = rulenum;
-	limit->set = set;
 	limit->hashval = hashval;
 	limit->expire = time_uptime + V_dyn_short_lifetime;
 	return (limit);
@@ -1487,7 +1483,7 @@ dyn_alloc_parent(void *parent, uint32_t ruleid, uint16
 
 static struct dyn_data *
 dyn_alloc_dyndata(void *parent, uint32_t ruleid, uint16_t rulenum,
-    uint8_t set, const struct ipfw_flow_id *pkt, const void *ulp, int pktlen,
+    const struct ipfw_flow_id *pkt, const void *ulp, int pktlen,
     uint32_t hashval, uint16_t fibnum)
 {
 	struct dyn_data *data;
@@ -1506,7 +1502,6 @@ dyn_alloc_dyndata(void *parent, uint32_t ruleid, uint1
 	data->parent = parent;
 	data->ruleid = ruleid;
 	data->rulenum = rulenum;
-	data->set = set;
 	data->fibnum = fibnum;
 	data->hashval = hashval;
 	data->expire = time_uptime + V_dyn_syn_lifetime;
@@ -1543,8 +1538,8 @@ dyn_alloc_ipv4_state(const struct ipfw_flow_id *pkt, u
  */
 static struct dyn_ipv4_state *
 dyn_add_ipv4_parent(void *rule, uint32_t ruleid, uint16_t rulenum,
-    uint8_t set, const struct ipfw_flow_id *pkt, uint32_t hashval,
-    uint32_t version, uint16_t kidx)
+    const struct ipfw_flow_id *pkt, uint32_t hashval, uint32_t version,
+    uint16_t kidx)
 {
 	struct dyn_ipv4_state *s;
 	struct dyn_parent *limit;
@@ -1571,7 +1566,7 @@ dyn_add_ipv4_parent(void *rule, uint32_t ruleid, uint1
 		}
 	}
 
-	limit = dyn_alloc_parent(rule, ruleid, rulenum, set, hashval);
+	limit = dyn_alloc_parent(rule, ruleid, rulenum, hashval);
 	if (limit == NULL) {
 		DYN_BUCKET_UNLOCK(bucket);
 		return (NULL);
@@ -1596,7 +1591,7 @@ dyn_add_ipv4_parent(void *rule, uint32_t ruleid, uint1
 
 static int
 dyn_add_ipv4_state(void *parent, uint32_t ruleid, uint16_t rulenum,
-    uint8_t set, const struct ipfw_flow_id *pkt, const void *ulp, int pktlen,
+    const struct ipfw_flow_id *pkt, const void *ulp, int pktlen,
     uint32_t hashval, struct ipfw_dyn_info *info, uint16_t fibnum,
     uint16_t kidx, uint8_t type)
 {
@@ -1621,7 +1616,7 @@ dyn_add_ipv4_state(void *parent, uint32_t ruleid, uint
 		}
 	}
 
-	data = dyn_alloc_dyndata(parent, ruleid, rulenum, set, pkt, ulp,
+	data = dyn_alloc_dyndata(parent, ruleid, rulenum, pkt, ulp,
 	    pktlen, hashval, fibnum);
 	if (data == NULL) {
 		DYN_BUCKET_UNLOCK(bucket);
@@ -1674,8 +1669,8 @@ dyn_alloc_ipv6_state(const struct ipfw_flow_id *pkt, u
  */
 static struct dyn_ipv6_state *
 dyn_add_ipv6_parent(void *rule, uint32_t ruleid, uint16_t rulenum,
-    uint8_t set, const struct ipfw_flow_id *pkt, uint32_t zoneid,
-    uint32_t hashval, uint32_t version, uint16_t kidx)
+    const struct ipfw_flow_id *pkt, uint32_t zoneid, uint32_t hashval,
+    uint32_t version, uint16_t kidx)
 {
 	struct dyn_ipv6_state *s;
 	struct dyn_parent *limit;
@@ -1702,7 +1697,7 @@ dyn_add_ipv6_parent(void *rule, uint32_t ruleid, uint1
 		}
 	}
 
-	limit = dyn_alloc_parent(rule, ruleid, rulenum, set, hashval);
+	limit = dyn_alloc_parent(rule, ruleid, rulenum, hashval);
 	if (limit == NULL) {
 		DYN_BUCKET_UNLOCK(bucket);
 		return (NULL);
@@ -1727,8 +1722,8 @@ dyn_add_ipv6_parent(void *rule, uint32_t ruleid, uint1
 
 static int
 dyn_add_ipv6_state(void *parent, uint32_t ruleid, uint16_t rulenum,
-    uint8_t set, const struct ipfw_flow_id *pkt, uint32_t zoneid,
-    const void *ulp, int pktlen, uint32_t hashval, struct ipfw_dyn_info *info,
+    const struct ipfw_flow_id *pkt, uint32_t zoneid, const void *ulp,
+    int pktlen, uint32_t hashval, struct ipfw_dyn_info *info,
     uint16_t fibnum, uint16_t kidx, uint8_t type)
 {
 	struct dyn_ipv6_state *s;
@@ -1752,7 +1747,7 @@ dyn_add_ipv6_state(void *parent, uint32_t ruleid, uint
 		}
 	}
 
-	data = dyn_alloc_dyndata(parent, ruleid, rulenum, set, pkt, ulp,
+	data = dyn_alloc_dyndata(parent, ruleid, rulenum, pkt, ulp,
 	    pktlen, hashval, fibnum);
 	if (data == NULL) {
 		DYN_BUCKET_UNLOCK(bucket);
@@ -1802,8 +1797,7 @@ dyn_get_parent_state(const struct ipfw_flow_id *pkt, u
 			DYNSTATE_CRITICAL_EXIT();
 
 			s = dyn_add_ipv4_parent(rule, rule->id,
-			    rule->rulenum, rule->set, pkt, hashval,
-			    version, kidx);
+			    rule->rulenum, pkt, hashval, version, kidx);
 			if (s == NULL)
 				return (NULL);
 			/* Now we are in critical section again. */
@@ -1826,8 +1820,8 @@ dyn_get_parent_state(const struct ipfw_flow_id *pkt, u
 			DYNSTATE_CRITICAL_EXIT();
 
 			s = dyn_add_ipv6_parent(rule, rule->id,
-			    rule->rulenum, rule->set, pkt, zoneid, hashval,
-			    version, kidx);
+			    rule->rulenum, pkt, zoneid, hashval, version,
+			    kidx);
 			if (s == NULL)
 				return (NULL);
 			/* Now we are in critical section again. */
@@ -1870,8 +1864,7 @@ dyn_get_parent_state(const struct ipfw_flow_id *pkt, u
 
 static int
 dyn_install_state(const struct ipfw_flow_id *pkt, uint32_t zoneid,
-    uint16_t fibnum, const void *ulp, int pktlen, void *rule,
-    uint32_t ruleid, uint16_t rulenum, uint8_t set,
+    uint16_t fibnum, const void *ulp, int pktlen, struct ip_fw *rule,
     struct ipfw_dyn_info *info, uint32_t limit, uint16_t limit_mask,
     uint16_t kidx, uint8_t type)
 {
@@ -1935,11 +1928,11 @@ dyn_install_state(const struct ipfw_flow_id *pkt, uint
 
 	hashval = hash_packet(pkt);
 	if (IS_IP4_FLOW_ID(pkt))
-		ret = dyn_add_ipv4_state(rule, ruleid, rulenum, set, pkt,
+		ret = dyn_add_ipv4_state(rule, rule->id, rule->rulenum, pkt,
 		    ulp, pktlen, hashval, info, fibnum, kidx, type);
 #ifdef INET6
 	else if (IS_IP6_FLOW_ID(pkt))
-		ret = dyn_add_ipv6_state(rule, ruleid, rulenum, set, pkt,
+		ret = dyn_add_ipv6_state(rule, rule->id, rule->rulenum, pkt,
 		    zoneid, ulp, pktlen, hashval, info, fibnum, kidx, type);
 #endif /* INET6 */
 	else
@@ -2012,8 +2005,8 @@ ipfw_dyn_install_state(struct ip_fw_chain *chain, stru
 #ifdef INET6
 	    IS_IP6_FLOW_ID(&args->f_id) ? dyn_getscopeid(args):
 #endif
-	    0, M_GETFIB(args->m), ulp, pktlen, rule, rule->id, rule->rulenum,
-	    rule->set, info, limit, limit_mask, cmd->o.arg1, cmd->o.opcode));
+	    0, M_GETFIB(args->m), ulp, pktlen, rule, info, limit,
+	    limit_mask, cmd->o.arg1, cmd->o.opcode));
 }
 
 /*
@@ -2198,17 +2191,19 @@ dyn_match_ipv4_state(struct ip_fw_chain *ch, struct dy
 	struct ip_fw *rule;
 	int ret;
 
-	if (s->type == O_LIMIT_PARENT)
-		return (dyn_match_range(s->limit->rulenum,
-		    s->limit->set, rt));
+	if (s->type == O_LIMIT_PARENT) {
+		rule = s->limit->parent;
+		return (dyn_match_range(s->limit->rulenum, rule->set, rt));
+	}
 
-	ret = dyn_match_range(s->data->rulenum, s->data->set, rt);
-	if (ret == 0 || V_dyn_keep_states == 0 || ret > 1)
-		return (ret);
-
 	rule = s->data->parent;
 	if (s->type == O_LIMIT)
 		rule = ((struct dyn_ipv4_state *)rule)->limit->parent;
+
+	ret = dyn_match_range(s->data->rulenum, rule->set, rt);
+	if (ret == 0 || V_dyn_keep_states == 0 || ret > 1)
+		return (ret);
+
 	dyn_acquire_rule(ch, s->data, rule, s->kidx);
 	return (0);
 }
@@ -2221,17 +2216,19 @@ dyn_match_ipv6_state(struct ip_fw_chain *ch, struct dy
 	struct ip_fw *rule;
 	int ret;
 
-	if (s->type == O_LIMIT_PARENT)
-		return (dyn_match_range(s->limit->rulenum,
-		    s->limit->set, rt));
+	if (s->type == O_LIMIT_PARENT) {
+		rule = s->limit->parent;
+		return (dyn_match_range(s->limit->rulenum, rule->set, rt));
+	}
 
-	ret = dyn_match_range(s->data->rulenum, s->data->set, rt);
-	if (ret == 0 || V_dyn_keep_states == 0 || ret > 1)
-		return (ret);
-
 	rule = s->data->parent;
 	if (s->type == O_LIMIT)
 		rule = ((struct dyn_ipv6_state *)rule)->limit->parent;
+
+	ret = dyn_match_range(s->data->rulenum, rule->set, rt);
+	if (ret == 0 || V_dyn_keep_states == 0 || ret > 1)
+		return (ret);
+
 	dyn_acquire_rule(ch, s->data, rule, s->kidx);
 	return (0);
 }
@@ -2899,7 +2896,7 @@ ipfw_is_dyn_rule(struct ip_fw *rule)
 }
 
 static void
-dyn_export_parent(const struct dyn_parent *p, uint16_t kidx,
+dyn_export_parent(const struct dyn_parent *p, uint16_t kidx, uint8_t set,
     ipfw_dyn_rule *dst)
 {
 
@@ -2911,9 +2908,9 @@ dyn_export_parent(const struct dyn_parent *p, uint16_t
 
 	/* 'rule' is used to pass up the rule number and set */
 	memcpy(&dst->rule, &p->rulenum, sizeof(p->rulenum));
+
 	/* store set number into high word of dst->rule pointer. */
-	memcpy((char *)&dst->rule + sizeof(p->rulenum), &p->set,
-	    sizeof(p->set));
+	memcpy((char *)&dst->rule + sizeof(p->rulenum), &set, sizeof(set));
 
 	/* unused fields */
 	dst->pcnt = 0;
@@ -2932,7 +2929,7 @@ dyn_export_parent(const struct dyn_parent *p, uint16_t
 
 static void
 dyn_export_data(const struct dyn_data *data, uint16_t kidx, uint8_t type,
-    ipfw_dyn_rule *dst)
+    uint8_t set, ipfw_dyn_rule *dst)
 {
 
 	dst->dyn_type = type;
@@ -2944,9 +2941,9 @@ dyn_export_data(const struct dyn_data *data, uint16_t 
 
 	/* 'rule' is used to pass up the rule number and set */
 	memcpy(&dst->rule, &data->rulenum, sizeof(data->rulenum));
+
 	/* store set number into high word of dst->rule pointer. */
-	memcpy((char *)&dst->rule + sizeof(data->rulenum), &data->set,
-	    sizeof(data->set));
+	memcpy((char *)&dst->rule + sizeof(data->rulenum), &set, sizeof(set));
 
 	dst->state = data->state;
 	if (data->flags & DYN_REFERENCED)
@@ -2968,13 +2965,18 @@ dyn_export_data(const struct dyn_data *data, uint16_t 
 static void
 dyn_export_ipv4_state(const struct dyn_ipv4_state *s, ipfw_dyn_rule *dst)
 {
+	struct ip_fw *rule;
 
 	switch (s->type) {
 	case O_LIMIT_PARENT:
-		dyn_export_parent(s->limit, s->kidx, dst);
+		rule = s->limit->parent;
+		dyn_export_parent(s->limit, s->kidx, rule->set, dst);
 		break;
 	default:
-		dyn_export_data(s->data, s->kidx, s->type, dst);
+		rule = s->data->parent;
+		if (s->type == O_LIMIT)
+			rule = ((struct dyn_ipv4_state *)rule)->limit->parent;
+		dyn_export_data(s->data, s->kidx, s->type, rule->set, dst);
 	}
 
 	dst->id.dst_ip = s->dst;
@@ -2995,13 +2997,18 @@ dyn_export_ipv4_state(const struct dyn_ipv4_state *s, 
 static void
 dyn_export_ipv6_state(const struct dyn_ipv6_state *s, ipfw_dyn_rule *dst)
 {
+	struct ip_fw *rule;
 
 	switch (s->type) {
 	case O_LIMIT_PARENT:
-		dyn_export_parent(s->limit, s->kidx, dst);
+		rule = s->limit->parent;
+		dyn_export_parent(s->limit, s->kidx, rule->set, dst);
 		break;
 	default:
-		dyn_export_data(s->data, s->kidx, s->type, dst);
+		rule = s->data->parent;
+		if (s->type == O_LIMIT)
+			rule = ((struct dyn_ipv6_state *)rule)->limit->parent;
+		dyn_export_data(s->data, s->kidx, s->type, rule->set, dst);
 	}
 
 	dst->id.src_ip6 = s->src;



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