On Tue, Jul 19, 2022 at 10:58:08PM +0200, Alexander Bluhm wrote:
> On Tue, Jul 19, 2022 at 07:18:54PM +0200, Moritz Buhl wrote:
> > A solution would be to move the allocation of the pf_anchor struct
> > into a pool. One final question would be regarding the size of the
> > hiwat or hardlimit. Any suggestions?
>
> 10 seems way to low. We want to limit resource abuse. But regular
> users should never hit this. Especially existing configurations
> should still work after upgrade.
>
> Perhaps 512 anchors. Noone shoul ever need more than 512 anchors :-)
Sure.
> > Any other comments?
>
> Please do not use PR_WAITOK in pool_init() unless you know what you
> are doing. I think it should work, but who knows. The other pf
> pools have 0 flags, just use that.
Ok.
> Comment says rs_malloc is needed to compile pfctl. Have you tested
> that?
The new diff addresses this. As well as documentation.
> Replacing M_WAITOK|M_CANFAIL|M_ZERO with PR_NOWAIT|PR_ZERO looks
> wrong. PR_WAITOK|PR_LIMITFAIL|PR_ZERO should do something equivalent.
I don't know how I got this wrong.
> I am not sure if pf_find_anchor() should fail if the limit is hit.
> It uses only a temporary buffer. All calls to pf_find_ruleset()
> should be checked whether permanent failure, after the limit has
> been reached, is what we want. Or we keep the malloc there?
I guess I was overzealous replacing all mallocs for pf_anchor,
thanks for paying attention on the actual lifetime.
Is the following diff OK?
mbuhl
Index: sbin/pfctl/pfctl.c
===================================================================
RCS file: /mount/openbsd/cvs/src/sbin/pfctl/pfctl.c,v
retrieving revision 1.385
diff -u -p -r1.385 pfctl.c
--- sbin/pfctl/pfctl.c 11 Nov 2021 12:49:53 -0000 1.385
+++ sbin/pfctl/pfctl.c 20 Jul 2022 06:55:36 -0000
@@ -158,6 +158,7 @@ static const struct {
{ "tables", PF_LIMIT_TABLES },
{ "table-entries", PF_LIMIT_TABLE_ENTRIES },
{ "pktdelay-pkts", PF_LIMIT_PKTDELAY_PKTS },
+ { "anchors", PF_LIMIT_ANCHORS },
{ NULL, 0 }
};
Index: share/man/man4/pf.4
===================================================================
RCS file: /mount/openbsd/cvs/src/share/man/man4/pf.4,v
retrieving revision 1.92
diff -u -p -r1.92 pf.4
--- share/man/man4/pf.4 26 May 2019 02:06:55 -0000 1.92
+++ share/man/man4/pf.4 20 Jul 2022 07:40:38 -0000
@@ -416,7 +416,7 @@ struct pfioc_limit {
enum { PF_LIMIT_STATES, PF_LIMIT_SRC_NODES, PF_LIMIT_FRAGS,
PF_LIMIT_TABLES, PF_LIMIT_TABLE_ENTRIES, PF_LIMIT_PKTDELAY_PKTS,
- PF_LIMIT_MAX };
+ PF_LIMIT_ANCHORS, PF_LIMIT_MAX };
.Ed
.It Dv DIOCGETLIMIT Fa "struct pfioc_limit *pl"
Get the hard
@@ -1033,6 +1033,7 @@ static const struct {
{ "frags", PF_LIMIT_FRAGS },
{ "tables", PF_LIMIT_TABLES },
{ "table-entries", PF_LIMIT_TABLE_ENTRIES },
+ { "anchors", PF_LIMIT_ANCHORS },
{ NULL, 0 }
};
Index: sys/net/pf.c
===================================================================
RCS file: /mount/openbsd/cvs/src/sys/net/pf.c,v
retrieving revision 1.1135
diff -u -p -r1.1135 pf.c
--- sys/net/pf.c 28 Jun 2022 13:48:06 -0000 1.1135
+++ sys/net/pf.c 20 Jul 2022 06:59:37 -0000
@@ -276,7 +276,8 @@ struct pf_pool_limit pf_pool_limits[PF_L
{ &pf_frent_pl, PFFRAG_FRENT_HIWAT, PFFRAG_FRENT_HIWAT },
{ &pfr_ktable_pl, PFR_KTABLE_HIWAT, PFR_KTABLE_HIWAT },
{ &pfr_kentry_pl, PFR_KENTRY_HIWAT, PFR_KENTRY_HIWAT },
- { &pf_pktdelay_pl, PF_PKTDELAY_MAXPKTS, PF_PKTDELAY_MAXPKTS }
+ { &pf_pktdelay_pl, PF_PKTDELAY_MAXPKTS, PF_PKTDELAY_MAXPKTS },
+ { &pf_anchor_pl, PF_ANCHOR_HIWAT, PF_ANCHOR_HIWAT }
};
#define BOUND_IFACE(r, k) \
Index: sys/net/pf_ioctl.c
===================================================================
RCS file: /mount/openbsd/cvs/src/sys/net/pf_ioctl.c,v
retrieving revision 1.382
diff -u -p -r1.382 pf_ioctl.c
--- sys/net/pf_ioctl.c 26 Jun 2022 11:37:08 -0000 1.382
+++ sys/net/pf_ioctl.c 20 Jul 2022 06:22:15 -0000
@@ -191,6 +191,8 @@ pfattach(int num)
IPL_SOFTNET, 0, "pftag", NULL);
pool_init(&pf_pktdelay_pl, sizeof(struct pf_pktdelay), 0,
IPL_SOFTNET, 0, "pfpktdelay", NULL);
+ pool_init(&pf_anchor_pl, sizeof(struct pf_anchor), 0,
+ IPL_SOFTNET, 0, "pfanchor", NULL);
hfsc_initialize();
pfr_initialize();
@@ -200,6 +202,8 @@ pfattach(int num)
pool_sethardlimit(pf_pool_limits[PF_LIMIT_STATES].pp,
pf_pool_limits[PF_LIMIT_STATES].limit, NULL, 0);
+ pool_sethardlimit(pf_pool_limits[PF_LIMIT_ANCHORS].pp,
+ pf_pool_limits[PF_LIMIT_ANCHORS].limit, NULL, 0);
if (physmem <= atop(100*1024*1024))
pf_pool_limits[PF_LIMIT_TABLE_ENTRIES].limit =
Index: sys/net/pf_ruleset.c
===================================================================
RCS file: /mount/openbsd/cvs/src/sys/net/pf_ruleset.c,v
retrieving revision 1.18
diff -u -p -r1.18 pf_ruleset.c
--- sys/net/pf_ruleset.c 27 Dec 2018 16:54:01 -0000 1.18
+++ sys/net/pf_ruleset.c 20 Jul 2022 07:06:24 -0000
@@ -40,6 +40,7 @@
#ifdef _KERNEL
#include <sys/systm.h>
#include <sys/mbuf.h>
+#include <sys/pool.h>
#endif /* _KERNEL */
#include <sys/syslog.h>
@@ -58,6 +59,11 @@
#ifdef _KERNEL
#define rs_malloc(x) malloc(x, M_TEMP, M_WAITOK|M_CANFAIL|M_ZERO)
#define rs_free(x, siz) free(x, M_TEMP, siz)
+#define rs_pool_get_anchor() pool_get(&pf_anchor_pl, \
+ PR_WAITOK|PR_LIMITFAIL|PR_ZERO)
+#define rs_pool_put_anchor(x) pool_put(&pf_anchor_pl, x)
+
+struct pool pf_anchor_pl;
#else /* !_KERNEL */
/* Userland equivalents so we can lend code to pfctl et al. */
@@ -67,8 +73,10 @@
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
-#define rs_malloc(x) calloc(1, x)
-#define rs_free(x, siz) freezero(x, siz)
+#define rs_malloc(x) calloc(1, x)
+#define rs_free(x, siz) freezero(x, siz)
+#define rs_pool_get_anchor() calloc(1, sizeof(struct pf_anchor))
+#define rs_pool_put_anchor(x) freezero(x, sizeof(struct pf_anchor))
#ifdef PFDEBUG
#include <sys/stdarg.h> /* for DPFPRINTF() */
@@ -184,7 +192,7 @@ pf_create_anchor(struct pf_anchor *paren
((parent != NULL) && (strlen(parent->path) >= PF_ANCHOR_MAXPATH)))
return (NULL);
- anchor = rs_malloc(sizeof(*anchor));
+ anchor = rs_pool_get_anchor();
if (anchor == NULL)
return (NULL);
@@ -204,7 +212,7 @@ pf_create_anchor(struct pf_anchor *paren
DPFPRINTF(LOG_NOTICE,
"%s: RB_INSERT to global '%s' '%s' collides with '%s' '%s'",
__func__, anchor->path, anchor->name, dup->path, dup->name);
- rs_free(anchor, sizeof(*anchor));
+ rs_pool_put_anchor(anchor);
return (NULL);
}
@@ -218,7 +226,7 @@ pf_create_anchor(struct pf_anchor *paren
dup->path, dup->name);
RB_REMOVE(pf_anchor_global, &pf_anchors,
anchor);
- rs_free(anchor, sizeof(*anchor));
+ rs_pool_put_anchor(anchor);
return (NULL);
}
}
@@ -300,7 +308,7 @@ pf_remove_if_empty_ruleset(struct pf_rul
if ((parent = ruleset->anchor->parent) != NULL)
RB_REMOVE(pf_anchor_node, &parent->children,
ruleset->anchor);
- rs_free(ruleset->anchor, sizeof(*(ruleset->anchor)));
+ rs_pool_put_anchor(ruleset->anchor);
if (parent == NULL)
return;
ruleset = &parent->ruleset;
Index: sys/net/pfvar.h
===================================================================
RCS file: /mount/openbsd/cvs/src/sys/net/pfvar.h,v
retrieving revision 1.508
diff -u -p -r1.508 pfvar.h
--- sys/net/pfvar.h 26 Jun 2022 11:37:08 -0000 1.508
+++ sys/net/pfvar.h 20 Jul 2022 06:58:35 -0000
@@ -136,7 +136,7 @@ enum { PFTM_TCP_FIRST_PACKET, PFTM_TCP_O
enum { PF_NOPFROUTE, PF_ROUTETO, PF_DUPTO, PF_REPLYTO };
enum { PF_LIMIT_STATES, PF_LIMIT_SRC_NODES, PF_LIMIT_FRAGS,
PF_LIMIT_TABLES, PF_LIMIT_TABLE_ENTRIES, PF_LIMIT_PKTDELAY_PKTS,
- PF_LIMIT_MAX };
+ PF_LIMIT_ANCHORS, PF_LIMIT_MAX };
#define PF_POOL_IDMASK 0x0f
enum { PF_POOL_NONE, PF_POOL_BITMASK, PF_POOL_RANDOM,
PF_POOL_SRCHASH, PF_POOL_ROUNDROBIN, PF_POOL_LEASTSTATES };
@@ -476,6 +476,7 @@ union pf_rule_ptr {
#define PF_ANCHOR_NAME_SIZE 64
#define PF_ANCHOR_MAXPATH (PATH_MAX - PF_ANCHOR_NAME_SIZE - 1)
+#define PF_ANCHOR_HIWAT 512
#define PF_OPTIMIZER_TABLE_PFX "__automatic_"
struct pf_rule {
@@ -1703,7 +1704,7 @@ extern u_int32_t ticket_pabuf;
extern struct pool pf_src_tree_pl, pf_sn_item_pl, pf_rule_pl;
extern struct pool pf_state_pl, pf_state_key_pl, pf_state_item_pl,
pf_rule_item_pl, pf_queue_pl,
- pf_pktdelay_pl;
+ pf_pktdelay_pl, pf_anchor_pl;
extern struct pool pf_state_scrub_pl;
extern struct ifnet *sync_ifp;
extern struct pf_rule pf_default_rule;
