Hello Hrvoje,
> Hi,
>
> with this diff i'm getting this panic:
>
> # pfctl -nvf /etc/pf.conf
> set limit states 1000000
> set skip on { lo em0 }
> block return all
> pass all flags S/SA
> anchor "test1" on ix1 all {
> pass all flags S/SA
> }
>
>
> # pfctl -f /etc/pf.conf
> uvm_fault(0xffffff07835f3100, 0x90, 0, 1) -> e
> kernel: page fault trap, code=0
> Stopped at strlcpy+0x25: movzbl 0(%rax),%edx
> ddb{3}> trace
> strlcpy(10,ffff800023c75010,10282) at strlcpy+0x25
> pf_create_anchor(0,ffff800000cf6400) at pf_create_anchor+0xb0
> pf_find_or_create_ruleset(ffff80000145caa0) at
> pf_find_or_create_ruleset+0x1c2
> pf_anchor_setup(16,ffff80000145cdf8,ffff80000145caa0) at
> pf_anchor_setup+0x16b
> pfioctl() at pfioctl+0x31ea
</snip>
thank you for for covering my back (again). I suck so much in testing, forgot
to reboot my box to see the same panic.
My earlier patch, you were testing, uncovered inconsistency between
kernel side and pfctl side of pf_main_ruleset. The inconsistency is fixed
by change as follows:
--------8<---------------8<---------------8<------------------8<--------
diff -r 817d4d197b87 src/sbin/pfctl/pfctl.c
--- src/sbin/pfctl/pfctl.c Thu Aug 31 21:28:31 2017 +0200
+++ src/sbin/pfctl/pfctl.c Fri Sep 01 22:47:57 2017 +0200
@@ -1572,7 +1572,6 @@ pfctl_rules(int dev, char *filename, int
RB_INIT(&pf_anchors);
memset(&pf_main_anchor, 0, sizeof(pf_main_anchor));
pf_init_ruleset(&pf_main_anchor.ruleset);
- pf_main_anchor.ruleset.anchor = &pf_main_anchor;
if (trans == NULL) {
bzero(&buf, sizeof(buf));
buf.pfrb_type = PFRB_TRANS;
diff -r 817d4d197b87 src/sys/net/pf_ruleset.c
--- src/sys/net/pf_ruleset.c Thu Aug 31 21:28:31 2017 +0200
+++ src/sys/net/pf_ruleset.c Fri Sep 01 22:47:57 2017 +0200
@@ -190,7 +190,7 @@ pf_create_anchor(struct pf_anchor *paren
RB_INIT(&anchor->children);
strlcpy(anchor->name, aname, sizeof(anchor->name));
- if (parent != &pf_main_anchor) {
+ if (parent != NULL) {
/*
* Make sure path for levels 2, 3, ... is terminated by '/':
* 1/2/3/...
--------8<---------------8<---------------8<------------------8<--------
as you can see the kernel sets ruleset.anchor to NULL (see pfattach() and then
do also a 'grep -n kludge pf_ioctl.c'), while userland links it to
pf_main_anchor.
I've remember to changing 'parent != NULL' to 'parent != &pf_main_anchor' in
pf_create_anchor() just to make regression tests passing. Fortunately you did
run my code in kernel. With change above my patch works for kernel as well as
for regression tests.
updated patch is attached.
thanks and
regards
sasha
diff -r 78ea302507cb src/sbin/pfctl/pfctl.c
--- src/sbin/pfctl/pfctl.c Thu Aug 31 21:27:47 2017 +0200
+++ src/sbin/pfctl/pfctl.c Fri Sep 01 22:52:00 2017 +0200
@@ -1572,7 +1572,6 @@ pfctl_rules(int dev, char *filename, int
RB_INIT(&pf_anchors);
memset(&pf_main_anchor, 0, sizeof(pf_main_anchor));
pf_init_ruleset(&pf_main_anchor.ruleset);
- pf_main_anchor.ruleset.anchor = &pf_main_anchor;
if (trans == NULL) {
bzero(&buf, sizeof(buf));
buf.pfrb_type = PFRB_TRANS;
diff -r 78ea302507cb src/sys/net/pf_ruleset.c
--- src/sys/net/pf_ruleset.c Thu Aug 31 21:27:47 2017 +0200
+++ src/sys/net/pf_ruleset.c Fri Sep 01 22:52:00 2017 +0200
@@ -133,95 +133,150 @@ pf_find_ruleset(const char *path)
}
struct pf_ruleset *
+pf_get_leaf_ruleset(char *path, char **path_remainder)
+{
+ struct pf_ruleset *ruleset;
+ char *leaf, *p;
+ int i = 0;
+
+ p = path;
+ while (*p == '/')
+ p++;
+
+ ruleset = pf_find_ruleset(p);
+ leaf = p;
+ while (ruleset == NULL) {
+ leaf = strrchr(p, '/');
+ if (leaf != NULL) {
+ *leaf = '\0';
+ i++;
+ ruleset = pf_find_ruleset(p);
+ } else {
+ leaf = path;
+ /*
+ * if no path component exists, then main ruleset is
+ * our parent.
+ */
+ ruleset = &pf_main_ruleset;
+ }
+ }
+
+ if (path_remainder != NULL)
+ *path_remainder = leaf;
+
+ /* restore slashes in path. */
+ while (i != 0) {
+ while (*leaf != '\0')
+ leaf++;
+ *leaf = '/';
+ i--;
+ }
+
+ return (ruleset);
+}
+
+struct pf_anchor *
+pf_create_anchor(struct pf_anchor *parent, const char *aname)
+{
+ struct pf_anchor *anchor, *dup;
+
+ if (!*aname || (strlen(aname) >= PF_ANCHOR_NAME_SIZE) ||
+ ((parent != NULL) && (strlen(parent->path) >= PF_ANCHOR_MAXPATH)))
+ return (NULL);
+
+ anchor = rs_malloc(sizeof(*anchor));
+ if (anchor == NULL)
+ return (NULL);
+
+ RB_INIT(&anchor->children);
+ strlcpy(anchor->name, aname, sizeof(anchor->name));
+ if (parent != NULL) {
+ /*
+ * Make sure path for levels 2, 3, ... is terminated by '/':
+ * 1/2/3/...
+ */
+ strlcpy(anchor->path, parent->path, sizeof (anchor->path));
+ strlcat(anchor->path, "/", sizeof(anchor->path));
+ }
+ strlcat(anchor->path, anchor->name, sizeof(anchor->path));
+
+ if ((dup = RB_INSERT(pf_anchor_global, &pf_anchors, anchor)) != NULL) {
+ 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));
+ return (NULL);
+ }
+
+ if (parent != NULL) {
+ anchor->parent = parent;
+ dup = RB_INSERT(pf_anchor_node, &parent->children, anchor);
+ if (dup != NULL) {
+ DPFPRINTF(LOG_NOTICE,
+ "%s: RB_INSERT to parent '%s' '%s' collides with "
+ "'%s' '%s'", __func__, anchor->path, anchor->name,
+ dup->path, dup->name);
+ RB_REMOVE(pf_anchor_global, &pf_anchors,
+ anchor);
+ rs_free(anchor, sizeof(*anchor));
+ return (NULL);
+ }
+ }
+
+ pf_init_ruleset(&anchor->ruleset);
+ anchor->ruleset.anchor = anchor;
+
+ return (anchor);
+}
+
+struct pf_ruleset *
pf_find_or_create_ruleset(const char *path)
{
- char *p, *q, *r;
+ char *p, *aname, *r;
struct pf_ruleset *ruleset;
- struct pf_anchor *anchor, *dup, *parent = NULL;
+ struct pf_anchor *anchor;
if (path[0] == 0)
return (&pf_main_ruleset);
+
while (*path == '/')
path++;
+
ruleset = pf_find_ruleset(path);
if (ruleset != NULL)
return (ruleset);
+
p = rs_malloc(MAXPATHLEN);
if (p == NULL)
return (NULL);
strlcpy(p, path, MAXPATHLEN);
- while (parent == NULL && (q = strrchr(p, '/')) != NULL) {
- *q = 0;
- if ((ruleset = pf_find_ruleset(p)) != NULL) {
- parent = ruleset->anchor;
- break;
- }
- }
- if (q == NULL)
- q = p;
- else
- q++;
- strlcpy(p, path, MAXPATHLEN);
- if (!*q) {
- rs_free(p, MAXPATHLEN);
- return (NULL);
- }
- while ((r = strchr(q, '/')) != NULL || *q) {
+
+ ruleset = pf_get_leaf_ruleset(p, &aname);
+ anchor = ruleset->anchor;
+
+ while (*aname == '/')
+ aname++;
+ /*
+ * aname is a path remainder, which contains nodes we must create. We
+ * process the aname path from left to right, effectively descending
+ * from parents to children.
+ */
+ while ((r = strchr(aname, '/')) != NULL || *aname) {
if (r != NULL)
*r = 0;
- if (!*q || strlen(q) >= PF_ANCHOR_NAME_SIZE ||
- (parent != NULL && strlen(parent->path) >=
- MAXPATHLEN - PF_ANCHOR_NAME_SIZE - 1)) {
- rs_free(p, MAXPATHLEN);
- return (NULL);
- }
- anchor = rs_malloc(sizeof(*anchor));
+
+ anchor = pf_create_anchor(anchor, aname);
if (anchor == NULL) {
rs_free(p, MAXPATHLEN);
return (NULL);
}
- RB_INIT(&anchor->children);
- strlcpy(anchor->name, q, sizeof(anchor->name));
- if (parent != NULL) {
- strlcpy(anchor->path, parent->path,
- sizeof(anchor->path));
- strlcat(anchor->path, "/", sizeof(anchor->path));
- }
- strlcat(anchor->path, anchor->name, sizeof(anchor->path));
- if ((dup = RB_INSERT(pf_anchor_global, &pf_anchors, anchor)) !=
- NULL) {
- DPFPRINTF(LOG_NOTICE,
- "pf_find_or_create_ruleset: RB_INSERT1 "
- "'%s' '%s' collides with '%s' '%s'",
- anchor->path, anchor->name, dup->path, dup->name);
- rs_free(anchor, sizeof(*anchor));
- rs_free(p, MAXPATHLEN);
- return (NULL);
- }
- if (parent != NULL) {
- anchor->parent = parent;
- if ((dup = RB_INSERT(pf_anchor_node, &parent->children,
- anchor)) != NULL) {
- DPFPRINTF(LOG_NOTICE,
- "pf_find_or_create_ruleset: "
- "RB_INSERT2 '%s' '%s' collides with "
- "'%s' '%s'", anchor->path, anchor->name,
- dup->path, dup->name);
- RB_REMOVE(pf_anchor_global, &pf_anchors,
- anchor);
- rs_free(anchor, sizeof(*anchor));
- rs_free(p, MAXPATHLEN);
- return (NULL);
- }
- }
- pf_init_ruleset(&anchor->ruleset);
- anchor->ruleset.anchor = anchor;
- parent = anchor;
- if (r != NULL)
- q = r + 1;
+
+ if (r == NULL)
+ break;
else
- *q = 0;
+ aname = r + 1;
}
+
rs_free(p, MAXPATHLEN);
return (&anchor->ruleset);
}
diff -r 78ea302507cb src/sys/net/pfvar.h
--- src/sys/net/pfvar.h Thu Aug 31 21:27:47 2017 +0200
+++ src/sys/net/pfvar.h Fri Sep 01 22:52:00 2017 +0200
@@ -463,6 +463,7 @@ union pf_rule_ptr {
};
#define PF_ANCHOR_NAME_SIZE 64
+#define PF_ANCHOR_MAXPATH (MAXPATHLEN - PF_ANCHOR_NAME_SIZE - 1)
struct pf_rule {
struct pf_rule_addr src;
@@ -1855,6 +1856,8 @@ void pf_anchor_remove(struct
pf_rule
void pf_remove_if_empty_ruleset(struct pf_ruleset *);
struct pf_anchor *pf_find_anchor(const char *);
struct pf_ruleset *pf_find_ruleset(const char *);
+struct pf_ruleset *pf_get_leaf_ruleset(char *, char **);
+struct pf_anchor *pf_create_anchor(struct pf_anchor *, const char *);
struct pf_ruleset *pf_find_or_create_ruleset(const char *);
void pf_rs_initialize(void);
