On Tue, Jan 12, 2021 at 08:45:22PM +0100, Alexandr Nedvedicky wrote:
> I think bluhm@ and dlg@ have committed part of that change already.
I have only commited a refactoring change. Next step in kernel
would be to remove the check in pf_find_state() and see what happens.
I was waiting for dlg@ to do it, but maybe he waited for me.
Index: net/pf.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/net/pf.c,v
retrieving revision 1.1098
diff -u -p -r1.1098 pf.c
--- net/pf.c 14 Jan 2021 09:44:33 -0000 1.1098
+++ net/pf.c 15 Jan 2021 16:46:42 -0000
@@ -1122,12 +1122,6 @@ pf_find_state(struct pf_pdesc *pd, struc
}
*state = s;
- if (pd->dir == PF_OUT && s->rt_kif != NULL && s->rt_kif != pd->kif &&
- ((s->rule.ptr->rt == PF_ROUTETO &&
- s->rule.ptr->direction == PF_OUT) ||
- (s->rule.ptr->rt == PF_REPLYTO &&
- s->rule.ptr->direction == PF_IN)))
- return (PF_PASS);
return (PF_MATCH);
}
> the proposed diff updates pfctl(8) so parser will do 'a right thing',
Does it work without the kernel changes from dlg@ ?
> the diff also breaks existing regression tests. We can update
> them once, we will agree on proposed diff.
I have adapted my regress pf.conf and regress/sys/net/pf_forward
fails in the route-to test. It worked with dlg@'s diff. So your
standalone pfctl change does not seem to be sufficient.
bluhm
> --------8<---------------8<---------------8<------------------8<--------
> diff --git a/sbin/pfctl/parse.y b/sbin/pfctl/parse.y
> index 2b3e62b1a7e..536aec3286b 100644
> --- a/sbin/pfctl/parse.y
> +++ b/sbin/pfctl/parse.y
> @@ -3745,23 +3745,13 @@ pool_opt : BITMASK {
> ;
>
> route_host : STRING {
> - /* try to find @if0 address specs */
> - if (strrchr($1, '@') != NULL) {
> - if (($$ = host($1, pf->opts)) == NULL) {
> - yyerror("invalid host for route spec");
> - YYERROR;
> - }
> + if (($$ = next_hop($1, pf->opts)) == NULL) {
> + /* error. "any" is handled elsewhere */
> free($1);
> - } else {
> - $$ = calloc(1, sizeof(struct node_host));
> - if ($$ == NULL)
> - err(1, "route_host: calloc");
> - $$->ifname = $1;
> - $$->addr.type = PF_ADDR_NONE;
> - set_ipmask($$, 128);
> - $$->next = NULL;
> - $$->tail = $$;
> + yyerror("could not parse host specification");
> + YYERROR;
> }
> + free($1);
> }
> | STRING '/' STRING {
> char *buf;
> @@ -3769,7 +3759,7 @@ route_host : STRING {
> if (asprintf(&buf, "%s/%s", $1, $3) == -1)
> err(1, "host: asprintf");
> free($1);
> - if (($$ = host(buf, pf->opts)) == NULL) {
> + if (($$ = next_hop(buf, pf->opts)) == NULL) {
> /* error. "any" is handled elsewhere */
> free(buf);
> yyerror("could not parse host specification");
> @@ -3795,33 +3785,6 @@ route_host : STRING {
> $$->next = NULL;
> $$->tail = $$;
> }
> - | dynaddr '/' NUMBER {
> - struct node_host *n;
> -
> - if ($3 < 0 || $3 > 128) {
> - yyerror("bit number too big");
> - YYERROR;
> - }
> - $$ = $1;
> - for (n = $1; n != NULL; n = n->next)
> - set_ipmask(n, $3);
> - }
> - | '(' STRING host ')' {
> - struct node_host *n;
> -
> - $$ = $3;
> - /* XXX check masks, only full mask should be allowed */
> - for (n = $3; n != NULL; n = n->next) {
> - if ($$->ifname) {
> - yyerror("cannot specify interface twice
> "
> - "in route spec");
> - YYERROR;
> - }
> - if (($$->ifname = strdup($2)) == NULL)
> - errx(1, "host: strdup");
> - }
> - free($2);
> - }
> ;
>
> route_host_list : route_host optweight optnl {
> diff --git a/sbin/pfctl/pfctl_parser.c b/sbin/pfctl/pfctl_parser.c
> index 03317844e91..534b53b1198 100644
> --- a/sbin/pfctl/pfctl_parser.c
> +++ b/sbin/pfctl/pfctl_parser.c
> @@ -1615,17 +1615,16 @@ host(const char *s, int opts)
> {
> struct node_host *h = NULL, *n;
> int mask = -1;
> - char *p, *ps, *if_name;
> + char *p, *ps;
> const char *errstr;
>
> + if (strchr(s, '@') != NULL)
> + err(1, "%s: '@' not expected in host spec. (%s)\n",
> + __func__, s);
> +
> if ((ps = strdup(s)) == NULL)
> err(1, "%s: strdup", __func__);
>
> - if ((if_name = strrchr(ps, '@')) != NULL) {
> - if_name[0] = '\0';
> - if_name++;
> - }
> -
> if ((p = strchr(ps, '/')) != NULL) {
> mask = strtonum(p+1, 0, 128, &errstr);
> if (errstr) {
> @@ -1642,10 +1641,46 @@ host(const char *s, int opts)
> goto error;
> }
>
> - if (if_name && if_name[0])
> - for (n = h; n != NULL; n = n->next)
> - if ((n->ifname = strdup(if_name)) == NULL)
> - err(1, "%s: strdup", __func__);
> + for (n = h; n != NULL; n = n->next) {
> + n->addr.type = PF_ADDR_ADDRMASK;
> + n->weight = 0;
> + }
> +
> +error:
> + free(ps);
> + return (h);
> +}
> +
> +struct node_host *
> +next_hop(const char *s, int opts)
> +{
> + struct node_host *h = NULL, *n;
> + int mask = -1;
> + char *p, *ps;
> + const char *errstr;
> +
> + if (strchr(s, '@') != NULL)
> + err(1, "%s: '@' not expected in next-hop spec. (%s)\n",
> + __func__, s);
> +
> + if ((ps = strdup(s)) == NULL)
> + err(1, "%s: strdup", __func__);
> +
> + if ((p = strchr(ps, '/')) != NULL) {
> + mask = strtonum(p+1, 0, 128, &errstr);
> + if (errstr) {
> + fprintf(stderr, "netmask is %s: %s\n", errstr, p);
> + goto error;
> + }
> + p[0] = '\0';
> + }
> +
> + if ((h = host_ip(ps, mask)) == NULL &&
> + (h = host_dns(ps, mask, (opts & PF_OPT_NODNS))) == NULL) {
> + fprintf(stderr, "no IP address found for %s\n", s);
> + goto error;
> + }
> +
> for (n = h; n != NULL; n = n->next) {
> n->addr.type = PF_ADDR_ADDRMASK;
> n->weight = 0;
> diff --git a/sbin/pfctl/pfctl_parser.h b/sbin/pfctl/pfctl_parser.h
> index a82854a0fea..c7ab3f967b6 100644
> --- a/sbin/pfctl/pfctl_parser.h
> +++ b/sbin/pfctl/pfctl_parser.h
> @@ -292,6 +292,7 @@ char *ifa_indextoname(unsigned int,
> char *);
> struct node_host *ifa_exists(const char *);
> struct node_host *ifa_lookup(const char *, int);
> struct node_host *host(const char *, int);
> +struct node_host *next_hop(const char *, int);
>
> int append_addr(struct pfr_buffer *, char *, int, int);
> int append_addr_host(struct pfr_buffer *,
