[iptables PATCH] nft: cache: Fix iptables-save segfault under stress

[iptables PATCH] nft: cache: Fix iptables-save segfault under stress

[Phil Sutter]

If kernel ruleset is constantly changing, code called by
nft_is_table_compatible() may crash: For each item in table's chain
list, nft_is_chain_compatible() is called. This in turn calls
nft_build_cache() to fetch chain's rules. Though if kernel genid has changed
meanwhile, cache is flushed and rebuilt from scratch, thereby freeing
table's chain list - the foreach loop in nft_is_table_compatible() then
operates on freed memory.

A simple reproducer (may need a few calls):

| RULESET='*filter
| :INPUT ACCEPT [10517:1483527]
| :FORWARD ACCEPT [0:0]
| :OUTPUT ACCEPT [1714:105671]
| COMMIT
| '
|
| for ((i = 0; i < 100; i++)); do
|         iptables-nft-restore <<< "$RULESET" &
| done &
| iptables-nft-save

To fix the problem, basically revert commit ab1cd3b510fa5 ("nft: ensure
cache consistency") so that __nft_build_cache() no longer flushes the
cache. Instead just record kernel's genid when fetching for the first
time. If kernel rule set changes until the changes are committed, the
commit simply fails and local cache is being rebuilt.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
Pablo,

This is a similar situation as with commit c550c81fd373e ("nft: cache:
Fix nft_release_cache() under stress"): Your caching rewrite avoids this
problem as well, but kills some of my performance improvements. I'm
currently working on restoring them with your approach but that's not a
quick "fix". Can we go with this patch until I'm done?

Cheers, Phil
---
 iptables/nft-cache.c | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/iptables/nft-cache.c b/iptables/nft-cache.c
index e3c9655739187..a0c76705c848e 100644
--- a/iptables/nft-cache.c
+++ b/iptables/nft-cache.c
@@ -449,15 +449,11 @@ __nft_build_cache(struct nft_handle *h, enum nft_cache_level level,
 		  const struct builtin_table *t, const char *set,
 		  const char *chain)
 {
-	uint32_t genid_start, genid_stop;
-
 	if (level <= h->cache_level)
 		return;
-retry:
-	mnl_genid_get(h, &genid_start);
 
-	if (h->cache_level && genid_start != h->nft_genid)
-		flush_chain_cache(h, NULL);
+	if (!h->nft_genid)
+		mnl_genid_get(h, &h->nft_genid);
 
 	switch (h->cache_level) {
 	case NFT_CL_NONE:
@@ -486,18 +482,10 @@ retry:
 		break;
 	}
 
-	mnl_genid_get(h, &genid_stop);
-	if (genid_start != genid_stop) {
-		flush_chain_cache(h, NULL);
-		goto retry;
-	}
-
 	if (!t && !chain)
 		h->cache_level = level;
 	else if (h->cache_level < NFT_CL_TABLES)
 		h->cache_level = NFT_CL_TABLES;
-
-	h->nft_genid = genid_start;
 }
 
 void nft_build_cache(struct nft_handle *h, struct nftnl_chain *c)
-- 
2.25.1

Re: [iptables PATCH] nft: cache: Fix iptables-save segfault under stress

[Pablo Neira Ayuso]

On Fri, Mar 13, 2020 at 06:42:06PM +0100, Phil Sutter wrote:
> If kernel ruleset is constantly changing, code called by
> nft_is_table_compatible() may crash: For each item in table's chain
> list, nft_is_chain_compatible() is called. This in turn calls
> nft_build_cache() to fetch chain's rules. Though if kernel genid has changed
> meanwhile, cache is flushed and rebuilt from scratch, thereby freeing
> table's chain list - the foreach loop in nft_is_table_compatible() then
> operates on freed memory.
> 
> A simple reproducer (may need a few calls):
> 
> | RULESET='*filter
> | :INPUT ACCEPT [10517:1483527]
> | :FORWARD ACCEPT [0:0]
> | :OUTPUT ACCEPT [1714:105671]
> | COMMIT
> | '
> |
> | for ((i = 0; i < 100; i++)); do
> |         iptables-nft-restore <<< "$RULESET" &
> | done &
> | iptables-nft-save
> 
> To fix the problem, basically revert commit ab1cd3b510fa5 ("nft: ensure
> cache consistency") so that __nft_build_cache() no longer flushes the
> cache. Instead just record kernel's genid when fetching for the first
> time. If kernel rule set changes until the changes are committed, the
> commit simply fails and local cache is being rebuilt.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
> Pablo,
> 
> This is a similar situation as with commit c550c81fd373e ("nft: cache:
> Fix nft_release_cache() under stress"): Your caching rewrite avoids this
> problem as well, but kills some of my performance improvements. I'm
> currently working on restoring them with your approach but that's not a
> quick "fix". Can we go with this patch until I'm done?

Fair enough.