Date: Tue, 11 Mar 2014 15:43:06 +0000 (UTC) From: Gleb Smirnoff <glebius@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-10@freebsd.org Subject: svn commit: r263029 - in stable/10: sbin/pfctl sys/net sys/netpfil/pf Message-ID: <201403111543.s2BFh6LW053673@svn.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: glebius Date: Tue Mar 11 15:43:06 2014 New Revision: 263029 URL: http://svnweb.freebsd.org/changeset/base/263029 Log: Merge r261882, r261898, r261937, r262760, r262799: Once pf became not covered by a single mutex, many counters in it became race prone. Some just gather statistics, but some are later used in different calculations. A real problem was the race provoked underflow of the states_cur counter on a rule. Once it goes below zero, it wraps to UINT32_MAX. Later this value is used in pf_state_expires() and any state created by this rule is immediately expired. Thus, make fields states_cur, states_tot and src_nodes of struct pf_rule be counter(9)s. Modified: stable/10/sbin/pfctl/pfctl.c stable/10/sys/net/pfvar.h stable/10/sys/netpfil/pf/if_pfsync.c stable/10/sys/netpfil/pf/pf.c stable/10/sys/netpfil/pf/pf_ioctl.c Directory Properties: stable/10/ (props changed) Modified: stable/10/sbin/pfctl/pfctl.c ============================================================================== --- stable/10/sbin/pfctl/pfctl.c Tue Mar 11 15:28:41 2014 (r263028) +++ stable/10/sbin/pfctl/pfctl.c Tue Mar 11 15:43:06 2014 (r263029) @@ -55,6 +55,7 @@ __FBSDID("$FreeBSD$"); #include <fcntl.h> #include <limits.h> #include <netdb.h> +#include <stdint.h> #include <stdio.h> #include <stdlib.h> #include <string.h> @@ -796,17 +797,17 @@ pfctl_print_rule_counters(struct pf_rule } if (opts & PF_OPT_VERBOSE) { printf(" [ Evaluations: %-8llu Packets: %-8llu " - "Bytes: %-10llu States: %-6u]\n", + "Bytes: %-10llu States: %-6ju]\n", (unsigned long long)rule->evaluations, (unsigned long long)(rule->packets[0] + rule->packets[1]), (unsigned long long)(rule->bytes[0] + - rule->bytes[1]), rule->states_cur); + rule->bytes[1]), (uintmax_t)rule->u_states_cur); if (!(opts & PF_OPT_DEBUG)) printf(" [ Inserted: uid %u pid %u " - "State Creations: %-6u]\n", + "State Creations: %-6ju]\n", (unsigned)rule->cuid, (unsigned)rule->cpid, - rule->states_tot); + (uintmax_t)rule->u_states_tot); } } @@ -908,7 +909,7 @@ pfctl_show_rules(int dev, char *path, in case PFCTL_SHOW_LABELS: if (pr.rule.label[0]) { printf("%s %llu %llu %llu %llu" - " %llu %llu %llu %llu\n", + " %llu %llu %llu %ju\n", pr.rule.label, (unsigned long long)pr.rule.evaluations, (unsigned long long)(pr.rule.packets[0] + @@ -919,7 +920,7 @@ pfctl_show_rules(int dev, char *path, in (unsigned long long)pr.rule.bytes[0], (unsigned long long)pr.rule.packets[1], (unsigned long long)pr.rule.bytes[1], - (unsigned long long)pr.rule.states_tot); + (uintmax_t)pr.rule.u_states_tot); } break; case PFCTL_SHOW_RULES: Modified: stable/10/sys/net/pfvar.h ============================================================================== --- stable/10/sys/net/pfvar.h Tue Mar 11 15:28:41 2014 (r263028) +++ stable/10/sys/net/pfvar.h Tue Mar 11 15:43:06 2014 (r263029) @@ -35,6 +35,7 @@ #include <sys/param.h> #include <sys/queue.h> +#include <sys/counter.h> #include <sys/refcount.h> #include <sys/tree.h> @@ -588,13 +589,9 @@ struct pf_rule { int rtableid; u_int32_t timeout[PFTM_MAX]; - u_int32_t states_cur; - u_int32_t states_tot; u_int32_t max_states; - u_int32_t src_nodes; u_int32_t max_src_nodes; u_int32_t max_src_states; - u_int32_t spare1; /* netgraph */ u_int32_t max_src_conn; struct { u_int32_t limit; @@ -608,6 +605,10 @@ struct pf_rule { uid_t cuid; pid_t cpid; + counter_u64_t states_cur; + counter_u64_t states_tot; + counter_u64_t src_nodes; + u_int16_t return_icmp; u_int16_t return_icmp6; u_int16_t max_mss; @@ -655,6 +656,10 @@ struct pf_rule { struct pf_addr addr; u_int16_t port; } divert; + + uint64_t u_states_cur; + uint64_t u_states_tot; + uint64_t u_src_nodes; }; /* rule flags */ Modified: stable/10/sys/netpfil/pf/if_pfsync.c ============================================================================== --- stable/10/sys/netpfil/pf/if_pfsync.c Tue Mar 11 15:28:41 2014 (r263028) +++ stable/10/sys/netpfil/pf/if_pfsync.c Tue Mar 11 15:43:06 2014 (r263029) @@ -436,7 +436,8 @@ pfsync_state_import(struct pfsync_state else r = &V_pf_default_rule; - if ((r->max_states && r->states_cur >= r->max_states)) + if ((r->max_states && + counter_u64_fetch(r->states_cur) >= r->max_states)) goto cleanup; /* @@ -514,18 +515,15 @@ pfsync_state_import(struct pfsync_state st->pfsync_time = time_uptime; st->sync_state = PFSYNC_S_NONE; - /* XXX when we have nat_rule/anchors, use STATE_INC_COUNTERS */ - r->states_cur++; - r->states_tot++; - if (!(flags & PFSYNC_SI_IOCTL)) st->state_flags |= PFSTATE_NOSYNC; - if ((error = pf_state_insert(kif, skw, sks, st)) != 0) { - /* XXX when we have nat_rule/anchors, use STATE_DEC_COUNTERS */ - r->states_cur--; + if ((error = pf_state_insert(kif, skw, sks, st)) != 0) goto cleanup_state; - } + + /* XXX when we have nat_rule/anchors, use STATE_INC_COUNTERS */ + counter_u64_add(r->states_cur, 1); + counter_u64_add(r->states_tot, 1); if (!(flags & PFSYNC_SI_IOCTL)) { st->state_flags &= ~PFSTATE_NOSYNC; Modified: stable/10/sys/netpfil/pf/pf.c ============================================================================== --- stable/10/sys/netpfil/pf/pf.c Tue Mar 11 15:28:41 2014 (r263028) +++ stable/10/sys/netpfil/pf/pf.c Tue Mar 11 15:43:06 2014 (r263029) @@ -327,27 +327,27 @@ VNET_DEFINE(struct pf_limit, pf_limits[P #define BOUND_IFACE(r, k) \ ((r)->rule_flag & PFRULE_IFBOUND) ? (k) : V_pfi_all -#define STATE_INC_COUNTERS(s) \ - do { \ - s->rule.ptr->states_cur++; \ - s->rule.ptr->states_tot++; \ - if (s->anchor.ptr != NULL) { \ - s->anchor.ptr->states_cur++; \ - s->anchor.ptr->states_tot++; \ - } \ - if (s->nat_rule.ptr != NULL) { \ - s->nat_rule.ptr->states_cur++; \ - s->nat_rule.ptr->states_tot++; \ - } \ +#define STATE_INC_COUNTERS(s) \ + do { \ + counter_u64_add(s->rule.ptr->states_cur, 1); \ + counter_u64_add(s->rule.ptr->states_tot, 1); \ + if (s->anchor.ptr != NULL) { \ + counter_u64_add(s->anchor.ptr->states_cur, 1); \ + counter_u64_add(s->anchor.ptr->states_tot, 1); \ + } \ + if (s->nat_rule.ptr != NULL) { \ + counter_u64_add(s->nat_rule.ptr->states_cur, 1);\ + counter_u64_add(s->nat_rule.ptr->states_tot, 1);\ + } \ } while (0) -#define STATE_DEC_COUNTERS(s) \ - do { \ - if (s->nat_rule.ptr != NULL) \ - s->nat_rule.ptr->states_cur--; \ - if (s->anchor.ptr != NULL) \ - s->anchor.ptr->states_cur--; \ - s->rule.ptr->states_cur--; \ +#define STATE_DEC_COUNTERS(s) \ + do { \ + if (s->nat_rule.ptr != NULL) \ + counter_u64_add(s->nat_rule.ptr->states_cur, -1);\ + if (s->anchor.ptr != NULL) \ + counter_u64_add(s->anchor.ptr->states_cur, -1); \ + counter_u64_add(s->rule.ptr->states_cur, -1); \ } while (0) static MALLOC_DEFINE(M_PFHASH, "pf_hash", "pf(4) hash header structures"); @@ -639,7 +639,7 @@ pf_insert_src_node(struct pf_src_node ** PF_HASHROW_ASSERT(sh); if (!rule->max_src_nodes || - rule->src_nodes < rule->max_src_nodes) + counter_u64_fetch(rule->src_nodes) < rule->max_src_nodes) (*sn) = uma_zalloc(V_pf_sources_z, M_NOWAIT | M_ZERO); else V_pf_status.lcounters[LCNT_SRCNODES]++; @@ -659,7 +659,7 @@ pf_insert_src_node(struct pf_src_node ** (*sn)->creation = time_uptime; (*sn)->ruletype = rule->action; if ((*sn)->rule.ptr != NULL) - (*sn)->rule.ptr->src_nodes++; + counter_u64_add((*sn)->rule.ptr->src_nodes, 1); PF_HASHROW_UNLOCK(sh); V_pf_status.scounters[SCNT_SRC_NODE_INSERT]++; V_pf_status.src_nodes++; @@ -684,7 +684,7 @@ pf_unlink_src_node_locked(struct pf_src_ #endif LIST_REMOVE(src, entry); if (src->rule.ptr) - src->rule.ptr->src_nodes--; + counter_u64_add(src->rule.ptr->src_nodes, -1); V_pf_status.scounters[SCNT_SRC_NODE_REMOVALS]++; V_pf_status.src_nodes--; } @@ -1470,7 +1470,7 @@ pf_state_expires(const struct pf_state * start = state->rule.ptr->timeout[PFTM_ADAPTIVE_START]; if (start) { end = state->rule.ptr->timeout[PFTM_ADAPTIVE_END]; - states = state->rule.ptr->states_cur; /* XXXGL */ + states = counter_u64_fetch(state->rule.ptr->states_cur); } else { start = V_pf_default_rule.timeout[PFTM_ADAPTIVE_START]; end = V_pf_default_rule.timeout[PFTM_ADAPTIVE_END]; @@ -1579,11 +1579,7 @@ pf_unlink_state(struct pf_state *s, u_in if (pfsync_delete_state_ptr != NULL) pfsync_delete_state_ptr(s); - --s->rule.ptr->states_cur; - if (s->nat_rule.ptr != NULL) - --s->nat_rule.ptr->states_cur; - if (s->anchor.ptr != NULL) - --s->anchor.ptr->states_cur; + STATE_DEC_COUNTERS(s); s->timeout = PFTM_UNLINKED; @@ -3444,7 +3440,8 @@ pf_create_state(struct pf_rule *r, struc u_short reason; /* check maximums */ - if (r->max_states && (r->states_cur >= r->max_states)) { + if (r->max_states && + (counter_u64_fetch(r->states_cur) >= r->max_states)) { V_pf_status.lcounters[LCNT_STATES]++; REASON_SET(&reason, PFRES_MAXSTATES); return (PF_DROP); Modified: stable/10/sys/netpfil/pf/pf_ioctl.c ============================================================================== --- stable/10/sys/netpfil/pf/pf_ioctl.c Tue Mar 11 15:28:41 2014 (r263028) +++ stable/10/sys/netpfil/pf/pf_ioctl.c Tue Mar 11 15:43:06 2014 (r263029) @@ -225,6 +225,10 @@ pfattach(void) V_pf_default_rule.nr = -1; V_pf_default_rule.rtableid = -1; + V_pf_default_rule.states_cur = counter_u64_alloc(M_WAITOK); + V_pf_default_rule.states_tot = counter_u64_alloc(M_WAITOK); + V_pf_default_rule.src_nodes = counter_u64_alloc(M_WAITOK); + /* initialize default timeouts */ my_timeout[PFTM_TCP_FIRST_PACKET] = PFTM_TCP_FIRST_PACKET_VAL; my_timeout[PFTM_TCP_OPENING] = PFTM_TCP_OPENING_VAL; @@ -394,6 +398,9 @@ pf_free_rule(struct pf_rule *rule) pfi_kif_unref(rule->kif); pf_anchor_remove(rule); pf_empty_pool(&rule->rpool.list); + counter_u64_free(rule->states_cur); + counter_u64_free(rule->states_tot); + counter_u64_free(rule->src_nodes); free(rule, M_PFRULE); } @@ -1145,6 +1152,9 @@ pfioctl(struct cdev *dev, u_long cmd, ca bcopy(&pr->rule, rule, sizeof(struct pf_rule)); if (rule->ifname[0]) kif = malloc(sizeof(*kif), PFI_MTYPE, M_WAITOK); + rule->states_cur = counter_u64_alloc(M_WAITOK); + rule->states_tot = counter_u64_alloc(M_WAITOK); + rule->src_nodes = counter_u64_alloc(M_WAITOK); rule->cuid = td->td_ucred->cr_ruid; rule->cpid = td->td_proc ? td->td_proc->p_pid : 0; TAILQ_INIT(&rule->rpool.list); @@ -1261,6 +1271,9 @@ pfioctl(struct cdev *dev, u_long cmd, ca #undef ERROUT DIOCADDRULE_error: PF_RULES_WUNLOCK(); + counter_u64_free(rule->states_cur); + counter_u64_free(rule->states_tot); + counter_u64_free(rule->src_nodes); free(rule, M_PFRULE); if (kif) free(kif, PFI_MTYPE); @@ -1332,6 +1345,9 @@ DIOCADDRULE_error: break; } bcopy(rule, &pr->rule, sizeof(struct pf_rule)); + pr->rule.u_states_cur = counter_u64_fetch(rule->states_cur); + pr->rule.u_states_tot = counter_u64_fetch(rule->states_tot); + pr->rule.u_src_nodes = counter_u64_fetch(rule->src_nodes); if (pf_anchor_copyout(ruleset, rule, pr)) { PF_RULES_WUNLOCK(); error = EBUSY; @@ -1350,7 +1366,7 @@ DIOCADDRULE_error: rule->evaluations = 0; rule->packets[0] = rule->packets[1] = 0; rule->bytes[0] = rule->bytes[1] = 0; - rule->states_tot = 0; + counter_u64_zero(rule->states_tot); } PF_RULES_WUNLOCK(); break; @@ -1390,15 +1406,14 @@ DIOCADDRULE_error: #endif /* INET6 */ newrule = malloc(sizeof(*newrule), M_PFRULE, M_WAITOK); bcopy(&pcr->rule, newrule, sizeof(struct pf_rule)); + if (newrule->ifname[0]) + kif = malloc(sizeof(*kif), PFI_MTYPE, M_WAITOK); + newrule->states_cur = counter_u64_alloc(M_WAITOK); + newrule->states_tot = counter_u64_alloc(M_WAITOK); + newrule->src_nodes = counter_u64_alloc(M_WAITOK); newrule->cuid = td->td_ucred->cr_ruid; newrule->cpid = td->td_proc ? td->td_proc->p_pid : 0; TAILQ_INIT(&newrule->rpool.list); - /* Initialize refcounting. */ - newrule->states_cur = 0; - newrule->entries.tqe_prev = NULL; - - if (newrule->ifname[0]) - kif = malloc(sizeof(*kif), PFI_MTYPE, M_WAITOK); } #define ERROUT(x) { error = (x); goto DIOCCHANGERULE_error; } @@ -1566,8 +1581,12 @@ DIOCADDRULE_error: #undef ERROUT DIOCCHANGERULE_error: PF_RULES_WUNLOCK(); - if (newrule != NULL) + if (newrule != NULL) { + counter_u64_free(newrule->states_cur); + counter_u64_free(newrule->states_tot); + counter_u64_free(newrule->src_nodes); free(newrule, M_PFRULE); + } if (kif != NULL) free(kif, PFI_MTYPE); break; @@ -3423,6 +3442,11 @@ shutdown_pf(void) char nn = '\0'; V_pf_status.running = 0; + + counter_u64_free(V_pf_default_rule.states_cur); + counter_u64_free(V_pf_default_rule.states_tot); + counter_u64_free(V_pf_default_rule.src_nodes); + do { if ((error = pf_begin_rules(&t[0], PF_RULESET_SCRUB, &nn)) != 0) {
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201403111543.s2BFh6LW053673>