Hello, with state key mutex in a tree [1]. I'd like to add yet another diff. during h2k22 David and I split original change [2] into two chunks.
OK to commit diff below? thanks and regards sashan [1] https://marc.info/?l=openbsd-cvs&m=166817856414079&w=2 [2] https://marc.info/?l=openbsd-bugs&m=166006758231954&w=2 --------8<---------------8<---------------8<------------------8<-------- diff --git a/sys/net/if_pfsync.c b/sys/net/if_pfsync.c index f69790ee98d..e1a006a8faf 100644 --- a/sys/net/if_pfsync.c +++ b/sys/net/if_pfsync.c @@ -157,16 +157,16 @@ const struct { }; struct pfsync_q { - void (*write)(struct pf_state *, void *); + int (*write)(struct pf_state *, void *); size_t len; u_int8_t action; }; /* we have one of these for every PFSYNC_S_ */ -void pfsync_out_state(struct pf_state *, void *); -void pfsync_out_iack(struct pf_state *, void *); -void pfsync_out_upd_c(struct pf_state *, void *); -void pfsync_out_del(struct pf_state *, void *); +int pfsync_out_state(struct pf_state *, void *); +int pfsync_out_iack(struct pf_state *, void *); +int pfsync_out_upd_c(struct pf_state *, void *); +int pfsync_out_del(struct pf_state *, void *); struct pfsync_q pfsync_qs[] = { { pfsync_out_iack, sizeof(struct pfsync_ins_ack), PFSYNC_ACT_INS_ACK }, @@ -1301,24 +1301,26 @@ pfsyncioctl(struct ifnet *ifp, u_long cmd, caddr_t data) return (0); } -void +int pfsync_out_state(struct pf_state *st, void *buf) { struct pfsync_state *sp = buf; pf_state_export(sp, st); + return (0); } -void +int pfsync_out_iack(struct pf_state *st, void *buf) { struct pfsync_ins_ack *iack = buf; iack->id = st->id; iack->creatorid = st->creatorid; + return (0); } -void +int pfsync_out_upd_c(struct pf_state *st, void *buf) { struct pfsync_upd_c *up = buf; @@ -1329,9 +1331,10 @@ pfsync_out_upd_c(struct pf_state *st, void *buf) pf_state_peer_hton(&st->dst, &up->dst); up->creatorid = st->creatorid; up->timeout = st->timeout; + return (0); } -void +int pfsync_out_del(struct pf_state *st, void *buf) { struct pfsync_del_c *dp = buf; @@ -1340,6 +1343,7 @@ pfsync_out_del(struct pf_state *st, void *buf) dp->creatorid = st->creatorid; SET(st->state_flags, PFSTATE_NOSYNC); + return (0); } void @@ -1671,8 +1675,8 @@ pfsync_sendout(void) KASSERT(st->snapped == 1); st->sync_state = PFSYNC_S_NONE; st->snapped = 0; - pfsync_qs[q].write(st, m->m_data + offset); - offset += pfsync_qs[q].len; + if (pfsync_qs[q].write(st, m->m_data + offset) == 0) + offset += pfsync_qs[q].len; pf_state_unref(st); count++; diff --git a/sys/net/if_pfsync.h b/sys/net/if_pfsync.h index ff26ac3669e..cc1d78d47a3 100644 --- a/sys/net/if_pfsync.h +++ b/sys/net/if_pfsync.h @@ -326,7 +326,7 @@ int pfsync_sysctl(int *, u_int, void *, size_t *, #define PFSYNC_SI_CKSUM 0x02 #define PFSYNC_SI_ACK 0x04 int pfsync_state_import(struct pfsync_state *, int); -void pfsync_state_export(struct pfsync_state *, +int pfsync_state_export(struct pfsync_state *, struct pf_state *); void pfsync_insert_state(struct pf_state *); diff --git a/sys/net/pf.c b/sys/net/pf.c index 4203911ea60..3653e1359cf 100644 --- a/sys/net/pf.c +++ b/sys/net/pf.c @@ -181,7 +181,8 @@ int pf_translate_icmp_af(struct pf_pdesc*, int, void *); void pf_send_icmp(struct mbuf *, u_int8_t, u_int8_t, int, sa_family_t, struct pf_rule *, u_int); void pf_detach_state(struct pf_state *); -void pf_state_key_detach(struct pf_state *, int); +void pf_state_key_detach(struct pf_state *, + struct pf_state_key *); u_int32_t pf_tcp_iss(struct pf_pdesc *); void pf_rule_to_actions(struct pf_rule *, struct pf_rule_actions *); @@ -256,6 +257,9 @@ void pf_state_key_unlink_inpcb(struct pf_state_key *); void pf_inpcb_unlink_state_key(struct inpcb *); void pf_pktenqueue_delayed(void *); int32_t pf_state_expires(const struct pf_state *, uint8_t); +void pf_state_keys_take(struct pf_state *, + struct pf_state_key **); +void pf_state_keys_rele(struct pf_state_key **); #if NPFLOG > 0 void pf_log_matches(struct pf_pdesc *, struct pf_rule *, @@ -772,7 +776,8 @@ pf_state_key_attach(struct pf_state_key *sk, struct pf_state *s, int idx) s->key[idx] = sk; if ((si = pool_get(&pf_state_item_pl, PR_NOWAIT)) == NULL) { - pf_state_key_detach(s, idx); + pf_state_key_detach(s, s->key[idx]); + s->key[idx] = NULL; return (-1); } si->s = s; @@ -792,42 +797,50 @@ pf_state_key_attach(struct pf_state_key *sk, struct pf_state *s, int idx) void pf_detach_state(struct pf_state *s) { - if (s->key[PF_SK_WIRE] == s->key[PF_SK_STACK]) - s->key[PF_SK_WIRE] = NULL; + struct pf_state_key *key[2]; + + mtx_enter(&s->mtx); + key[PF_SK_WIRE] = s->key[PF_SK_WIRE]; + key[PF_SK_STACK] = s->key[PF_SK_STACK]; + s->key[PF_SK_WIRE] = NULL; + s->key[PF_SK_STACK] = NULL; + mtx_leave(&s->mtx); + + if (key[PF_SK_WIRE] == key[PF_SK_STACK]) + key[PF_SK_WIRE] = NULL; - if (s->key[PF_SK_STACK] != NULL) - pf_state_key_detach(s, PF_SK_STACK); + if (key[PF_SK_STACK] != NULL) + pf_state_key_detach(s, key[PF_SK_STACK]); - if (s->key[PF_SK_WIRE] != NULL) - pf_state_key_detach(s, PF_SK_WIRE); + if (key[PF_SK_WIRE] != NULL) + pf_state_key_detach(s, key[PF_SK_WIRE]); } void -pf_state_key_detach(struct pf_state *s, int idx) +pf_state_key_detach(struct pf_state *s, struct pf_state_key *key) { struct pf_state_item *si; - struct pf_state_key *sk; - if (s->key[idx] == NULL) + PF_STATE_ASSERT_LOCKED(); + + if (key == NULL) return; - si = TAILQ_FIRST(&s->key[idx]->states); + si = TAILQ_FIRST(&key->states); while (si && si->s != s) si = TAILQ_NEXT(si, entry); if (si) { - TAILQ_REMOVE(&s->key[idx]->states, si, entry); + TAILQ_REMOVE(&key->states, si, entry); pool_put(&pf_state_item_pl, si); } - sk = s->key[idx]; - s->key[idx] = NULL; - if (TAILQ_EMPTY(&sk->states)) { - RB_REMOVE(pf_state_tree, &pf_statetbl, sk); - sk->removed = 1; - pf_state_key_unlink_reverse(sk); - pf_state_key_unlink_inpcb(sk); - pf_state_key_unref(sk); + if (TAILQ_EMPTY(&key->states)) { + RB_REMOVE(pf_state_tree, &pf_statetbl, key); + key->removed = 1; + pf_state_key_unlink_reverse(key); + pf_state_key_unlink_inpcb(key); + pf_state_key_unref(key); } } @@ -990,7 +1003,9 @@ pf_state_insert(struct pfi_kif *kif, struct pf_state_key **skw, } *skw = s->key[PF_SK_WIRE]; if (pf_state_key_attach(*sks, s, PF_SK_STACK)) { - pf_state_key_detach(s, PF_SK_WIRE); + pf_state_key_detach(s, s->key[PF_SK_WIRE]); + s->key[PF_SK_WIRE] = NULL; + *skw = NULL; PF_STATE_EXIT_WRITE(); return (-1); } @@ -1220,30 +1235,35 @@ pf_state_peer_ntoh(const struct pfsync_state_peer *s, struct pf_state_peer *d) } } -void +int pf_state_export(struct pfsync_state *sp, struct pf_state *st) { int32_t expire; + struct pf_state_key *key[2]; memset(sp, 0, sizeof(struct pfsync_state)); /* copy from state key */ - sp->key[PF_SK_WIRE].addr[0] = st->key[PF_SK_WIRE]->addr[0]; - sp->key[PF_SK_WIRE].addr[1] = st->key[PF_SK_WIRE]->addr[1]; - sp->key[PF_SK_WIRE].port[0] = st->key[PF_SK_WIRE]->port[0]; - sp->key[PF_SK_WIRE].port[1] = st->key[PF_SK_WIRE]->port[1]; - sp->key[PF_SK_WIRE].rdomain = htons(st->key[PF_SK_WIRE]->rdomain); - sp->key[PF_SK_WIRE].af = st->key[PF_SK_WIRE]->af; - sp->key[PF_SK_STACK].addr[0] = st->key[PF_SK_STACK]->addr[0]; - sp->key[PF_SK_STACK].addr[1] = st->key[PF_SK_STACK]->addr[1]; - sp->key[PF_SK_STACK].port[0] = st->key[PF_SK_STACK]->port[0]; - sp->key[PF_SK_STACK].port[1] = st->key[PF_SK_STACK]->port[1]; - sp->key[PF_SK_STACK].rdomain = htons(st->key[PF_SK_STACK]->rdomain); - sp->key[PF_SK_STACK].af = st->key[PF_SK_STACK]->af; + pf_state_keys_take(st, key); + if ((key[PF_SK_WIRE] == NULL) || (key[PF_SK_STACK] == NULL)) + return (-1); + + sp->key[PF_SK_WIRE].addr[0] = key[PF_SK_WIRE]->addr[0]; + sp->key[PF_SK_WIRE].addr[1] = key[PF_SK_WIRE]->addr[1]; + sp->key[PF_SK_WIRE].port[0] = key[PF_SK_WIRE]->port[0]; + sp->key[PF_SK_WIRE].port[1] = key[PF_SK_WIRE]->port[1]; + sp->key[PF_SK_WIRE].rdomain = htons(key[PF_SK_WIRE]->rdomain); + sp->key[PF_SK_WIRE].af = key[PF_SK_WIRE]->af; + sp->key[PF_SK_STACK].addr[0] = key[PF_SK_STACK]->addr[0]; + sp->key[PF_SK_STACK].addr[1] = key[PF_SK_STACK]->addr[1]; + sp->key[PF_SK_STACK].port[0] = key[PF_SK_STACK]->port[0]; + sp->key[PF_SK_STACK].port[1] = key[PF_SK_STACK]->port[1]; + sp->key[PF_SK_STACK].rdomain = htons(key[PF_SK_STACK]->rdomain); + sp->key[PF_SK_STACK].af = key[PF_SK_STACK]->af; sp->rtableid[PF_SK_WIRE] = htonl(st->rtableid[PF_SK_WIRE]); sp->rtableid[PF_SK_STACK] = htonl(st->rtableid[PF_SK_STACK]); - sp->proto = st->key[PF_SK_WIRE]->proto; - sp->af = st->key[PF_SK_WIRE]->af; + sp->proto = key[PF_SK_WIRE]->proto; + sp->af = key[PF_SK_WIRE]->af; /* copy from state */ strlcpy(sp->ifname, st->kif->pfik_name, sizeof(sp->ifname)); @@ -1290,6 +1310,10 @@ pf_state_export(struct pfsync_state *sp, struct pf_state *st) sp->set_tos = st->set_tos; sp->set_prio[0] = st->set_prio[0]; sp->set_prio[1] = st->set_prio[1]; + + pf_state_keys_rele(key); + + return (0); } int @@ -8105,3 +8129,19 @@ pf_pktenqueue_delayed(void *arg) pool_put(&pf_pktdelay_pl, pdy); } + +void +pf_state_keys_take(struct pf_state *st, struct pf_state_key *keys[]) +{ + mtx_enter(&st->mtx); + keys[PF_SK_WIRE] = pf_state_key_ref(st->key[PF_SK_WIRE]); + keys[PF_SK_STACK] = pf_state_key_ref(st->key[PF_SK_STACK]); + mtx_leave(&st->mtx); +} + +void +pf_state_keys_rele(struct pf_state_key *keys[]) +{ + pf_state_key_unref(keys[PF_SK_WIRE]); + pf_state_key_unref(keys[PF_SK_STACK]); +} diff --git a/sys/net/pfvar.h b/sys/net/pfvar.h index deabd17741f..998f4b4850a 100644 --- a/sys/net/pfvar.h +++ b/sys/net/pfvar.h @@ -741,7 +741,6 @@ struct pf_state_cmp { u_int8_t pad[3]; }; -/* struct pf_state.state_flags */ #define PFSTATE_ALLOWOPTS 0x0001 #define PFSTATE_SLOPPY 0x0002 #define PFSTATE_PFLOW 0x0004 @@ -1655,7 +1654,7 @@ void pf_state_rm_src_node(struct pf_state *, extern struct pf_state *pf_find_state_byid(struct pf_state_cmp *); extern struct pf_state *pf_find_state_all(struct pf_state_key_cmp *, u_int, int *); -extern void pf_state_export(struct pfsync_state *, +extern int pf_state_export(struct pfsync_state *, struct pf_state *); int pf_state_import(const struct pfsync_state *, int);
