From: Pablo Neira Ayuso <pablo@netfilter.org>
To: Phil Sutter <phil@nwl.cc>, netfilter-devel@vger.kernel.org
Subject: Re: [iptables PATCH 1/4] nft: cache: Fix nft_release_cache() under stress
Date: Tue, 3 Mar 2020 21:55:54 +0100 [thread overview]
Message-ID: <20200303205554.oc5dwigdvje6whed@salvia> (raw)
In-Reply-To: <20200303010252.GB5627@orbyte.nwl.cc>
On Tue, Mar 03, 2020 at 02:02:52AM +0100, Phil Sutter wrote:
> Hi Pablo,
>
> On Mon, Mar 02, 2020 at 08:19:30PM +0100, Pablo Neira Ayuso wrote:
> > On Mon, Mar 02, 2020 at 06:53:55PM +0100, Phil Sutter wrote:
> > > iptables-nft-restore calls nft_action(h, NFT_COMPAT_COMMIT) for each
> > > COMMIT line in input. When restoring a dump containing multiple large
> > > tables, chances are nft_rebuild_cache() has to run multiple times.
It is true that chances that this code runs multiple times since the
new fine-grain caching logic is in place.
> > Then, fix nft_rebuild_cache() please.
>
> This is not the right place to fix the problem: nft_rebuild_cache()
> simply rebuilds the cache, switching to a secondary instance if not done
> so before to avoid freeing objects referenced from batch jobs.
>
> When creating batch jobs (e.g., adding a rule or chain), code is not
> aware of which cache instance is currently in use. It will just add
> those objects to nft_handle->cache pointer.
>
> It is the job of nft_release_cache() to return things back to normal
> after each COMMIT line, which includes restoring nft_handle->cache
> pointer to point at first cache instance.
>
> If you see a flaw in my reasoning, I'm all ears. Also, if you see a
> better solution, please elaborate - IMO, nft_release_cache() should undo
> what nft_rebuild_cache() may have done. From nft_action() perspective,
> they are related.
Would this patch still work after this series are applied:
https://patchwork.ozlabs.org/project/netfilter-devel/list/?series=151404
That is working and passing tests. It is just missing the code to
restore the fine grain dumping, that should be easy to add.
That logic will really reduce the chances to exercise all this cache
dump / cache cancel. Bugs in this cache consistency code is usually
not that easy to trigger and usually hard to fix.
I just think it would be a pity if that work ends up in the trash can.
next prev parent reply other threads:[~2020-03-03 20:55 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-02 17:53 [iptables PATCH 0/4] Fix for iptables-nft-restore under pressure Phil Sutter
2020-03-02 17:53 ` [iptables PATCH 1/4] nft: cache: Fix nft_release_cache() under stress Phil Sutter
2020-03-02 19:19 ` Pablo Neira Ayuso
2020-03-03 1:02 ` Phil Sutter
2020-03-03 20:55 ` Pablo Neira Ayuso [this message]
2020-03-04 2:13 ` Phil Sutter
2020-03-04 17:02 ` Pablo Neira Ayuso
2020-03-02 17:53 ` [iptables PATCH 2/4] nft: cache: Make nft_rebuild_cache() respect fake cache Phil Sutter
2020-03-02 19:26 ` Pablo Neira Ayuso
2020-03-03 1:15 ` Phil Sutter
2020-03-02 17:53 ` [iptables PATCH 3/4] nft: cache: Simplify chain list allocation Phil Sutter
2020-03-02 17:53 ` [iptables PATCH 4/4] nft: cache: Review flush_cache() Phil Sutter
2020-03-02 19:22 ` Pablo Neira Ayuso
2020-03-03 1:22 ` Phil Sutter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200303205554.oc5dwigdvje6whed@salvia \
--to=pablo@netfilter.org \
--cc=netfilter-devel@vger.kernel.org \
--cc=phil@nwl.cc \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.