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