introduce 'pfctl -FR' to reset settings to defaults

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

introduce 'pfctl -FR' to reset settings to defaults

Alexandr Nedvedicky
Hello,

tedu@ has planted idea for diff below here [1].  That particular email is part
of thread [2], where various cleanup/unconfigure options for PF are discussed.
To keep progressing in small steps I've decided to factor out the first diff
here, which introduces '-FR' (a.k.a. reset settings) for pfctl(8).

OK?

thanks and
regards
sashan

[1] https://marc.info/?l=openbsd-tech&m=155356735115005&w=2

[2] https://marc.info/?l=openbsd-tech&m=155341612701577&w=2
    [ this is a good start point where to gather the context ]

--------8<---------------8<---------------8<------------------8<--------
diff --git a/sbin/pfctl/pfctl.8 b/sbin/pfctl/pfctl.8
index 48b2893cfcd..ab1693e5854 100644
--- a/sbin/pfctl/pfctl.8
+++ b/sbin/pfctl/pfctl.8
@@ -197,8 +197,10 @@ Flush the filter information (statistics that are not bound to rules).
 Flush the tables.
 .It Fl F Cm osfp
 Flush the passive operating system fingerprints.
+.It Fl F Cm Reset
+Reset limits, timeouts and options back to default settings.
 .It Fl F Cm all
-Flush all of the above.
+Flush all of the above (+ reset settings).
 .El
 .It Fl f Ar file
 Replace the current ruleset with
diff --git a/sbin/pfctl/pfctl.c b/sbin/pfctl/pfctl.c
index 493ff47af2f..a6cf265451c 100644
--- a/sbin/pfctl/pfctl.c
+++ b/sbin/pfctl/pfctl.c
@@ -105,6 +105,7 @@ int pfctl_load_rule(struct pfctl *, char *, struct pf_rule *, int);
 const char *pfctl_lookup_option(char *, const char **);
 void pfctl_state_store(int, const char *);
 void pfctl_state_load(int, const char *);
+void pfctl_restore_defaults(int, int);
 
 const char *clearopt;
 char *rulesopt;
@@ -205,7 +206,8 @@ static const struct {
 };
 
 static const char *clearopt_list[] = {
- "rules", "Sources", "states", "info", "Tables", "osfp", "all", NULL
+ "rules", "Sources", "states", "info", "Tables", "osfp", "Reset",
+ "all", NULL
 };
 
 static const char *showopt_list[] = {
@@ -2232,6 +2234,40 @@ pfctl_state_load(int dev, const char *file)
  fclose(f);
 }
 
+void
+pfctl_restore_defaults(int dev, int opts)
+{
+ struct pfctl pf;
+ struct pfr_buffer t;
+ int i;
+
+ pf.dev = dev;
+ pfctl_init_options(&pf);
+
+ pf.debug_set = 1;
+ pf.reass_set = 1;
+ pf.syncookieswat_set = 1;
+ pf.ifname = strdup("none");
+ pf.ifname_set = 1;
+
+ memset(&t, 0, sizeof(t));
+ t.pfrb_type = PFRB_TRANS;
+ if (pfctl_trans(dev, &t, DIOCXBEGIN, 0))
+ warn("%s, DIOCXBEGIN", __func__);
+
+
+ for (i = 0; pf_limits[i].name; i++)
+ pf.limit_set[pf_limits[i].index] = 1;
+
+ for (i = 0; pf_timeouts[i].name; i++)
+ pf.timeout_set[pf_timeouts[i].timeout] = 1;
+
+ pfctl_load_options(&pf);
+
+ if (pfctl_trans(dev, &t, DIOCXCOMMIT, 0))
+ warn("%s, DIOCXCOMMIT", __func__);
+}
+
 int
 main(int argc, char *argv[])
 {
@@ -2558,6 +2594,7 @@ main(int argc, char *argv[])
  pfctl_clear_stats(dev, ifaceopt, opts);
  pfctl_clear_fingerprints(dev, opts);
  pfctl_clear_interface_flags(dev, opts);
+ pfctl_restore_defaults(dev, opts);
  }
  break;
  case 'o':
@@ -2566,6 +2603,9 @@ main(int argc, char *argv[])
  case 'T':
  pfctl_clear_tables(anchorname, opts);
  break;
+ case 'R':
+ pfctl_restore_defaults(dev, opts);
+ break;
  }
  }
  if (state_killers) {

Reply | Threaded
Open this post in threaded view
|

Re: introduce 'pfctl -FR' to reset settings to defaults

Klemens Nanni-2
On Wed, Mar 27, 2019 at 02:17:03AM +0100, Alexandr Nedvedicky wrote:
> tedu@ has planted idea for diff below here [1].  That particular email is part
> of thread [2], where various cleanup/unconfigure options for PF are discussed.
> To keep progressing in small steps I've decided to factor out the first diff
> here, which introduces '-FR' (a.k.a. reset settings) for pfctl(8).
A bit late, but I generally agree on "reset" and reusing `-F'.

Diff looks sane to me, two comments:
 
> -Flush all of the above.
> +Flush all of the above (+ reset settings).
This is fine as is, I think.

> +void pfctl_restore_defaults(int, int);
Why not simply pfctl_reset()?

Reply | Threaded
Open this post in threaded view
|

Re: introduce 'pfctl -FR' to reset settings to defaults

Alexandr Nedvedicky
Hello,

</snip>

> > +Flush all of the above (+ reset settings).
> This is fine as is, I think.
>
> > +void pfctl_restore_defaults(int, int);
> Why not simply pfctl_reset()?

    I had used pfctl_reset() at some point of history, then
    I stopped to like it. Now I like it again.

updated diff below.

thanks and
regards
sashan

--------8<---------------8<---------------8<------------------8<--------
diff --git a/sbin/pfctl/pfctl.8 b/sbin/pfctl/pfctl.8
index 48b2893cfcd..00bd27c200a 100644
--- a/sbin/pfctl/pfctl.8
+++ b/sbin/pfctl/pfctl.8
@@ -197,6 +197,8 @@ Flush the filter information (statistics that are not bound to rules).
 Flush the tables.
 .It Fl F Cm osfp
 Flush the passive operating system fingerprints.
+.It Fl F Cm Reset
+Reset limits, timeouts and options back to default settings.
 .It Fl F Cm all
 Flush all of the above.
 .El
diff --git a/sbin/pfctl/pfctl.c b/sbin/pfctl/pfctl.c
index 493ff47af2f..1932649defc 100644
--- a/sbin/pfctl/pfctl.c
+++ b/sbin/pfctl/pfctl.c
@@ -105,6 +105,7 @@ int pfctl_load_rule(struct pfctl *, char *, struct pf_rule *, int);
 const char *pfctl_lookup_option(char *, const char **);
 void pfctl_state_store(int, const char *);
 void pfctl_state_load(int, const char *);
+void pfctl_reset(int, int);
 
 const char *clearopt;
 char *rulesopt;
@@ -205,7 +206,8 @@ static const struct {
 };
 
 static const char *clearopt_list[] = {
- "rules", "Sources", "states", "info", "Tables", "osfp", "all", NULL
+ "rules", "Sources", "states", "info", "Tables", "osfp", "Reset",
+ "all", NULL
 };
 
 static const char *showopt_list[] = {
@@ -2232,6 +2234,40 @@ pfctl_state_load(int dev, const char *file)
  fclose(f);
 }
 
+void
+pfctl_reset(int dev, int opts)
+{
+ struct pfctl pf;
+ struct pfr_buffer t;
+ int i;
+
+ pf.dev = dev;
+ pfctl_init_options(&pf);
+
+ pf.debug_set = 1;
+ pf.reass_set = 1;
+ pf.syncookieswat_set = 1;
+ pf.ifname = strdup("none");
+ pf.ifname_set = 1;
+
+ memset(&t, 0, sizeof(t));
+ t.pfrb_type = PFRB_TRANS;
+ if (pfctl_trans(dev, &t, DIOCXBEGIN, 0))
+ warn("%s, DIOCXBEGIN", __func__);
+
+
+ for (i = 0; pf_limits[i].name; i++)
+ pf.limit_set[pf_limits[i].index] = 1;
+
+ for (i = 0; pf_timeouts[i].name; i++)
+ pf.timeout_set[pf_timeouts[i].timeout] = 1;
+
+ pfctl_load_options(&pf);
+
+ if (pfctl_trans(dev, &t, DIOCXCOMMIT, 0))
+ warn("%s, DIOCXCOMMIT", __func__);
+}
+
 int
 main(int argc, char *argv[])
 {
@@ -2558,6 +2594,7 @@ main(int argc, char *argv[])
  pfctl_clear_stats(dev, ifaceopt, opts);
  pfctl_clear_fingerprints(dev, opts);
  pfctl_clear_interface_flags(dev, opts);
+ pfctl_reset(dev, opts);
  }
  break;
  case 'o':
@@ -2566,6 +2603,9 @@ main(int argc, char *argv[])
  case 'T':
  pfctl_clear_tables(anchorname, opts);
  break;
+ case 'R':
+ pfctl_reset(dev, opts);
+ break;
  }
  }
  if (state_killers) {

Reply | Threaded
Open this post in threaded view
|

Re: introduce 'pfctl -FR' to reset settings to defaults

Alexandr Nedvedicky
Hello,

below is diff I plan to commit. I did add a comment to pfctl_reset()
and wording in manpage.

thanks and
regards
sashan

--------8<---------------8<---------------8<---------------8<-----------
diff --git a/sbin/pfctl/pfctl.8 b/sbin/pfctl/pfctl.8
index 48b2893cfcd..00bd27c200a 100644
--- a/sbin/pfctl/pfctl.8
+++ b/sbin/pfctl/pfctl.8
@@ -197,6 +197,8 @@ Flush the filter information (statistics that are not bound to rules).
 Flush the tables.
 .It Fl F Cm osfp
 Flush the passive operating system fingerprints.
+.It Fl F Cm Reset
+Reset limits, timeouts and options back to default settings.
 .It Fl F Cm all
 Flush all of the above.
 .El
diff --git a/sbin/pfctl/pfctl.c b/sbin/pfctl/pfctl.c
index 493ff47af2f..40929d90530 100644
--- a/sbin/pfctl/pfctl.c
+++ b/sbin/pfctl/pfctl.c
@@ -105,6 +105,7 @@ int pfctl_load_rule(struct pfctl *, char *, struct pf_rule *, int);
 const char *pfctl_lookup_option(char *, const char **);
 void pfctl_state_store(int, const char *);
 void pfctl_state_load(int, const char *);
+void pfctl_reset(int, int);
 
 const char *clearopt;
 char *rulesopt;
@@ -205,7 +206,8 @@ static const struct {
 };
 
 static const char *clearopt_list[] = {
- "rules", "Sources", "states", "info", "Tables", "osfp", "all", NULL
+ "rules", "Sources", "states", "info", "Tables", "osfp", "Reset",
+ "all", NULL
 };
 
 static const char *showopt_list[] = {
@@ -2232,6 +2234,41 @@ pfctl_state_load(int dev, const char *file)
  fclose(f);
 }
 
+void
+pfctl_reset(int dev, int opts)
+{
+ struct pfctl pf;
+ struct pfr_buffer t;
+ int i;
+
+ pf.dev = dev;
+ pfctl_init_options(&pf);
+
+ /* Force reset upon pfctl_load_options() */
+ pf.debug_set = 1;
+ pf.reass_set = 1;
+ pf.syncookieswat_set = 1;
+ pf.ifname = strdup("none");
+ pf.ifname_set = 1;
+
+ memset(&t, 0, sizeof(t));
+ t.pfrb_type = PFRB_TRANS;
+ if (pfctl_trans(dev, &t, DIOCXBEGIN, 0))
+ warn("%s, DIOCXBEGIN", __func__);
+
+
+ for (i = 0; pf_limits[i].name; i++)
+ pf.limit_set[pf_limits[i].index] = 1;
+
+ for (i = 0; pf_timeouts[i].name; i++)
+ pf.timeout_set[pf_timeouts[i].timeout] = 1;
+
+ pfctl_load_options(&pf);
+
+ if (pfctl_trans(dev, &t, DIOCXCOMMIT, 0))
+ warn("%s, DIOCXCOMMIT", __func__);
+}
+
 int
 main(int argc, char *argv[])
 {
@@ -2558,6 +2595,7 @@ main(int argc, char *argv[])
  pfctl_clear_stats(dev, ifaceopt, opts);
  pfctl_clear_fingerprints(dev, opts);
  pfctl_clear_interface_flags(dev, opts);
+ pfctl_reset(dev, opts);
  }
  break;
  case 'o':
@@ -2566,6 +2604,9 @@ main(int argc, char *argv[])
  case 'T':
  pfctl_clear_tables(anchorname, opts);
  break;
+ case 'R':
+ pfctl_reset(dev, opts);
+ break;
  }
  }
  if (state_killers) {

Reply | Threaded
Open this post in threaded view
|

Re: introduce 'pfctl -FR' to reset settings to defaults

Petr Hoffmann-2
Hi,

seeing this in the manpage
--8<--------------------------------------------------------------------------
+.It Fl F Cm Reset
+Reset limits, timeouts and options back to default settings.
-------------------------------------------------------------------------->8--
would make me believe everything mentioned as OPTIONS in pf.conf(5) is
about to be reset. I see e.g. the debug level is reset, but what about
the other stuff like fingerprints, 'skip on' and other options set via
the 'set' command? Maybe the manpage should be more precise here?

PH

On 02.04.2019 9:40, Alexandr Nedvedicky wrote:

> Hello,
>
> below is diff I plan to commit. I did add a comment to pfctl_reset()
> and wording in manpage.
>
> thanks and
> regards
> sashan
>
> --------8<---------------8<---------------8<---------------8<-----------
> diff --git a/sbin/pfctl/pfctl.8 b/sbin/pfctl/pfctl.8
> index 48b2893cfcd..00bd27c200a 100644
> --- a/sbin/pfctl/pfctl.8
> +++ b/sbin/pfctl/pfctl.8
> @@ -197,6 +197,8 @@ Flush the filter information (statistics that are not bound to rules).
>   Flush the tables.
>   .It Fl F Cm osfp
>   Flush the passive operating system fingerprints.
> +.It Fl F Cm Reset
> +Reset limits, timeouts and options back to default settings.
>   .It Fl F Cm all
>   Flush all of the above.
>   .El
> diff --git a/sbin/pfctl/pfctl.c b/sbin/pfctl/pfctl.c
> index 493ff47af2f..40929d90530 100644
> --- a/sbin/pfctl/pfctl.c
> +++ b/sbin/pfctl/pfctl.c
> @@ -105,6 +105,7 @@ int pfctl_load_rule(struct pfctl *, char *, struct pf_rule *, int);
>   const char *pfctl_lookup_option(char *, const char **);
>   void pfctl_state_store(int, const char *);
>   void pfctl_state_load(int, const char *);
> +void pfctl_reset(int, int);
>  
>   const char *clearopt;
>   char *rulesopt;
> @@ -205,7 +206,8 @@ static const struct {
>   };
>  
>   static const char *clearopt_list[] = {
> - "rules", "Sources", "states", "info", "Tables", "osfp", "all", NULL
> + "rules", "Sources", "states", "info", "Tables", "osfp", "Reset",
> + "all", NULL
>   };
>  
>   static const char *showopt_list[] = {
> @@ -2232,6 +2234,41 @@ pfctl_state_load(int dev, const char *file)
>   fclose(f);
>   }
>  
> +void
> +pfctl_reset(int dev, int opts)
> +{
> + struct pfctl pf;
> + struct pfr_buffer t;
> + int i;
> +
> + pf.dev = dev;
> + pfctl_init_options(&pf);
> +
> + /* Force reset upon pfctl_load_options() */
> + pf.debug_set = 1;
> + pf.reass_set = 1;
> + pf.syncookieswat_set = 1;
> + pf.ifname = strdup("none");
> + pf.ifname_set = 1;
> +
> + memset(&t, 0, sizeof(t));
> + t.pfrb_type = PFRB_TRANS;
> + if (pfctl_trans(dev, &t, DIOCXBEGIN, 0))
> + warn("%s, DIOCXBEGIN", __func__);
> +
> +
> + for (i = 0; pf_limits[i].name; i++)
> + pf.limit_set[pf_limits[i].index] = 1;
> +
> + for (i = 0; pf_timeouts[i].name; i++)
> + pf.timeout_set[pf_timeouts[i].timeout] = 1;
> +
> + pfctl_load_options(&pf);
> +
> + if (pfctl_trans(dev, &t, DIOCXCOMMIT, 0))
> + warn("%s, DIOCXCOMMIT", __func__);
> +}
> +
>   int
>   main(int argc, char *argv[])
>   {
> @@ -2558,6 +2595,7 @@ main(int argc, char *argv[])
>   pfctl_clear_stats(dev, ifaceopt, opts);
>   pfctl_clear_fingerprints(dev, opts);
>   pfctl_clear_interface_flags(dev, opts);
> + pfctl_reset(dev, opts);
>   }
>   break;
>   case 'o':
> @@ -2566,6 +2604,9 @@ main(int argc, char *argv[])
>   case 'T':
>   pfctl_clear_tables(anchorname, opts);
>   break;
> + case 'R':
> + pfctl_reset(dev, opts);
> + break;
>   }
>   }
>   if (state_killers) {
>

Reply | Threaded
Open this post in threaded view
|

Re: introduce 'pfctl -FR' to reset settings to defaults

Klemens Nanni-2
On Tue, Apr 02, 2019 at 11:28:43AM +0200, Petr Hoffmann wrote:
> would make me believe everything mentioned as OPTIONS in pf.conf(5) is about
> to be reset. I see e.g. the debug level is reset, but what about the other
> stuff like fingerprints, 'skip on' and other options set via the 'set'
> command? Maybe the manpage should be more precise here?
Seems fine to me, given that a) some options are not persisted in the
driver but only effective during ruleset parsing and b) stuff like
fingerprints is already flushed separately, see pfctl(8) `-F osfp'.

Reply | Threaded
Open this post in threaded view
|

Re: introduce 'pfctl -FR' to reset settings to defaults

Petr Hoffmann-2
On 02.04.2019 12:06, Klemens Nanni wrote:
> On Tue, Apr 02, 2019 at 11:28:43AM +0200, Petr Hoffmann wrote:
>> would make me believe everything mentioned as OPTIONS in pf.conf(5) is about
>> to be reset. I see e.g. the debug level is reset, but what about the other
>> stuff like fingerprints, 'skip on' and other options set via the 'set'
>> command? Maybe the manpage should be more precise here?
> Seems fine to me, given that a) some options are not persisted in the
> driver but only effective during ruleset parsing and b) stuff like
> fingerprints is already flushed separately, see pfctl(8) `-F osfp'.
For me, forcing the user to think what is meant by 'options' is not very
friendly, though I understand the idea behind *some* options being used
only while parsing. Let's assume I'm the smart user who is able to
distinguish them. But then, 'set skip on' is the persistent one, right,
but still not reset, I guess.

Petr


On 02.04.2019 12:06, Klemens Nanni wrote:
> On Tue, Apr 02, 2019 at 11:28:43AM +0200, Petr Hoffmann wrote:
>> would make me believe everything mentioned as OPTIONS in pf.conf(5) is about
>> to be reset. I see e.g. the debug level is reset, but what about the other
>> stuff like fingerprints, 'skip on' and other options set via the 'set'
>> command? Maybe the manpage should be more precise here?
> Seems fine to me, given that a) some options are not persisted in the
> driver but only effective during ruleset parsing and b) stuff like
> fingerprints is already flushed separately, see pfctl(8) `-F osfp'.
>

Reply | Threaded
Open this post in threaded view
|

Re: introduce 'pfctl -FR' to reset settings to defaults

Alexandr Nedvedicky
Hello,


On Tue, Apr 02, 2019 at 12:59:33PM +0200, Petr Hoffmann wrote:

> On 02.04.2019 12:06, Klemens Nanni wrote:
> >On Tue, Apr 02, 2019 at 11:28:43AM +0200, Petr Hoffmann wrote:
> >>would make me believe everything mentioned as OPTIONS in pf.conf(5) is about
> >>to be reset. I see e.g. the debug level is reset, but what about the other
> >>stuff like fingerprints, 'skip on' and other options set via the 'set'
> >>command? Maybe the manpage should be more precise here?
> >Seems fine to me, given that a) some options are not persisted in the
> >driver but only effective during ruleset parsing and b) stuff like
> >fingerprints is already flushed separately, see pfctl(8) `-F osfp'.
> For me, forcing the user to think what is meant by 'options' is not
> very friendly, though I understand the idea behind *some* options
> being used only while parsing. Let's assume I'm the smart user who
> is able to distinguish them. But then, 'set skip on' is the
> persistent one, right, but still not reset, I guess.
>

    I think Petr is right here. my patch requires yet another finishing touch:

--------8<---------------8<---------------8<------------------8<--------
diff --git a/sbin/pfctl/pfctl.c b/sbin/pfctl/pfctl.c
index 40929d90530..032fdd08b57 100644
--- a/sbin/pfctl/pfctl.c
+++ b/sbin/pfctl/pfctl.c
@@ -2267,6 +2267,8 @@ pfctl_reset(int dev, int opts)
 
        if (pfctl_trans(dev, &t, DIOCXCOMMIT, 0))
                warn("%s, DIOCXCOMMIT", __func__);
+
+       pfctl_clear_interface_flags(dev, opts);
 }
 
 int
@@ -2594,7 +2596,6 @@ main(int argc, char *argv[])
                                pfctl_clear_src_nodes(dev, opts);
                                pfctl_clear_stats(dev, ifaceopt, opts);
                                pfctl_clear_fingerprints(dev, opts);
-                               pfctl_clear_interface_flags(dev, opts);
                                pfctl_reset(dev, opts);
                        }
                        break;
--------8<---------------8<---------------8<------------------8<--------

I'll walk through my change one more time to check if there are similar
oversights.

thanks and
regards
sasha

Reply | Threaded
Open this post in threaded view
|

Re: introduce 'pfctl -FR' to reset settings to defaults

Klemens Nanni-2
On Tue, Apr 02, 2019 at 02:01:05PM +0200, Alexandr Nedvedicky wrote:
>     I think Petr is right here. my patch requires yet another finishing touch:
Fair enough, but it should be noted that this somewhat changes behaviour
of the existing interface:

> --------8<---------------8<---------------8<------------------8<--------
> diff --git a/sbin/pfctl/pfctl.c b/sbin/pfctl/pfctl.c
> index 40929d90530..032fdd08b57 100644
> --- a/sbin/pfctl/pfctl.c
> +++ b/sbin/pfctl/pfctl.c
> @@ -2267,6 +2267,8 @@ pfctl_reset(int dev, int opts)
>  
>         if (pfctl_trans(dev, &t, DIOCXCOMMIT, 0))
>                 warn("%s, DIOCXCOMMIT", __func__);
> +
> +       pfctl_clear_interface_flags(dev, opts);
Now this is done with `-F reset' and therefore `-F all'...

>  }
>  
>  int
> @@ -2594,7 +2596,6 @@ main(int argc, char *argv[])
>                                 pfctl_clear_src_nodes(dev, opts);
>                                 pfctl_clear_stats(dev, ifaceopt, opts);
>                                 pfctl_clear_fingerprints(dev, opts);
> -                               pfctl_clear_interface_flags(dev, opts);
Where previously, without being documented, only `-F all' would do so.

>                                 pfctl_reset(dev, opts);
>                         }

I think that is fine in this particular case, but clearing things in
specific flush commands that were previously only touched by the `all'
hammer can be dangerous.

Reply | Threaded
Open this post in threaded view
|

Re: introduce 'pfctl -FR' to reset settings to defaults

Alexandr Nedvedicky
In reply to this post by Petr Hoffmann-2
Hello,

On Tue, Apr 02, 2019 at 11:28:43AM +0200, Petr Hoffmann wrote:

> Hi,
>
> seeing this in the manpage
> --8<--------------------------------------------------------------------------
> +.It Fl F Cm Reset
> +Reset limits, timeouts and options back to default settings.
> -------------------------------------------------------------------------->8--
> would make me believe everything mentioned as OPTIONS in pf.conf(5)
> is about to be reset. I see e.g. the debug level is reset, but what
> about the other stuff like fingerprints, 'skip on' and other options
> set via the 'set' command? Maybe the manpage should be more precise
> here?
>

    I did look at pf.conf(5) manpage yesterday. It requires more updates, which
    I would like to leave for another diff. For example pf.conf(5) does not
    mention default values for limits and time outs. I expect some discussion
    on how far we want to get with details. So let's exclude pf.conf(5) this
    time.

below is the diff I'd like to commit.

thanks and
regards
sashan

--------8<---------------8<---------------8<------------------8<--------
diff --git a/sbin/pfctl/pfctl.8 b/sbin/pfctl/pfctl.8
index 48b2893cfcd..00bd27c200a 100644
--- a/sbin/pfctl/pfctl.8
+++ b/sbin/pfctl/pfctl.8
@@ -197,6 +197,8 @@ Flush the filter information (statistics that are not bound to rules).
 Flush the tables.
 .It Fl F Cm osfp
 Flush the passive operating system fingerprints.
+.It Fl F Cm Reset
+Reset limits, timeouts and options back to default settings.
 .It Fl F Cm all
 Flush all of the above.
 .El
diff --git a/sbin/pfctl/pfctl.c b/sbin/pfctl/pfctl.c
index 493ff47af2f..032fdd08b57 100644
--- a/sbin/pfctl/pfctl.c
+++ b/sbin/pfctl/pfctl.c
@@ -105,6 +105,7 @@ int pfctl_load_rule(struct pfctl *, char *, struct pf_rule *, int);
 const char *pfctl_lookup_option(char *, const char **);
 void pfctl_state_store(int, const char *);
 void pfctl_state_load(int, const char *);
+void pfctl_reset(int, int);
 
 const char *clearopt;
 char *rulesopt;
@@ -205,7 +206,8 @@ static const struct {
 };
 
 static const char *clearopt_list[] = {
- "rules", "Sources", "states", "info", "Tables", "osfp", "all", NULL
+ "rules", "Sources", "states", "info", "Tables", "osfp", "Reset",
+ "all", NULL
 };
 
 static const char *showopt_list[] = {
@@ -2232,6 +2234,43 @@ pfctl_state_load(int dev, const char *file)
  fclose(f);
 }
 
+void
+pfctl_reset(int dev, int opts)
+{
+ struct pfctl pf;
+ struct pfr_buffer t;
+ int i;
+
+ pf.dev = dev;
+ pfctl_init_options(&pf);
+
+ /* Force reset upon pfctl_load_options() */
+ pf.debug_set = 1;
+ pf.reass_set = 1;
+ pf.syncookieswat_set = 1;
+ pf.ifname = strdup("none");
+ pf.ifname_set = 1;
+
+ memset(&t, 0, sizeof(t));
+ t.pfrb_type = PFRB_TRANS;
+ if (pfctl_trans(dev, &t, DIOCXBEGIN, 0))
+ warn("%s, DIOCXBEGIN", __func__);
+
+
+ for (i = 0; pf_limits[i].name; i++)
+ pf.limit_set[pf_limits[i].index] = 1;
+
+ for (i = 0; pf_timeouts[i].name; i++)
+ pf.timeout_set[pf_timeouts[i].timeout] = 1;
+
+ pfctl_load_options(&pf);
+
+ if (pfctl_trans(dev, &t, DIOCXCOMMIT, 0))
+ warn("%s, DIOCXCOMMIT", __func__);
+
+ pfctl_clear_interface_flags(dev, opts);
+}
+
 int
 main(int argc, char *argv[])
 {
@@ -2557,7 +2596,7 @@ main(int argc, char *argv[])
  pfctl_clear_src_nodes(dev, opts);
  pfctl_clear_stats(dev, ifaceopt, opts);
  pfctl_clear_fingerprints(dev, opts);
- pfctl_clear_interface_flags(dev, opts);
+ pfctl_reset(dev, opts);
  }
  break;
  case 'o':
@@ -2566,6 +2605,9 @@ main(int argc, char *argv[])
  case 'T':
  pfctl_clear_tables(anchorname, opts);
  break;
+ case 'R':
+ pfctl_reset(dev, opts);
+ break;
  }
  }
  if (state_killers) {

Reply | Threaded
Open this post in threaded view
|

Re: introduce 'pfctl -FR' to reset settings to defaults

Klemens Nanni-2
On Wed, Apr 03, 2019 at 11:10:21AM +0200, Alexandr Nedvedicky wrote:
>     I did look at pf.conf(5) manpage yesterday. It requires more updates, which
>     I would like to leave for another diff. For example pf.conf(5) does not
>     mention default values for limits and time outs. I expect some discussion
>     on how far we want to get with details. So let's exclude pf.conf(5) this
>     time.
Yes, this bothered me as well;  I also agree on tackling this with
separate diffs.

> below is the diff I'd like to commit.
OK kn

Reply | Threaded
Open this post in threaded view
|

Re: introduce 'pfctl -FR' to reset settings to defaults

Ted Unangst-6
In reply to this post by Alexandr Nedvedicky
Alexandr Nedvedicky wrote:
> below is the diff I'd like to commit.

this is fine with me.

Reply | Threaded
Open this post in threaded view
|

Re: introduce 'pfctl -FR' to reset settings to defaults

Hiltjo Posthuma
In reply to this post by Alexandr Nedvedicky
On Wed, Apr 03, 2019 at 11:10:21AM +0200, Alexandr Nedvedicky wrote:

> Hello,
>
> On Tue, Apr 02, 2019 at 11:28:43AM +0200, Petr Hoffmann wrote:
> > Hi,
> >
> > seeing this in the manpage
> > --8<--------------------------------------------------------------------------
> > +.It Fl F Cm Reset
> > +Reset limits, timeouts and options back to default settings.
> > -------------------------------------------------------------------------->8--
> > would make me believe everything mentioned as OPTIONS in pf.conf(5)
> > is about to be reset. I see e.g. the debug level is reset, but what
> > about the other stuff like fingerprints, 'skip on' and other options
> > set via the 'set' command? Maybe the manpage should be more precise
> > here?
> >
>
>     I did look at pf.conf(5) manpage yesterday. It requires more updates, which
>     I would like to leave for another diff. For example pf.conf(5) does not
>     mention default values for limits and time outs. I expect some discussion
>     on how far we want to get with details. So let's exclude pf.conf(5) this
>     time.
>
> below is the diff I'd like to commit.
>
> thanks and
> regards
> sashan
>
> --------8<---------------8<---------------8<------------------8<--------
> diff --git a/sbin/pfctl/pfctl.8 b/sbin/pfctl/pfctl.8
> index 48b2893cfcd..00bd27c200a 100644
> --- a/sbin/pfctl/pfctl.8
> +++ b/sbin/pfctl/pfctl.8
> @@ -197,6 +197,8 @@ Flush the filter information (statistics that are not bound to rules).
>  Flush the tables.
>  .It Fl F Cm osfp
>  Flush the passive operating system fingerprints.
> +.It Fl F Cm Reset
> +Reset limits, timeouts and options back to default settings.
>  .It Fl F Cm all
>  Flush all of the above.
>  .El
> diff --git a/sbin/pfctl/pfctl.c b/sbin/pfctl/pfctl.c
> index 493ff47af2f..032fdd08b57 100644
> --- a/sbin/pfctl/pfctl.c
> +++ b/sbin/pfctl/pfctl.c
> @@ -105,6 +105,7 @@ int pfctl_load_rule(struct pfctl *, char *, struct pf_rule *, int);
>  const char *pfctl_lookup_option(char *, const char **);
>  void pfctl_state_store(int, const char *);
>  void pfctl_state_load(int, const char *);
> +void pfctl_reset(int, int);
>  
>  const char *clearopt;
>  char *rulesopt;
> @@ -205,7 +206,8 @@ static const struct {
>  };
>  
>  static const char *clearopt_list[] = {
> - "rules", "Sources", "states", "info", "Tables", "osfp", "all", NULL
> + "rules", "Sources", "states", "info", "Tables", "osfp", "Reset",
> + "all", NULL
>  };
>  
>  static const char *showopt_list[] = {
> @@ -2232,6 +2234,43 @@ pfctl_state_load(int dev, const char *file)
>   fclose(f);
>  }
>  
> +void
> +pfctl_reset(int dev, int opts)
> +{
> + struct pfctl pf;
> + struct pfr_buffer t;
> + int i;
> +
> + pf.dev = dev;
> + pfctl_init_options(&pf);
> +
> + /* Force reset upon pfctl_load_options() */
> + pf.debug_set = 1;
> + pf.reass_set = 1;
> + pf.syncookieswat_set = 1;
> + pf.ifname = strdup("none");

I think strdup should be checked for NULL.

> + pf.ifname_set = 1;
> +
> + memset(&t, 0, sizeof(t));
> + t.pfrb_type = PFRB_TRANS;
> + if (pfctl_trans(dev, &t, DIOCXBEGIN, 0))
> + warn("%s, DIOCXBEGIN", __func__);
> +
> +

There is an extra white-space line here.

> + for (i = 0; pf_limits[i].name; i++)
> + pf.limit_set[pf_limits[i].index] = 1;
> +
> + for (i = 0; pf_timeouts[i].name; i++)
> + pf.timeout_set[pf_timeouts[i].timeout] = 1;
> +
> + pfctl_load_options(&pf);
> +
> + if (pfctl_trans(dev, &t, DIOCXCOMMIT, 0))
> + warn("%s, DIOCXCOMMIT", __func__);
> +
> + pfctl_clear_interface_flags(dev, opts);
> +}
> +
>  int
>  main(int argc, char *argv[])
>  {
> @@ -2557,7 +2596,7 @@ main(int argc, char *argv[])
>   pfctl_clear_src_nodes(dev, opts);
>   pfctl_clear_stats(dev, ifaceopt, opts);
>   pfctl_clear_fingerprints(dev, opts);
> - pfctl_clear_interface_flags(dev, opts);
> + pfctl_reset(dev, opts);
>   }
>   break;
>   case 'o':
> @@ -2566,6 +2605,9 @@ main(int argc, char *argv[])
>   case 'T':
>   pfctl_clear_tables(anchorname, opts);
>   break;
> + case 'R':
> + pfctl_reset(dev, opts);
> + break;
>   }
>   }
>   if (state_killers) {
>

--
Kind regards,
Hiltjo

Reply | Threaded
Open this post in threaded view
|

Re: introduce 'pfctl -FR' to reset settings to defaults

Alexandr Nedvedicky
Hello,

</snip>

> > +void
> > +pfctl_reset(int dev, int opts)
> > +{
> > + struct pfctl pf;
> > + struct pfr_buffer t;
> > + int i;
> > +
> > + pf.dev = dev;
> > + pfctl_init_options(&pf);
> > +
> > + /* Force reset upon pfctl_load_options() */
> > + pf.debug_set = 1;
> > + pf.reass_set = 1;
> > + pf.syncookieswat_set = 1;
> > + pf.ifname = strdup("none");
>
> I think strdup should be checked for NULL.

    good point. does something like this look good?

--------8<---------------8<---------------8<------------------8<--------
diff --git a/sbin/pfctl/pfctl.c b/sbin/pfctl/pfctl.c
index 032fdd08b57..88d31d6190e 100644
--- a/sbin/pfctl/pfctl.c
+++ b/sbin/pfctl/pfctl.c
@@ -2249,7 +2249,10 @@ pfctl_reset(int dev, int opts)
        pf.reass_set = 1;
        pf.syncookieswat_set = 1;
        pf.ifname = strdup("none");
-       pf.ifname_set = 1;
+       if (pf.ifname == NULL)
+               warn("%s: Warning: can't reset loginterface\n", __func__);
+       else
+               pf.ifname_set = 1;
 
        memset(&t, 0, sizeof(t));
        t.pfrb_type = PFRB_TRANS;
--------8<---------------8<---------------8<------------------8<--------

>
> > + pf.ifname_set = 1;
> > +
> > + memset(&t, 0, sizeof(t));
> > + t.pfrb_type = PFRB_TRANS;
> > + if (pfctl_trans(dev, &t, DIOCXBEGIN, 0))
> > + warn("%s, DIOCXBEGIN", __func__);
> > +
> > +
>
> There is an extra white-space line here.

    fixed.

updated diff is attached. I'll commit the change after unlock.

thanks and
regards
sasha

--------8<---------------8<---------------8<------------------8<--------
diff --git a/sbin/pfctl/pfctl.8 b/sbin/pfctl/pfctl.8
index 48b2893cfcd..00bd27c200a 100644
--- a/sbin/pfctl/pfctl.8
+++ b/sbin/pfctl/pfctl.8
@@ -197,6 +197,8 @@ Flush the filter information (statistics that are not bound to rules).
 Flush the tables.
 .It Fl F Cm osfp
 Flush the passive operating system fingerprints.
+.It Fl F Cm Reset
+Reset limits, timeouts and options back to default settings.
 .It Fl F Cm all
 Flush all of the above.
 .El
diff --git a/sbin/pfctl/pfctl.c b/sbin/pfctl/pfctl.c
index 493ff47af2f..470c54644ba 100644
--- a/sbin/pfctl/pfctl.c
+++ b/sbin/pfctl/pfctl.c
@@ -105,6 +105,7 @@ int pfctl_load_rule(struct pfctl *, char *, struct pf_rule *, int);
 const char *pfctl_lookup_option(char *, const char **);
 void pfctl_state_store(int, const char *);
 void pfctl_state_load(int, const char *);
+void pfctl_reset(int, int);
 
 const char *clearopt;
 char *rulesopt;
@@ -205,7 +206,8 @@ static const struct {
 };
 
 static const char *clearopt_list[] = {
- "rules", "Sources", "states", "info", "Tables", "osfp", "all", NULL
+ "rules", "Sources", "states", "info", "Tables", "osfp", "Reset",
+ "all", NULL
 };
 
 static const char *showopt_list[] = {
@@ -2232,6 +2234,45 @@ pfctl_state_load(int dev, const char *file)
  fclose(f);
 }
 
+void
+pfctl_reset(int dev, int opts)
+{
+ struct pfctl pf;
+ struct pfr_buffer t;
+ int i;
+
+ pf.dev = dev;
+ pfctl_init_options(&pf);
+
+ /* Force reset upon pfctl_load_options() */
+ pf.debug_set = 1;
+ pf.reass_set = 1;
+ pf.syncookieswat_set = 1;
+ pf.ifname = strdup("none");
+ if (pf.ifname == NULL)
+ warn("%s: Warning: can't reset loginterface\n", __func__);
+ else
+ pf.ifname_set = 1;
+
+ memset(&t, 0, sizeof(t));
+ t.pfrb_type = PFRB_TRANS;
+ if (pfctl_trans(dev, &t, DIOCXBEGIN, 0))
+ warn("%s, DIOCXBEGIN", __func__);
+
+ for (i = 0; pf_limits[i].name; i++)
+ pf.limit_set[pf_limits[i].index] = 1;
+
+ for (i = 0; pf_timeouts[i].name; i++)
+ pf.timeout_set[pf_timeouts[i].timeout] = 1;
+
+ pfctl_load_options(&pf);
+
+ if (pfctl_trans(dev, &t, DIOCXCOMMIT, 0))
+ warn("%s, DIOCXCOMMIT", __func__);
+
+ pfctl_clear_interface_flags(dev, opts);
+}
+
 int
 main(int argc, char *argv[])
 {
@@ -2557,7 +2598,7 @@ main(int argc, char *argv[])
  pfctl_clear_src_nodes(dev, opts);
  pfctl_clear_stats(dev, ifaceopt, opts);
  pfctl_clear_fingerprints(dev, opts);
- pfctl_clear_interface_flags(dev, opts);
+ pfctl_reset(dev, opts);
  }
  break;
  case 'o':
@@ -2566,6 +2607,9 @@ main(int argc, char *argv[])
  case 'T':
  pfctl_clear_tables(anchorname, opts);
  break;
+ case 'R':
+ pfctl_reset(dev, opts);
+ break;
  }
  }
  if (state_killers) {

Reply | Threaded
Open this post in threaded view
|

Re: introduce 'pfctl -FR' to reset settings to defaults

Gleydson Soares-5
 +void

> +pfctl_reset(int dev, int opts)
> +{
> + struct pfctl pf;
> + struct pfr_buffer t;
> + int i;
> +
> + pf.dev = dev;
> + pfctl_init_options(&pf);
> +
> + /* Force reset upon pfctl_load_options() */
> + pf.debug_set = 1;
> + pf.reass_set = 1;
> + pf.syncookieswat_set = 1;
> + pf.ifname = strdup("none");
> + if (pf.ifname == NULL)
> + warn("%s: Warning: can't reset loginterface\n", __func__);
                                                        ^^^^^^^^
                                                do you really need this
                                                extra newline here?
                                                warn() itself already includes
                                                one.

> + else
> + pf.ifname_set = 1;
> +
> + memset(&t, 0, sizeof(t));
> + t.pfrb_type = PFRB_TRANS;
> + if (pfctl_trans(dev, &t, DIOCXBEGIN, 0))
> + warn("%s, DIOCXBEGIN", __func__);
> +
> + for (i = 0; pf_limits[i].name; i++)
> + pf.limit_set[pf_limits[i].index] = 1;
> +
> + for (i = 0; pf_timeouts[i].name; i++)
> + pf.timeout_set[pf_timeouts[i].timeout] = 1;
> +
> + pfctl_load_options(&pf);
> +
> + if (pfctl_trans(dev, &t, DIOCXCOMMIT, 0))
> + warn("%s, DIOCXCOMMIT", __func__);
> +
> + pfctl_clear_interface_flags(dev, opts);
> +}

Reply | Threaded
Open this post in threaded view
|

Re: introduce 'pfctl -FR' to reset settings to defaults

Klemens Nanni-2
In reply to this post by Alexandr Nedvedicky
On Sat, Apr 06, 2019 at 02:37:05AM +0200, Alexandr Nedvedicky wrote:
> updated diff is attached. I'll commit the change after unlock.
OK kn with comments inline.

> + pf.ifname = strdup("none");
> + if (pf.ifname == NULL)
> + warn("%s: Warning: can't reset loginterface\n", __func__);
> + else
> + pf.ifname_set = 1;

We should fail hard as in almost all other strdup(3) use cases.
Failure means the system ran out of memory, there's no point in going
any further.

So just something like

        pf.ifname = strdup("none");
        if (pf.ifname == NULL)
                err(1, "%s: strdup", __func__);

> + if (pfctl_trans(dev, &t, DIOCXBEGIN, 0))
> + warn("%s, DIOCXBEGIN", __func__);
Turn this comma into a double colon.

> + if (pfctl_trans(dev, &t, DIOCXCOMMIT, 0))
> + warn("%s, DIOCXCOMMIT", __func__);
Same here.

Reply | Threaded
Open this post in threaded view
|

Re: introduce 'pfctl -FR' to reset settings to defaults

Alexandr Nedvedicky
Hello,

</snip>

> We should fail hard as in almost all other strdup(3) use cases.
> Failure means the system ran out of memory, there's no point in going
> any further.
>
> So just something like
>
> pf.ifname = strdup("none");
> if (pf.ifname == NULL)
> err(1, "%s: strdup", __func__);
>

    yes, you are right. so this is the change:

--------8<---------------8<---------------8<------------------8<--------
diff --git a/sbin/pfctl/pfctl.c b/sbin/pfctl/pfctl.c
index 470c54644ba..28f8723cac3 100644
--- a/sbin/pfctl/pfctl.c
+++ b/sbin/pfctl/pfctl.c
@@ -2250,14 +2250,14 @@ pfctl_reset(int dev, int opts)
        pf.syncookieswat_set = 1;
        pf.ifname = strdup("none");
        if (pf.ifname == NULL)
-               warn("%s: Warning: can't reset loginterface\n", __func__);
+               err(1, "%s: strdup", __func__);
        else
                pf.ifname_set = 1;
 
        memset(&t, 0, sizeof(t));
        t.pfrb_type = PFRB_TRANS;
        if (pfctl_trans(dev, &t, DIOCXBEGIN, 0))
-               warn("%s, DIOCXBEGIN", __func__);
+               err(1, "%s: DIOCXBEGIN", __func__);
 
        for (i = 0; pf_limits[i].name; i++)
                pf.limit_set[pf_limits[i].index] = 1;
@@ -2268,7 +2268,7 @@ pfctl_reset(int dev, int opts)
        pfctl_load_options(&pf);
 
        if (pfctl_trans(dev, &t, DIOCXCOMMIT, 0))
-               warn("%s, DIOCXCOMMIT", __func__);
+               err(1, "%s: DIOCXCOMMIT", __func__);
 
        pfctl_clear_interface_flags(dev, opts);
 }
--------8<---------------8<---------------8<------------------8<--------

updated patch follows.

thanks and
regards
sashan
--------8<---------------8<---------------8<------------------8<--------
diff --git a/sbin/pfctl/pfctl.8 b/sbin/pfctl/pfctl.8
index 48b2893cfcd..00bd27c200a 100644
--- a/sbin/pfctl/pfctl.8
+++ b/sbin/pfctl/pfctl.8
@@ -197,6 +197,8 @@ Flush the filter information (statistics that are not bound to rules).
 Flush the tables.
 .It Fl F Cm osfp
 Flush the passive operating system fingerprints.
+.It Fl F Cm Reset
+Reset limits, timeouts and options back to default settings.
 .It Fl F Cm all
 Flush all of the above.
 .El
diff --git a/sbin/pfctl/pfctl.c b/sbin/pfctl/pfctl.c
index 493ff47af2f..28f8723cac3 100644
--- a/sbin/pfctl/pfctl.c
+++ b/sbin/pfctl/pfctl.c
@@ -105,6 +105,7 @@ int pfctl_load_rule(struct pfctl *, char *, struct pf_rule *, int);
 const char *pfctl_lookup_option(char *, const char **);
 void pfctl_state_store(int, const char *);
 void pfctl_state_load(int, const char *);
+void pfctl_reset(int, int);
 
 const char *clearopt;
 char *rulesopt;
@@ -205,7 +206,8 @@ static const struct {
 };
 
 static const char *clearopt_list[] = {
- "rules", "Sources", "states", "info", "Tables", "osfp", "all", NULL
+ "rules", "Sources", "states", "info", "Tables", "osfp", "Reset",
+ "all", NULL
 };
 
 static const char *showopt_list[] = {
@@ -2232,6 +2234,45 @@ pfctl_state_load(int dev, const char *file)
  fclose(f);
 }
 
+void
+pfctl_reset(int dev, int opts)
+{
+ struct pfctl pf;
+ struct pfr_buffer t;
+ int i;
+
+ pf.dev = dev;
+ pfctl_init_options(&pf);
+
+ /* Force reset upon pfctl_load_options() */
+ pf.debug_set = 1;
+ pf.reass_set = 1;
+ pf.syncookieswat_set = 1;
+ pf.ifname = strdup("none");
+ if (pf.ifname == NULL)
+ err(1, "%s: strdup", __func__);
+ else
+ pf.ifname_set = 1;
+
+ memset(&t, 0, sizeof(t));
+ t.pfrb_type = PFRB_TRANS;
+ if (pfctl_trans(dev, &t, DIOCXBEGIN, 0))
+ err(1, "%s: DIOCXBEGIN", __func__);
+
+ for (i = 0; pf_limits[i].name; i++)
+ pf.limit_set[pf_limits[i].index] = 1;
+
+ for (i = 0; pf_timeouts[i].name; i++)
+ pf.timeout_set[pf_timeouts[i].timeout] = 1;
+
+ pfctl_load_options(&pf);
+
+ if (pfctl_trans(dev, &t, DIOCXCOMMIT, 0))
+ err(1, "%s: DIOCXCOMMIT", __func__);
+
+ pfctl_clear_interface_flags(dev, opts);
+}
+
 int
 main(int argc, char *argv[])
 {
@@ -2557,7 +2598,7 @@ main(int argc, char *argv[])
  pfctl_clear_src_nodes(dev, opts);
  pfctl_clear_stats(dev, ifaceopt, opts);
  pfctl_clear_fingerprints(dev, opts);
- pfctl_clear_interface_flags(dev, opts);
+ pfctl_reset(dev, opts);
  }
  break;
  case 'o':
@@ -2566,6 +2607,9 @@ main(int argc, char *argv[])
  case 'T':
  pfctl_clear_tables(anchorname, opts);
  break;
+ case 'R':
+ pfctl_reset(dev, opts);
+ break;
  }
  }
  if (state_killers) {

Reply | Threaded
Open this post in threaded view
|

Re: introduce 'pfctl -FR' to reset settings to defaults

Klemens Nanni-2
OK either way, but see below.

On Mon, Apr 08, 2019 at 09:56:46AM +0200, Alexandr Nedvedicky wrote:
> + pf.ifname = strdup("none");
> + if (pf.ifname == NULL)
> + err(1, "%s: strdup", __func__);
> + else
> + pf.ifname_set = 1;
This branch is redundant and confusing, that pattern is also rarely
seen in the tree.  The following is more obvious to the reader and
resembles the code flow more clearly, I'd say:

        pf.ifname = strdup("none");
        if (pf.ifname == NULL)
                err(1, "%s: strdup", __func__);
        pf.ifname_set = 1;

Reply | Threaded
Open this post in threaded view
|

Re: introduce 'pfctl -FR' to reset settings to defaults

Alexandr Nedvedicky
Hello Klemens,

On Tue, Apr 09, 2019 at 04:02:06PM +0200, Klemens Nanni wrote:

> OK either way, but see below.
>
> On Mon, Apr 08, 2019 at 09:56:46AM +0200, Alexandr Nedvedicky wrote:
> > + pf.ifname = strdup("none");
> > + if (pf.ifname == NULL)
> > + err(1, "%s: strdup", __func__);
> > + else
> > + pf.ifname_set = 1;
> This branch is redundant and confusing, that pattern is also rarely
> seen in the tree.  The following is more obvious to the reader and
> resembles the code flow more clearly, I'd say:
>
> pf.ifname = strdup("none");
> if (pf.ifname == NULL)
> err(1, "%s: strdup", __func__);
> pf.ifname_set = 1;

    you are absolutely right.

    Looks like I need glasses, but not the glasses with beer.

thanks and
regards
sashan

--------8<---------------8<---------------8<------------------8<--------
diff --git a/sbin/pfctl/pfctl.8 b/sbin/pfctl/pfctl.8
index 48b2893cfcd..00bd27c200a 100644
--- a/sbin/pfctl/pfctl.8
+++ b/sbin/pfctl/pfctl.8
@@ -197,6 +197,8 @@ Flush the filter information (statistics that are not bound to rules).
 Flush the tables.
 .It Fl F Cm osfp
 Flush the passive operating system fingerprints.
+.It Fl F Cm Reset
+Reset limits, timeouts and options back to default settings.
 .It Fl F Cm all
 Flush all of the above.
 .El
diff --git a/sbin/pfctl/pfctl.c b/sbin/pfctl/pfctl.c
index 493ff47af2f..17461a4bf77 100644
--- a/sbin/pfctl/pfctl.c
+++ b/sbin/pfctl/pfctl.c
@@ -105,6 +105,7 @@ int pfctl_load_rule(struct pfctl *, char *, struct pf_rule *, int);
 const char *pfctl_lookup_option(char *, const char **);
 void pfctl_state_store(int, const char *);
 void pfctl_state_load(int, const char *);
+void pfctl_reset(int, int);
 
 const char *clearopt;
 char *rulesopt;
@@ -205,7 +206,8 @@ static const struct {
 };
 
 static const char *clearopt_list[] = {
- "rules", "Sources", "states", "info", "Tables", "osfp", "all", NULL
+ "rules", "Sources", "states", "info", "Tables", "osfp", "Reset",
+ "all", NULL
 };
 
 static const char *showopt_list[] = {
@@ -2232,6 +2234,44 @@ pfctl_state_load(int dev, const char *file)
  fclose(f);
 }
 
+void
+pfctl_reset(int dev, int opts)
+{
+ struct pfctl pf;
+ struct pfr_buffer t;
+ int i;
+
+ pf.dev = dev;
+ pfctl_init_options(&pf);
+
+ /* Force reset upon pfctl_load_options() */
+ pf.debug_set = 1;
+ pf.reass_set = 1;
+ pf.syncookieswat_set = 1;
+ pf.ifname = strdup("none");
+ if (pf.ifname == NULL)
+ err(1, "%s: strdup", __func__);
+ pf.ifname_set = 1;
+
+ memset(&t, 0, sizeof(t));
+ t.pfrb_type = PFRB_TRANS;
+ if (pfctl_trans(dev, &t, DIOCXBEGIN, 0))
+ err(1, "%s: DIOCXBEGIN", __func__);
+
+ for (i = 0; pf_limits[i].name; i++)
+ pf.limit_set[pf_limits[i].index] = 1;
+
+ for (i = 0; pf_timeouts[i].name; i++)
+ pf.timeout_set[pf_timeouts[i].timeout] = 1;
+
+ pfctl_load_options(&pf);
+
+ if (pfctl_trans(dev, &t, DIOCXCOMMIT, 0))
+ err(1, "%s: DIOCXCOMMIT", __func__);
+
+ pfctl_clear_interface_flags(dev, opts);
+}
+
 int
 main(int argc, char *argv[])
 {
@@ -2557,7 +2597,7 @@ main(int argc, char *argv[])
  pfctl_clear_src_nodes(dev, opts);
  pfctl_clear_stats(dev, ifaceopt, opts);
  pfctl_clear_fingerprints(dev, opts);
- pfctl_clear_interface_flags(dev, opts);
+ pfctl_reset(dev, opts);
  }
  break;
  case 'o':
@@ -2566,6 +2606,9 @@ main(int argc, char *argv[])
  case 'T':
  pfctl_clear_tables(anchorname, opts);
  break;
+ case 'R':
+ pfctl_reset(dev, opts);
+ break;
  }
  }
  if (state_killers) {
diff --git a/share/man/man5/pf.conf.5 b/share/man/man5/pf.conf.5
index 247ceef40a5..dfa8d15d37a 100644
--- a/share/man/man5/pf.conf.5
+++ b/share/man/man5/pf.conf.5
@@ -1129,12 +1129,24 @@ can be used.
 .Xr pf 4
 may be tuned for various situations using the
 .Ic set
-command.
+command. Two sorts of options should be distinguished.
+.Em runtime
+options, which define parameters for
+.Xr pf 4
+driver and
+.Em parser
+options, which fine-tune interpretation of rules, while
+they are being loaded from file. The runtime options
+may be restored to their default values using:
+.Pp
+.Dl # pfctl -FReset
+.Pp
+
 .Bl -tag -width Ds
 .It Ic set Cm block-policy drop | return
 The
 .Cm block-policy
-option sets the default behaviour for the packet
+parser option sets the default behaviour for the packet
 .Ic block
 action:
 .Pp
@@ -1146,8 +1158,13 @@ A TCP RST is returned for blocked TCP packets,
 an ICMP UNREACHABLE is returned for blocked UDP packets,
 and all other packets are silently dropped.
 .El
+.Pp
+The default value is
+.Cm drop .
 .It Ic set Cm debug Ar level
-Set the debug
+The
+.Cm debug
+runtime option defines
 .Ar level ,
 which limits the severity of log messages printed by
 .Xr pf 4 .
@@ -1164,9 +1181,10 @@ and
 .Cm debug .
 These keywords correspond to the similar (LOG_) values specified to the
 .Xr syslog 3
-library routine.
+library routine. The default value is
+.Cm err .
 .It Cm set Cm fingerprints Ar filename
-Load fingerprints of known operating systems from the given
+Parser option loads fingerprints of known operating systems from the given
 .Ar filename .
 By default fingerprints of known operating systems are automatically
 loaded from
@@ -1174,23 +1192,29 @@ loaded from
 but can be overridden via this option.
 Setting this option may leave a small period of time where the fingerprints
 referenced by the currently active ruleset are inconsistent until the new
-ruleset finishes loading.
+ruleset finishes loading. The default location for fingerprints is
+/etc/pf.os file.
 .It Ic set Cm hostid Ar number
-The 32-bit hostid
-.Ar number
-identifies this firewall's state table entries to other firewalls
+The runtime option specifies 32-bit hostid
+.Ar number ,
+which identifies this firewall's state table entries to other firewalls
 in a
 .Xr pfsync 4
 failover cluster.
 By default the hostid is set to a pseudo-random value, however it may be
 desirable to manually configure it, for example to more easily identify the
 source of state table entries.
-The hostid may be specified in either decimal or hexadecimal.
+The hostid may be specified in either decimal or hexadecimal. The
+.Cm hostid
+option value does not get changed by
+.Xr pfctl 8
+.Fl F
+.Cm Reset .
 .It Ic set Cm limit Ar limit-item number
 Sets hard limits on the memory pools used by the packet filter.
 See
 .Xr pool 9
-for an explanation of memory pools.
+for an explanation of memory pools. All limits are runtime options.
 .Pp
 For example,
 to set the maximum number of entries in the memory pool used by state table
@@ -1235,6 +1259,16 @@ Various limits can be combined on a single line:
 .Bd -literal -offset indent
 set limit { states 20000, frags 2000, src-nodes 2000 }
 .Ed
+.Pp
+.Xr pf 4
+uses defaults as follows:
+.Bd -literal -offset indent
+states PFSTATE_HIWAT (100000)
+tables PFR_KTABLE_HIWAT (1000)
+table-entries PFR_KENTRY_HIWAT (200000)
+ PFR_KENTRY_HIWAT_SMALL (100000)
+frags NMBCLUSTERS/32 (platform dependent)
+.Ed
 .It Ic set Cm loginterface Ar interface | Cm none
 Enable collection of packet and byte count statistics for the given
 interface or interface group.