Date: Wed, 17 Sep 2025 14:15:39 GMT From: Kristof Provost <kp@FreeBSD.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Subject: git: 9dfc5e03da50 - main - pfctl: allow tables to be defined inside anchors Message-ID: <202509171415.58HEFdN0010140@gitrepo.freebsd.org>
next in thread | raw e-mail | index | archive | help
The branch main has been updated by kp: URL: https://cgit.FreeBSD.org/src/commit/?id=9dfc5e03da50d12f02c2b481139acf9f089d504f commit 9dfc5e03da50d12f02c2b481139acf9f089d504f Author: Kristof Provost <kp@FreeBSD.org> AuthorDate: 2025-08-22 11:34:39 +0000 Commit: Kristof Provost <kp@FreeBSD.org> CommitDate: 2025-09-17 14:15:13 +0000 pfctl: allow tables to be defined inside anchors This change allows user to define table inside the anchor like that: anchor foo { table <bar> { 192.168.1.1 } pass in from <bar> to <self> } Without this diff one must either create table <bar> in main ruleset (root) or use 'pfctl -a foo -t bar -T add 192.168.1.1' This glitch is hard to notice. Not many human admins try to attach tables to non-global anchors. Deamons which configure pf(4) automatically at run time such as relayd(8) and spamd(8) create tables attached to thair anchors (for example 'relayd/*') but the deamons use way similar to pfctl(8) to add and manage those tables. The reason why I'd like to seal this gap is that my long term goal is to turn global `pfr_ktable` in pf(4) into member of pf_anchor. So each ruleset will get its own tree of tables. feedback and OK bluhm@ Obtained from: OpenBSD, sashan <sashan@openbsd.org>, 30269bc362 Sponsored by: Rubicon Communications, LLC ("Netgate") --- sbin/pfctl/parse.y | 104 ++++++++++++++++++++++++++++++++++++++++++-- sbin/pfctl/pfctl.c | 52 +++++++++++++++++++--- sbin/pfctl/pfctl.h | 41 +++++++++++++++++ sbin/pfctl/pfctl_optimize.c | 3 +- sbin/pfctl/pfctl_parser.h | 12 ++++- sbin/pfctl/pfctl_radix.c | 15 +++++++ sbin/pfctl/pfctl_table.c | 51 +++++++++++++++++----- 7 files changed, 256 insertions(+), 22 deletions(-) diff --git a/sbin/pfctl/parse.y b/sbin/pfctl/parse.y index 46e9f60fe48e..0628b387b4b3 100644 --- a/sbin/pfctl/parse.y +++ b/sbin/pfctl/parse.y @@ -420,6 +420,8 @@ int rt_tableid_max(void); void mv_rules(struct pfctl_ruleset *, struct pfctl_ruleset *); void mv_eth_rules(struct pfctl_eth_ruleset *, struct pfctl_eth_ruleset *); +void mv_tables(struct pfctl *, struct pfr_ktablehead *, + struct pfctl_anchor *, struct pfctl_anchor *); void decide_address_family(struct node_host *, sa_family_t *); void remove_invalid_hosts(struct node_host **, sa_family_t *); int invalid_redirect(struct node_host *, sa_family_t); @@ -949,6 +951,7 @@ anchorname : STRING { pfa_anchorlist : /* empty */ | pfa_anchorlist '\n' + | pfa_anchorlist tabledef '\n' | pfa_anchorlist pfrule '\n' | pfa_anchorlist anchorrule '\n' | pfa_anchorlist include '\n' @@ -974,7 +977,7 @@ pfa_anchor : '{' snprintf(ta, PF_ANCHOR_NAME_SIZE, "_%d", pf->bn); rs = pf_find_or_create_ruleset(ta); if (rs == NULL) - err(1, "pfa_anchor: pf_find_or_create_ruleset"); + err(1, "pfa_anchor: pf_find_or_create_ruleset (%s)", ta); pf->astack[pf->asd] = rs->anchor; pf->anchor = rs->anchor; } '\n' pfa_anchorlist '}' @@ -1026,6 +1029,7 @@ anchorrule : ANCHOR anchorname dir quick interface af proto fromto } mv_rules(&pf->alast->ruleset, &r.anchor->ruleset); + mv_tables(pf, &pfr_ktables, r.anchor, pf->alast); } pf_remove_if_empty_ruleset(&pf->alast->ruleset); pf->alast = r.anchor; @@ -5456,6 +5460,7 @@ process_tabledef(char *name, struct table_opts *opts, int popts) { struct pfr_buffer ab; struct node_tinit *ti; + struct pfr_uktable *ukt; unsigned long maxcount; size_t s = sizeof(maxcount); @@ -5488,9 +5493,23 @@ process_tabledef(char *name, struct table_opts *opts, int popts) else if (pf->opts & PF_OPT_VERBOSE) fprintf(stderr, "%s:%d: skipping duplicate table checks" " for <%s>\n", file->name, yylval.lineno, name); + /* + * postpone definition of non-root tables to moment + * when path is fully resolved. + */ + if (pf->asd > 0) { + ukt = calloc(1, sizeof(struct pfr_uktable)); + if (ukt == NULL) { + DBGPRINT( + "%s:%d: not enough memory for <%s>\n", file->name, + yylval.lineno, name); + goto _error; + } + } else + ukt = NULL; if (!(pf->opts & PF_OPT_NOACTION) && pfctl_define_table(name, opts->flags, opts->init_addr, - pf->anchor->path, &ab, pf->anchor->ruleset.tticket)) { + pf->anchor->path, &ab, pf->anchor->ruleset.tticket, ukt)) { if (sysctlbyname("net.pf.request_maxcount", &maxcount, &s, NULL, 0) == -1) @@ -5506,6 +5525,28 @@ process_tabledef(char *name, struct table_opts *opts, int popts) goto _error; } + + if (ukt != NULL) { + ukt->pfrukt_init_addr = opts->init_addr; + if (RB_INSERT(pfr_ktablehead, &pfr_ktables, + &ukt->pfrukt_kt) != NULL) { + /* + * I think this should not happen, because + * pfctl_define_table() above does the same check + * effectively. + */ + DBGPRINT( + "%s:%d table %s already exists in %s\n", + file->name, yylval.lineno, + ukt->pfrukt_name, pf->anchor->path); + free(ukt); + goto _error; + } + DBGPRINT("%s %s@%s inserted to tree\n", + __func__, ukt->pfrukt_name, pf->anchor->path); + } else + DBGPRINT("%s ukt is null\n", __func__); + pf->tdirty = 1; pfr_buf_clear(&ab); return (0); @@ -7276,6 +7317,61 @@ mv_eth_rules(struct pfctl_eth_ruleset *src, struct pfctl_eth_ruleset *dst) src->anchor->match = 0; } +void +mv_tables(struct pfctl *pf, struct pfr_ktablehead *ktables, + struct pfctl_anchor *a, struct pfctl_anchor *alast) +{ + struct pfr_ktable *kt, *kt_safe; + char new_path[PF_ANCHOR_MAXPATH]; + char *path_cut; + int sz; + struct pfr_uktable *ukt; + SLIST_HEAD(, pfr_uktable) ukt_list; + + /* + * Here we need to rename anchor path from temporal names such as + * _1/_2/foo to _1/bar/foo etc. + * + * This also means we need to remove and insert table to ktables + * tree as anchor path is being updated. + */ + SLIST_INIT(&ukt_list); + DBGPRINT("%s [ %s ] (%s)\n", __func__, a->path, alast->path); + RB_FOREACH_SAFE(kt, pfr_ktablehead, ktables, kt_safe) { + path_cut = strstr(kt->pfrkt_anchor, alast->path); + if (path_cut != NULL) { + path_cut += strlen(alast->path); + if (*path_cut) + sz = snprintf(new_path, sizeof (new_path), + "%s%s", a->path, path_cut); + else + sz = snprintf(new_path, sizeof (new_path), + "%s", a->path); + if (sz >= sizeof (new_path)) + errx(1, "new path is too long for %s@%s\n", + kt->pfrkt_name, kt->pfrkt_anchor); + + DBGPRINT("%s %s@%s -> %s@%s\n", __func__, + kt->pfrkt_name, kt->pfrkt_anchor, + kt->pfrkt_name, new_path); + RB_REMOVE(pfr_ktablehead, ktables, kt); + strlcpy(kt->pfrkt_anchor, new_path, + sizeof(kt->pfrkt_anchor)); + SLIST_INSERT_HEAD(&ukt_list, (struct pfr_uktable *)kt, + pfrukt_entry); + } + } + + while ((ukt = SLIST_FIRST(&ukt_list)) != NULL) { + SLIST_REMOVE_HEAD(&ukt_list, pfrukt_entry); + if (RB_INSERT(pfr_ktablehead, ktables, + (struct pfr_ktable *)ukt) != NULL) + errx(1, "%s@%s exists already\n", + ukt->pfrukt_name, + ukt->pfrukt_anchor); + } +} + void decide_address_family(struct node_host *n, sa_family_t *af) { @@ -7485,7 +7581,7 @@ parseport(char *port, struct range *r, int extensions) } int -pfctl_load_anchors(int dev, struct pfctl *pf, struct pfr_buffer *trans) +pfctl_load_anchors(int dev, struct pfctl *pf) { struct loadanchors *la; @@ -7494,7 +7590,7 @@ pfctl_load_anchors(int dev, struct pfctl *pf, struct pfr_buffer *trans) fprintf(stderr, "\nLoading anchor %s from %s\n", la->anchorname, la->filename); if (pfctl_rules(dev, la->filename, pf->opts, pf->optimize, - la->anchorname, trans) == -1) + la->anchorname, pf->trans) == -1) return (-1); } diff --git a/sbin/pfctl/pfctl.c b/sbin/pfctl/pfctl.c index 2c12387ee388..601b7651e40b 100644 --- a/sbin/pfctl/pfctl.c +++ b/sbin/pfctl/pfctl.c @@ -2065,6 +2065,41 @@ pfctl_load_eth_rule(struct pfctl *pf, char *path, struct pfctl_eth_rule *r, return (0); } +static int +pfctl_load_tables(struct pfctl *pf, char *path, struct pfctl_anchor *a, + int rs_num) +{ + struct pfr_ktable *kt, *ktw; + struct pfr_uktable *ukt; + char anchor_path[PF_ANCHOR_MAXPATH]; + int e; + + RB_FOREACH_SAFE(kt, pfr_ktablehead, &pfr_ktables, ktw) { + if (strcmp(kt->pfrkt_anchor, a->path) != 0) + continue; + + if (path != NULL && *path) { + strlcpy(anchor_path, kt->pfrkt_anchor, + sizeof(anchor_path)); + snprintf(kt->pfrkt_anchor, PF_ANCHOR_MAXPATH, "%s/%s", + path, anchor_path); + } + ukt = (struct pfr_uktable *)kt; + e = pfr_ina_define(&ukt->pfrukt_t, ukt->pfrukt_addrs.pfrb_caddr, + ukt->pfrukt_addrs.pfrb_size, NULL, NULL, + pf->anchor->ruleset.tticket, + ukt->pfrukt_init_addr ? PFR_FLAG_ADDRSTOO : 0); + if (e != 0) + err(1, "%s pfr_ina_define() %s@%s", __func__, + kt->pfrkt_name, kt->pfrkt_anchor); + RB_REMOVE(pfr_ktablehead, &pfr_ktables, kt); + pfr_buf_clear(&ukt->pfrukt_addrs); + free(ukt); + } + + return (0); +} + int pfctl_load_ruleset(struct pfctl *pf, char *path, struct pfctl_ruleset *rs, int rs_num, int depth) @@ -2113,6 +2148,8 @@ pfctl_load_ruleset(struct pfctl *pf, char *path, struct pfctl_ruleset *rs, if ((error = pfctl_load_ruleset(pf, path, &r->anchor->ruleset, rs_num, depth + 1))) goto error; + if ((error = pfctl_load_tables(pf, path, r->anchor, rs_num))) + goto error; } else if (pf->opts & PF_OPT_VERBOSE) printf("\n"); free(r); @@ -2135,15 +2172,17 @@ pfctl_load_rule(struct pfctl *pf, char *path, struct pfctl_rule *r, int depth) { u_int8_t rs_num = pf_get_ruleset_number(r->action); char *name; - u_int32_t ticket; char anchor[PF_ANCHOR_NAME_SIZE]; int len = strlen(path); int error; bool was_present; /* set up anchor before adding to path for anchor_call */ - if ((pf->opts & PF_OPT_NOACTION) == 0) - ticket = pfctl_get_ticket(pf->trans, rs_num, path); + if ((pf->opts & PF_OPT_NOACTION) == 0) { + if (pf->trans == NULL) + errx(1, "pfctl_load_rule: no transaction"); + pf->anchor->ruleset.tticket = pfctl_get_ticket(pf->trans, rs_num, path); + } if (strlcpy(anchor, path, sizeof(anchor)) >= sizeof(anchor)) errx(1, "pfctl_load_rule: strlcpy"); @@ -2175,7 +2214,7 @@ pfctl_load_rule(struct pfctl *pf, char *path, struct pfctl_rule *r, int depth) return (1); if (pfctl_add_pool(pf, &r->route, PF_RT)) return (1); - error = pfctl_add_rule_h(pf->h, r, anchor, name, ticket, + error = pfctl_add_rule_h(pf->h, r, anchor, name, pf->anchor->ruleset.tticket, pf->paddr.ticket); switch (error) { case 0: @@ -2245,6 +2284,8 @@ pfctl_rules(int dev, char *filename, int opts, int optimize, RB_INIT(&pf_anchors); memset(&pf_main_anchor, 0, sizeof(pf_main_anchor)); pf_init_ruleset(&pf_main_anchor.ruleset); + memset(&pf, 0, sizeof(pf)); + memset(&trs, 0, sizeof(trs)); pf_main_anchor.ruleset.anchor = &pf_main_anchor; memset(&pf_eth_main_anchor, 0, sizeof(pf_eth_main_anchor)); @@ -2254,6 +2295,7 @@ pfctl_rules(int dev, char *filename, int opts, int optimize, if (trans == NULL) { bzero(&buf, sizeof(buf)); buf.pfrb_type = PFRB_TRANS; + pf.trans = &buf; t = &buf; osize = 0; } else { @@ -2364,7 +2406,7 @@ pfctl_rules(int dev, char *filename, int opts, int optimize, if (trans == NULL) { /* process "load anchor" directives */ - if (pfctl_load_anchors(dev, &pf, t) == -1) + if (pfctl_load_anchors(dev, &pf) == -1) ERRX("load anchors"); if ((opts & PF_OPT_NOACTION) == 0) { diff --git a/sbin/pfctl/pfctl.h b/sbin/pfctl/pfctl.h index 37e06e93dde6..136f51ea08f9 100644 --- a/sbin/pfctl/pfctl.h +++ b/sbin/pfctl/pfctl.h @@ -36,6 +36,12 @@ #include <libpfctl.h> +#ifdef PFCTL_DEBUG +#define DBGPRINT(...) fprintf(stderr, __VA_ARGS__) +#else +#define DBGPRINT(...) (void)(0) +#endif + extern struct pfctl_handle *pfh; struct pfctl; @@ -55,6 +61,41 @@ struct pfr_buffer { (var) != NULL; \ (var) = pfr_buf_next((buf), (var))) +RB_HEAD(pfr_ktablehead, pfr_ktable); +struct pfr_ktable { + struct pfr_tstats pfrkt_ts; + RB_ENTRY(pfr_ktable) pfrkt_tree; + SLIST_ENTRY(pfr_ktable) pfrkt_workq; + struct radix_node_head *pfrkt_ip4; + struct radix_node_head *pfrkt_ip6; + struct pfr_ktable *pfrkt_shadow; + struct pfr_ktable *pfrkt_root; + struct pf_kruleset *pfrkt_rs; + long pfrkt_larg; + int pfrkt_nflags; +}; +#define pfrkt_t pfrkt_ts.pfrts_t +#define pfrkt_name pfrkt_t.pfrt_name +#define pfrkt_anchor pfrkt_t.pfrt_anchor +#define pfrkt_ruleset pfrkt_t.pfrt_ruleset +#define pfrkt_flags pfrkt_t.pfrt_flags +#define pfrkt_cnt pfrkt_kts.pfrkts_cnt +#define pfrkt_refcnt pfrkt_kts.pfrkts_refcnt +#define pfrkt_tzero pfrkt_kts.pfrkts_tzero + +struct pfr_uktable { + struct pfr_ktable pfrukt_kt; + struct pfr_buffer pfrukt_addrs; + int pfrukt_init_addr; + SLIST_ENTRY(pfr_uktable) pfrukt_entry; +}; + +#define pfrukt_t pfrukt_kt.pfrkt_ts.pfrts_t +#define pfrukt_name pfrukt_kt.pfrkt_t.pfrt_name +#define pfrukt_anchor pfrukt_kt.pfrkt_t.pfrt_anchor + +extern struct pfr_ktablehead pfr_ktables; + struct pfr_anchoritem { SLIST_ENTRY(pfr_anchoritem) pfra_sle; char *pfra_anchorname; diff --git a/sbin/pfctl/pfctl_optimize.c b/sbin/pfctl/pfctl_optimize.c index 1d2a60555f19..2d16bbd22b39 100644 --- a/sbin/pfctl/pfctl_optimize.c +++ b/sbin/pfctl/pfctl_optimize.c @@ -1331,7 +1331,8 @@ again: if (pfctl_define_table(tbl->pt_name, PFR_TFLAG_CONST, 1, - pf->astack[0]->path, tbl->pt_buf, pf->astack[0]->ruleset.tticket)) { + pf->astack[0]->path, tbl->pt_buf, pf->astack[0]->ruleset.tticket, + NULL)) { warn("failed to create table %s in %s", tbl->pt_name, pf->astack[0]->name); return (1); diff --git a/sbin/pfctl/pfctl_parser.h b/sbin/pfctl/pfctl_parser.h index 58d3abc36691..e96ff0195e03 100644 --- a/sbin/pfctl/pfctl_parser.h +++ b/sbin/pfctl/pfctl_parser.h @@ -36,6 +36,8 @@ #include <libpfctl.h> +#include <pfctl.h> + #define PF_OSFP_FILE "/etc/pf.os" #define PF_OPT_DISABLE 0x00001 @@ -90,6 +92,7 @@ struct pfctl { struct pfioc_queue *pqueue; struct pfr_buffer *trans; struct pfctl_anchor *anchor, *alast; + struct pfr_ktablehead pfr_ktlast; int eth_nr; struct pfctl_eth_anchor *eanchor, *ealast; struct pfctl_eth_anchor *eastack[PFCTL_ANCHOR_STACK_DEPTH]; @@ -277,6 +280,8 @@ struct pf_opt_rule { TAILQ_HEAD(pf_opt_queue, pf_opt_rule); +struct pfr_uktable; + void copy_satopfaddr(struct pf_addr *, struct sockaddr *); int pfctl_rules(int, char *, int, int, char *, struct pfr_buffer *); @@ -303,7 +308,7 @@ int pfctl_cfg_syncookies(struct pfctl *, uint8_t, struct pfctl_watermarks *); int parse_config(char *, struct pfctl *); int parse_flags(char *); -int pfctl_load_anchors(int, struct pfctl *, struct pfr_buffer *); +int pfctl_load_anchors(int, struct pfctl *); void print_pool(struct pfctl_pool *, u_int16_t, u_int16_t, int); void print_src_node(struct pfctl_src_node *, int); @@ -324,7 +329,7 @@ void print_queue(const struct pf_altq *, unsigned, struct node_queue_bw *, int, struct node_queue_opt *); int pfctl_define_table(char *, int, int, const char *, struct pfr_buffer *, - u_int32_t); + u_int32_t, struct pfr_uktable *); void pfctl_clear_fingerprints(int, int); int pfctl_file_fingerprints(int, int, const char *); @@ -380,5 +385,8 @@ struct node_host *host(const char *, int); int append_addr(struct pfr_buffer *, char *, int, int); int append_addr_host(struct pfr_buffer *, struct node_host *, int, int); +int pfr_ktable_compare(struct pfr_ktable *, + struct pfr_ktable *); +RB_PROTOTYPE(pfr_ktablehead, pfr_ktable, pfrkt_tree, pfr_ktable_compare); #endif /* _PFCTL_PARSER_H_ */ diff --git a/sbin/pfctl/pfctl_radix.c b/sbin/pfctl/pfctl_radix.c index 398c5e998330..98f907738d95 100644 --- a/sbin/pfctl/pfctl_radix.c +++ b/sbin/pfctl/pfctl_radix.c @@ -48,6 +48,7 @@ #include <err.h> #include "pfctl.h" +#include "pfctl_parser.h" #define BUF_SIZE 256 @@ -55,6 +56,19 @@ extern int dev; static int pfr_next_token(char buf[BUF_SIZE], FILE *); +struct pfr_ktablehead pfr_ktables = { 0 }; +RB_GENERATE(pfr_ktablehead, pfr_ktable, pfrkt_tree, pfr_ktable_compare); + +int +pfr_ktable_compare(struct pfr_ktable *p, struct pfr_ktable *q) +{ + int d; + + if ((d = strncmp(p->pfrkt_name, q->pfrkt_name, PF_TABLE_NAME_SIZE))) + return (d); + return (strcmp(p->pfrkt_anchor, q->pfrkt_anchor)); +} + static void pfr_report_error(struct pfr_table *tbl, struct pfioc_table *io, const char *err) @@ -256,6 +270,7 @@ pfr_ina_define(struct pfr_table *tbl, struct pfr_addr *addr, int size, struct pfioc_table io; if (tbl == NULL || size < 0 || (size && addr == NULL)) { + DBGPRINT("%s %p %d %p\n", __func__, tbl, size, addr); errno = EINVAL; return (-1); } diff --git a/sbin/pfctl/pfctl_table.c b/sbin/pfctl/pfctl_table.c index 745ab84b3159..4955e1791fd7 100644 --- a/sbin/pfctl/pfctl_table.c +++ b/sbin/pfctl/pfctl_table.c @@ -570,19 +570,50 @@ print_astats(struct pfr_astats *as, int dns) int pfctl_define_table(char *name, int flags, int addrs, const char *anchor, - struct pfr_buffer *ab, u_int32_t ticket) + struct pfr_buffer *ab, u_int32_t ticket, struct pfr_uktable *ukt) { - struct pfr_table tbl; + struct pfr_table tbl_buf; + struct pfr_table *tbl; + + if (ukt == NULL) { + bzero(&tbl_buf, sizeof(tbl_buf)); + tbl = &tbl_buf; + } else { + if (ab->pfrb_size != 0) { + /* + * copy IP addresses which come with table from + * temporal buffer to buffer attached to table. + */ + ukt->pfrukt_addrs = *ab; + ab->pfrb_size = 0; + ab->pfrb_msize = 0; + ab->pfrb_caddr = NULL; + } else + memset(&ukt->pfrukt_addrs, 0, + sizeof(struct pfr_buffer)); + + tbl = &ukt->pfrukt_t; + } - bzero(&tbl, sizeof(tbl)); - if (strlcpy(tbl.pfrt_name, name, sizeof(tbl.pfrt_name)) >= - sizeof(tbl.pfrt_name) || strlcpy(tbl.pfrt_anchor, anchor, - sizeof(tbl.pfrt_anchor)) >= sizeof(tbl.pfrt_anchor)) - errx(1, "pfctl_define_table: strlcpy"); - tbl.pfrt_flags = flags; + if (strlcpy(tbl->pfrt_name, name, sizeof(tbl->pfrt_name)) >= + sizeof(tbl->pfrt_name) || + strlcpy(tbl->pfrt_anchor, anchor, sizeof(tbl->pfrt_anchor)) >= + sizeof(tbl->pfrt_anchor)) + errx(1, "%s: strlcpy", __func__); + tbl->pfrt_flags = flags; + DBGPRINT("%s %s@%s [%x]\n", __func__, tbl->pfrt_name, tbl->pfrt_anchor, + tbl->pfrt_flags); + + /* + * non-root anchors processed by parse.y are loaded to kernel later. + * Here we load tables, which are either created for root anchor + * or by 'pfctl -t ... -T ...' command. + */ + if (ukt != NULL) + return (0); - return pfr_ina_define(&tbl, ab->pfrb_caddr, ab->pfrb_size, NULL, - NULL, ticket, addrs ? PFR_FLAG_ADDRSTOO : 0); + return (pfr_ina_define(tbl, ab->pfrb_caddr, ab->pfrb_size, NULL, NULL, + ticket, addrs ? PFR_FLAG_ADDRSTOO : 0)); } void
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202509171415.58HEFdN0010140>