Hi Alexandr,
Alexandr Nedvedicky wrote on Wed, Apr 17, 2019 at 12:09:10AM +0200:
> my oracle fellow pointed out [1] a PF documentation can be improved
> a bit, when it comes to newly introduced 'pfctl -FR' (a reset flush
> modifier). I've decided to make manpage changes in separate diff as
> I expect some discussion on how much detailed the manpage should be.
> The diff here is my proposal.
First off, thank you for working on the pf.conf(5) manual page.
I appreciate that very much. To me, the pf.conf(5) manual page
feels like a manual page of considerably above-average importance,
yet i have often felt it could probably be improved. Improving it
isn't easy though: the manual is long and complex, and the subject
matter is more complicated than for most other manual pages.
So this is a place where expert attention from somebody who
actively works on the code is particularly helpful.
[...]
> From 'pfctl -FR' perspective the most important is to document whether
> particular option is parameter for PF driver (a.k.a. runtime option)
> or is just interpreted by parser as rules are being loaded.
> Documenting default values is just bonus, which I think might be
> useful for administrators. On the other hand it adds more work to
> keep pf.conf(5) up-to-date when doing changes in code.
> I full understand the trade here between being precise and up-to-date.
I don't feel strongly about mentioning the defaults either way.
But i tend to think that if something is important enough to provide
users with a knob to tweak it, then they will probably need to know what
the default is before they can even start to figure out whether
they might need to tweak it. So my guess would be that every time
people read the description of the option, they will also profit
from seeing the default right away.
If others feel the risk is too high that these defaults might get
outdated in the manual page, is there a central place in the code
you can point readers to for looking up the defaults? If so,
pointing to that place might be a viable compromise. If no such
place exists (which might imply the defaults are scattered / hard
to find in the code?) that would strengthen the argument to document
them.
The below nits are not objections, nor conditions, nor reproaches.
You are welcome to incorporate them; then again, such details can also
be fixed after commit when needed. The main job, in particular in
a tricky page like this, is getting the content correct!
[...]
> diff --git a/share/man/man5/pf.conf.5 b/share/man/man5/pf.conf.5
> index 247ceef40a5..ab6b8c0447e 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.
New sentence, new line: this needs to be
command.
+Two sorts...
The same small problem occurs again below, several times.
I'm not annotating all the instances. For spotting such trivial
mistakes,
mandoc -T lint pf.conf.5
can be useful.
The word "should" is often somewhat confusing because it can leave
doubt whether something is a syntax requirement, or a stylistic
recommendation, or even a plan to change the code and the rules
in the future.
I'd prefer stating facts, for example:
command.
+There are two kinds of options:
+.Em runtime
> +.Em runtime
A capital letter would be needed at the beginning of the sentence,
but instead, i'd prefer the colon as shown above.
> +options, which define parameters for
The word "the" is missing here.
> +.Xr pf 4
> +driver and
+driver, and
> +.Em parser
> +options, which fine-tune interpretation of rules, while
> +they are being loaded from file. The runtime options
s/from file/from the file/
... and again, new sentence, new line.
> +may be restored to their default values using:
> +.Pp
> +.Dl # pfctl -FReset
> +.Pp
> +
Please show a blank between an option and its argument, unless
the syntax *requires* that there be no blank.
Blank lines are not allowed in mdoc(7), except inside .Bd -unfilled,
and .Pp is needless before .Bl without -compact, so:
+may be restored to their default values using:
+.Pp
+.Dl # pfctl -F Reset
.Bl -tag -width Ds
Alternatively, you could say, interrupting the text flow less:
+may be restored to their default values using the
+.Xr pfctl 8
+.Fl F Cm Reset
option.
> .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
That's indeed a nice, concise way to make the distinction.
> .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 .
The wording "defines level" feels a bit confusing. Maybe:
-Set the debug
+The
+.Cm debug
+runtime option sets the debug
.Ar level ,
> @@ -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
s/Parser/This parser/ and probably split the line.
> .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.
That information is already present in that very paragraph, so you
should probably revert this chunk.
> .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
s/The/This/ and s/32-bit/a 32-bit/
> +.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,9 +1259,28 @@ 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
Something like the following seems preferable:
.Bl -column table-entries PFR_KENTRY_HIWAT_SMALL platform_dependent
.It states Ta Dv PFSTATE_HIWAT Ta Pq 100000
.It tables Ta Dv PFR_KTABLE_HIWAT Ta Pq 1000
...
.El
.Pp
> +.Pp
> +.Cm NMBCLUSTERS
> +defines the total number of packets, which can exist in system at any
> +instance of the time. Refer to
> +.Sy sys/arch/`uname -p`/param.h
> +to find out a value for your platform. The number of fragments should
> +not exceed the number of
> +.Cm NMBCLUSTERS .
I fear this information is already present, see a few lines above:
This maximum may not exceed, and should be well below, the
maximum number of mbuf clusters (sysctl kern.maxclusters) in the
system.
> +
Another blank line, please remove.
> .It Ic set Cm loginterface Ar interface | Cm none
> -Enable collection of packet and byte count statistics for the given
> -interface or interface group.
> +Runtime option enables collection of packet and byte count statistics for the
s/Runtime/This runtime/
> +given interface or interface group.
> These statistics can be viewed using:
> .Pp
> .Dl # pfctl -s info
> @@ -1251,8 +1294,12 @@ collects statistics on the interface named dc0:
> One can disable the loginterface using:
> .Pp
> .Dl set loginterface none
> +.Pp
> +The default value is
> +.Cm none .
> .It Ic set Cm optimization Ar environment
> -Optimize state timeouts for one of the following network environments:
> +Runtime option hints how to adjust state timeouts for particular
s/Runtime/This runtime/ and s/for/for a/
> +network environment. There are following values possible:
network environment.
+The following values are possible:
> .Pp
> .Bl -tag -width Ds -compact
> .It Cm aggressive
> @@ -1273,11 +1320,14 @@ Suitable for almost all networks.
> Alias for
> .Cm high-latency .
> .El
> +.Pp
> +The default value is
> +.Cm normal .
> .It Ic set Cm reassemble yes | no Op Cm no-df
> The
> .Cm reassemble
> -option is used to enable or disable the reassembly of fragmented packets,
> -and can be set to
> +runtime option is used to enable or disable the reassembly of fragmented
> +packets, and can be set to
> .Cm yes
> (the default) or
> .Cm no .
> @@ -1291,6 +1341,7 @@ the reassembled packet will have the
> .Dq dont-fragment
> bit cleared.
> .It Ic set Cm ruleset-optimization Ar level
> +Parser option can be set by either three possible values:
I'd simply say:
+Parser option.
+The
+.Ar level
+can be:
> .Bl -tag -width profile -compact
> .It Cm basic
> Enable basic ruleset optimization.
> @@ -1331,18 +1382,18 @@ Optimization can also be set as a command-line
> argument to
> overriding the settings in
> .Nm .
> .It Ic set Cm skip on Ar ifspec
> -List interfaces for which packets should not be filtered.
> -Packets passing in or out on such interfaces are passed as if pf was
> -disabled, i.e. pf does not process them in any way.
> -This can be useful on loopback and other virtual interfaces, when
> -packet filtering is not desired and can have unexpected effects.
> +Runtime option lists interfaces for which packets should not be filtered.
s/Runtime/This runtime/
> +Packets passing in or out on such interfaces are passed as if pf was
> disabled,
> +i.e. pf does not process them in any way. This can be useful on loopback and
> +other virtual interfaces, when packet filtering is not desired and can have
> +unexpected effects.
Why are you changing these four lines?
It seems to me you are only making lines too long and violating
the rule "new sentence, new line".
> .Ar ifspec
> is only evaluated when the ruleset is loaded; interfaces created
> -later will not be skipped.
> +later will not be skipped. PF filters traffic on all interfaces by default.
> .It Ic set Cm state-defaults Ar state-option , ...
> The
> .Cm state-defaults
> -option sets the state options for states created from rules
> +parser option sets the state options for states created from rules
> without an explicit
> .Cm keep state .
> For example:
> @@ -1360,7 +1411,7 @@ States are bound to an interface.
> States can match packets on any interfaces (the default).
> .El
> .It Ic set Cm syncookies never | always | adaptive
> -When
> +Runtime option allows/prevents PF to use syncookies. When
"Allow to" and "prevent from" don't harmonize with each other. Maybe:
-When
+When the
.Cm syncookies
+runtime option is active, ...
> .Cm syncookies
> are active, pf will answer each and every incoming TCP SYN with a
> syncookie SYNACK, without allocating any resources.
> @@ -1389,14 +1440,17 @@ set syncookies adaptive (start 25%, end 12%)
> .Ed
> .El
> .It Ic set Cm timeout Ar variable value
> +.Pp
No .Pp right after .It.
> +All timeouts are runtime options.
> .Bl -tag -width "src.track" -compact
> .It Cm frag
> -Seconds before an unassembled fragment is expired.
> +Seconds before an unassembled fragment is expired (60 secs by default).
No need to repeat "seconds" here.
Either way, "secs" is not a good abbreviation.
My order of preference would be:
(60 by default)
(60 seconds by default)
(60s by default)
> .It Cm interval
> -Interval between purging expired states and fragments.
> +Interval between purging expired states and fragments (10 secs by default).
(10 seconds by default)
> .It Cm src.track
> Length of time to retain a source tracking entry after the last state
> -expires.
> +expires (0 by default, which means there is no global limit, value is
> +defined by rule, which creates state).
(0 by default, which means there is no global limit.
Instead, the value is defined by the rule which creates the state.)
> .El
> .Pp
> When a packet matches a stateful connection, the seconds to live for the
> @@ -1408,13 +1462,13 @@ Tuning these values may improve the performance of the
> firewall at the risk of dropping valid idle connections.
> .Pp
> .Bl -tag -width Ds -compact
> -.It Cm tcp.closed
> +.It Cm tcp.closed No (90 secs. by default)
(90 seconds by default) and similarly all the way below
Thanks again for polishing this manual page!
Yours,
Ingo
