Re: [PATCH nft 3/3] src: evaluate: return immediately if no op was requested

[Pablo Neira Ayuso <pablo@netfilter.org>]

[-- Attachment #1: Type: text/plain, Size: 1451 bytes --]

On Sun, Jul 21, 2019 at 08:50:40PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Sun, Jul 21, 2019 at 02:14:07AM +0200, Florian Westphal wrote:
> > > This makes nft behave like 0.9.0 -- the ruleset
> > > 
> > > flush ruleset
> > > table inet filter {
> > > }
> > > table inet filter {
> > >       chain test {
> > >         counter
> > >     }
> > > }
> > > 
> > > loads again without generating an error message.
> > > I've added a test case for this, without this it will create an error,
> > > and with a checkout of the 'fixes' tag we get crash.
> > > 
> > > Closes: https://bugzilla.netfilter.org/show_bug.cgi?id=1351
> > > Fixes: e5382c0d08e3c ("src: Support intra-transaction rule references")
> > 
> > This one is causing the cache corruption, right?
> 
> There is no cache corruption.  This patch makes us enter a code
> path that we did not take before.

Sorry, I mean, this is a cache bug :-)

cache_flush() is cheating, it sets flags to CACHE_FULL, hence this
enters this codepath we dit not take before. This propagates from the
previous logic, as a workaround.

I made this patch, to skip the cache in case "flush ruleset" is
requested.

This breaks testcases/transactions/0024rule_0, which is a recent test
from Phil to check for intra-transaction references, I don't know yet
what makes this code unhappy with my changes.

Phil, would you help me have a look at what assumption breaks? Thanks.

[-- Attachment #2: x.patch --]
[-- Type: text/x-diff, Size: 2993 bytes --]

diff --git a/include/cache.h b/include/cache.h
index d3502a8a6039..526f6ca57f74 100644
--- a/include/cache.h
+++ b/include/cache.h
@@ -30,6 +30,7 @@ enum cache_level_flags {
 				  NFT_CACHE_CHAIN_BIT |
 				  NFT_CACHE_RULE_BIT,
 	NFT_CACHE_FULL		= __NFT_CACHE_MAX_BIT - 1,
+	NFT_CACHE_FLUSHED	= (1 << 31),
 };
 
 #endif /* _NFT_CACHE_H_ */
diff --git a/include/rule.h b/include/rule.h
index 67c3d3314953..d66e03456ad2 100644
--- a/include/rule.h
+++ b/include/rule.h
@@ -679,7 +679,6 @@ extern int do_command(struct netlink_ctx *ctx, struct cmd *cmd);
 extern unsigned int cache_evaluate(struct nft_ctx *nft, struct list_head *cmds);
 extern int cache_update(struct nft_ctx *ctx, enum cmd_ops cmd,
 			struct list_head *msgs);
-extern void cache_flush(struct nft_ctx *ctx, struct list_head *msgs);
 extern void cache_release(struct nft_cache *cache);
 extern bool cache_is_complete(struct nft_cache *cache, enum cmd_ops cmd);
 
diff --git a/src/cache.c b/src/cache.c
index e04ead85c830..2f16eee17780 100644
--- a/src/cache.c
+++ b/src/cache.c
@@ -72,6 +72,8 @@ static unsigned int evaluate_cache_flush(struct cmd *cmd, unsigned int flags)
 		flags |= NFT_CACHE_SET;
 		break;
 	case CMD_OBJ_RULESET:
+		flags |= NFT_CACHE_FLUSHED;
+		break;
 	default:
 		flags = NFT_CACHE_EMPTY;
 		break;
diff --git a/src/evaluate.c b/src/evaluate.c
index c6cc6ccad75d..b83c77ae4991 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -3776,7 +3776,6 @@ static int cmd_evaluate_flush(struct eval_ctx *ctx, struct cmd *cmd)
 
 	switch (cmd->obj) {
 	case CMD_OBJ_RULESET:
-		cache_flush(ctx->nft, ctx->msgs);
 		break;
 	case CMD_OBJ_TABLE:
 		/* Flushing a table does not empty the sets in the table nor remove
diff --git a/src/rule.c b/src/rule.c
index b957b4571249..d8a243342434 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -234,6 +234,11 @@ static bool cache_is_updated(struct nft_cache *cache, uint16_t genid)
 	return genid && genid == cache->genid;
 }
 
+static bool cache_is_flushed(struct nft_cache *cache)
+{
+	return cache->flags & NFT_CACHE_FLUSHED;
+}
+
 int cache_update(struct nft_ctx *nft, unsigned int flags, struct list_head *msgs)
 {
 	struct netlink_ctx ctx = {
@@ -255,6 +260,9 @@ replay:
 	if (cache->genid)
 		cache_release(cache);
 
+	if (cache_is_flushed(cache))
+		goto skip;
+
 	ret = cache_init(&ctx, flags);
 	if (ret < 0) {
 		cache_release(cache);
@@ -270,7 +278,7 @@ replay:
 		cache_release(cache);
 		goto replay;
 	}
-
+skip:
 	cache->genid = genid;
 	cache->flags = flags;
 	return 0;
@@ -286,20 +294,6 @@ static void __cache_flush(struct list_head *table_list)
 	}
 }
 
-void cache_flush(struct nft_ctx *nft, struct list_head *msgs)
-{
-	struct netlink_ctx ctx = {
-		.list		= LIST_HEAD_INIT(ctx.list),
-		.nft		= nft,
-		.msgs		= msgs,
-	};
-	struct nft_cache *cache = &nft->cache;
-
-	__cache_flush(&cache->list);
-	cache->genid = mnl_genid_get(&ctx);
-	cache->flags = NFT_CACHE_FULL;
-}
-
 void cache_release(struct nft_cache *cache)
 {
 	__cache_flush(&cache->list);