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

Kristof Provost kp at FreeBSD.org
Sat Feb 23 15:48:19 UTC 2019


On 19 Feb 2019, at 22:53, Andreas Longwitz wrote:
> 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.
>
Thank you, that makes sense. I’d missed the third case.

The patch is in my queue and should get committed soon. Your explanation 
makes a great commit message.

Regards,
Kristof


More information about the freebsd-pf mailing list