Date: Mon, 18 Feb 2019 21:17:21 +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: <222311AF-CA32-4C78-8550-215D9B4360AC@FreeBSD.org> In-Reply-To: <5C6AEBB8.2030305@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>
next in thread | previous in thread | raw e-mail | index | archive | help
On 18 Feb 2019, at 18:30, Andreas Longwitz wrote:
>> Ok, thanks, I will commit the patch shortly. I do not see a point in
>> waiting
>> for two more weeks, sure report me if anything goes wrong.
>
> your patch for counter(9) on i386 definitely solves my problem
> discussed
> in this thread.
>
> 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?
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.
Regards,
Kristof
From owner-freebsd-pf@freebsd.org Tue Feb 19 21:53:16 2019
Return-Path: <owner-freebsd-pf@freebsd.org>
Delivered-To: freebsd-pf@mailman.ysv.freebsd.org
Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1])
by mailman.ysv.freebsd.org (Postfix) with ESMTP id D159214DAFA8
for <freebsd-pf@mailman.ysv.freebsd.org>; Tue, 19 Feb 2019 21:53:16 +0000 (UTC)
(envelope-from longwitz@incore.de)
Received: from dss.incore.de (dss.incore.de [195.145.1.138])
(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
(Client did not present a certificate)
by mx1.freebsd.org (Postfix) with ESMTPS id 673D376004;
Tue, 19 Feb 2019 21:53:15 +0000 (UTC)
(envelope-from longwitz@incore.de)
Received: from inetmail.dmz (inetmail.dmz [10.3.0.3])
by dss.incore.de (Postfix) with ESMTP id 54CE227DC5;
Tue, 19 Feb 2019 22:53:07 +0100 (CET)
X-Virus-Scanned: amavisd-new at incore.de
Received: from dss.incore.de ([10.3.0.3])
by inetmail.dmz (inetmail.dmz [10.3.0.3]) (amavisd-new, port 10024)
with LMTP id wLMHAAo4O-S3; Tue, 19 Feb 2019 22:53:06 +0100 (CET)
Received: from mail.local.incore (fwintern.dmz [10.0.0.253])
by dss.incore.de (Postfix) with ESMTP id 374D727DAD;
Tue, 19 Feb 2019 22:53:06 +0100 (CET)
Received: from bsdmhs.longwitz (unknown [192.168.99.6])
by mail.local.incore (Postfix) with ESMTP id DF8ED1D2;
Tue, 19 Feb 2019 22:53:05 +0100 (CET)
Message-ID: <5C6C7AC1.4080201@incore.de>
Date: Tue, 19 Feb 2019 22:53:05 +0100
From: Andreas Longwitz <longwitz@incore.de>
User-Agent: Thunderbird 2.0.0.19 (X11/20090113)
MIME-Version: 1.0
To: Kristof Provost <kp@FreeBSD.org>
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
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>
In-Reply-To: <222311AF-CA32-4C78-8550-215D9B4360AC@FreeBSD.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
X-Rspamd-Queue-Id: 673D376004
X-Spamd-Bar: --
Authentication-Results: mx1.freebsd.org;
spf=pass (mx1.freebsd.org: domain of longwitz@incore.de designates
195.145.1.138 as permitted sender) smtp.mailfrom=longwitz@incore.de
X-Spamd-Result: default: False [-2.26 / 15.00]; ARC_NA(0.00)[];
NEURAL_HAM_MEDIUM(-0.91)[-0.907,0]; RCVD_COUNT_FIVE(0.00)[5];
FROM_HAS_DN(0.00)[]; RCPT_COUNT_THREE(0.00)[4];
R_SPF_ALLOW(-0.20)[+mx]; TO_MATCH_ENVRCPT_ALL(0.00)[];
MIME_GOOD(-0.10)[text/plain]; RCVD_TLS_LAST(0.00)[];
DMARC_NA(0.00)[incore.de]; TO_DN_SOME(0.00)[];
NEURAL_HAM_LONG(-0.97)[-0.975,0];
IP_SCORE(0.04)[asn: 3320(0.21), country: DE(-0.01)];
MX_GOOD(-0.01)[dss.incore.de];
NEURAL_HAM_SHORT(-0.11)[-0.106,0];
RCVD_IN_DNSWL_NONE(0.00)[138.1.145.195.list.dnswl.org : 127.0.10.0];
FROM_EQ_ENVFROM(0.00)[]; R_DKIM_NA(0.00)[];
MIME_TRACE(0.00)[0:+];
ASN(0.00)[asn:3320, ipnet:195.145.0.0/16, country:DE];
MID_RHS_MATCH_FROM(0.00)[]
X-BeenThere: freebsd-pf@freebsd.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: "Technical discussion and general questions about packet filter
\(pf\)" <freebsd-pf.freebsd.org>
List-Unsubscribe: <https://lists.freebsd.org/mailman/options/freebsd-pf>,
<mailto:freebsd-pf-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/freebsd-pf/>;
List-Post: <mailto:freebsd-pf@freebsd.org>
List-Help: <mailto:freebsd-pf-request@freebsd.org?subject=help>
List-Subscribe: <https://lists.freebsd.org/mailman/listinfo/freebsd-pf>,
<mailto:freebsd-pf-request@freebsd.org?subject=subscribe>
X-List-Received-Date: Tue, 19 Feb 2019 21:53:17 -0000
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?222311AF-CA32-4C78-8550-215D9B4360AC>