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 :-)
> 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.
Comment says rs_malloc is needed to compile pfctl. Have you tested
that?
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 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?
bluhm
> 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 19 Jul 2022 11:34:23 -0000
> @@ -269,6 +269,7 @@ void pf_log_matches(struct pf_pdesc
> *
>
> extern struct pool pfr_ktable_pl;
> extern struct pool pfr_kentry_pl;
> +extern struct pool pf_anchor_pl;
>
> struct pf_pool_limit pf_pool_limits[PF_LIMIT_MAX] = {
> { &pf_state_pl, PFSTATE_HIWAT, PFSTATE_HIWAT },
> @@ -276,7 +277,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 19 Jul 2022 16:56:23 -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, PR_WAITOK, "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 19 Jul 2022 12:56:35 -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>
>
> @@ -55,6 +56,7 @@
> #endif /* INET6 */
>
>
> +struct pool pf_anchor_pl;
> #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)
> @@ -107,12 +109,12 @@ pf_find_anchor(const char *path)
> {
> struct pf_anchor *key, *found;
>
> - key = rs_malloc(sizeof(*key));
> + key = pool_get(&pf_anchor_pl, PR_NOWAIT | PR_ZERO);
> if (key == NULL)
> return (NULL);
> strlcpy(key->path, path, sizeof(key->path));
> found = RB_FIND(pf_anchor_global, &pf_anchors, key);
> - rs_free(key, sizeof(*key));
> + pool_put(&pf_anchor_pl, key);
> return (found);
> }
>
> @@ -184,7 +186,7 @@ pf_create_anchor(struct pf_anchor *paren
> ((parent != NULL) && (strlen(parent->path) >= PF_ANCHOR_MAXPATH)))
> return (NULL);
>
> - anchor = rs_malloc(sizeof(*anchor));
> + anchor = pool_get(&pf_anchor_pl, PR_NOWAIT | PR_ZERO);
> if (anchor == NULL)
> return (NULL);
>
> @@ -204,7 +206,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));
> + pool_put(&pf_anchor_pl, anchor);
> return (NULL);
> }
>
> @@ -218,7 +220,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));
> + pool_put(&pf_anchor_pl, anchor);
> return (NULL);
> }
> }
> @@ -300,7 +302,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)));
> + pool_put(&pf_anchor_pl, 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 19 Jul 2022 16:34:54 -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 10
> #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;
