On Sun, Sep 03, 2023 at 11:00:56PM +0200, Alexandr Nedvedicky wrote:
> Hello,
>
> On Sun, Sep 03, 2023 at 09:26:29PM +0200, Florian Obser wrote:
> > FYI, I'm not using sloppy, and I don't have a network with asymmetric
> > routing
> > at the moment. I only remembered that we used sloppy for a while at my
> > previous job. I think we settled on no-state because it was faster than
> > sloppy and less hastle.
> >
>
> From my perspective 'no state' vs. 'keep state (sloppy)' are valid
> approaches.
> Both are equally good. Perhaps 'no state' option keeps code bit more
> simple.
> Because if we will go with sloppy state, then we need to include a small
> tweak to pf_test_rule() too, so 'keep state' (and nat-to) are not ignored.
I do not think that ICMP error messages should create a state. If
the rule is sloppy, just pass them. nat-to does not work in this
case, but I would ignore that. If you use sloppy states, don't be
surprized about strange effects in corner cases.
First two chunks are OK bluhm@, but not the third.
> Updated diff is below.
>
> thanks and
> regards
> sashan
>
> --------8<---------------8<---------------8<------------------8<--------
> diff --git a/sys/net/pf.c b/sys/net/pf.c
> index 4f0fc3f91a9..6f7b782612c 100644
> --- a/sys/net/pf.c
> +++ b/sys/net/pf.c
> @@ -4148,6 +4148,10 @@ enter_ruleset:
> (r->rule_flag & PFRULE_STATESLOPPY) == 0 &&
> ctx->icmp_dir != PF_IN),
> TAILQ_NEXT(r, entries));
> + /* icmp packet must match existing state */
> + PF_TEST_ATTRIB(r->keep_state && ctx->state_icmp &&
> + (r->rule_flag & PFRULE_STATESLOPPY) == 0,
> + TAILQ_NEXT(r, entries));
> break;
>
> case IPPROTO_ICMPV6:
> @@ -4165,6 +4169,10 @@ enter_ruleset:
> ctx->icmp_dir != PF_IN &&
> ctx->icmptype != ND_NEIGHBOR_ADVERT),
> TAILQ_NEXT(r, entries));
> + /* icmp packet must match existing state */
> + PF_TEST_ATTRIB(r->keep_state && ctx->state_icmp &&
> + (r->rule_flag & PFRULE_STATESLOPPY) == 0,
> + TAILQ_NEXT(r, entries));
> break;
>
> default:
> @@ -4469,8 +4477,7 @@ pf_test_rule(struct pf_pdesc *pd, struct pf_rule **rm,
> struct pf_state **sm,
>
> action = PF_PASS;
>
> - if (pd->virtual_proto != PF_VPROTO_FRAGMENT
> - && !ctx.state_icmp && r->keep_state) {
> + if (pd->virtual_proto != PF_VPROTO_FRAGMENT && r->keep_state) {
>
> if (r->rule_flag & PFRULE_SRCTRACK &&
> pf_insert_src_node(&ctx.sns[PF_SN_NONE], r, PF_SN_NONE,
