diff: pfctl: error message for nonexisting rtable

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

diff: pfctl: error message for nonexisting rtable

YASUOKA Masahiko-3
Hi,

When pf rule with a "on rdomain n" with nonexisting rdomain n causes

  /etc/pf.conf:XXX: rdomain n does not exist

error.  But with a "rtable n" with nonexisting rtable n will cause

  pfctl: DIOCADDRULE: Device busy

error.  It is hard to find the cause by this error message.

  /etc/pf.conf:XXX: rtable n does not exist

is better.  

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 -0000 1.701
+++ sbin/pfctl/parse.y 14 Sep 2020 04:54:39 -0000
@@ -393,6 +393,7 @@ u_int16_t parseicmpspec(char *, sa_famil
 int kw_casecmp(const void *, const void *);
 int map_tos(char *string, 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 +1218,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 +2006,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 {
@@ -5899,6 +5908,36 @@ rdomain_exists(u_int rdomain)
  }
  /* rdomain is a table, but not an rdomain */
  return 0;
+}
+
+int
+rtable_exists(u_int rtable)
+{
+ size_t len;
+ struct rt_tableinfo info;
+ int mib[6];
+ static u_int found[RT_TABLEID_MAX+1];
+
+ if (found[rtable] == 1)
+ return 1;
+
+ mib[0] = CTL_NET;
+ mib[1] = PF_ROUTE;
+ mib[2] = 0;
+ mib[3] = 0;
+ mib[4] = NET_RT_TABLE;
+ mib[5] = rtable;
+
+ len = sizeof(info);
+ if (sysctl(mib, 6, &info, &len, NULL, 0) == -1) {
+ if (errno == ENOENT) {
+ /* table nonexistent */
+ return 0;
+ }
+ err(1, "%s", __func__);
+ }
+ found[rtable] = 1;
+ return 1;
 }
 
 int

Reply | Threaded
Open this post in threaded view
|

Re: diff: pfctl: error message for nonexisting rtable

Klemens Nanni-2
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.

Reply | Threaded
Open this post in threaded view
|

Re: diff: pfctl: error message for nonexisting rtable

Klemens Nanni-2
In reply to this post by YASUOKA Masahiko-3
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 -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.701
diff -u -p -r1.701 parse.y
--- sbin/pfctl/parse.y 28 Jan 2020 15:40:35 -0000 1.701
+++ sbin/pfctl/parse.y 14 Sep 2020 21:52:54 -0000
@@ -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, &info, &len, 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

Reply | Threaded
Open this post in threaded view
|

Re: diff: pfctl: error message for nonexisting rtable

Klemens Nanni-2
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.

Reply | Threaded
Open this post in threaded view
|

Re: diff: pfctl: error message for nonexisting rtable

YASUOKA Masahiko-3
Hi,

On Tue, 15 Sep 2020 02:31:24 +0200
Klemens Nanni <[hidden email]> 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.

Reply | Threaded
Open this post in threaded view
|

Re: diff: pfctl: error message for nonexisting rtable

Klemens Nanni-2
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.

Reply | Threaded
Open this post in threaded view
|

Re: diff: pfctl: error message for nonexisting rtable

YASUOKA Masahiko-3
In reply to this post by Klemens Nanni-2
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 <[hidden email]> 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 -0000 1.701
+++ sbin/pfctl/parse.y 16 Sep 2020 09:11:21 -0000
@@ -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, &info, &len, 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;
 }
 

Reply | Threaded
Open this post in threaded view
|

Re: diff: pfctl: error message for nonexisting rtable

Klemens Nanni-2
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.y 28 Jan 2020 15:40:35 -0000 1.701
> +++ sbin/pfctl/parse.y 16 Sep 2020 09:11:21 -0000
> @@ -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) {
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, &info, &len, 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;
>  }

Reply | Threaded
Open this post in threaded view
|

Re: diff: pfctl: error message for nonexisting rtable

YASUOKA Masahiko-3
Hi,

On Wed, 16 Sep 2020 12:04:55 +0200
Klemens Nanni <[hidden email]> 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, &info, &len, 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 -0000 1.701
+++ sbin/pfctl/parse.y 16 Sep 2020 10:40:25 -0000
@@ -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, &info, &len, 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;
+ rtables[rtable].found = 1;
+
+ return rtables[rtable].domainid;
+}
+
+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 >= 0)
+ return 1;
  return 0;
 }
 

Reply | Threaded
Open this post in threaded view
|

Re: diff: pfctl: error message for nonexisting rtable

Klemens Nanni-2
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 -0000 1.701
+++ parse.y 16 Sep 2020 13:58:23 -0000
@@ -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, &info, &len, 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

Reply | Threaded
Open this post in threaded view
|

Re: diff: pfctl: error message for nonexisting rtable

YASUOKA Masahiko-3
Hi,

I just committed yours.

Thanks,

On Wed, 16 Sep 2020 16:07:40 +0200
Klemens Nanni <[hidden email]> 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 -0000 1.701
> +++ parse.y 16 Sep 2020 13:58:23 -0000
> @@ -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, &info, &len, 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

Reply | Threaded
Open this post in threaded view
|

Re: diff: pfctl: error message for nonexisting rtable

YASUOKA Masahiko-4
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 -0000 1.702
+++ parse.y 17 Sep 2020 14:23:42 -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) < 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;
  }