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;
> >
> >
>
