On 12/10/15(Mon) 22:29, Alexandr Nedvedicky wrote: > 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.
ok mpi@ > > 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; > > > > >