regression in pflog output

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

regression in pflog output

Alexandr Nedvedicky
Hello,

Matthias Pitzl discovered a regression introduced by my earlier commit [1].
Matthias has noticed the pflogd output changes for ruleset here:
--------8<---------------8<---------------8<------------------8<--------
    block out log quick from any to 1.1.1.1
    block out log quick from any to 1.1.1.2
    anchor log_test {
        block out log quick from any to 2.1.1.1
        block out log quick from any to 2.1.1.2
    }
    block out log quick from any to 3.1.1.1
    block out log quick from any to 3.1.1.2
--------8<---------------8<---------------8<------------------8<--------

pinging addresses used in rules above Matthias noticed the rule numbers
and anchors in log are incorrect:

    Feb  7 16:34:47 sys pf: rule 0/(match) [usid 0, pid 95203] block out ... > 1.1.1.1
    Feb  7 16:34:48 sys pf: rule 1/(match) [usid 0, pid 95203] block out ... > 1.1.1.2
    Feb  7 16:34:50 sys pf: rule 2.log_test.0s/(match) [uid 0, pid 95203] block out on ... > 2.1.1.1
    Feb  7 16:34:52 sys pf: rule 2.log_test.1s/(match) [uid 0, pid 95203] block out on ... > 2.1.1.2
    Feb  7 16:34:55 sys pf: rule 2/(match) [usid 0, pid 95203] block out on ... > 3.1.1.1
    Feb  7 16:34:57 sys pf: rule 2/(match) [uid 0, pid 95203] block out on ... > 3.1.1.2

in output above the entries for 3.1.1.1 and 3.1.1.2 have a wrong rule number.
we should see rule number 3 for 3.1.1.1 and 4 for 3.1.1.2

With joint effort we could identify two problems in my earlier change.

    1) pf_match_rule() must remember anchor rule and its ruleset
        kept in ctx, before it updates ctx for descent:

    3689                         ctx->a = r;             /* remember anchor */
    3690                         ctx->aruleset = ruleset;        /* and its ruleset */
    3691                         if (pf_step_into_anchor(ctx, r) != PF_TEST_OK)
    3692                                 break;

        once pf_step_into_anchor() returns and we are supposed to continue
        with rule evaluation, we are better to restore ctx->a and ctx->aruleset,
        which match our nesting level.

    2) PFLOG_PACKET() called from pf_test_rule():

    3789 #if NPFLOG > 0
    3790         if (r->log)
    3791                 PFLOG_PACKET(pd, ctx.reason, r, ctx.a, ruleset, NULL);
    3792         if (ctx.act.log & PF_LOG_MATCHES)
    3793                 pf_log_matches(pd, r, ctx.a, ruleset, &ctx.rules);
    3794 #endif  /* NPFLOG > 0 */

    uses anchor kept in ctx, instead of local variable, which holds anchor for
    matching rule.


OK?

thanks and
regards
sasha

[1] https://github.com/openbsd/src/commit/e236f0fa7b23e94c7258b2055ec8e7c9804957c7#diff-9517dfce4e8db974781a4536fd38cfc1

--------8<---------------8<---------------8<------------------8<--------
diff --git a/sys/net/pf.c b/sys/net/pf.c
index 51a91114c74..75d4e7158c2 100644
--- a/sys/net/pf.c
+++ b/sys/net/pf.c
@@ -3108,9 +3108,9 @@ pf_step_into_anchor(struct pf_test_ctx *ctx, struct pf_rule *r)
  rv = pf_match_rule(ctx, &child->ruleset);
  if ((rv == PF_TEST_QUICK) || (rv == PF_TEST_FAIL)) {
  /*
- * we either hit a rule qith quick action
+ * we either hit a rule with quick action
  * (more likely), or hit some runtime
- * error (e.g. pool_get() faillure).
+ * error (e.g. pool_get() failure).
  */
  break;
  }
@@ -3497,6 +3497,8 @@ enum pf_test_status
 pf_match_rule(struct pf_test_ctx *ctx, struct pf_ruleset *ruleset)
 {
  struct pf_rule *r;
+ struct pf_rule *save_a;
+ struct pf_ruleset *save_aruleset;
 
  r = TAILQ_FIRST(ruleset->rules.active.ptr);
  while (r != NULL) {
@@ -3682,11 +3684,18 @@ pf_match_rule(struct pf_test_ctx *ctx, struct pf_ruleset *ruleset)
  break;
  }
  } else {
+ save_a = ctx->a;
+ save_aruleset = ctx->aruleset;
  ctx->a = r; /* remember anchor */
  ctx->aruleset = ruleset; /* and its ruleset */
- if (pf_step_into_anchor(ctx, r) != PF_TEST_OK) {
+ /*
+ * Note: we don't need to restore if we are not going
+ * to continue with ruleset evaluation.
+ */
+ if (pf_step_into_anchor(ctx, r) != PF_TEST_OK)
  break;
- }
+ ctx->a = save_a;
+ ctx->aruleset = save_aruleset;
  }
  r = TAILQ_NEXT(r, entries);
  }
@@ -3768,8 +3777,6 @@ pf_test_rule(struct pf_pdesc *pd, struct pf_rule **rm, struct pf_state **sm,
  ruleset = *ctx.rsm;/* ruleset of the anchor defined by the rule 'a' */
  ctx.aruleset = ctx.arsm;/* ruleset of the 'a' rule itself */
 
-
-
  /* apply actions for last matching pass/block rule */
  pf_rule_to_actions(r, &ctx.act);
  if (r->rule_flag & PFRULE_AFTO)
@@ -3782,9 +3789,9 @@ pf_test_rule(struct pf_pdesc *pd, struct pf_rule **rm, struct pf_state **sm,
 
 #if NPFLOG > 0
  if (r->log)
- PFLOG_PACKET(pd, ctx.reason, r, ctx.a, ruleset, NULL);
+ PFLOG_PACKET(pd, ctx.reason, r, a, ruleset, NULL);
  if (ctx.act.log & PF_LOG_MATCHES)
- pf_log_matches(pd, r, ctx.a, ruleset, &ctx.rules);
+ pf_log_matches(pd, r, a, ruleset, &ctx.rules);
 #endif /* NPFLOG > 0 */
 
  if (pd->virtual_proto != PF_VPROTO_FRAGMENT &&

Reply | Threaded
Open this post in threaded view
|

Re: regression in pflog output

trondd-2
On Thu, February 8, 2018 11:24 am, Alexandr Nedvedicky wrote:

> Hello,
>
> Matthias Pitzl discovered a regression introduced by my earlier commit
> [1].
> Matthias has noticed the pflogd output changes for ruleset here:
> --------8<---------------8<---------------8<------------------8<--------
>     block out log quick from any to 1.1.1.1
>     block out log quick from any to 1.1.1.2
>     anchor log_test {
> block out log quick from any to 2.1.1.1
> block out log quick from any to 2.1.1.2
>     }
>     block out log quick from any to 3.1.1.1
>     block out log quick from any to 3.1.1.2
> --------8<---------------8<---------------8<------------------8<--------
>
> pinging addresses used in rules above Matthias noticed the rule numbers
> and anchors in log are incorrect:
>
>     Feb  7 16:34:47 sys pf: rule 0/(match) [usid 0, pid 95203] block out
> ... > 1.1.1.1
>     Feb  7 16:34:48 sys pf: rule 1/(match) [usid 0, pid 95203] block out
> ... > 1.1.1.2
>     Feb  7 16:34:50 sys pf: rule 2.log_test.0s/(match) [uid 0, pid 95203]
> block out on ... > 2.1.1.1
>     Feb  7 16:34:52 sys pf: rule 2.log_test.1s/(match) [uid 0, pid 95203]
> block out on ... > 2.1.1.2
>     Feb  7 16:34:55 sys pf: rule 2/(match) [usid 0, pid 95203] block out
> on ... > 3.1.1.1
>     Feb  7 16:34:57 sys pf: rule 2/(match) [uid 0, pid 95203] block out on
> ... > 3.1.1.2
>
> in output above the entries for 3.1.1.1 and 3.1.1.2 have a wrong rule
> number.
> we should see rule number 3 for 3.1.1.1 and 4 for 3.1.1.2
>
> With joint effort we could identify two problems in my earlier change.
>
>     1) pf_match_rule() must remember anchor rule and its ruleset
> kept in ctx, before it updates ctx for descent:
>
>     3689                         ctx->a = r;             /* remember
> anchor */
>     3690                         ctx->aruleset = ruleset;        /* and
> its ruleset */
>     3691                         if (pf_step_into_anchor(ctx, r) !=
> PF_TEST_OK)
>     3692                                 break;
>
> once pf_step_into_anchor() returns and we are supposed to continue
> with rule evaluation, we are better to restore ctx->a and ctx->aruleset,
> which match our nesting level.
>
>     2) PFLOG_PACKET() called from pf_test_rule():
>
>     3789 #if NPFLOG > 0
>     3790         if (r->log)
>     3791                 PFLOG_PACKET(pd, ctx.reason, r, ctx.a, ruleset,
> NULL);
>     3792         if (ctx.act.log & PF_LOG_MATCHES)
>     3793                 pf_log_matches(pd, r, ctx.a, ruleset,
> &ctx.rules);
>     3794 #endif  /* NPFLOG > 0 */
>
>     uses anchor kept in ctx, instead of local variable, which holds anchor
> for
>     matching rule.
>
>
> OK?
>
> thanks and
> regards
> sasha
>
> [1]
> https://github.com/openbsd/src/commit/e236f0fa7b23e94c7258b2055ec8e7c9804957c7#diff-9517dfce4e8db974781a4536fd38cfc1
>

I ran into this as well and can at least confirm that this fixes it.

Tim.


> --------8<---------------8<---------------8<------------------8<--------
> diff --git a/sys/net/pf.c b/sys/net/pf.c
> index 51a91114c74..75d4e7158c2 100644
> --- a/sys/net/pf.c
> +++ b/sys/net/pf.c
> @@ -3108,9 +3108,9 @@ pf_step_into_anchor(struct pf_test_ctx *ctx, struct
> pf_rule *r)
>   rv = pf_match_rule(ctx, &child->ruleset);
>   if ((rv == PF_TEST_QUICK) || (rv == PF_TEST_FAIL)) {
>   /*
> - * we either hit a rule qith quick action
> + * we either hit a rule with quick action
>   * (more likely), or hit some runtime
> - * error (e.g. pool_get() faillure).
> + * error (e.g. pool_get() failure).
>   */
>   break;
>   }
> @@ -3497,6 +3497,8 @@ enum pf_test_status
>  pf_match_rule(struct pf_test_ctx *ctx, struct pf_ruleset *ruleset)
>  {
>   struct pf_rule *r;
> + struct pf_rule *save_a;
> + struct pf_ruleset *save_aruleset;
>
>   r = TAILQ_FIRST(ruleset->rules.active.ptr);
>   while (r != NULL) {
> @@ -3682,11 +3684,18 @@ pf_match_rule(struct pf_test_ctx *ctx, struct
> pf_ruleset *ruleset)
>   break;
>   }
>   } else {
> + save_a = ctx->a;
> + save_aruleset = ctx->aruleset;
>   ctx->a = r; /* remember anchor */
>   ctx->aruleset = ruleset; /* and its ruleset */
> - if (pf_step_into_anchor(ctx, r) != PF_TEST_OK) {
> + /*
> + * Note: we don't need to restore if we are not going
> + * to continue with ruleset evaluation.
> + */
> + if (pf_step_into_anchor(ctx, r) != PF_TEST_OK)
>   break;
> - }
> + ctx->a = save_a;
> + ctx->aruleset = save_aruleset;
>   }
>   r = TAILQ_NEXT(r, entries);
>   }
> @@ -3768,8 +3777,6 @@ pf_test_rule(struct pf_pdesc *pd, struct pf_rule
> **rm, struct pf_state **sm,
>   ruleset = *ctx.rsm;/* ruleset of the anchor defined by the rule 'a' */
>   ctx.aruleset = ctx.arsm;/* ruleset of the 'a' rule itself */
>
> -
> -
>   /* apply actions for last matching pass/block rule */
>   pf_rule_to_actions(r, &ctx.act);
>   if (r->rule_flag & PFRULE_AFTO)
> @@ -3782,9 +3789,9 @@ pf_test_rule(struct pf_pdesc *pd, struct pf_rule
> **rm, struct pf_state **sm,
>
>  #if NPFLOG > 0
>   if (r->log)
> - PFLOG_PACKET(pd, ctx.reason, r, ctx.a, ruleset, NULL);
> + PFLOG_PACKET(pd, ctx.reason, r, a, ruleset, NULL);
>   if (ctx.act.log & PF_LOG_MATCHES)
> - pf_log_matches(pd, r, ctx.a, ruleset, &ctx.rules);
> + pf_log_matches(pd, r, a, ruleset, &ctx.rules);
>  #endif /* NPFLOG > 0 */
>
>   if (pd->virtual_proto != PF_VPROTO_FRAGMENT &&
>


Reply | Threaded
Open this post in threaded view
|

Re: regression in pflog output

Alexander Bluhm
In reply to this post by Alexandr Nedvedicky
On Thu, Feb 08, 2018 at 05:24:13PM +0100, Alexandr Nedvedicky wrote:
> OK?

OK bluhm@

> [1] https://github.com/openbsd/src/commit/e236f0fa7b23e94c7258b2055ec8e7c9804957c7#diff-9517dfce4e8db974781a4536fd38cfc1
>
> --------8<---------------8<---------------8<------------------8<--------
> diff --git a/sys/net/pf.c b/sys/net/pf.c
> index 51a91114c74..75d4e7158c2 100644
> --- a/sys/net/pf.c
> +++ b/sys/net/pf.c
> @@ -3108,9 +3108,9 @@ pf_step_into_anchor(struct pf_test_ctx *ctx, struct pf_rule *r)
>   rv = pf_match_rule(ctx, &child->ruleset);
>   if ((rv == PF_TEST_QUICK) || (rv == PF_TEST_FAIL)) {
>   /*
> - * we either hit a rule qith quick action
> + * we either hit a rule with quick action
>   * (more likely), or hit some runtime
> - * error (e.g. pool_get() faillure).
> + * error (e.g. pool_get() failure).
>   */
>   break;
>   }
> @@ -3497,6 +3497,8 @@ enum pf_test_status
>  pf_match_rule(struct pf_test_ctx *ctx, struct pf_ruleset *ruleset)
>  {
>   struct pf_rule *r;
> + struct pf_rule *save_a;
> + struct pf_ruleset *save_aruleset;
>  
>   r = TAILQ_FIRST(ruleset->rules.active.ptr);
>   while (r != NULL) {
> @@ -3682,11 +3684,18 @@ pf_match_rule(struct pf_test_ctx *ctx, struct pf_ruleset *ruleset)
>   break;
>   }
>   } else {
> + save_a = ctx->a;
> + save_aruleset = ctx->aruleset;
>   ctx->a = r; /* remember anchor */
>   ctx->aruleset = ruleset; /* and its ruleset */
> - if (pf_step_into_anchor(ctx, r) != PF_TEST_OK) {
> + /*
> + * Note: we don't need to restore if we are not going
> + * to continue with ruleset evaluation.
> + */
> + if (pf_step_into_anchor(ctx, r) != PF_TEST_OK)
>   break;
> - }
> + ctx->a = save_a;
> + ctx->aruleset = save_aruleset;
>   }
>   r = TAILQ_NEXT(r, entries);
>   }
> @@ -3768,8 +3777,6 @@ pf_test_rule(struct pf_pdesc *pd, struct pf_rule **rm, struct pf_state **sm,
>   ruleset = *ctx.rsm;/* ruleset of the anchor defined by the rule 'a' */
>   ctx.aruleset = ctx.arsm;/* ruleset of the 'a' rule itself */
>  
> -
> -
>   /* apply actions for last matching pass/block rule */
>   pf_rule_to_actions(r, &ctx.act);
>   if (r->rule_flag & PFRULE_AFTO)
> @@ -3782,9 +3789,9 @@ pf_test_rule(struct pf_pdesc *pd, struct pf_rule **rm, struct pf_state **sm,
>  
>  #if NPFLOG > 0
>   if (r->log)
> - PFLOG_PACKET(pd, ctx.reason, r, ctx.a, ruleset, NULL);
> + PFLOG_PACKET(pd, ctx.reason, r, a, ruleset, NULL);
>   if (ctx.act.log & PF_LOG_MATCHES)
> - pf_log_matches(pd, r, ctx.a, ruleset, &ctx.rules);
> + pf_log_matches(pd, r, a, ruleset, &ctx.rules);
>  #endif /* NPFLOG > 0 */
>  
>   if (pd->virtual_proto != PF_VPROTO_FRAGMENT &&