Date: Sat, 23 Feb 2019 16:48:12 +0100 From: "Kristof Provost" <kp@FreeBSD.org> To: "Andreas Longwitz" <longwitz@incore.de> Cc: "Konstantin Belousov" <kib@freebsd.org>, freebsd-pf@freebsd.org, "Gleb Smirnoff" <glebius@freebsd.org> Subject: Re: rdr pass for proto tcp sometimes creates states with expire time zero and so breaking connections Message-ID: <051F7C53-CDC6-4A8E-87E5-EB4D22431EAC@FreeBSD.org> In-Reply-To: <5C6C7AC1.4080201@incore.de> References: <C4D1F141-2979-4103-957F-F0314637D978@sigsegv.be> <5BD45882.1000207@incore.de> <D5EEA773-1F0F-4FA0-A39A-486EE323907D@sigsegv.be> <5BEB3B9A.9080402@incore.de> <20181113222533.GJ9744@FreeBSD.org> <5C49ECAA.7060505@incore.de> <20190124203802.GU24863@kib.kiev.ua> <5C4A37A1.80206@incore.de> <20190125131409.GZ24863@kib.kiev.ua> <5C557065.10600@incore.de> <20190202184208.GG24863@kib.kiev.ua> <5C6AEBB8.2030305@incore.de> <222311AF-CA32-4C78-8550-215D9B4360AC@FreeBSD.org> <5C6C7AC1.4080201@incore.de>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?051F7C53-CDC6-4A8E-87E5-EB4D22431EAC>