netfilter-devel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Phil Sutter <phil@nwl.cc>
To: Pablo Neira Ayuso <pablo@netfilter.org>
Cc: netfilter-devel@vger.kernel.org, Eric Garver <e@erig.me>
Subject: [nft PATCH v4 4/7] src: Restore local entries after cache update
Date: Tue, 28 May 2019 23:03:20 +0200	[thread overview]
Message-ID: <20190528210323.14605-5-phil@nwl.cc> (raw)
In-Reply-To: <20190528210323.14605-1-phil@nwl.cc>

When batching up multiple commands, one may run into a situation where
the current command requires a cache update while the previous ones
didn't and that causes objects added by previous commands to be removed
from cache. If the current or any following command references any of
these objects, the command is rejected.

Resolve this by copying Florian's solution from iptables-nft: After
droping the whole cache and populating it again with entries fetched
from kernel, use the current list of commands to restore local entries
again.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
Changes since v2:
- Move cache_add_commands() and called helper functions to a better
  place, it calls cache_is_complete().
- Add error handling to cache_add_commands() and helper functions. This
  should cover the case where required ruleset parts have disappeared
  from kernel. Error reporting makes use of {table,chain}_not_found(),
  so prepare a struct eval_ctx which also serves fine for passing
  pointers around.
- Drop cache bug workaround in tests/json_echo.

Changes since v1:
- Don't add anonymous sets to cache when restoring, as suggested by Eric
  Garver.
---
 src/rule.c                  | 83 ++++++++++++++++++++++++++++++++++++-
 tests/json_echo/run-test.py |  6 +--
 2 files changed, 84 insertions(+), 5 deletions(-)

diff --git a/src/rule.c b/src/rule.c
index 966948cd7ae90..78e0388e41e93 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -242,6 +242,87 @@ static bool cache_is_updated(struct nft_cache *cache, uint16_t genid)
 	return genid && genid == cache->genid;
 }
 
+static int cache_add_table_cmd(struct eval_ctx *ectx)
+{
+	struct table *table;
+
+	if (table_lookup(&ectx->cmd->handle, &ectx->nft->cache))
+		return 0;
+
+	if (!ectx->cmd->table) {
+		table = table_alloc();
+		handle_merge(&table->handle, &ectx->cmd->handle);
+		table_add_hash(table, &ectx->nft->cache);
+	} else {
+		table_add_hash(table_get(ectx->cmd->table), &ectx->nft->cache);
+	}
+	return 0;
+}
+
+static int cache_add_chain_cmd(struct eval_ctx *ectx)
+{
+	struct table *table;
+	struct chain *chain;
+
+	table = table_lookup(&ectx->cmd->handle, &ectx->nft->cache);
+	if (!table)
+		return table_not_found(ectx);
+
+	if (!ectx->cmd->chain) {
+		if (!chain_lookup(table, &ectx->cmd->handle)) {
+			chain = chain_alloc(NULL);
+			handle_merge(&chain->handle, &ectx->cmd->handle);
+			chain_add_hash(chain, table);
+		}
+	} else if (!chain_lookup(table, &ectx->cmd->chain->handle)) {
+		chain_add_hash(chain_get(ectx->cmd->chain), table);
+	}
+	return 0;
+}
+
+static int cache_add_set_cmd(struct eval_ctx *ectx)
+{
+	struct table *table;
+
+	table = table_lookup(&ectx->cmd->handle, &ectx->nft->cache);
+	if (!table)
+		return table_not_found(ectx);
+
+	if (!set_lookup(table, ectx->cmd->set->handle.set.name))
+		set_add_hash(set_get(ectx->cmd->set), table);
+	return 0;
+}
+
+static int cache_add_commands(struct nft_ctx *nft, struct list_head *msgs)
+{
+	struct eval_ctx ectx = {
+		.nft = nft,
+		.msgs = msgs,
+	};
+	int ret = 0;
+
+	list_for_each_entry(ectx.cmd, &nft->cmds, list) {
+		switch (ectx.cmd->obj) {
+		case CMD_OBJ_TABLE:
+			ret = cache_add_table_cmd(&ectx);
+			break;
+		case CMD_OBJ_CHAIN:
+			ret = cache_add_chain_cmd(&ectx);
+			break;
+		case CMD_OBJ_SET:
+			if (ectx.cmd->set->flags & NFT_SET_ANONYMOUS)
+				continue;
+			ret = cache_add_set_cmd(&ectx);
+			break;
+		default:
+			break;
+		}
+		if (ret)
+			break;
+	}
+	return ret;
+}
+
 int cache_update(struct nft_ctx *nft, enum cmd_ops cmd, struct list_head *msgs)
 {
 	uint16_t genid;
@@ -275,7 +356,7 @@ replay:
 	}
 	cache->genid = genid;
 	cache->cmd = cmd;
-	return 0;
+	return cache_add_commands(nft, msgs);
 }
 
 static void __cache_flush(struct list_head *table_list)
diff --git a/tests/json_echo/run-test.py b/tests/json_echo/run-test.py
index 0132b1393860a..7d191292b4dd5 100755
--- a/tests/json_echo/run-test.py
+++ b/tests/json_echo/run-test.py
@@ -270,12 +270,10 @@ add_quota["add"]["quota"]["name"] = "q"
 do_flush()
 
 print "doing multi add"
-# XXX: Add table separately, otherwise this triggers cache bug
-out = do_command(add_table)
-thandle = get_handle(out, add_table["add"])
-add_multi = [ add_chain, add_set, add_rule ]
+add_multi = [ add_table, add_chain, add_set, add_rule ]
 out = do_command(add_multi)
 
+thandle = get_handle(out, add_table["add"])
 chandle = get_handle(out, add_chain["add"])
 shandle = get_handle(out, add_set["add"])
 rhandle = get_handle(out, add_rule["add"])
-- 
2.21.0


  parent reply	other threads:[~2019-05-28 21:03 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-28 21:03 [nft PATCH v4 0/7] Cache update fix && intra-transaction rule references Phil Sutter
2019-05-28 21:03 ` [nft PATCH v4 1/7] src: Fix cache_flush() in cache_needs_more() logic Phil Sutter
2019-05-28 21:32   ` Eric Garver
2019-05-28 22:23     ` Phil Sutter
2019-05-28 21:03 ` [nft PATCH v4 2/7] libnftables: Keep list of commands in nft context Phil Sutter
2019-05-28 21:03 ` [nft PATCH v4 3/7] src: Make {table,chain}_not_found() public Phil Sutter
2019-05-28 21:03 ` Phil Sutter [this message]
2019-05-28 21:03 ` [nft PATCH v4 5/7] rule: Introduce rule_lookup_by_index() Phil Sutter
2019-05-28 21:03 ` [nft PATCH v4 6/7] src: Make cache_is_complete() public Phil Sutter
2019-05-28 21:03 ` [nft PATCH v4 7/7] src: Support intra-transaction rule references Phil Sutter
2019-05-31 16:56   ` Eric Garver
2019-06-03 16:59     ` Pablo Neira Ayuso
2019-06-04  7:17       ` 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=20190528210323.14605-5-phil@nwl.cc \
    --to=phil@nwl.cc \
    --cc=e@erig.me \
    --cc=netfilter-devel@vger.kernel.org \
    --cc=pablo@netfilter.org \
    /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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).