Download raw body.
let's make pf(4) anchors and tables better friends
Hello,
updated diff below. Addresses pfctl regression test failure
(spotted by bluhm@). The pf41.ok test running my earlier
changed tripped with assertion 'pfctl_get_ticket: assertion failed'.
the reason was code tried to load tables to pf(4) although
there were no tables to load.
Also bluhm@ pointed off-list there is a glitch in mv_tables() function
which updates table keys (pfrkt_name, pfrkt_anchor). Updating
keys requires as to remove table from tree, update key and insert
it again. Trying to do RB_REMOVE() followed by RB_INSERT() inside
RB_FOREACH_SAFE() does not feel like correct operation. RB_REMOVE()
is fine, but RB_INSERT() is questionable. The fix is to use
RB_REMOVE(), update key and do SLIST_INSERT(). Once RB_FOREACH_SAFE()
is done we walk the list and insert tables to table with updated
keys.
thanks and
regards
sashan
--------8<---------------8<---------------8<------------------8<--------
diff --git a/sbin/pfctl/parse.y b/sbin/pfctl/parse.y
index ee5c00f3b8b..e2bda79ebf7 100644
--- a/sbin/pfctl/parse.y
+++ b/sbin/pfctl/parse.y
@@ -379,6 +379,8 @@ int getservice(char *);
int rule_label(struct pf_rule *, char *);
void mv_rules(struct pf_ruleset *, struct pf_ruleset *);
+void mv_tables(struct pfctl *, struct pfr_ktablehead *,
+ struct pf_anchor *, struct pf_anchor *);
void decide_address_family(struct node_host *, sa_family_t *);
int invalid_redirect(struct node_host *, sa_family_t);
u_int16_t parseicmpspec(char *, sa_family_t);
@@ -827,6 +829,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'
@@ -853,7 +856,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 '}'
@@ -899,6 +902,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;
@@ -3976,6 +3980,7 @@ process_tabledef(char *name, struct table_opts *opts, int popts)
{
struct pfr_buffer ab;
struct node_tinit *ti;
+ struct pfr_uktable *ukt;
bzero(&ab, sizeof(ab));
ab.pfrb_type = PFRB_ADDRS;
@@ -4006,12 +4011,51 @@ 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);
- if (!(pf->opts & PF_OPT_NOACTION) &&
- pfctl_define_table(name, opts->flags, opts->init_addr,
- pf->anchor->path, &ab, pf->anchor->ruleset.tticket)) {
- yyerror("cannot define table %s: %s", name,
- pf_strerror(errno));
- goto _error;
+
+ if (!(pf->opts & PF_OPT_NOACTION)) {
+ /*
+ * 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 (pfctl_define_table(name, opts->flags, opts->init_addr,
+ pf->anchor->path, &ab, pf->anchor->ruleset.tticket, ukt)) {
+ yyerror("cannot define table %s: %s", name,
+ pf_strerror(errno));
+ 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);
@@ -5555,6 +5599,62 @@ mv_rules(struct pf_ruleset *src, struct pf_ruleset *dst)
TAILQ_CONCAT(dst->rules.inactive.ptr, src->rules.inactive.ptr, entries);
}
+void
+mv_tables(struct pfctl *pf, struct pfr_ktablehead *ktables,
+ struct pf_anchor *a, struct pf_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)
{
@@ -5711,7 +5811,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;
@@ -5720,7 +5820,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 27cc175c871..f2970c1603d 100644
--- a/sbin/pfctl/pfctl.c
+++ b/sbin/pfctl/pfctl.c
@@ -1424,6 +1424,41 @@ pfctl_check_qassignments(struct pf_ruleset *rs)
return (errs);
}
+static int
+pfctl_load_tables(struct pfctl *pf, char *path, struct pf_anchor *a)
+{
+ struct pfr_ktable *kt, *ktw;
+ struct pfr_uktable *ukt;
+ uint32_t ticket;
+ 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;
+ ticket = pfctl_get_ticket(pf->trans, PF_TRANS_TABLE, path);
+ e = pfr_ina_define(&ukt->pfrukt_t, ukt->pfrukt_addrs.pfrb_caddr,
+ ukt->pfrukt_addrs.pfrb_size, NULL, NULL, ticket,
+ 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 pf_ruleset *rs,
int depth)
@@ -1469,6 +1504,8 @@ pfctl_load_ruleset(struct pfctl *pf, char *path, struct pf_ruleset *rs,
if ((error = pfctl_load_ruleset(pf, path,
&r->anchor->ruleset, depth + 1)))
goto error;
+ if ((error = pfctl_load_tables(pf, path, r->anchor)))
+ goto error;
} else if (pf->opts & PF_OPT_VERBOSE)
printf("\n");
free(r);
@@ -1495,8 +1532,11 @@ pfctl_load_rule(struct pfctl *pf, char *path, struct pf_rule *r, int depth)
bzero(&pr, sizeof(pr));
/* set up anchor before adding to path for anchor_call */
- if ((pf->opts & PF_OPT_NOACTION) == 0)
+ if ((pf->opts & PF_OPT_NOACTION) == 0) {
+ if (pf->trans == NULL)
+ errx(1, "pfctl_load_rule: no transaction");
pr.ticket = pfctl_get_ticket(pf->trans, PF_TRANS_RULESET, path);
+ }
if (strlcpy(pr.anchor, path, sizeof(pr.anchor)) >= sizeof(pr.anchor))
errx(1, "pfctl_load_rule: strlcpy");
@@ -1535,8 +1575,8 @@ int
pfctl_rules(int dev, char *filename, int opts, int optimize,
char *anchorname, struct pfr_buffer *trans)
{
-#define ERR(x) do { warn(x); goto _error; } while(0)
-#define ERRX(x) do { warnx(x); goto _error; } while(0)
+#define ERR(...) do { warn(__VA_ARGS__); goto _error; } while(0)
+#define ERRX(...) do { warnx(__VA_ARGS__); goto _error; } while(0)
struct pfr_buffer *t, buf;
struct pfctl pf;
@@ -1549,9 +1589,13 @@ 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));
+
if (trans == NULL) {
bzero(&buf, sizeof(buf));
buf.pfrb_type = PFRB_TRANS;
+ pf.trans = &buf;
t = &buf;
osize = 0;
} else {
@@ -1559,20 +1603,18 @@ pfctl_rules(int dev, char *filename, int opts, int optimize,
osize = t->pfrb_size;
}
- memset(&pf, 0, sizeof(pf));
- memset(&trs, 0, sizeof(trs));
if ((path = calloc(1, PATH_MAX)) == NULL)
- ERRX("pfctl_rules: calloc");
+ ERR("%s: calloc", __func__);
if (strlcpy(trs.pfrt_anchor, anchorname,
sizeof(trs.pfrt_anchor)) >= sizeof(trs.pfrt_anchor))
- ERRX("pfctl_rules: strlcpy");
+ ERRX("%s: strlcpy", __func__);
pf.dev = dev;
pf.opts = opts;
pf.optimize = optimize;
/* non-brace anchor, create without resolving the path */
if ((pf.anchor = calloc(1, sizeof(*pf.anchor))) == NULL)
- ERRX("pfctl_rules: calloc");
+ ERR("%s: calloc", __func__);
rs = &pf.anchor->ruleset;
pf_init_ruleset(rs);
rs->anchor = pf.anchor;
@@ -1637,7 +1679,7 @@ pfctl_rules(int dev, char *filename, int opts, int optimize,
/*
* process "load anchor" directives that might have used queues
*/
- if (pfctl_load_anchors(dev, &pf, t) == -1)
+ if (pfctl_load_anchors(dev, &pf) == -1)
ERRX("load anchors");
pfctl_clear_queues(&qspecs);
pfctl_clear_queues(&rootqs);
diff --git a/sbin/pfctl/pfctl.h b/sbin/pfctl/pfctl.h
index 253b8242d45..3a4a44df58b 100644
--- a/sbin/pfctl/pfctl.h
+++ b/sbin/pfctl/pfctl.h
@@ -33,6 +33,12 @@
#ifndef _PFCTL_H_
#define _PFCTL_H_
+#ifdef PFCTL_DEBUG
+#define DBGPRINT(...) fprintf(stderr, __VA_ARGS__)
+#else
+#define DBGPRINT(...) (void)(0)
+#endif
+
enum pfctl_show { PFCTL_SHOW_RULES, PFCTL_SHOW_LABELS, PFCTL_SHOW_NOTHING };
enum { PFRB_TABLES = 1, PFRB_TSTATS, PFRB_ADDRS, PFRB_ASTATS,
@@ -54,6 +60,20 @@ struct pfr_anchoritem {
char *pfra_anchorname;
};
+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;
+
SLIST_HEAD(pfr_anchors, pfr_anchoritem);
int pfr_clr_tables(struct pfr_table *, int *, int);
diff --git a/sbin/pfctl/pfctl_optimize.c b/sbin/pfctl/pfctl_optimize.c
index dc93b882bef..c1bd6f6f6cc 100644
--- a/sbin/pfctl/pfctl_optimize.c
+++ b/sbin/pfctl/pfctl_optimize.c
@@ -1288,7 +1288,8 @@ again:
tablenum++;
if (pfctl_define_table(tbl->pt_name, PFR_TFLAG_CONST | tbl->pt_flags, 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 146580db2b8..bff3f640421 100644
--- a/sbin/pfctl/pfctl_parser.h
+++ b/sbin/pfctl/pfctl_parser.h
@@ -85,6 +85,7 @@ struct pfctl {
struct pfioc_queue *pqueue;
struct pfr_buffer *trans;
struct pf_anchor *anchor, *alast;
+ struct pfr_ktablehead pfr_ktlast;
const char *ruleset;
/* 'set foo' options */
@@ -211,6 +212,8 @@ struct pfctl_watermarks {
u_int32_t lo;
};
+struct pfr_uktable;
+
void copy_satopfaddr(struct pf_addr *, struct sockaddr *);
int pfctl_rules(int, char *, int, int, char *, struct pfr_buffer *);
@@ -234,7 +237,7 @@ int pfctl_set_interface_flags(struct pfctl *, char *, int, int);
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 *);
int pfctl_load_queues(struct pfctl *);
int pfctl_add_queue(struct pfctl *, struct pf_queuespec *);
@@ -248,7 +251,7 @@ void print_status(struct pf_status *, struct pfctl_watermarks *, int);
void print_queuespec(struct pf_queuespec *);
int pfctl_define_table(char *, int, int, const char *, struct pfr_buffer *,
- u_int32_t);
+ u_int32_t, struct pfr_uktable *);
void pfctl_expand_label_nr(struct pf_rule *, unsigned int);
void pfctl_clear_fingerprints(int, int);
@@ -298,5 +301,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 446329daf9e..4823d19efb8 100644
--- a/sbin/pfctl/pfctl_radix.c
+++ b/sbin/pfctl/pfctl_radix.c
@@ -55,6 +55,18 @@ 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));
+}
int
pfr_clr_tables(struct pfr_table *filter, int *ndel, int flags)
@@ -352,6 +364,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 6443126900e..6f4acfeec82 100644
--- a/sbin/pfctl/pfctl_table.c
+++ b/sbin/pfctl/pfctl_table.c
@@ -520,18 +520,47 @@ 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;
-
- 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;
+ 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;
+ }
- return pfr_ina_define(&tbl, ab->pfrb_caddr, ab->pfrb_size, NULL,
+ 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);
}
let's make pf(4) anchors and tables better friends