rdr pass for proto tcp sometimes creates states with expire time zero and so breaking connections

[Andreas Longwitz]

Kristof Provost wrote:
> 
>     Because fetching a counter is a rather expansive function we should use
>     counter_u64_fetch() in pf_state_expires() only when necessary. A "rdr
>     pass" rule should not cause more effort than separate "rdr" and "pass"
>     rules. For rules with adaptive timeout values the call of
>     counter_u64_fetch() should be accepted, but otherwise not.
> 
>     For a small gain in performance especially for "rdr pass" rules I
>     suggest something like
> 
>     --- pf.c.orig 2019-02-18 17:49:22.944751000 +0100
>     +++ pf.c 2019-02-18 17:55:07.396163000 +0100
>     @@ -1558,7 +1558,7 @@
>     if (!timeout)
>        timeout = V_pf_default_rule.timeout[state->timeout];
>     start = state->rule.ptr->timeout[PFTM_ADAPTIVE_START];
>   - if (start) {
>   + if (start && state->rule.ptr != &V_pf_default_rule) {
>        end = state->rule.ptr->timeout[PFTM_ADAPTIVE_END];
>        states = counter_u64_fetch(state->rule.ptr->states_cur);
>     } else {
> 
> I think that looks correct. Do you have any performance measurements on
> this?

No

> Although presumably it only really matters in cases where there’s no
> explicit catch-all rule, so I do wonder if it’s worth it.

Sorry, but I do not understand this argument.

>From manpage:
   The adaptive timeout values can be defined both globally and for
   each rule.  When used on a per-rule basis, the values relate to the
   number of states created by the rule, otherwise to the total number
   of states.

This handling of adaptive timeouts is done in pf_state_expires():

     start = state->rule.ptr->timeout[PFTM_ADAPTIVE_START];
     if (start) {
             end = state->rule.ptr->timeout[PFTM_ADAPTIVE_END];
             states = counter_u64_fetch(state->rule.ptr->states_cur);
     } else {
             start = V_pf_default_rule.timeout[PFTM_ADAPTIVE_START];
             end = V_pf_default_rule.timeout[PFTM_ADAPTIVE_END];
             states = V_pf_status.states;
     }

The following calculation needs three values: start, end and states.

1. Normal rules "pass .." without adaptive setting meaning "start = 0"
   runs in the else-section of the code snippet and therefore takes
   "start" and "end" from the global default settings and sets "states"
   to pf_status.states (= total number of states).

2. Special rules like
       "pass .. keep state (adaptive.start 500 adaptive.end 1000)"
   have start != 0, run in the if-section of the code snippet and take
   "start" and "end" from the rule and set "states" to the number of
   states created by their rule using counter_u64_fetch().

Thats all ok, but there is a third case without special handling in the
above code snippet:

3. All "rdr/nat pass .." statements use together the pf_default_rule.
   Therefore we have "start != 0" in this case and we run the
   if-section of the code snippet but we better should run the
   else-section in this case and do not fetch the counter of the
   pf_default_rule but take the total number of states.

Thats what the patch does.


Regards
Andreas