On Tue, Oct 16, 2018 at 10:03:01AM +0200, Alexandr Nedvedicky wrote:
> I also wonder if we should apply same change to wildcard anchors here:
They need fixing as well, but I have yet to look into it.
> diff --git a/sys/net/pf.c b/sys/net/pf.c
> index 0bdf90a8d13..5a5c739773a 100644
> --- a/sys/net/pf.c
> +++ b/sys/net/pf.c
> @@ -3129,10 +3129,32 @@ pf_step_into_anchor(struct pf_test_ctx *ctx, struct
> pf_rule *r)
> } else {
> rv = pf_match_rule(ctx, &r->anchor->ruleset);
This comment is way too much, imho. How about this:
Unless errors occured, stop iff any rule matched
within quick anchors.
It's dense but we're inside pf_step_into_anchor() so context is given
while the condition speaks for itself as well. I'd like to leave
detailed explanations to the manual.
> - if (rv != PF_TEST_FAIL && r->quick == PF_TEST_QUICK)
> + if (rv != PF_TEST_FAIL && r->quick == PF_TEST_QUICK &&
> + *ctx->am == r)
> rv = PF_TEST_QUICK;
> }
That makes sense and works as expected with *all* examples and real use
cases I could come up with.
Index: net/pf.c
===================================================================
RCS file: /cvs/src/sys/net/pf.c,v
retrieving revision 1.1076
diff -u -p -r1.1076 pf.c
--- net/pf.c 4 Oct 2018 20:25:59 -0000 1.1076
+++ net/pf.c 16 Oct 2018 11:32:10 -0000
@@ -3129,10 +3129,11 @@ pf_step_into_anchor(struct pf_test_ctx *
} else {
rv = pf_match_rule(ctx, &r->anchor->ruleset);
/*
- * Unless there was an error inside the anchor,
- * retain its quick state.
+ * Unless errors occured, stop iff any rule matched
+ * within quick anchors.
*/
- if (rv != PF_TEST_FAIL && r->quick == PF_TEST_QUICK)
+ if (rv != PF_TEST_FAIL && r->quick == PF_TEST_QUICK &&
+ *ctx->am == r)
rv = PF_TEST_QUICK;
}
