On Tue, Jun 09, 2015 at 18:11 +0200, Mike Belopuhov wrote:
> Hi,
>
> I was surprised to see 'State Creations' rule counter go up when
I've just realised that I might have been a bit too vague in my
description.
> no real state creation happens.
Should read: when state creation/insertion fails.
> This is because we increment all
> counters too early, but then don't decrement 'states_tot'
We don't decrement it in case of a failure.
> which
> is a total number of states created by the rule.
> Not entirely
> sure why was it done but I see no reason for it to stay this way.
>
> IMO total has to increase only when we succeed inserting the state
> into the table.
>
> This has an additional benefit of making error handling simpler:
> we don't need to unroll our counter increments if we do them right
> after pf_state_insert.
>
> OK?
>
> diff --git sys/net/pf.c sys/net/pf.c
> index 4192f14..efa1388 100644
> --- sys/net/pf.c
> +++ sys/net/pf.c
> @@ -273,20 +273,10 @@ struct pf_pool_limit pf_pool_limits[PF_LIMIT_MAX] = {
> } \
> SLIST_FOREACH(mrm, &s->match_rules, entry) \
> mrm->r->states_cur++; \
> } while (0)
>
> -#define STATE_DEC_COUNTERS(s) \
> - do { \
> - struct pf_rule_item *mrm; \
> - if (s->anchor.ptr != NULL) \
> - s->anchor.ptr->states_cur--; \
> - s->rule.ptr->states_cur--; \
> - SLIST_FOREACH(mrm, &s->match_rules, entry) \
> - mrm->r->states_cur--; \
> - } while (0)
> -
> static __inline int pf_src_compare(struct pf_src_node *, struct pf_src_node
> *);
> static __inline int pf_state_compare_key(struct pf_state_key *,
> struct pf_state_key *);
> static __inline int pf_state_compare_id(struct pf_state *,
> struct pf_state *);
> @@ -3464,11 +3454,10 @@ pf_create_state(struct pf_pdesc *pd, struct pf_rule
> *r, struct pf_rule *a,
> }
> s->rule.ptr = r;
> s->anchor.ptr = a;
> s->natrule.ptr = nr;
> memcpy(&s->match_rules, rules, sizeof(s->match_rules));
> - STATE_INC_COUNTERS(s);
> if (r->allow_opts)
> s->state_flags |= PFSTATE_ALLOWOPTS;
> if (r->rule_flag & PFRULE_STATESLOPPY)
> s->state_flags |= PFSTATE_SLOPPY;
> if (r->rule_flag & PFRULE_PFLOW)
> @@ -3591,10 +3580,12 @@ pf_create_state(struct pf_pdesc *pd, struct pf_rule
> *r, struct pf_rule *a,
> REASON_SET(&reason, PFRES_STATEINS);
> goto csfailed;
> } else
> *sm = s;
>
> + STATE_INC_COUNTERS(s);
> +
> if (tag > 0) {
> pf_tag_ref(tag);
> s->tag = tag;
> }
> if (pd->proto == IPPROTO_TCP && (th->th_flags & (TH_SYN|TH_ACK)) ==
> @@ -3621,21 +3612,17 @@ pf_create_state(struct pf_pdesc *pd, struct pf_rule
> *r, struct pf_rule *a,
>
> csfailed:
> if (s) {
> pf_normalize_tcp_cleanup(s); /* safe even w/o init */
> pf_src_tree_remove_state(s);
> + pool_put(&pf_state_pl, s);
> }
>
> for (i = 0; i < PF_SN_MAX; i++)
> if (sns[i] != NULL)
> pf_remove_src_node(sns[i]);
>
> - if (s) {
> - STATE_DEC_COUNTERS(s);
> - pool_put(&pf_state_pl, s);
> - }
> -
> return (PF_DROP);
> }
>
> int
> pf_translate(struct pf_pdesc *pd, struct pf_addr *saddr, u_int16_t sport,
