On 21 June 2014 15:36, Alexandr Nedvedicky <alexandr.nedvedi...@oracle.com>
wrote:
> Hello,
>
> I'm not sure it is the right place to submit patches. Let me know if there is
> better/more appropriate address for this.
>
> during our testing we've found the once rules are not removed,
> when used in main anchor.
>
Correct. I've addedd the ruleset pointer check to prevent that
shortly before the release since it caused panics...
> during debugging we found the rules in main anchor have member anchor set to
> NULL (pf_rule::anchor). This makes pf_purge_rule() function to bail out
> to early without removing the rule from ruleset.
>
> patch below fixed problem for us.
>
However, this solution is not correct for us. Perhaps you have some
other changes in your tree to make it work.
> Index: pf_ioctl.c
> ===================================================================
> RCS file: /cvs/src/sys/net/pf_ioctl.c,v
> retrieving revision 1.272
> diff -u -r1.272 pf_ioctl.c
> --- pf_ioctl.c 22 Apr 2014 14:41:03 -0000 1.272
> +++ pf_ioctl.c 20 Jun 2014 14:26:22 -0000
> @@ -312,7 +312,7 @@
> {
> u_int32_t nr;
>
> - if (ruleset == NULL || ruleset->anchor == NULL)
> + if (ruleset == NULL)
> return;
>
ruleset->anchor is useless since nothing really checks for it if
ruleset is NULL.
> pf_rm_rule(ruleset->rules.active.ptr, rule);
> @@ -325,7 +325,10 @@
> ruleset->rules.active.ticket++;
>
> pf_calc_skip_steps(ruleset->rules.active.ptr);
> - pf_remove_if_empty_ruleset(ruleset);
> +
> + if (ruleset != &pf_main_ruleset) {
> + pf_remove_if_empty_ruleset(ruleset);
> + }
> }
>
You don't need to check ruleset against &pf_main_ruleset since
the first thing pf_remove_if_empty_ruleset does is bail if
ruleset == &pf_main_ruleset. This bit confused me quite a bit.
What really is a problem for us is that when pf_purge_rule is
called on a main ruleset from pf_test_rule the first argument
(ruleset) is NULL and not &pf_main_ruleset, which would be
sensible. The only other user of it, AFAICT, is pflog_packet
but it checks for ruleset->anchor before it does it's thing.
I don't see any reason why we can't start our rule set iteration
with 'ruleset' set to &pf_main_ruleset (regress tests agree).
OK?
diff --git sys/net/pf.c sys/net/pf.c
index 71f85d1..562901d 100644
--- sys/net/pf.c
+++ sys/net/pf.c
@@ -3163,10 +3163,11 @@ pf_test_rule(struct pf_pdesc *pd, struct pf_rule **rm,
struct pf_state **sm,
}
break;
#endif /* INET6 */
}
+ ruleset = &pf_main_ruleset;
r = TAILQ_FIRST(pf_main_ruleset.rules.active.ptr);
while (r != NULL) {
r->evaluations++;
PF_TEST_ATTRIB((pfi_kif_match(r->kif, pd->kif) == r->ifnot),
r->skip[PF_SKIP_IFP].ptr);
diff --git sys/net/pf_ioctl.c sys/net/pf_ioctl.c
index 5ad762c..86e987f 100644
--- sys/net/pf_ioctl.c
+++ sys/net/pf_ioctl.c
@@ -308,24 +308,19 @@ pf_rm_rule(struct pf_rulequeue *rulequeue, struct pf_rule
*rule)
}
void
pf_purge_rule(struct pf_ruleset *ruleset, struct pf_rule *rule)
{
- u_int32_t nr;
+ u_int32_t nr = 0;
- if (ruleset == NULL || ruleset->anchor == NULL)
- return;
+ KASSERT(ruleset != NULL && rule != NULL);
pf_rm_rule(ruleset->rules.active.ptr, rule);
ruleset->rules.active.rcount--;
-
- nr = 0;
TAILQ_FOREACH(rule, ruleset->rules.active.ptr, entries)
rule->nr = nr++;
-
ruleset->rules.active.ticket++;
-
pf_calc_skip_steps(ruleset->rules.active.ptr);
pf_remove_if_empty_ruleset(ruleset);
}
u_int16_t
