From 8ec81e30bf59110b131953d745cc4cbd8dff0d1e Mon Sep 17 00:00:00 2001 From: Igor Ostapenko Date: Thu, 23 Nov 2023 00:37:57 +0200 Subject: [PATCH 1/5] pf: fix mem leak upon module unload It had no removal of anchor rulesets, considering nested ones as well. PR: 274310 --- sys/netpfil/pf/pf_ioctl.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/sys/netpfil/pf/pf_ioctl.c b/sys/netpfil/pf/pf_ioctl.c index 1598939c237582..40ad8b8728a1f6 100644 --- a/sys/netpfil/pf/pf_ioctl.c +++ b/sys/netpfil/pf/pf_ioctl.c @@ -6211,6 +6211,8 @@ shutdown_pf(void) int error = 0; u_int32_t t[5]; char nn = '\0'; + struct pf_kanchor *anchor; + int rs_num; do { if ((error = pf_begin_rules(&t[0], PF_RULESET_SCRUB, &nn)) @@ -6246,6 +6248,22 @@ shutdown_pf(void) pf_commit_rules(t[3], PF_RULESET_BINAT, &nn); pf_commit_rules(t[4], PF_RULESET_RDR, &nn); + /* Repeat the same for all user defined anchors */ + RB_FOREACH_REVERSE(anchor, pf_kanchor_global, &V_pf_anchors) { + for (rs_num = 0; rs_num < PF_RULESET_MAX; ++rs_num) { + if ((error = pf_begin_rules(&t[rs_num], rs_num, + anchor->path)) != 0) { + DPFPRINTF(PF_DEBUG_MISC, ("shutdown_pf: " + "anchor=%s, rs_num=%d\n", + anchor->path, rs_num)); + goto error; /* XXX: rollback? */ + } + /* XXX: these should always succeed here */ + pf_commit_rules(t[rs_num], rs_num, + anchor->path); + } + } + if ((error = pf_clear_tables()) != 0) break; @@ -6271,6 +6289,7 @@ shutdown_pf(void) /* fingerprints and interfaces have their own cleanup code */ } while(0); +error: return (error); } From 2e32ab903b513e622e8cf5efb9a7e49cb6e4b6aa Mon Sep 17 00:00:00 2001 From: Igor Ostapenko Date: Fri, 24 Nov 2023 02:15:43 +0200 Subject: [PATCH 2/5] Cover anchor wildcard cases --- sys/netpfil/pf/pf_ioctl.c | 39 +++++++++++++++++++++++---------------- 1 file changed, 23 insertions(+), 16 deletions(-) diff --git a/sys/netpfil/pf/pf_ioctl.c b/sys/netpfil/pf/pf_ioctl.c index 40ad8b8728a1f6..3682595bd3ce3d 100644 --- a/sys/netpfil/pf/pf_ioctl.c +++ b/sys/netpfil/pf/pf_ioctl.c @@ -6215,6 +6215,29 @@ shutdown_pf(void) int rs_num; do { + /* Unlink rules of all user defined anchors */ + RB_FOREACH(anchor, pf_kanchor_global, &V_pf_anchors) { + /* Wildcard based anchors may not have a respective + * explicit anchor rule, it leads to refcnt=0, and the + * rest of the logic does not expect it. */ + if (anchor->refcnt == 0) + anchor->refcnt = 1; + for (rs_num = 0; rs_num < PF_RULESET_MAX; ++rs_num) { + if ((error = pf_begin_rules(&t[rs_num], rs_num, + anchor->path)) != 0) { + DPFPRINTF(PF_DEBUG_MISC, ("shutdown_pf: " + "anchor.path=%s, rs_num=%d\n", + anchor->path, rs_num)); + goto error; /* XXX: rollback? */ + } + } + for (rs_num = 0; rs_num < PF_RULESET_MAX; ++rs_num) { + /* XXX: these should always succeed here */ + pf_commit_rules(t[rs_num], rs_num, + anchor->path); + } + } + if ((error = pf_begin_rules(&t[0], PF_RULESET_SCRUB, &nn)) != 0) { DPFPRINTF(PF_DEBUG_MISC, ("shutdown_pf: SCRUB\n")); @@ -6248,22 +6271,6 @@ shutdown_pf(void) pf_commit_rules(t[3], PF_RULESET_BINAT, &nn); pf_commit_rules(t[4], PF_RULESET_RDR, &nn); - /* Repeat the same for all user defined anchors */ - RB_FOREACH_REVERSE(anchor, pf_kanchor_global, &V_pf_anchors) { - for (rs_num = 0; rs_num < PF_RULESET_MAX; ++rs_num) { - if ((error = pf_begin_rules(&t[rs_num], rs_num, - anchor->path)) != 0) { - DPFPRINTF(PF_DEBUG_MISC, ("shutdown_pf: " - "anchor=%s, rs_num=%d\n", - anchor->path, rs_num)); - goto error; /* XXX: rollback? */ - } - /* XXX: these should always succeed here */ - pf_commit_rules(t[rs_num], rs_num, - anchor->path); - } - } - if ((error = pf_clear_tables()) != 0) break; From 267cd5af8752c33fd95227abf014da349a4aaf40 Mon Sep 17 00:00:00 2001 From: Igor Ostapenko Date: Fri, 24 Nov 2023 02:56:35 +0200 Subject: [PATCH 3/5] Remove tables for all rulesets --- sys/netpfil/pf/pf_ioctl.c | 1 + 1 file changed, 1 insertion(+) diff --git a/sys/netpfil/pf/pf_ioctl.c b/sys/netpfil/pf/pf_ioctl.c index 3682595bd3ce3d..9260bfc5fecae7 100644 --- a/sys/netpfil/pf/pf_ioctl.c +++ b/sys/netpfil/pf/pf_ioctl.c @@ -5801,6 +5801,7 @@ pf_clear_tables(void) int error; bzero(&io, sizeof(io)); + io.pfrio_flags |= PFR_FLAG_ALLRSETS; error = pfr_clr_tables(&io.pfrio_table, &io.pfrio_ndel, io.pfrio_flags); From 061654a023edc3cf924bc8b91d1110b5ce26a005 Mon Sep 17 00:00:00 2001 From: Igor Ostapenko Date: Fri, 24 Nov 2023 13:49:57 +0200 Subject: [PATCH 4/5] Remove user defined eth anchors upon vnet destroy --- sys/netpfil/pf/pf_ioctl.c | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/sys/netpfil/pf/pf_ioctl.c b/sys/netpfil/pf/pf_ioctl.c index 9260bfc5fecae7..dc016e35b9e718 100644 --- a/sys/netpfil/pf/pf_ioctl.c +++ b/sys/netpfil/pf/pf_ioctl.c @@ -6213,13 +6213,15 @@ shutdown_pf(void) u_int32_t t[5]; char nn = '\0'; struct pf_kanchor *anchor; + struct pf_keth_anchor *eth_anchor; int rs_num; do { /* Unlink rules of all user defined anchors */ RB_FOREACH(anchor, pf_kanchor_global, &V_pf_anchors) { /* Wildcard based anchors may not have a respective - * explicit anchor rule, it leads to refcnt=0, and the + * explicit anchor rule or they may be left empty + * without rules. It leads to anchor.refcnt=0, and the * rest of the logic does not expect it. */ if (anchor->refcnt == 0) anchor->refcnt = 1; @@ -6227,7 +6229,7 @@ shutdown_pf(void) if ((error = pf_begin_rules(&t[rs_num], rs_num, anchor->path)) != 0) { DPFPRINTF(PF_DEBUG_MISC, ("shutdown_pf: " - "anchor.path=%s, rs_num=%d\n", + "anchor.path=%s rs_num=%d\n", anchor->path, rs_num)); goto error; /* XXX: rollback? */ } @@ -6239,6 +6241,24 @@ shutdown_pf(void) } } + /* Unlink rules of all user defined ether anchors */ + RB_FOREACH(eth_anchor, pf_keth_anchor_global, + &V_pf_keth_anchors) { + /* Wildcard based anchors may not have a respective + * explicit anchor rule or they may be left empty + * without rules. It leads to anchor.refcnt=0, and the + * rest of the logic does not expect it. */ + if (eth_anchor->refcnt == 0) + eth_anchor->refcnt = 1; + if ((error = pf_begin_eth(&t[0], eth_anchor->path)) + != 0) { + DPFPRINTF(PF_DEBUG_MISC, ("shutdown_pf: eth " + "anchor.path=%s\n", eth_anchor->path)); + goto error; + } + pf_commit_eth(t[0], eth_anchor->path); + } + if ((error = pf_begin_rules(&t[0], PF_RULESET_SCRUB, &nn)) != 0) { DPFPRINTF(PF_DEBUG_MISC, ("shutdown_pf: SCRUB\n")); From a3397008c439a04828ec4c9633ac95e491b845a7 Mon Sep 17 00:00:00 2001 From: Igor Ostapenko Date: Tue, 28 Nov 2023 15:31:11 +0200 Subject: [PATCH 5/5] Assert our expections of commit actions It resolves kp@ review comments. --- sys/netpfil/pf/pf_ioctl.c | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/sys/netpfil/pf/pf_ioctl.c b/sys/netpfil/pf/pf_ioctl.c index dc016e35b9e718..20046f4f4e2333 100644 --- a/sys/netpfil/pf/pf_ioctl.c +++ b/sys/netpfil/pf/pf_ioctl.c @@ -6235,9 +6235,9 @@ shutdown_pf(void) } } for (rs_num = 0; rs_num < PF_RULESET_MAX; ++rs_num) { - /* XXX: these should always succeed here */ - pf_commit_rules(t[rs_num], rs_num, + error = pf_commit_rules(t[rs_num], rs_num, anchor->path); + MPASS(error == 0); } } @@ -6256,7 +6256,8 @@ shutdown_pf(void) "anchor.path=%s\n", eth_anchor->path)); goto error; } - pf_commit_eth(t[0], eth_anchor->path); + error = pf_commit_eth(t[0], eth_anchor->path); + MPASS(error == 0); } if ((error = pf_begin_rules(&t[0], PF_RULESET_SCRUB, &nn)) @@ -6285,12 +6286,16 @@ shutdown_pf(void) break; /* XXX: rollback? */ } - /* XXX: these should always succeed here */ - pf_commit_rules(t[0], PF_RULESET_SCRUB, &nn); - pf_commit_rules(t[1], PF_RULESET_FILTER, &nn); - pf_commit_rules(t[2], PF_RULESET_NAT, &nn); - pf_commit_rules(t[3], PF_RULESET_BINAT, &nn); - pf_commit_rules(t[4], PF_RULESET_RDR, &nn); + error = pf_commit_rules(t[0], PF_RULESET_SCRUB, &nn); + MPASS(error == 0); + error = pf_commit_rules(t[1], PF_RULESET_FILTER, &nn); + MPASS(error == 0); + error = pf_commit_rules(t[2], PF_RULESET_NAT, &nn); + MPASS(error == 0); + error = pf_commit_rules(t[3], PF_RULESET_BINAT, &nn); + MPASS(error == 0); + error = pf_commit_rules(t[4], PF_RULESET_RDR, &nn); + MPASS(error == 0); if ((error = pf_clear_tables()) != 0) break; @@ -6299,7 +6304,8 @@ shutdown_pf(void) DPFPRINTF(PF_DEBUG_MISC, ("shutdown_pf: eth\n")); break; } - pf_commit_eth(t[0], &nn); + error = pf_commit_eth(t[0], &nn); + MPASS(error == 0); #ifdef ALTQ if ((error = pf_begin_altq(&t[0])) != 0) {