Hello,
long time ago mpi@ asked, what I would improve in PF to make the code ready for
SMP massage. Everybody knows PF is perfect, right? So it took me a while to
find a code for facelift.
Patch below breaks pf_find_or_create_ruleset() spaghetti to more chunks:
pf_find_or_create_ruleset()
which still tights all pieces together
pf_get_leaf_ruleset(*path, **path_remainder)
it traverses the path to the leaf, which exists PF. let's assume this
is the path to ruleset administrator wants to add:
the/ruleset/that/exists/path/we/are/adding
the function will traverse the path to:
the/ruleset/that/exists
will return a pointer to the matching ruleset and it will
also update the path_remainder to:
path/we/are/adding
Thus the caller (pf_find_or_create_ruleset()), is able to create
the rulesets, which match path_remainder and attach them to
existing ruleset.
pf_create_anchor()
it just creates anchor and attaches it to parent.
I did run regression tests for pfctl. I had to workaround a test pf104.in.
The resolver always returned IPv4 for 'localhost' instead of IPv6. I eventually
gave up and skipped the test. No other problems popped up.
Do you think I should get it in?
thanks and
regards
sasha
--------8<---------------8<---------------8<------------------8<--------
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 Thu Aug 31 21:30:45 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 != &pf_main_anchor) {
+ /*
+ * 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 Thu Aug 31 21:30:45 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);
--------8<---------------8<---------------8<------------------8<--------
