Re: diff: pfctl: error message for nonexisting rtable
Hello, On Wed, Sep 30, 2020 at 11:02:28PM +0200, Klemens Nanni wrote: > On Sun, Sep 20, 2020 at 07:29:38PM +0200, Klemens Nanni wrote: > > Rebased diff after yasouka's pfctl commit; it still takes care of > > rdomains only, but I'd appreciate folks using `on rdomain' in their > > pf.conf test this. If this works out I'd like to put it in shortly > > after release and work on rtables next. > Working for me so far, anyone else playing around with it? > > I also checked CVS log and the existing rtable_l2() check goes back to > claudio's introduction of `on rdomain N' in pf, i.e. it's been there > from the start as a harmless but needless check and has not been added > after the fact to fix anything. > > The time is now, so here's an updated diff after sashan pointed out how > I erroneously checked the rtable ID instead of the rdomain ID in the > ioctl (copy/pasta mistake). > > Feedback? OK? fix looks OK to me. thanks and regards sashan
Re: diff: pfctl: error message for nonexisting rtable
On Sun, Sep 20, 2020 at 07:29:38PM +0200, Klemens Nanni wrote:
> Rebased diff after yasouka's pfctl commit; it still takes care of
> rdomains only, but I'd appreciate folks using `on rdomain' in their
> pf.conf test this. If this works out I'd like to put it in shortly
> after release and work on rtables next.
Working for me so far, anyone else playing around with it?
I also checked CVS log and the existing rtable_l2() check goes back to
claudio's introduction of `on rdomain N' in pf, i.e. it's been there
from the start as a harmless but needless check and has not been added
after the fact to fix anything.
The time is now, so here's an updated diff after sashan pointed out how
I erroneously checked the rtable ID instead of the rdomain ID in the
ioctl (copy/pasta mistake).
Feedback? OK?
Index: sys/net/pf_ioctl.c
===
RCS file: /cvs/src/sys/net/pf_ioctl.c,v
retrieving revision 1.356
diff -u -p -r1.356 pf_ioctl.c
--- sys/net/pf_ioctl.c 24 Aug 2020 15:41:15 - 1.356
+++ sys/net/pf_ioctl.c 30 Sep 2020 20:48:34 -
@@ -2820,10 +2820,8 @@ pf_rule_copyin(struct pf_rule *from, str
if (to->rtableid >= 0 && !rtable_exists(to->rtableid))
return (EBUSY);
to->onrdomain = from->onrdomain;
- if (to->onrdomain >= 0 && !rtable_exists(to->onrdomain))
- return (EBUSY);
- if (to->onrdomain >= 0) /* make sure it is a real rdomain */
- to->onrdomain = rtable_l2(to->onrdomain);
+ if (to->onrdomain < 0 || to->onrdomain > RT_TABLEID_MAX)
+ return (EINVAL);
for (i = 0; i < PFTM_MAX; i++)
to->timeout[i] = from->timeout[i];
Index: sbin/pfctl/parse.y
===
RCS file: /cvs/src/sbin/pfctl/parse.y,v
retrieving revision 1.703
diff -u -p -r1.703 parse.y
--- sbin/pfctl/parse.y 17 Sep 2020 14:26:59 - 1.703
+++ sbin/pfctl/parse.y 20 Sep 2020 17:28:10 -
@@ -1216,7 +1216,7 @@ antispoof_opt : LABEL label {
if ($2 < 0 || $2 > RT_TABLEID_MAX) {
yyerror("invalid rtable id");
YYERROR;
- } else if (lookup_rtable($2) < 1) {
+ } else if (!lookup_rtable($2)) {
yyerror("rtable %lld does not exist", $2);
YYERROR;
}
@@ -2003,7 +2003,7 @@ filter_opt: USER uids {
if ($2 < 0 || $2 > RT_TABLEID_MAX) {
yyerror("invalid rtable id");
YYERROR;
- } else if (lookup_rtable($2) < 1) {
+ } else if (!lookup_rtable($2)) {
yyerror("rtable %lld does not exist", $2);
YYERROR;
}
@@ -2481,8 +2481,6 @@ if_item : STRING{
| RDOMAIN NUMBER{
if ($2 < 0 || $2 > RT_TABLEID_MAX)
yyerror("rdomain %lld outside range", $2);
- else if (lookup_rtable($2) != 2)
- yyerror("rdomain %lld does not exist", $2);
$$ = calloc(1, sizeof(struct node_if));
if ($$ == NULL)
@@ -5899,10 +5897,6 @@ lookup_rtable(u_int rtableid)
return 0;
}
err(1, "%s", __func__);
- }
- if (info.rti_domainid == rtableid) {
- found[rtableid] = 2;
- return 2;
}
found[rtableid] = 1;
return 1;
Re: diff: pfctl: error message for nonexisting rtable
On Tue, Sep 15, 2020 at 02:31:24AM +0200, Klemens Nanni wrote:
> On Tue, Sep 15, 2020 at 12:30:35AM +0200, Klemens Nanni wrote:
> > Actually, that should just work regardless of whether the rounting
> > domain exists at ruleset creation time; just like it is the case with
> > interface names/groups which may come and go at runtime without
> > requiring changes to the ruleset.
> >
> > Rules on nonexistent interfaces won't match, routing domains (and
> > ultimately routing tables) should behave the same, I think.
> >
> > Here's a diff that does this for routing domains allowing me to always
> > use `on rdomain 5' - I've tested it with a few examplatory rulesets and
> > behaviour is as expected.
> >
> > It will need more eye balling and I am not pushing such changes before
> > release, but if that is a general direction we agree, your proposed
> > `rtable' fix could move along and become just as flexible instead.
> More on this:
>
> # ifconfig lo1 rdomain 1
> # echo pass on rdomain 1 | pfctl -f-
> # ifconfig lo1 destroy
> # pfctl -sr
>
> pass on rdomain 1 all flags S/SA
>
> The ruleset stays valid and continues to work as soon as routing domain
> `1' reappears, there is no reason to require existence of it at ruleset
> creation; this is safe because routing domains are just normative
> numbers, there's no further state when it comes to filtering - either
> the id on the packet matches the number in the ruleset or it doesn't.
>
> Routing tables however are more involved as they can be used to *alter*
> a packet's flow in pf.conf(5), so requiring them to be present at
> ruleset creation makes sense to guarantee that pf will only ever change
> routing table ids to valid ones.
>
> Routing domains can be deleted, but that doesn't invalidate rules like
> `on rdomain 1', which simply won't match when the given id does not
> exist.
>
> Routing tables however cannot be deleted, they get moved to the default
> routing domain whenever their corresponding routing domain disappears;
> this is in line with only ever loading valid routing table ids into pf.
>
> So unless I missed something, that ruleset creation (`pfctl -f ...')
> is the only occasion pf actually needs to validate routing table ids:
> they are guaranteed to always exist from then on.
>
> Given this, my diff looks fine as is and should not change `rtable'
> behaviour - YASUOKA's diff is also fine as is and actually implements
> the validity check I just mentioned, obsoleting my initial feedback.
Rebased diff after yasouka's pfctl commit; it still takes care of
rdomains only, but I'd appreciate folks using `on rdomain' in their
pf.conf test this. If this works out I'd like to put it in shortly
after release and work on rtables next.
Index: sys/net/pf_ioctl.c
===
RCS file: /cvs/src/sys/net/pf_ioctl.c,v
retrieving revision 1.356
diff -u -p -r1.356 pf_ioctl.c
--- sys/net/pf_ioctl.c 24 Aug 2020 15:41:15 - 1.356
+++ sys/net/pf_ioctl.c 14 Sep 2020 22:27:55 -
@@ -2820,10 +2820,8 @@ pf_rule_copyin(struct pf_rule *from, str
if (to->rtableid >= 0 && !rtable_exists(to->rtableid))
return (EBUSY);
to->onrdomain = from->onrdomain;
- if (to->onrdomain >= 0 && !rtable_exists(to->onrdomain))
- return (EBUSY);
- if (to->onrdomain >= 0) /* make sure it is a real rdomain */
- to->onrdomain = rtable_l2(to->onrdomain);
+ if (to->rtableid < 0 || to->rtableid > RT_TABLEID_MAX)
+ return (EINVAL);
for (i = 0; i < PFTM_MAX; i++)
to->timeout[i] = from->timeout[i];
Index: sbin/pfctl/parse.y
===
RCS file: /cvs/src/sbin/pfctl/parse.y,v
retrieving revision 1.703
diff -u -p -r1.703 parse.y
--- sbin/pfctl/parse.y 17 Sep 2020 14:26:59 - 1.703
+++ sbin/pfctl/parse.y 20 Sep 2020 17:28:10 -
@@ -1216,7 +1216,7 @@ antispoof_opt : LABEL label {
if ($2 < 0 || $2 > RT_TABLEID_MAX) {
yyerror("invalid rtable id");
YYERROR;
- } else if (lookup_rtable($2) < 1) {
+ } else if (!lookup_rtable($2)) {
yyerror("rtable %lld does not exist", $2);
YYERROR;
}
@@ -2003,7 +2003,7 @@ filter_opt: USER uids {
if ($2 < 0 || $2 > RT_TABLEID_MAX) {
yyerror("invalid rtable id");
YYERROR;
- } else if (lookup_rtable($2) < 1) {
+ } else if (!lookup_rtable($2)) {
yyerror("rtable %lld does not exist", $2);
YYERROR;
Re: diff: pfctl: error message for nonexisting rtable
the condition was reversed.
ok?
Index: parse.y
===
RCS file: /cvs/src/sbin/pfctl/parse.y,v
retrieving revision 1.702
diff -u -p -r1.702 parse.y
--- parse.y 17 Sep 2020 10:09:43 - 1.702
+++ parse.y 17 Sep 2020 14:23:42 -
@@ -1216,7 +1216,7 @@ antispoof_opt : LABEL label {
if ($2 < 0 || $2 > RT_TABLEID_MAX) {
yyerror("invalid rtable id");
YYERROR;
- } else if (lookup_rtable($2) >= 1) {
+ } else if (lookup_rtable($2) < 1) {
yyerror("rtable %lld does not exist", $2);
YYERROR;
}
@@ -2003,7 +2003,7 @@ filter_opt: USER uids {
if ($2 < 0 || $2 > RT_TABLEID_MAX) {
yyerror("invalid rtable id");
YYERROR;
- } else if (lookup_rtable($2) >= 1) {
+ } else if (lookup_rtable($2) < 1) {
yyerror("rtable %lld does not exist", $2);
YYERROR;
}
Re: diff: pfctl: error message for nonexisting rtable
Hi,
I just committed yours.
Thanks,
On Wed, 16 Sep 2020 16:07:40 +0200
Klemens Nanni wrote:
> On Wed, Sep 16, 2020 at 07:49:19PM +0900, YASUOKA Masahiko wrote:
>> New diff is using -1 for ENOENT.
>>
>> Also domainid == 0 is a valid domain id, but previous diff cannot make
>> a cache of it since 0 is the default value. So new diff is doing
>>
>> -static u_int found[RT_TABLEID_MAX+1];
>> +static struct {
>> +int found;
>> +int domainid;
>> +}rtables[RT_TABLEID_MAX+1];
>>
>> to distinguish the default 0 and domainid 0.
> This looks more complicated than it needs to be, but I also don't want
> to bikeshed it; given that the parser is happy with this and we plan to
> remove this code alltogether anyway in the next release cycle: OK kn.
>
> Alternatively, here's a much simpler diff resembling what I had in mind.
> Feel free to commit this instead (with my OK), give me an OK for it or
> go ahead with yours.
>
> It uses the same function and reflects the fact that every rdomain is a
> rtable but not every rtable is also a rdomain (your choice of `domainid'
> seems inconsistent with that).
>
> Index: parse.y
> ===
> RCS file: /cvs/src/sbin/pfctl/parse.y,v
> retrieving revision 1.701
> diff -u -p -r1.701 parse.y
> --- parse.y 28 Jan 2020 15:40:35 - 1.701
> +++ parse.y 16 Sep 2020 13:58:23 -
> @@ -392,7 +392,7 @@ intinvalid_redirect(struct node_host *
> u_int16_t parseicmpspec(char *, sa_family_t);
> int kw_casecmp(const void *, const void *);
> int map_tos(char *string, int *);
> -int rdomain_exists(u_int);
> +int lookup_rtable(u_int);
> int filteropts_to_rule(struct pf_rule *, struct filter_opts *);
>
> TAILQ_HEAD(loadanchorshead, loadanchors)
> @@ -1216,6 +1216,9 @@ antispoof_opt : LABEL label {
> if ($2 < 0 || $2 > RT_TABLEID_MAX) {
> yyerror("invalid rtable id");
> YYERROR;
> + } else if (lookup_rtable($2) >= 1) {
> + yyerror("rtable %lld does not exist", $2);
> + YYERROR;
> }
> antispoof_opts.rtableid = $2;
> }
> @@ -2000,6 +2003,9 @@ filter_opt : USER uids {
> if ($2 < 0 || $2 > RT_TABLEID_MAX) {
> yyerror("invalid rtable id");
> YYERROR;
> + } else if (lookup_rtable($2) >= 1) {
> + yyerror("rtable %lld does not exist", $2);
> + YYERROR;
> }
> filter_opts.rtableid = $2;
> }
> @@ -2475,7 +2481,7 @@ if_item : STRING{
> | RDOMAIN NUMBER{
> if ($2 < 0 || $2 > RT_TABLEID_MAX)
> yyerror("rdomain %lld outside range", $2);
> - else if (rdomain_exists($2) != 1)
> + else if (lookup_rtable($2) != 2)
> yyerror("rdomain %lld does not exist", $2);
>
> $$ = calloc(1, sizeof(struct node_if));
> @@ -5868,37 +5874,38 @@ map_tos(char *s, int *val)
> }
>
> int
> -rdomain_exists(u_int rdomain)
> +lookup_rtable(u_int rtableid)
> {
> size_t len;
> struct rt_tableinfo info;
> int mib[6];
> static u_int found[RT_TABLEID_MAX+1];
>
> - if (found[rdomain] == 1)
> - return 1;
> + if (found[rtableid])
> + return found[rtableid];
>
> mib[0] = CTL_NET;
> mib[1] = PF_ROUTE;
> mib[2] = 0;
> mib[3] = 0;
> mib[4] = NET_RT_TABLE;
> - mib[5] = rdomain;
> + mib[5] = rtableid;
>
> len = sizeof(info);
> if (sysctl(mib, 6, , , NULL, 0) == -1) {
> if (errno == ENOENT) {
> /* table nonexistent */
> + found[rtableid] = 0;
> return 0;
> }
> err(1, "%s", __func__);
> }
> - if (info.rti_domainid == rdomain) {
> - found[rdomain] = 1;
> - return 1;
> + if (info.rti_domainid == rtableid) {
> + found[rtableid] = 2;
> + return 2;
> }
> - /* rdomain is a table, but not an rdomain */
> - return 0;
> + found[rtableid] = 1;
> + return 1;
> }
>
> int
Re: diff: pfctl: error message for nonexisting rtable
On Wed, Sep 16, 2020 at 07:49:19PM +0900, YASUOKA Masahiko wrote:
> New diff is using -1 for ENOENT.
>
> Also domainid == 0 is a valid domain id, but previous diff cannot make
> a cache of it since 0 is the default value. So new diff is doing
>
> - static u_int found[RT_TABLEID_MAX+1];
> + static struct {
> + int found;
> + int domainid;
> + }rtables[RT_TABLEID_MAX+1];
>
> to distinguish the default 0 and domainid 0.
This looks more complicated than it needs to be, but I also don't want
to bikeshed it; given that the parser is happy with this and we plan to
remove this code alltogether anyway in the next release cycle: OK kn.
Alternatively, here's a much simpler diff resembling what I had in mind.
Feel free to commit this instead (with my OK), give me an OK for it or
go ahead with yours.
It uses the same function and reflects the fact that every rdomain is a
rtable but not every rtable is also a rdomain (your choice of `domainid'
seems inconsistent with that).
Index: parse.y
===
RCS file: /cvs/src/sbin/pfctl/parse.y,v
retrieving revision 1.701
diff -u -p -r1.701 parse.y
--- parse.y 28 Jan 2020 15:40:35 - 1.701
+++ parse.y 16 Sep 2020 13:58:23 -
@@ -392,7 +392,7 @@ int invalid_redirect(struct node_host *
u_int16_t parseicmpspec(char *, sa_family_t);
int kw_casecmp(const void *, const void *);
int map_tos(char *string, int *);
-int rdomain_exists(u_int);
+int lookup_rtable(u_int);
int filteropts_to_rule(struct pf_rule *, struct filter_opts *);
TAILQ_HEAD(loadanchorshead, loadanchors)
@@ -1216,6 +1216,9 @@ antispoof_opt : LABEL label {
if ($2 < 0 || $2 > RT_TABLEID_MAX) {
yyerror("invalid rtable id");
YYERROR;
+ } else if (lookup_rtable($2) >= 1) {
+ yyerror("rtable %lld does not exist", $2);
+ YYERROR;
}
antispoof_opts.rtableid = $2;
}
@@ -2000,6 +2003,9 @@ filter_opt: USER uids {
if ($2 < 0 || $2 > RT_TABLEID_MAX) {
yyerror("invalid rtable id");
YYERROR;
+ } else if (lookup_rtable($2) >= 1) {
+ yyerror("rtable %lld does not exist", $2);
+ YYERROR;
}
filter_opts.rtableid = $2;
}
@@ -2475,7 +2481,7 @@ if_item : STRING{
| RDOMAIN NUMBER{
if ($2 < 0 || $2 > RT_TABLEID_MAX)
yyerror("rdomain %lld outside range", $2);
- else if (rdomain_exists($2) != 1)
+ else if (lookup_rtable($2) != 2)
yyerror("rdomain %lld does not exist", $2);
$$ = calloc(1, sizeof(struct node_if));
@@ -5868,37 +5874,38 @@ map_tos(char *s, int *val)
}
int
-rdomain_exists(u_int rdomain)
+lookup_rtable(u_int rtableid)
{
size_t len;
struct rt_tableinfo info;
int mib[6];
static u_int found[RT_TABLEID_MAX+1];
- if (found[rdomain] == 1)
- return 1;
+ if (found[rtableid])
+ return found[rtableid];
mib[0] = CTL_NET;
mib[1] = PF_ROUTE;
mib[2] = 0;
mib[3] = 0;
mib[4] = NET_RT_TABLE;
- mib[5] = rdomain;
+ mib[5] = rtableid;
len = sizeof(info);
if (sysctl(mib, 6, , , NULL, 0) == -1) {
if (errno == ENOENT) {
/* table nonexistent */
+ found[rtableid] = 0;
return 0;
}
err(1, "%s", __func__);
}
- if (info.rti_domainid == rdomain) {
- found[rdomain] = 1;
- return 1;
+ if (info.rti_domainid == rtableid) {
+ found[rtableid] = 2;
+ return 2;
}
- /* rdomain is a table, but not an rdomain */
- return 0;
+ found[rtableid] = 1;
+ return 1;
}
int
Re: diff: pfctl: error message for nonexisting rtable
Hi,
On Wed, 16 Sep 2020 12:04:55 +0200
Klemens Nanni wrote:
> Using the function verb would reads a bit clearer/more intuitive,
> i.e.
Yes, "if (!rtable_exists($2))" seems better.
>> @@ -5887,17 +5897,37 @@ rdomain_exists(u_int rdomain)
>>
>> len = sizeof(info);
>> if (sysctl(mib, 6, , , NULL, 0) == -1) {
>> -if (errno == ENOENT) {
>> +if (errno == ENOENT)
>> /* table nonexistent */
>> -return 0;
>> -}
>> -err(1, "%s", __func__);
>> -}
>> -if (info.rti_domainid == rdomain) {
>> -found[rdomain] = 1;
>> +domainid[rdomain] = RT_TABLEID_MAX;
> This does not look correct, RT_TABLEID_MAX (255) is the biggest *valid*
> id, so you cannot use it to denote a nonexistent routing table.
Good catch. Thanks,
> Perhaps use `static int domainid[RT_TABLEID_MAX+1]' and `-1' to reflect
> ENOENT?
New diff is using -1 for ENOENT.
Also domainid == 0 is a valid domain id, but previous diff cannot make
a cache of it since 0 is the default value. So new diff is doing
- static u_int found[RT_TABLEID_MAX+1];
+ static struct {
+ int found;
+ int domainid;
+ }rtables[RT_TABLEID_MAX+1];
to distinguish the default 0 and domainid 0.
ok?
Make pfctl check if the rtable really exists when parsing the config.
Index: sbin/pfctl/parse.y
===
RCS file: /cvs/src/sbin/pfctl/parse.y,v
retrieving revision 1.701
diff -u -p -r1.701 parse.y
--- sbin/pfctl/parse.y 28 Jan 2020 15:40:35 - 1.701
+++ sbin/pfctl/parse.y 16 Sep 2020 10:40:25 -
@@ -392,7 +392,9 @@ int invalid_redirect(struct node_host *
u_int16_t parseicmpspec(char *, sa_family_t);
int kw_casecmp(const void *, const void *);
int map_tos(char *string, int *);
+int get_domainid(u_int);
int rdomain_exists(u_int);
+int rtable_exists(u_int);
int filteropts_to_rule(struct pf_rule *, struct filter_opts *);
TAILQ_HEAD(loadanchorshead, loadanchors)
@@ -1217,6 +1219,10 @@ antispoof_opt: LABEL label {
yyerror("invalid rtable id");
YYERROR;
}
+ else if (!rtable_exists($2)) {
+ yyerror("rtable %lld does not exist", $2);
+ YYERROR;
+ }
antispoof_opts.rtableid = $2;
}
;
@@ -2001,6 +2007,10 @@ filter_opt : USER uids {
yyerror("invalid rtable id");
YYERROR;
}
+ else if (!rtable_exists($2)) {
+ yyerror("rtable %lld does not exist", $2);
+ YYERROR;
+ }
filter_opts.rtableid = $2;
}
| DIVERTTO STRING PORT portplain {
@@ -2475,7 +2485,7 @@ if_item : STRING{
| RDOMAIN NUMBER{
if ($2 < 0 || $2 > RT_TABLEID_MAX)
yyerror("rdomain %lld outside range", $2);
- else if (rdomain_exists($2) != 1)
+ else if (!rdomain_exists($2))
yyerror("rdomain %lld does not exist", $2);
$$ = calloc(1, sizeof(struct node_if));
@@ -5868,36 +5878,60 @@ map_tos(char *s, int *val)
}
int
-rdomain_exists(u_int rdomain)
+get_domainid(u_int rtable)
{
size_t len;
struct rt_tableinfo info;
int mib[6];
- static u_int found[RT_TABLEID_MAX+1];
+ static struct {
+ int found;
+ int domainid;
+ }rtables[RT_TABLEID_MAX+1];
- if (found[rdomain] == 1)
- return 1;
+ if (rtables[rtable].found)
+ return rtables[rtable].domainid;
mib[0] = CTL_NET;
mib[1] = PF_ROUTE;
mib[2] = 0;
mib[3] = 0;
mib[4] = NET_RT_TABLE;
- mib[5] = rdomain;
+ mib[5] = rtable;
len = sizeof(info);
if (sysctl(mib, 6, , , NULL, 0) == -1) {
- if (errno == ENOENT) {
+ if (errno == ENOENT)
/* table nonexistent */
- return 0;
- }
- err(1, "%s", __func__);
- }
- if (info.rti_domainid == rdomain) {
- found[rdomain] = 1;
+ rtables[rtable].domainid = -1;
+ else
+ err(1, "%s", __func__);
+ } else
+ rtables[rtable].domainid = info.rti_domainid;
+
Re: diff: pfctl: error message for nonexisting rtable
On Wed, Sep 16, 2020 at 06:22:00PM +0900, YASUOKA Masahiko wrote:
> Let me continue this separetely.
Yes, let's get your diff in for release and then work out the other
approach.
> Make pfctl check if the rtable really exists when parsing the config.
The diff is a bit hard to read (nothing you can do), but after applying
the code reads good in principal.
> Index: sbin/pfctl/parse.y
> ===
> RCS file: /cvs/src/sbin/pfctl/parse.y,v
> retrieving revision 1.701
> diff -u -p -r1.701 parse.y
> --- sbin/pfctl/parse.y28 Jan 2020 15:40:35 - 1.701
> +++ sbin/pfctl/parse.y16 Sep 2020 09:11:21 -
> @@ -392,7 +392,9 @@ intinvalid_redirect(struct node_host *
> u_int16_t parseicmpspec(char *, sa_family_t);
> int kw_casecmp(const void *, const void *);
> int map_tos(char *string, int *);
> +int get_domainid(u_int);
> int rdomain_exists(u_int);
> +int rtable_exists(u_int);
> int filteropts_to_rule(struct pf_rule *, struct filter_opts *);
>
> TAILQ_HEAD(loadanchorshead, loadanchors)
> @@ -1217,6 +1219,10 @@ antispoof_opt : LABEL label {
> yyerror("invalid rtable id");
> YYERROR;
> }
> + else if (rtable_exists($2) != 1) {
Using the function verb would reads a bit clearer/more intuitive, i.e.
else if (!rtable_exists($2)) {
> + yyerror("rtable %lld does not exist", $2);
> + YYERROR;
> + }
> antispoof_opts.rtableid = $2;
> }
> ;
> @@ -2001,6 +2007,10 @@ filter_opt : USER uids {
> yyerror("invalid rtable id");
> YYERROR;
> }
> + else if (rtable_exists($2) != 1) {
Same here.
> + yyerror("rtable %lld does not exist", $2);
> + YYERROR;
> + }
> filter_opts.rtableid = $2;
> }
> | DIVERTTO STRING PORT portplain {
> @@ -5868,15 +5878,15 @@ map_tos(char *s, int *val)
> }
>
> int
> -rdomain_exists(u_int rdomain)
> +get_domainid(u_int rdomain)
> {
> size_t len;
> struct rt_tableinfo info;
> int mib[6];
> - static u_int found[RT_TABLEID_MAX+1];
> + static u_int domainid[RT_TABLEID_MAX+1];
>
> - if (found[rdomain] == 1)
> - return 1;
> + if (domainid[rdomain] != 0)
> + return domainid[rdomain];
>
> mib[0] = CTL_NET;
> mib[1] = PF_ROUTE;
> @@ -5887,17 +5897,37 @@ rdomain_exists(u_int rdomain)
>
> len = sizeof(info);
> if (sysctl(mib, 6, , , NULL, 0) == -1) {
> - if (errno == ENOENT) {
> + if (errno == ENOENT)
> /* table nonexistent */
> - return 0;
> - }
> - err(1, "%s", __func__);
> - }
> - if (info.rti_domainid == rdomain) {
> - found[rdomain] = 1;
> + domainid[rdomain] = RT_TABLEID_MAX;
This does not look correct, RT_TABLEID_MAX (255) is the biggest *valid*
id, so you cannot use it to denote a nonexistent routing table.
$ doas ifconfig lo255 rdomain 255
$ netstat -R | grep 255
Rdomain 255
Interface: lo255
Routing table: 255
Perhaps use `static int domainid[RT_TABLEID_MAX+1]' and `-1' to reflect
ENOENT?
> + else
> + err(1, "%s", __func__);
> + } else
> + domainid[rdomain] = info.rti_domainid;
> +
> + return domainid[rdomain];
> +}
> +
> +int
> +rdomain_exists(u_int rdomain)
> +{
> + int domainid;
> +
> + domainid = get_domainid(rdomain);
> + if (domainid == rdomain)
> return 1;
> - }
> /* rdomain is a table, but not an rdomain */
> + return 0;
> +}
> +
> +int
> +rtable_exists(u_int rtable)
> +{
> + int domainid;
> +
> + domainid = get_domainid(rtable);
> + if (domainid < RT_TABLEID_MAX)
As per above, RT_TABLEID_MAX is valid.
> + return 1;
> return 0;
> }
Re: diff: pfctl: error message for nonexisting rtable
Hi,
So, it seems we need to more code and test for pf(4) part.
Let me continue this separetely.
On Mon, 14 Sep 2020 11:07:53 +0200
Klemens Nanni wrote:
> On Mon, Sep 14, 2020 at 02:09:27PM +0900, YASUOKA Masahiko wrote:
>> Make pfctl check if the rtable really exists when parsing the config.
> I concur, but you can do this with less (duplicated) code.
>
> Instead of copying rdomain_exists() into rtable_exists() with the
> `rti_domainid' check omitted, tweak (and rename) rdomain_exists() into
> returning the information whether the given ID is just an rtable.
>
> rdomain_exists() merges the "invalid id" and "id is an rtable but not
> an rdmomain" cases - make those separate return codes, check/adjust
> existing callers and use it for your new checks.
Yes, I could reduce the code. Thanks,
ok?
Make pfctl check if the rtable really exists when parsing the config.
Index: sbin/pfctl/parse.y
===
RCS file: /cvs/src/sbin/pfctl/parse.y,v
retrieving revision 1.701
diff -u -p -r1.701 parse.y
--- sbin/pfctl/parse.y 28 Jan 2020 15:40:35 - 1.701
+++ sbin/pfctl/parse.y 16 Sep 2020 09:11:21 -
@@ -392,7 +392,9 @@ int invalid_redirect(struct node_host *
u_int16_t parseicmpspec(char *, sa_family_t);
int kw_casecmp(const void *, const void *);
int map_tos(char *string, int *);
+int get_domainid(u_int);
int rdomain_exists(u_int);
+int rtable_exists(u_int);
int filteropts_to_rule(struct pf_rule *, struct filter_opts *);
TAILQ_HEAD(loadanchorshead, loadanchors)
@@ -1217,6 +1219,10 @@ antispoof_opt: LABEL label {
yyerror("invalid rtable id");
YYERROR;
}
+ else if (rtable_exists($2) != 1) {
+ yyerror("rtable %lld does not exist", $2);
+ YYERROR;
+ }
antispoof_opts.rtableid = $2;
}
;
@@ -2001,6 +2007,10 @@ filter_opt : USER uids {
yyerror("invalid rtable id");
YYERROR;
}
+ else if (rtable_exists($2) != 1) {
+ yyerror("rtable %lld does not exist", $2);
+ YYERROR;
+ }
filter_opts.rtableid = $2;
}
| DIVERTTO STRING PORT portplain {
@@ -5868,15 +5878,15 @@ map_tos(char *s, int *val)
}
int
-rdomain_exists(u_int rdomain)
+get_domainid(u_int rdomain)
{
size_t len;
struct rt_tableinfo info;
int mib[6];
- static u_int found[RT_TABLEID_MAX+1];
+ static u_int domainid[RT_TABLEID_MAX+1];
- if (found[rdomain] == 1)
- return 1;
+ if (domainid[rdomain] != 0)
+ return domainid[rdomain];
mib[0] = CTL_NET;
mib[1] = PF_ROUTE;
@@ -5887,17 +5897,37 @@ rdomain_exists(u_int rdomain)
len = sizeof(info);
if (sysctl(mib, 6, , , NULL, 0) == -1) {
- if (errno == ENOENT) {
+ if (errno == ENOENT)
/* table nonexistent */
- return 0;
- }
- err(1, "%s", __func__);
- }
- if (info.rti_domainid == rdomain) {
- found[rdomain] = 1;
+ domainid[rdomain] = RT_TABLEID_MAX;
+ else
+ err(1, "%s", __func__);
+ } else
+ domainid[rdomain] = info.rti_domainid;
+
+ return domainid[rdomain];
+}
+
+int
+rdomain_exists(u_int rdomain)
+{
+ int domainid;
+
+ domainid = get_domainid(rdomain);
+ if (domainid == rdomain)
return 1;
- }
/* rdomain is a table, but not an rdomain */
+ return 0;
+}
+
+int
+rtable_exists(u_int rtable)
+{
+ int domainid;
+
+ domainid = get_domainid(rtable);
+ if (domainid < RT_TABLEID_MAX)
+ return 1;
return 0;
}
Re: diff: pfctl: error message for nonexisting rtable
On Tue, Sep 15, 2020 at 12:42:27PM +0900, YASUOKA Masahiko wrote: > It's not clear for me why non-existing rdomain is accepted but > non-existing rtable is rejected. I suppose we can make pf(4) can > handle a packet for the non-existing routing table as if the routing > table is empty. Probably possible, but not without further tests or even changes to pf; I did not want to imply that dynamic `rtable' in pf.conf cannot be done.
Re: diff: pfctl: error message for nonexisting rtable
Hi, On Tue, 15 Sep 2020 02:31:24 +0200 Klemens Nanni wrote: > On Tue, Sep 15, 2020 at 12:30:35AM +0200, Klemens Nanni wrote: >> Actually, that should just work regardless of whether the rounting >> domain exists at ruleset creation time; just like it is the case with >> interface names/groups which may come and go at runtime without >> requiring changes to the ruleset. >> >> Rules on nonexistent interfaces won't match, routing domains (and >> ultimately routing tables) should behave the same, I think. >> >> Here's a diff that does this for routing domains allowing me to always >> use `on rdomain 5' - I've tested it with a few examplatory rulesets and >> behaviour is as expected. >> >> It will need more eye balling and I am not pushing such changes before >> release, but if that is a general direction we agree, your proposed >> `rtable' fix could move along and become just as flexible instead. > More on this: > > # ifconfig lo1 rdomain 1 > # echo pass on rdomain 1 | pfctl -f- > # ifconfig lo1 destroy > # pfctl -sr > > pass on rdomain 1 all flags S/SA > > The ruleset stays valid and continues to work as soon as routing domain > `1' reappears, there is no reason to require existence of it at ruleset > creation; this is safe because routing domains are just normative > numbers, there's no further state when it comes to filtering - either > the id on the packet matches the number in the ruleset or it doesn't. > > Routing tables however are more involved as they can be used to *alter* > a packet's flow in pf.conf(5), so requiring them to be present at > ruleset creation makes sense to guarantee that pf will only ever change > routing table ids to valid ones. It's not clear for me why non-existing rdomain is accepted but non-existing rtable is rejected. I suppose we can make pf(4) can handle a packet for the non-existing routing table as if the routing table is empty. > Routing domains can be deleted, but that doesn't invalidate rules like > `on rdomain 1', which simply won't match when the given id does not > exist. > > Routing tables however cannot be deleted, they get moved to the default > routing domain whenever their corresponding routing domain disappears; > this is in line with only ever loading valid routing table ids into pf. > > So unless I missed something, that ruleset creation (`pfctl -f ...') > is the only occasion pf actually needs to validate routing table ids: > they are guaranteed to always exist from then on. > > Given this, my diff looks fine as is and should not change `rtable' > behaviour - YASUOKA's diff is also fine as is and actually implements > the validity check I just mentioned, obsoleting my initial feedback.
Re: diff: pfctl: error message for nonexisting rtable
On Tue, Sep 15, 2020 at 12:30:35AM +0200, Klemens Nanni wrote: > Actually, that should just work regardless of whether the rounting > domain exists at ruleset creation time; just like it is the case with > interface names/groups which may come and go at runtime without > requiring changes to the ruleset. > > Rules on nonexistent interfaces won't match, routing domains (and > ultimately routing tables) should behave the same, I think. > > Here's a diff that does this for routing domains allowing me to always > use `on rdomain 5' - I've tested it with a few examplatory rulesets and > behaviour is as expected. > > It will need more eye balling and I am not pushing such changes before > release, but if that is a general direction we agree, your proposed > `rtable' fix could move along and become just as flexible instead. More on this: # ifconfig lo1 rdomain 1 # echo pass on rdomain 1 | pfctl -f- # ifconfig lo1 destroy # pfctl -sr pass on rdomain 1 all flags S/SA The ruleset stays valid and continues to work as soon as routing domain `1' reappears, there is no reason to require existence of it at ruleset creation; this is safe because routing domains are just normative numbers, there's no further state when it comes to filtering - either the id on the packet matches the number in the ruleset or it doesn't. Routing tables however are more involved as they can be used to *alter* a packet's flow in pf.conf(5), so requiring them to be present at ruleset creation makes sense to guarantee that pf will only ever change routing table ids to valid ones. Routing domains can be deleted, but that doesn't invalidate rules like `on rdomain 1', which simply won't match when the given id does not exist. Routing tables however cannot be deleted, they get moved to the default routing domain whenever their corresponding routing domain disappears; this is in line with only ever loading valid routing table ids into pf. So unless I missed something, that ruleset creation (`pfctl -f ...') is the only occasion pf actually needs to validate routing table ids: they are guaranteed to always exist from then on. Given this, my diff looks fine as is and should not change `rtable' behaviour - YASUOKA's diff is also fine as is and actually implements the validity check I just mentioned, obsoleting my initial feedback.
Re: diff: pfctl: error message for nonexisting rtable
On Mon, Sep 14, 2020 at 02:09:27PM +0900, YASUOKA Masahiko wrote:
> When pf rule with a "on rdomain n" with nonexisting rdomain n causes
>
> /etc/pf.conf:XXX: rdomain n does not exist
Actually, that should just work regardless of whether the rounting
domain exists at ruleset creation time; just like it is the case with
interface names/groups which may come and go at runtime without
requiring changes to the ruleset.
Rules on nonexistent interfaces won't match, routing domains (and
ultimately routing tables) should behave the same, I think.
Here's a diff that does this for routing domains allowing me to always
use `on rdomain 5' - I've tested it with a few examplatory rulesets and
behaviour is as expected.
It will need more eye balling and I am not pushing such changes before
release, but if that is a general direction we agree, your proposed
`rtable' fix could move along and become just as flexible instead.
Discussed with claudio at k2k20.
Index: /sys/net/pf_ioctl.c
===
RCS file: /cvs/src/sys/net/pf_ioctl.c,v
retrieving revision 1.356
diff -u -p -r1.356 pf_ioctl.c
--- /sys/net/pf_ioctl.c 24 Aug 2020 15:41:15 - 1.356
+++ /sys/net/pf_ioctl.c 14 Sep 2020 22:27:55 -
@@ -2820,10 +2820,8 @@ pf_rule_copyin(struct pf_rule *from, str
if (to->rtableid >= 0 && !rtable_exists(to->rtableid))
return (EBUSY);
to->onrdomain = from->onrdomain;
- if (to->onrdomain >= 0 && !rtable_exists(to->onrdomain))
- return (EBUSY);
- if (to->onrdomain >= 0) /* make sure it is a real rdomain */
- to->onrdomain = rtable_l2(to->onrdomain);
+ if (to->rtableid < 0 || to->rtableid > RT_TABLEID_MAX)
+ return (EINVAL);
for (i = 0; i < PFTM_MAX; i++)
to->timeout[i] = from->timeout[i];
Index: sbin/pfctl/parse.y
===
RCS file: /cvs/src/sbin/pfctl/parse.y,v
retrieving revision 1.701
diff -u -p -r1.701 parse.y
--- sbin/pfctl/parse.y 28 Jan 2020 15:40:35 - 1.701
+++ sbin/pfctl/parse.y 14 Sep 2020 21:52:54 -
@@ -392,7 +392,6 @@ int invalid_redirect(struct node_host *
u_int16_t parseicmpspec(char *, sa_family_t);
int kw_casecmp(const void *, const void *);
int map_tos(char *string, int *);
-int rdomain_exists(u_int);
int filteropts_to_rule(struct pf_rule *, struct filter_opts *);
TAILQ_HEAD(loadanchorshead, loadanchors)
@@ -2475,8 +2474,6 @@ if_item : STRING{
| RDOMAIN NUMBER{
if ($2 < 0 || $2 > RT_TABLEID_MAX)
yyerror("rdomain %lld outside range", $2);
- else if (rdomain_exists($2) != 1)
- yyerror("rdomain %lld does not exist", $2);
$$ = calloc(1, sizeof(struct node_if));
if ($$ == NULL)
@@ -5865,40 +5862,6 @@ map_tos(char *s, int *val)
return (1);
}
return (0);
-}
-
-int
-rdomain_exists(u_int rdomain)
-{
- size_t len;
- struct rt_tableinfo info;
- int mib[6];
- static u_int found[RT_TABLEID_MAX+1];
-
- if (found[rdomain] == 1)
- return 1;
-
- mib[0] = CTL_NET;
- mib[1] = PF_ROUTE;
- mib[2] = 0;
- mib[3] = 0;
- mib[4] = NET_RT_TABLE;
- mib[5] = rdomain;
-
- len = sizeof(info);
- if (sysctl(mib, 6, , , NULL, 0) == -1) {
- if (errno == ENOENT) {
- /* table nonexistent */
- return 0;
- }
- err(1, "%s", __func__);
- }
- if (info.rti_domainid == rdomain) {
- found[rdomain] = 1;
- return 1;
- }
- /* rdomain is a table, but not an rdomain */
- return 0;
}
int
Re: diff: pfctl: error message for nonexisting rtable
On Mon, Sep 14, 2020 at 02:09:27PM +0900, YASUOKA Masahiko wrote: > Make pfctl check if the rtable really exists when parsing the config. I concur, but you can do this with less (duplicated) code. Instead of copying rdomain_exists() into rtable_exists() with the `rti_domainid' check omitted, tweak (and rename) rdomain_exists() into returning the information whether the given ID is just an rtable. rdomain_exists() merges the "invalid id" and "id is an rtable but not an rdmomain" cases - make those separate return codes, check/adjust existing callers and use it for your new checks.
