Re: refactoring of pf_find_or_create_ruleset()
Hello,
On Mon, Sep 04, 2017 at 09:51:29PM +0200, Alexander Bluhm wrote:
> On Mon, Sep 04, 2017 at 10:29:01AM +0200, Alexandr Nedvedicky wrote:
> > anyway below is the patch, which Hrvoje was testing and it worked for
> > him.
> > I'd like to get some OK to proceed to commit.
>
> I think it is correct. OK bluhm@
>
> > + if (parent != NULL) {
> > + /*
> > +* Make sure path for levels 2, 3, ... is terminated by '/':
> > +* 1/2/3/...
> > +*/
> > + strlcpy(anchor->path, parent->path, sizeof (anchor->path));
>
> Do not put space between sizeof and (
ooops, old Solaris habit I'm still used to ;-) thanks for catching that.
will commit today evening, once be back home.
regards
sasha
Re: refactoring of pf_find_or_create_ruleset()
On Mon, Sep 04, 2017 at 10:29:01AM +0200, Alexandr Nedvedicky wrote:
> anyway below is the patch, which Hrvoje was testing and it worked for him.
> I'd like to get some OK to proceed to commit.
I think it is correct. OK bluhm@
> + if (parent != NULL) {
> + /*
> + * Make sure path for levels 2, 3, ... is terminated by '/':
> + * 1/2/3/...
> + */
> + strlcpy(anchor->path, parent->path, sizeof (anchor->path));
Do not put space between sizeof and (
Re: refactoring of pf_find_or_create_ruleset()
Hello,
> with this patch i can't trigger panic with or without WITH_PF_LOCK if
> that's matter for some reason.
anyway below is the patch, which Hrvoje was testing and it worked for him.
I'd like to get some OK to proceed to commit.
> thank you sasha for great work on MP pf :)
I'm very last in the long queue of smart folks, who are making MP
dream come true...
thanks and
regards
sasha
8<---8<---8<--8<
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(_anchors);
memset(_main_anchor, 0, sizeof(pf_main_anchor));
pf_init_ruleset(_main_anchor.ruleset);
- pf_main_anchor.ruleset.anchor = _main_anchor;
if (trans == NULL) {
bzero(, sizeof(buf));
buf.pfrb_type = PFRB_TRANS;
diff -r 78ea302507cb src/sys/net/pf_ruleset.c
--- src/sys/net/pf_ruleset.cThu Aug 31 21:27:47 2017 +0200
+++ src/sys/net/pf_ruleset.cFri 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 = _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(>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, _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, >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, _anchors,
+ anchor);
+ rs_free(anchor, sizeof(*anchor));
+ return (NULL);
+ }
+ }
+
+ pf_init_ruleset(>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 (_main_ruleset);
+
while (*path == '/')
path++;
+
ruleset = pf_find_ruleset(path);
if (ruleset != NULL)
return
Re: refactoring of pf_find_or_create_ruleset()
On 1.9.2017. 22:57, Alexandr Nedvedicky wrote:
> 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 != _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
Hi,
with this patch i can't trigger panic with or without WITH_PF_LOCK if
that's matter for some reason.
pf conf:
# pfctl -nvf pf.conf
set skip on { lo em0 }
set limit states 100
block drop all
anchor "test1" on ix3 all {
pass all flags S/SA
anchor "test11" all {
pass all flags S/SA
}
}
anchor "test2" on ix2 all {
pass all flags S/SA
anchor "test21" all {
pass all flags S/SA
}
}
thank you sasha for great work on MP pf :)
Re: refactoring of pf_find_or_create_ruleset()
Hello Hrvoje,
> Hi,
>
> with this diff i'm getting this panic:
>
> # pfctl -nvf /etc/pf.conf
> set limit states 100
> 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(0xff07835f3100, 0x90, 0, 1) -> e
> kernel: page fault trap, code=0
> Stopped at strlcpy+0x25: movzbl 0(%rax),%edx
> ddb{3}> trace
> strlcpy(10,800023c75010,10282) at strlcpy+0x25
> pf_create_anchor(0,80cf6400) at pf_create_anchor+0xb0
> pf_find_or_create_ruleset(8145caa0) at
> pf_find_or_create_ruleset+0x1c2
> pf_anchor_setup(16,8145cdf8,8145caa0) at
> pf_anchor_setup+0x16b
> pfioctl() at pfioctl+0x31ea
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(_anchors);
memset(_main_anchor, 0, sizeof(pf_main_anchor));
pf_init_ruleset(_main_anchor.ruleset);
- pf_main_anchor.ruleset.anchor = _main_anchor;
if (trans == NULL) {
bzero(, sizeof(buf));
buf.pfrb_type = PFRB_TRANS;
diff -r 817d4d197b87 src/sys/net/pf_ruleset.c
--- src/sys/net/pf_ruleset.cThu Aug 31 21:28:31 2017 +0200
+++ src/sys/net/pf_ruleset.cFri Sep 01 22:47:57 2017 +0200
@@ -190,7 +190,7 @@ pf_create_anchor(struct pf_anchor *paren
RB_INIT(>children);
strlcpy(anchor->name, aname, sizeof(anchor->name));
- if (parent != _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 != _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(_anchors);
memset(_main_anchor, 0, sizeof(pf_main_anchor));
pf_init_ruleset(_main_anchor.ruleset);
- pf_main_anchor.ruleset.anchor = _main_anchor;
if (trans == NULL) {
bzero(, sizeof(buf));
buf.pfrb_type = PFRB_TRANS;
diff -r 78ea302507cb src/sys/net/pf_ruleset.c
--- src/sys/net/pf_ruleset.cThu Aug 31 21:27:47 2017 +0200
+++ src/sys/net/pf_ruleset.cFri 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 = _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));
