Hello, Richard Procter came back to me in private email with one more nit to fix:
we can get rid of if (sn->rule.ptr != NULL) test condition in pfioctl() function as well. The relevant snippet looks as follows: 2188 p = psn->psn_src_nodes; 2189 RB_FOREACH(n, pf_src_tree, &tree_src_tracking) { .... 2198 pstore->kif = NULL; 2199 if (n->rule.ptr != NULL) 2200 pstore->rule.nr = n->rule.ptr->nr; need one more OK for Richard's suggestion. Updated patch is below. Complete email from Richard follows the patch. thanks and regards sasha --------8<---------------8<---------------8<------------------8<-------- Index: pf.c =================================================================== RCS file: /cvs/src/sys/net/pf.c,v retrieving revision 1.946 diff -u -p -r1.946 pf.c --- pf.c 8 Oct 2015 11:36:51 -0000 1.946 +++ pf.c 12 Oct 2015 20:20:17 -0000 @@ -501,7 +501,7 @@ pf_src_connlimit(struct pf_state **state int pf_insert_src_node(struct pf_src_node **sn, struct pf_rule *rule, enum pf_sn_types type, sa_family_t af, struct pf_addr *src, - struct pf_addr *raddr, int global) + struct pf_addr *raddr) { struct pf_src_node k; @@ -509,10 +509,7 @@ pf_insert_src_node(struct pf_src_node ** k.af = af; k.type = type; PF_ACPY(&k.addr, src, af); - if (global) - k.rule.ptr = NULL; - else - k.rule.ptr = rule; + k.rule.ptr = rule; pf_status.scounters[SCNT_SRC_NODE_SEARCH]++; *sn = RB_FIND(pf_src_tree, &tree_src_tracking, &k); } @@ -531,10 +528,7 @@ pf_insert_src_node(struct pf_src_node ** (*sn)->type = type; (*sn)->af = af; - if (global) - (*sn)->rule.ptr = NULL; - else - (*sn)->rule.ptr = rule; + (*sn)->rule.ptr = rule; PF_ACPY(&(*sn)->addr, src, af); if (raddr) PF_ACPY(&(*sn)->raddr, raddr, af); @@ -550,8 +544,7 @@ pf_insert_src_node(struct pf_src_node ** return (-1); } (*sn)->creation = time_uptime; - if ((*sn)->rule.ptr != NULL) - (*sn)->rule.ptr->src_nodes++; + (*sn)->rule.ptr->src_nodes++; pf_status.scounters[SCNT_SRC_NODE_INSERT]++; pf_status.src_nodes++; } else { @@ -570,16 +563,14 @@ pf_remove_src_node(struct pf_src_node *s if (sn->states > 0 || sn->expire > time_uptime) return; - if (sn->rule.ptr != NULL) { - sn->rule.ptr->src_nodes--; - if (sn->rule.ptr->states_cur == 0 && - sn->rule.ptr->src_nodes == 0) - pf_rm_rule(NULL, sn->rule.ptr); - RB_REMOVE(pf_src_tree, &tree_src_tracking, sn); - pf_status.scounters[SCNT_SRC_NODE_REMOVALS]++; - pf_status.src_nodes--; - pool_put(&pf_src_tree_pl, sn); - } + sn->rule.ptr->src_nodes--; + if (sn->rule.ptr->states_cur == 0 && + sn->rule.ptr->src_nodes == 0) + pf_rm_rule(NULL, sn->rule.ptr); + RB_REMOVE(pf_src_tree, &tree_src_tracking, sn); + pf_status.scounters[SCNT_SRC_NODE_REMOVALS]++; + pf_status.src_nodes--; + pool_put(&pf_src_tree_pl, sn); } struct pf_src_node * @@ -3381,7 +3372,7 @@ pf_test_rule(struct pf_pdesc *pd, struct if (r->rule_flag & PFRULE_SRCTRACK && pf_insert_src_node(&sns[PF_SN_NONE], r, PF_SN_NONE, pd->af, - pd->src, NULL, 0) != 0) { + pd->src, NULL) != 0) { REASON_SET(&reason, PFRES_SRCLIMIT); goto cleanup; } Index: pf_ioctl.c =================================================================== RCS file: /cvs/src/sys/net/pf_ioctl.c,v retrieving revision 1.290 diff -u -p -r1.290 pf_ioctl.c --- pf_ioctl.c 4 Sep 2015 21:40:25 -0000 1.290 +++ pf_ioctl.c 12 Oct 2015 20:20:20 -0000 @@ -2175,8 +2175,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a bzero(&pstore->entry, sizeof(pstore->entry)); pstore->rule.ptr = NULL; pstore->kif = NULL; - if (n->rule.ptr != NULL) - pstore->rule.nr = n->rule.ptr->nr; + pstore->rule.nr = n->rule.ptr->nr; pstore->creation = secs - pstore->creation; if (pstore->expire > secs) pstore->expire -= secs; Index: pf_lb.c =================================================================== RCS file: /cvs/src/sys/net/pf_lb.c,v retrieving revision 1.49 diff -u -p -r1.49 pf_lb.c --- pf_lb.c 3 Aug 2015 13:33:12 -0000 1.49 +++ pf_lb.c 12 Oct 2015 20:20:22 -0000 @@ -621,8 +621,7 @@ pf_map_addr(sa_family_t af, struct pf_ru pf_remove_src_node(sns[type]); sns[type] = NULL; } - if (pf_insert_src_node(&sns[type], r, type, af, saddr, naddr, - 0)) + if (pf_insert_src_node(&sns[type], r, type, af, saddr, naddr)) return (1); } Index: pfvar.h =================================================================== RCS file: /cvs/src/sys/net/pfvar.h,v retrieving revision 1.420 diff -u -p -r1.420 pfvar.h --- pfvar.h 19 Aug 2015 21:22:41 -0000 1.420 +++ pfvar.h 12 Oct 2015 20:20:23 -0000 @@ -1681,7 +1681,7 @@ extern int pf_state_insert(struct pfi int pf_insert_src_node(struct pf_src_node **, struct pf_rule *, enum pf_sn_types, sa_family_t, struct pf_addr *, - struct pf_addr *, int); + struct pf_addr *); void pf_remove_src_node(struct pf_src_node *); struct pf_src_node *pf_get_src_node(struct pf_state *, enum pf_sn_types); --------8<---------------8<---------------8<------------------8<-------- On Tue, Oct 13, 2015 at 08:45:01AM +1300, Richard Procter wrote: > Hi Sasha, > > I found some time to look at the pf_remove_src_node() issue. > > As far as I can tell, pf_remove_src_node() never receives a NULL > sn->rule.ptr once the 'global' is removed. I first note that: > > pf_insert_src_node() alone inserts into tree_src_tracking /\ > pf_insert_src_node() ensures (*sn)->rule.ptr != NULL (0) > > I've traced the (bottom-up) call tree as follows, > > pf_remove_src_node() > pf_purge_expired_src_nodes() > iterates over tree_src_tracking > => { (0) } > cur->rule.ptr != NULL [good] > > pf_map_addr_sticky(*sns[]) > retrieves from tree_src_tracking > => { (0) } > retrieved source node->rule.ptr != NULL [good] > > pf_create_state(*sns[]) > pf_map_addr(*sns[]) > > Ok, let's try to establish that the last two always receive > (pointers to) struct pf_src_nodes from tree_src_tracking. > Their (bottom-up) call trees are: > > pf_create_state(*sns[]) > pf_test_rule() > > pf_map_addr(*sns[]) > pf_get_sport(*sn[]) > pf_get_transaddr(*sns[]) > pf_get_transaddr_af(*sns[]) > pf_get_transaddr(*sns[]) > pf_test_rule() > pf_get_transaddr_af(*sns[]) > pf_get_transaddr(*sns[]) > pf_set_rt_ifp() > pf_route() > pf_route6() > > If these always populate *sns[] with items added > by pf_insert_src_node(), we're done. > > pf_test_rule(), > pf_set_rt_ifp(), > pf_route() and > pf_route6() all initialise their *sns[] array to NULL > and never populate it directly. > > pf_get_sport(*sns[]), > pf_get_transaddr(*sns[]), > pf_get_transsaddr_af(*sns[]) never alter *sns[] directly. > > pf_map_addr() modifies *sns[] directly only by setting NULL on removal. > > So it looks good. > > Analagous to the guard in pf_remove_src_node(), removing 'global' > probably also allows the following two guards to be removed as they too > involve pf_src_nodes inserted into the tree_src_tracking tree. > > best, > Richard. > > Index: pf.c > =================================================================== > RCS file: /cvs/src/sys/net/pf.c,v > retrieving revision 1.946 > diff -u -p -u -r1.946 pf.c > --- pf.c 8 Oct 2015 11:36:51 -0000 1.946 > +++ pf.c 12 Oct 2015 03:25:22 -0000 > @@ -550,8 +550,7 @@ pf_insert_src_node(struct pf_src_node ** > return (-1); > } > (*sn)->creation = time_uptime; > - if ((*sn)->rule.ptr != NULL) > - (*sn)->rule.ptr->src_nodes++; > + (*sn)->rule.ptr->src_nodes++; > pf_status.scounters[SCNT_SRC_NODE_INSERT]++; > pf_status.src_nodes++; > } else { > Index: pf_ioctl.c > =================================================================== > RCS file: /cvs/src/sys/net/pf_ioctl.c,v > retrieving revision 1.290 > diff -u -p -u -r1.290 pf_ioctl.c > --- pf_ioctl.c 4 Sep 2015 21:40:25 -0000 1.290 > +++ pf_ioctl.c 12 Oct 2015 03:25:22 -0000 > @@ -2175,8 +2175,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a > bzero(&pstore->entry, sizeof(pstore->entry)); > pstore->rule.ptr = NULL; > pstore->kif = NULL; > - if (n->rule.ptr != NULL) > - pstore->rule.nr = n->rule.ptr->nr; > + pstore->rule.nr = n->rule.ptr->nr; > pstore->creation = secs - pstore->creation; > if (pstore->expire > secs) > pstore->expire -= secs; > >