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.
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,
