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>