Date: Mon, 13 Aug 2018 15:22:33 +0200 From: "Kristof Provost" <kp@FreeBSD.org> To: "Kajetan Staszkiewicz" <vegeta@tuxpowered.net> Cc: freebsd-pf@freebsd.org Subject: Re: pf tables locking Message-ID: <18F24996-29D6-4792-BCB7-88738F756077@FreeBSD.org> In-Reply-To: <8680316.SccKl5VnxN@energia> References: <8680316.SccKl5VnxN@energia>
next in thread | previous in thread | raw e-mail | index | archive | help
On 13 Aug 2018, at 0:09, Kajetan Staszkiewicz wrote: > Hello group, > > Can anybody help me iwth pf_table.c and all operations on tables, > especially > pfr_update_stats? I'm working on implementing stats for redirection > targets, > that is for nat or route-to. > > I'm going through the code and I've found out that many table-related > function > are guarded by lock on pf ruleset. But that is not true for > pfr_update_stats. > This function is called from pf_test only after PF_RULES_RUNLOCK(). > I think you’re right, this does look wrong. It’s very unlikely that this will actually lead to a crash, because rules (and associated tables) won’t just go away while there’s still state, but we could theoretically lose memory (in the pfrke_counters allocation), and miscount. I don’t want to re-take the rules lock for this, so my current thinking is that the best approach would be to already get rid of the potential memory leak by just always allocating the pfrke_counters when the table is created (i.e. when the rule is first set). That might waste a little memory if we didn’t need it, but it should simplify things a bit. We can resolve the counting issue by using the counter_u64_*() functions for them. We should be able to get away with not locking this. Regards, Kristof From owner-freebsd-pf@freebsd.org Mon Aug 13 15:06:52 2018 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 DF5B51075279 for <freebsd-pf@mailman.ysv.freebsd.org>; Mon, 13 Aug 2018 15:06:51 +0000 (UTC) (envelope-from vegeta@tuxpowered.net) Received: from mail-ed1-x544.google.com (mail-ed1-x544.google.com [IPv6:2a00:1450:4864:20::544]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 6A8577D959 for <freebsd-pf@freebsd.org>; Mon, 13 Aug 2018 15:06:51 +0000 (UTC) (envelope-from vegeta@tuxpowered.net) Received: by mail-ed1-x544.google.com with SMTP id s24-v6so8426720edr.8 for <freebsd-pf@freebsd.org>; Mon, 13 Aug 2018 08:06:51 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=tuxpowered-net.20150623.gappssmtp.com; s=20150623; h=from:to:cc:subject:date:message-id:organization:user-agent :in-reply-to:references:mime-version; bh=jM/Vo7z6SqJwDuoP8S6zF/nomT2qPu3zVJ5GAkg4K1g=; b=BUhieC/L7aOQfY2dI9+i60OS70bHf3c+CDtZj7fPTeREA1ZnQmgGQaZiPOfMOp6BPC f8AHU5Fq5YHndPKTn19q+PcYCrZgsTlEeDVg1Ft0GfIWz8lOaVV8OwASId2bbuAc9mev MlFtTcrS9icIyh7SeXu0kOs2LAkty1TNcAzAJw/K2Me1BD6fIgNiLCNQUddloONLdOZZ LdhtlxQyme4lirB1qtXkyx6LT3QIvJoy4QjRPp4jFIDFJrhFCpHc7muyPdrjM4rRdDN1 uIIr0vEjaDBENRTQY1z3O2rCyVci9kO4YoiPIswRTTvDFT9+u6nOLNpF5cxkaH45blq5 PsFg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:organization :user-agent:in-reply-to:references:mime-version; bh=jM/Vo7z6SqJwDuoP8S6zF/nomT2qPu3zVJ5GAkg4K1g=; b=jFgB0/Blw8dd1MaArz/dcSPRRknmx1hs1TmWLHZH13Hxj+GwYiv3kUsEztCLiasLgs Qq7aDMpA2bMaeXGA3k4I+aiLW3esRpyLkyjmbEJf8jWP5pybDyiI2JvM84lUmXRkeJVd CNMFcJWHff2SPiMiF3Rrg2xB+lrHSu7KSHYDDWHN+kbGvPBphKgpOvVOczMI+czxOi/P SpsiiVWDhXHZKKPGyhXrywnKhePZ8nsiOm0ai2S9ZP6i/BzlIzJBaax4NZj/ZdoPniiv I6keYJwbKK1/Bs1b+mAs7iojnUdhU3klAtdab3hnmmbnRL6fAUTebUjAwb06PyPZQxCr toig== X-Gm-Message-State: AOUpUlHK8ZNha0SX7jfWM2TBSalk8DPvw+6Gn+SeLfwQKKb+i8ztsDX6 1lGHvDj6xHdshx7tHskwLUp0vCFp1F8= X-Google-Smtp-Source: AA+uWPzp0dcBI3Axb5bbMEx/0YbUjyvS72qakyxr4LSUxoNKsNTDnE7NpO41pt+oRp0lev7XsYR28w== X-Received: by 2002:a50:a1c6:: with SMTP id 64-v6mr22042656edk.309.1534172810227; Mon, 13 Aug 2018 08:06:50 -0700 (PDT) Received: from energia.localnet ([212.48.107.10]) by smtp.gmail.com with ESMTPSA id c21-v6sm13074434eda.21.2018.08.13.08.06.48 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 13 Aug 2018 08:06:49 -0700 (PDT) From: Kajetan Staszkiewicz <vegeta@tuxpowered.net> To: Kristof Provost <kp@freebsd.org> Cc: freebsd-pf@freebsd.org Subject: Re: pf tables locking Date: Mon, 13 Aug 2018 17:06:45 +0200 Message-ID: <2313127.kTuY2QdDqf@energia> Organization: tuxpowered.net User-Agent: KMail/5.2.3 (Linux/4.16.0-16.2-liquorix-amd64; KDE/5.28.0; x86_64; ; ) In-Reply-To: <18F24996-29D6-4792-BCB7-88738F756077@FreeBSD.org> References: <8680316.SccKl5VnxN@energia> <18F24996-29D6-4792-BCB7-88738F756077@FreeBSD.org> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="nextPart2812603.zyBN6blRsM"; micalg="pgp-sha1"; protocol="application/pgp-signature" X-BeenThere: freebsd-pf@freebsd.org X-Mailman-Version: 2.1.27 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: Mon, 13 Aug 2018 15:06:52 -0000 --nextPart2812603.zyBN6blRsM Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="UTF-8" On Monday, 13 August 2018 15:22:33 CEST Kristof Provost wrote: > > I'm going through the code and I've found out that many table-related > > function > > are guarded by lock on pf ruleset. But that is not true for > > pfr_update_stats. > > This function is called from pf_test only after PF_RULES_RUNLOCK(). >=20 > I think you=E2=80=99re right, this does look wrong. >=20 > It=E2=80=99s very unlikely that this will actually lead to a crash, becau= se I don't like the word "unlikely". With my traffic and frequent ruleset and= =20 carp changes I'm catching all the fanciest locking bugs as it seems. > rules (and associated tables) won=E2=80=99t just go away while there=E2= =80=99s still > state, This is mostly what I wanted to ask about in this message. How is it ensure= d=20 that table and counters are gone only after everybody stops using them? Wha= t=20 if I delete a table, then change ruleset, but there is still active connect= ion=20 keeping a state? I really had hard time finding how this is guarded in sour= ce. > but we could theoretically lose memory (in the pfrke_counters > allocation), and miscount. Pre-allocating counters seems a good idea, it will simplify some other code. > I don=E2=80=99t want to re-take the rules lock for this, so my current > thinking is that the best approach would be to already get rid of the > potential memory leak by just always allocating the pfrke_counters when > the table is created (i.e. when the rule is first set). That might waste > a little memory if we didn=E2=80=99t need it, but it should simplify thin= gs a > bit. =20 > We can resolve the counting issue by using the counter_u64_*() functions > for them. We should be able to get away with not locking this. Sure, I can use counter(9). The question, as always with my patches, is wha= t=20 can go to FreeBSD and what won't go. My current goal is to modify round-robin pf target to always point to table= =20 entry with least amount of states. As I see it for now: 1. Modify pfrke_counters to be always allocated. 2. Rewrite pfrke_counters to use counter(9). 3. Provide state counter in pfrke_counters. 4. Modify round-robin target. 1. and 2. make a good PR. I'm not sure about 3. Do you want patches for lea= st- connections target too? I want to just replace existing round-robin but if= =20 there is any chance of getting it into kernel code, I could make it work as= =20 new target in pf.conf. Point 3. is the puzzle for me. For now I just call pfr_update_stats (modifi= ed=20 to handle state counter) in pf_create_state and pf_unlink_state. But again = =2D=20 how do I know if the table (I added a pointer in struct pf_state) is still= =20 allocated in memory? There are some more issues I found around pf_map_addr. Some of them I=20 mentioned in https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=3D229092. So= me=20 more came out while working on this least-states loadbalancing. I will grou= p=20 them into something meaningful and make another PR for them. =2D-=20 | pozdrawiam / greetings | powered by Debian, FreeBSD and CentOS | | Kajetan Staszkiewicz | jabber,email: vegeta()tuxpowered net | | Vegeta | www: http://vegeta.tuxpowered.net | `------------------------^---------------------------------------' --nextPart2812603.zyBN6blRsM Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part. Content-Transfer-Encoding: 7Bit -----BEGIN PGP SIGNATURE----- iF0EABECAB0WIQSOEQZObv2B8mf0JbnjtFCvbXs6FAUCW3GehQAKCRDjtFCvbXs6 FPiMAKCWbU5HlmpRZdlci0l3fXFYW6Ic+ACeNjCElC40Fw7z5NKxpqZjplZKDZg= =yHP4 -----END PGP SIGNATURE----- --nextPart2812603.zyBN6blRsM--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?18F24996-29D6-4792-BCB7-88738F756077>