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 -0000 1.356
+++ sys/net/pf_ioctl.c 14 Sep 2020 22:27:55 -0000
@@ -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 -0000 1.703
+++ sbin/pfctl/parse.y 20 Sep 2020 17:28:10 -0000
@@ -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;
