On 22.8.2012. 20:50, Florian Obser wrote:
> Hi,
>
> I think I got this now.
> - replace time_seconds with time_uptime
> - with that flow_finish in pflow can be simplified (pointed out by benno@)
> this should take care of flows with finish < start for localy created
> states
> - change various variables from unsigned to signed (mainly) in pfsync which
> could underflow (inspired by a patch from dlg@ from last year, pointed out
> by cameild@). This fixes pflow and at the same time an underflow problem
> in pfsync which dlg's patch was addressing. If I understand the history
> correctly cameild@ noticed this problem last year.
>
> Hrvoje Popovski tested an older version without the pfsync fix.
>
> Since this changes the semantics of a field in pfsync both pfsync machines
> need to be updated (I think it's ok to import 5.1 states into this version
> but the other way around will cause problems.)
>
> We are running with this patch since yesterday on a redundant firewall
> (amd64) with pfsync and no longer see broken flows (i.e. where finish
> < start). The pair survived various failovers.
>
> Index: share/man/man4/pflow.4
> ===================================================================
> RCS file: /opt/OpenBSD-CVS/src/share/man/man4/pflow.4,v
> retrieving revision 1.11
> diff -u -p -u -r1.11 pflow.4
> --- share/man/man4/pflow.4 2 Feb 2012 12:34:37 -0000 1.11
> +++ share/man/man4/pflow.4 22 Aug 2012 18:20:08 -0000
> @@ -127,6 +127,12 @@ The
> device first appeared in
> .Ox 4.5 .
> .Sh BUGS
> +A state created by
> +.Xr pfsync 4
> +can have a creation or expiration time before the machine came up.
> +.Nm
> +pretends the flow was created or expired when the machine came up.
> +
> The IPFIX implementation is incomplete:
> The required transport protocol SCTP is not supported.
> Transport over TCP and DTLS protected flow export is also not supported.
> Index: sys/net/if_pflow.c
> ===================================================================
> RCS file: /opt/OpenBSD-CVS/src/sys/net/if_pflow.c,v
> retrieving revision 1.20
> diff -u -p -u -r1.20 if_pflow.c
> --- sys/net/if_pflow.c 11 Apr 2012 17:42:53 -0000 1.20
> +++ sys/net/if_pflow.c 22 Aug 2012 18:20:08 -0000
> @@ -553,12 +553,15 @@ copy_flow_data(struct pflow_flow *flow1,
> flow1->flow_octets = htonl(st->bytes[0]);
> flow2->flow_octets = htonl(st->bytes[1]);
>
> - flow1->flow_start = flow2->flow_start =
> + /*
> + * Pretend the flow was created or expired when this machine
> + * came up when the state's creation or expiration is in the past.
> + */
> + flow1->flow_start = flow2->flow_start = st->creation < 0 ? 0 :
> htonl(st->creation * 1000);
> - flow1->flow_finish = flow2->flow_finish =
> - htonl((time_uptime - (st->rule.ptr->timeout[st->timeout] ?
> - st->rule.ptr->timeout[st->timeout] :
> - pf_default_rule.timeout[st->timeout])) * 1000);
> + flow1->flow_finish = flow2->flow_finish = st->expire < 0 ? 0 :
> + htonl(st->expire * 1000);
> +
> flow1->tcp_flags = flow2->tcp_flags = 0;
> flow1->protocol = flow2->protocol = sk->proto;
> flow1->tos = flow2->tos = st->rule.ptr->tos;
> @@ -580,12 +583,14 @@ copy_flow4_data(struct pflow_flow4 *flow
> flow1->flow_octets = htobe64(st->bytes[0]);
> flow2->flow_octets = htobe64(st->bytes[1]);
>
> - flow1->flow_start = flow2->flow_start =
> + /*
> + * Pretend the flow was created or expired when this machine
> + * came up when the state's creation or expiration is in the past.
> + */
> + flow1->flow_start = flow2->flow_start = st->creation < 0 ? 0 :
> htonl(st->creation * 1000);
> - flow1->flow_finish = flow2->flow_finish =
> - htonl((time_uptime - (st->rule.ptr->timeout[st->timeout] ?
> - st->rule.ptr->timeout[st->timeout] :
> - pf_default_rule.timeout[st->timeout])) * 1000);
> + flow1->flow_finish = flow2->flow_finish = st->expire < 0 ? 0 :
> + htonl(st->expire * 1000);
>
> flow1->protocol = flow2->protocol = sk->proto;
> flow1->tos = flow2->tos = st->rule.ptr->tos;
> @@ -608,12 +613,14 @@ copy_flow6_data(struct pflow_flow6 *flow
> flow1->flow_octets = htobe64(st->bytes[0]);
> flow2->flow_octets = htobe64(st->bytes[1]);
>
> - flow1->flow_start = flow2->flow_start =
> + /*
> + * Pretend the flow was created or expired when this machine
> + * came up when the state's creation or expiration is in the past.
> + */
> + flow1->flow_start = flow2->flow_start = st->creation < 0 ? 0 :
> htonl(st->creation * 1000);
> - flow1->flow_finish = flow2->flow_finish =
> - htonl((time_uptime - (st->rule.ptr->timeout[st->timeout] ?
> - st->rule.ptr->timeout[st->timeout] :
> - pf_default_rule.timeout[st->timeout])) * 1000);
> + flow1->flow_finish = flow2->flow_finish = st->expire < 0 ? 0 :
> + htonl(st->expire * 1000);
>
> flow1->protocol = flow2->protocol = sk->proto;
> flow1->tos = flow2->tos = st->rule.ptr->tos;
> Index: sys/net/if_pfsync.c
> ===================================================================
> RCS file: /opt/OpenBSD-CVS/src/sys/net/if_pfsync.c,v
> retrieving revision 1.189
> diff -u -p -u -r1.189 if_pfsync.c
> --- sys/net/if_pfsync.c 26 Jul 2012 12:25:31 -0000 1.189
> +++ sys/net/if_pfsync.c 22 Aug 2012 18:20:08 -0000
> @@ -464,6 +464,7 @@ void
> pfsync_state_export(struct pfsync_state *sp, struct pf_state *st)
> {
> pf_state_export(sp, st);
> + sp->expire = htonl(time_uptime - st->expire);
> }
>
> int
> @@ -574,17 +575,7 @@ pfsync_state_import(struct pfsync_state
> /* copy to state */
> bcopy(&sp->rt_addr, &st->rt_addr, sizeof(st->rt_addr));
> st->creation = time_uptime - ntohl(sp->creation);
> - st->expire = time_second;
> - if (sp->expire) {
> - u_int32_t timeout;
> -
> - timeout = r->timeout[sp->timeout];
> - if (!timeout)
> - timeout = pf_default_rule.timeout[sp->timeout];
> -
> - /* sp->expire may have been adaptively scaled by export. */
> - st->expire -= timeout - ntohl(sp->expire);
> - }
> + st->expire = time_uptime - ntohl(sp->expire);
>
> st->direction = sp->direction;
> st->log = sp->log;
> @@ -948,7 +939,7 @@ pfsync_in_upd(caddr_t buf, int len, int
> if (sync < 2) {
> pfsync_alloc_scrub_memory(&sp->dst, &st->dst);
> pf_state_peer_ntoh(&sp->dst, &st->dst);
> - st->expire = time_second;
> + st->expire = time_uptime;
> st->timeout = sp->timeout;
> }
> st->pfsync_time = time_uptime;
> @@ -1022,7 +1013,7 @@ pfsync_in_upd_c(caddr_t buf, int len, in
> if (sync < 2) {
> pfsync_alloc_scrub_memory(&up->dst, &st->dst);
> pf_state_peer_ntoh(&up->dst, &st->dst);
> - st->expire = time_second;
> + st->expire = time_uptime;
> st->timeout = up->timeout;
> }
> st->pfsync_time = time_uptime;
> Index: sys/net/pf.c
> ===================================================================
> RCS file: /opt/OpenBSD-CVS/src/sys/net/pf.c,v
> retrieving revision 1.809
> diff -u -p -u -r1.809 pf.c
> --- sys/net/pf.c 26 Jul 2012 12:25:31 -0000 1.809
> +++ sys/net/pf.c 22 Aug 2012 18:20:08 -0000
> @@ -389,13 +389,13 @@ pf_init_threshold(struct pf_threshold *t
> threshold->limit = limit * PF_THRESHOLD_MULT;
> threshold->seconds = seconds;
> threshold->count = 0;
> - threshold->last = time_second;
> + threshold->last = time_uptime;
> }
>
> void
> pf_add_threshold(struct pf_threshold *threshold)
> {
> - u_int32_t t = time_second, diff = t - threshold->last;
> + u_int32_t t = time_uptime, diff = t - threshold->last;
>
> if (diff >= threshold->seconds)
> threshold->count = 0;
> @@ -582,7 +582,7 @@ pf_insert_src_node(struct pf_src_node **
> void
> pf_remove_src_node(struct pf_src_node *sn)
> {
> - if (sn->states > 0 || sn->expire > time_second)
> + if (sn->states > 0 || sn->expire > time_uptime)
> return;
>
> if (sn->rule.ptr != NULL) {
> @@ -1105,10 +1105,10 @@ pf_state_export(struct pfsync_state *sp,
> bcopy(&st->rt_addr, &sp->rt_addr, sizeof(sp->rt_addr));
> sp->creation = htonl(time_uptime - st->creation);
> sp->expire = pf_state_expires(st);
> - if (sp->expire <= time_second)
> + if (sp->expire <= time_uptime)
> sp->expire = htonl(0);
> else
> - sp->expire = htonl(sp->expire - time_second);
> + sp->expire = htonl(sp->expire - time_uptime);
>
> sp->direction = st->direction;
> sp->log = st->log;
> @@ -1169,7 +1169,7 @@ pf_purge_thread(void *v)
> }
> }
>
> -u_int32_t
> +int32_t
> pf_state_expires(const struct pf_state *state)
> {
> u_int32_t timeout;
> @@ -1179,7 +1179,7 @@ pf_state_expires(const struct pf_state *
>
> /* handle all PFTM_* > PFTM_MAX here */
> if (state->timeout == PFTM_PURGE)
> - return (time_second);
> + return (time_uptime);
> KASSERT(state->timeout != PFTM_UNLINKED);
> KASSERT(state->timeout < PFTM_MAX);
> timeout = state->rule.ptr->timeout[state->timeout];
> @@ -1199,7 +1199,7 @@ pf_state_expires(const struct pf_state *
> return (state->expire + timeout * (end - states) /
> (end - start));
> else
> - return (time_second);
> + return (time_uptime);
> }
> return (state->expire + timeout);
> }
> @@ -1213,7 +1213,7 @@ pf_purge_expired_src_nodes(int waslocked
> for (cur = RB_MIN(pf_src_tree, &tree_src_tracking); cur; cur = next) {
> next = RB_NEXT(pf_src_tree, &tree_src_tracking, cur);
>
> - if (cur->states <= 0 && cur->expire <= time_second) {
> + if (cur->states <= 0 && cur->expire <= time_uptime) {
> if (! locked) {
> rw_enter_write(&pf_consistency_lock);
> next = RB_NEXT(pf_src_tree,
> @@ -1243,7 +1243,7 @@ pf_src_tree_remove_state(struct pf_state
> if (!timeout)
> timeout =
> pf_default_rule.timeout[PFTM_SRC_NODE];
> - sni->sn->expire = time_second + timeout;
> + sni->sn->expire = time_uptime + timeout;
> }
> pool_put(&pf_sn_item_pl, sni);
> }
> @@ -1343,7 +1343,7 @@ pf_purge_expired_states(u_int32_t maxche
> locked = 1;
> }
> pf_free_state(cur);
> - } else if (pf_state_expires(cur) <= time_second) {
> + } else if (pf_state_expires(cur) <= time_uptime) {
> /* unlink and free expired state */
> pf_unlink_state(cur);
> if (! locked) {
> @@ -3761,7 +3761,7 @@ pf_create_state(struct pf_pdesc *pd, str
> }
>
> s->creation = time_uptime;
> - s->expire = time_second;
> + s->expire = time_uptime;
>
> if (pd->proto == IPPROTO_TCP) {
> if (s->state_flags & PFSTATE_SCRUB_TCP &&
> @@ -4198,7 +4198,7 @@ pf_tcp_track_full(struct pf_pdesc *pd, s
> src->state = dst->state = TCPS_TIME_WAIT;
>
> /* update expire time */
> - (*state)->expire = time_second;
> + (*state)->expire = time_uptime;
> if (src->state >= TCPS_FIN_WAIT_2 &&
> dst->state >= TCPS_FIN_WAIT_2)
> (*state)->timeout = PFTM_TCP_CLOSED;
> @@ -4375,7 +4375,7 @@ pf_tcp_track_sloppy(struct pf_pdesc *pd,
> src->state = dst->state = TCPS_TIME_WAIT;
>
> /* update expire time */
> - (*state)->expire = time_second;
> + (*state)->expire = time_uptime;
> if (src->state >= TCPS_FIN_WAIT_2 &&
> dst->state >= TCPS_FIN_WAIT_2)
> (*state)->timeout = PFTM_TCP_CLOSED;
> @@ -4620,7 +4620,7 @@ pf_test_state_udp(struct pf_pdesc *pd, s
> dst->state = PFUDPS_MULTIPLE;
>
> /* update expire time */
> - (*state)->expire = time_second;
> + (*state)->expire = time_uptime;
> if (src->state == PFUDPS_MULTIPLE && dst->state == PFUDPS_MULTIPLE)
> (*state)->timeout = PFTM_UDP_MULTIPLE;
> else
> @@ -4762,7 +4762,7 @@ pf_test_state_icmp(struct pf_pdesc *pd,
> return (ret);
> }
>
> - (*state)->expire = time_second;
> + (*state)->expire = time_uptime;
> (*state)->timeout = PFTM_ICMP_ERROR_REPLY;
>
> /* translate source/destination address, if necessary */
> @@ -5570,7 +5570,7 @@ pf_test_state_other(struct pf_pdesc *pd,
> dst->state = PFOTHERS_MULTIPLE;
>
> /* update expire time */
> - (*state)->expire = time_second;
> + (*state)->expire = time_uptime;
> if (src->state == PFOTHERS_MULTIPLE && dst->state == PFOTHERS_MULTIPLE)
> (*state)->timeout = PFTM_OTHER_MULTIPLE;
> else
> Index: sys/net/pf_ioctl.c
> ===================================================================
> RCS file: /opt/OpenBSD-CVS/src/sys/net/pf_ioctl.c,v
> retrieving revision 1.253
> diff -u -p -u -r1.253 pf_ioctl.c
> --- sys/net/pf_ioctl.c 8 Jul 2012 07:58:09 -0000 1.253
> +++ sys/net/pf_ioctl.c 22 Aug 2012 18:20:08 -0000
> @@ -2333,7 +2333,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a
>
> p = psn->psn_src_nodes;
> RB_FOREACH(n, pf_src_tree, &tree_src_tracking) {
> - int secs = time_second, diff;
> + int secs = time_uptime, diff;
>
> if ((nr + 1) * sizeof(*p) > (unsigned)psn->psn_len)
> break;
> Index: sys/net/pf_norm.c
> ===================================================================
> RCS file: /opt/OpenBSD-CVS/src/sys/net/pf_norm.c,v
> retrieving revision 1.154
> diff -u -p -u -r1.154 pf_norm.c
> --- sys/net/pf_norm.c 12 May 2012 13:08:48 -0000 1.154
> +++ sys/net/pf_norm.c 22 Aug 2012 18:20:08 -0000
> @@ -173,7 +173,7 @@ void
> pf_purge_expired_fragments(void)
> {
> struct pf_fragment *frag;
> - u_int32_t expire = time_second -
> + int32_t expire = time_uptime -
> pf_default_rule.timeout[PFTM_FRAG];
>
> while ((frag = TAILQ_LAST(&pf_fragqueue, pf_fragqueue)) != NULL) {
> @@ -238,7 +238,7 @@ pf_find_fragment(struct pf_fragment_cmp
> frag = RB_FIND(pf_frag_tree, tree, (struct pf_fragment *)key);
> if (frag != NULL) {
> /* XXX Are we sure we want to update the timeout? */
> - frag->fr_timeout = time_second;
> + frag->fr_timeout = time_uptime;
> TAILQ_REMOVE(&pf_fragqueue, frag, frag_next);
> TAILQ_INSERT_HEAD(&pf_fragqueue, frag, frag_next);
> }
> @@ -314,7 +314,7 @@ pf_fillup_fragment(struct pf_fragment_cm
>
> *(struct pf_fragment_cmp *)frag = *key;
> TAILQ_INIT(&frag->fr_queue);
> - frag->fr_timeout = time_second;
> + frag->fr_timeout = time_uptime;
> frag->fr_maxlen = frent->fe_len;
>
> RB_INSERT(pf_frag_tree, &pf_frag_tree, frag);
> Index: sys/net/pfvar.h
> ===================================================================
> RCS file: /opt/OpenBSD-CVS/src/sys/net/pfvar.h,v
> retrieving revision 1.367
> diff -u -p -u -r1.367 pfvar.h
> --- sys/net/pfvar.h 26 Jul 2012 12:25:31 -0000 1.367
> +++ sys/net/pfvar.h 22 Aug 2012 18:20:08 -0000
> @@ -824,8 +824,8 @@ struct pf_state {
> struct pfi_kif *rt_kif;
> u_int64_t packets[2];
> u_int64_t bytes[2];
> - u_int32_t creation;
> - u_int32_t expire;
> + int32_t creation;
> + int32_t expire;
> u_int32_t pfsync_time;
> u_int16_t qid;
> u_int16_t pqid;
> @@ -895,8 +895,8 @@ struct pfsync_state {
> u_int32_t rule;
> u_int32_t anchor;
> u_int32_t nat_rule;
> - u_int32_t creation;
> - u_int32_t expire;
> + int32_t creation;
> + int32_t expire;
> u_int32_t packets[2][2];
> u_int32_t bytes[2][2];
> u_int32_t creatorid;
> @@ -1818,7 +1818,7 @@ int pf_normalize_tcp_stateful(struct pf_
> int *);
> int pf_normalize_mss(struct pf_pdesc *, u_int16_t);
> void pf_scrub(struct mbuf *, u_int16_t, sa_family_t, u_int8_t, u_int8_t);
> -u_int32_t
> +int32_t
> pf_state_expires(const struct pf_state *);
> void pf_purge_expired_fragments(void);
> int pf_routable(struct pf_addr *addr, sa_family_t af, struct pfi_kif *,
>
Hello everyone,
will Florian's patch be applied in OpenBSD 5.3?
