[nft PATCH v4 0/7] Cache update fix && intra-transaction rule references

[nft PATCH v4 0/7] Cache update fix && intra-transaction rule references

[Phil Sutter]

This series combines the two series submitted earlier since they became
closely related in this iteration.

Patch 1 fixes a basic problem with cache_flush() after Eric's
cache_needs_more() change.

Patches 2, 3, 5 and 6 are requirements for patches 4 and 7 which are the
interesting ones: Patch 4 restores needed cache entries from command
list after a cache update. Patch 7 enables referencing a rule added by
the same transaction from another new rule by further exploiting the
logic added by patch 4.

Changes since v2 of "Resolve cache update woes" and v1 of "Support
intra-transaction rule references":

- Adjust cache_release() just like cache_flush().
- Split preparation work into separate patches.
- Adjust cache_add_commands() for later reuse by rule reference code,
  also add error handling in case kernel ruleset changes incompatibly.
- Finally drop that workaround in tests/json_echo.
- Introduce rule_cache_update() as requested.
- Avoid fetching a full cache if the new rule does not contain any
  reference.

Phil Sutter (7):
  src: Fix cache_flush() in cache_needs_more() logic
  libnftables: Keep list of commands in nft context
  src: Make {table,chain}_not_found() public
  src: Restore local entries after cache update
  rule: Introduce rule_lookup_by_index()
  src: Make cache_is_complete() public
  src: Support intra-transaction rule references

 include/nftables.h                            |   1 +
 include/rule.h                                |  12 ++
 src/evaluate.c                                | 107 +++++++-----
 src/libnftables.c                             |  21 ++-
 src/mnl.c                                     |   4 +
 src/rule.c                                    | 152 +++++++++++++++++-
 tests/json_echo/run-test.py                   |   6 +-
 .../shell/testcases/cache/0003_cache_update_0 |   7 +
 .../shell/testcases/nft-f/0006action_object_0 |   2 +-
 tests/shell/testcases/transactions/0024rule_0 |  17 ++
 .../transactions/dumps/0024rule_0.nft         |   8 +
 11 files changed, 280 insertions(+), 57 deletions(-)
 create mode 100755 tests/shell/testcases/transactions/0024rule_0
 create mode 100644 tests/shell/testcases/transactions/dumps/0024rule_0.nft

-- 
2.21.0

[nft PATCH v4 1/7] src: Fix cache_flush() in cache_needs_more() logic

[Phil Sutter]

Commit 34a20645d54fa enabled cache updates depending on command causing
it. As a side-effect, this disabled measures in cache_flush() preventing
a later cache update. Re-establish this by setting cache->cmd in
addition to cache->genid after dropping cache entries.

While being at it, set cache->cmd in cache_release() as well. This
shouldn't be necessary since zeroing cache->genid should suffice for
cache_update(), but better be consistent (and future-proof) here.

Fixes: 34a20645d54fa ("src: update cache if cmd is more specific")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
Changes since v1:
- Adjust cache_release() as well, just to make sure.
---
 src/rule.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/rule.c b/src/rule.c
index 326edb5dd459a..966948cd7ae90 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -299,12 +299,15 @@ void cache_flush(struct nft_ctx *nft, enum cmd_ops cmd, struct list_head *msgs)
 
 	__cache_flush(&cache->list);
 	cache->genid = mnl_genid_get(&ctx);
+	cache->cmd = CMD_LIST;
 }
 
 void cache_release(struct nft_cache *cache)
 {
 	__cache_flush(&cache->list);
 	cache->genid = 0;
+	cache->cmd = CMD_INVALID;
+
 }
 
 /* internal ID to uniquely identify a set in the batch */
-- 
2.21.0

Re: [nft PATCH v4 1/7] src: Fix cache_flush() in cache_needs_more() logic

[Eric Garver]

On Tue, May 28, 2019 at 11:03:17PM +0200, Phil Sutter wrote:
> Commit 34a20645d54fa enabled cache updates depending on command causing

Actual hash is eeda228c2d17. 

> it. As a side-effect, this disabled measures in cache_flush() preventing
> a later cache update. Re-establish this by setting cache->cmd in
> addition to cache->genid after dropping cache entries.
> 
> While being at it, set cache->cmd in cache_release() as well. This
> shouldn't be necessary since zeroing cache->genid should suffice for
> cache_update(), but better be consistent (and future-proof) here.
> 
> Fixes: 34a20645d54fa ("src: update cache if cmd is more specific")

Actual hash is eeda228c2d17. 

> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
> Changes since v1:
> - Adjust cache_release() as well, just to make sure.
> ---
>  src/rule.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/src/rule.c b/src/rule.c
> index 326edb5dd459a..966948cd7ae90 100644
> --- a/src/rule.c
> +++ b/src/rule.c
> @@ -299,12 +299,15 @@ void cache_flush(struct nft_ctx *nft, enum cmd_ops cmd, struct list_head *msgs)
>  
>  	__cache_flush(&cache->list);
>  	cache->genid = mnl_genid_get(&ctx);
> +	cache->cmd = CMD_LIST;
>  }
>  
>  void cache_release(struct nft_cache *cache)
>  {
>  	__cache_flush(&cache->list);
>  	cache->genid = 0;
> +	cache->cmd = CMD_INVALID;
> +
>  }
>  
>  /* internal ID to uniquely identify a set in the batch */
> -- 
> 2.21.0

Otherwise looks good.

Re: [nft PATCH v4 1/7] src: Fix cache_flush() in cache_needs_more() logic

[Phil Sutter]

On Tue, May 28, 2019 at 05:32:44PM -0400, Eric Garver wrote:
> On Tue, May 28, 2019 at 11:03:17PM +0200, Phil Sutter wrote:
> > Commit 34a20645d54fa enabled cache updates depending on command causing
> 
> Actual hash is eeda228c2d17. 

Oh, thanks for the catch! No idea where the other hash came from, but it
exists in my repository at least.

Thanks, Phil

[nft PATCH v4 2/7] libnftables: Keep list of commands in nft context

[Phil Sutter]

To fix the pending issues with cache updates, the list of commands needs
to be accessible from within cache_update(). In theory, there is a path
via nft->state->cmds but that struct parser_state is used (and
initialized) by bison parser only so that does not work with JSON
parser.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/nftables.h |  1 +
 src/libnftables.c  | 21 ++++++++++-----------
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/include/nftables.h b/include/nftables.h
index bb9bb2091716d..faacf26509104 100644
--- a/include/nftables.h
+++ b/include/nftables.h
@@ -102,6 +102,7 @@ struct nft_ctx {
 	struct parser_state	*state;
 	void			*scanner;
 	void			*json_root;
+	struct list_head	cmds;
 	FILE			*f[MAX_INCLUDE_DEPTH];
 };
 
diff --git a/src/libnftables.c b/src/libnftables.c
index 199dbc97b801c..f6ea668f6770a 100644
--- a/src/libnftables.c
+++ b/src/libnftables.c
@@ -143,6 +143,7 @@ struct nft_ctx *nft_ctx_new(uint32_t flags)
 	nft_ctx_add_include_path(ctx, DEFAULT_INCLUDE_PATH);
 	ctx->parser_max_errors	= 10;
 	init_list_head(&ctx->cache.list);
+	init_list_head(&ctx->cmds);
 	ctx->flags = flags;
 	ctx->output.output_fp = stdout;
 	ctx->output.error_fp = stderr;
@@ -342,7 +343,7 @@ static int nft_parse_bison_buffer(struct nft_ctx *nft, const char *buf,
 	struct cmd *cmd;
 	int ret;
 
-	parser_init(nft, nft->state, msgs, cmds);
+	parser_init(nft, nft->state, msgs, &nft->cmds);
 	nft->scanner = scanner_init(nft->state);
 	scanner_push_buffer(nft->scanner, &indesc_cmdline, buf);
 
@@ -381,7 +382,6 @@ int nft_run_cmd_from_buffer(struct nft_ctx *nft, const char *buf)
 {
 	struct cmd *cmd, *next;
 	LIST_HEAD(msgs);
-	LIST_HEAD(cmds);
 	char *nlbuf;
 	int rc = -EINVAL;
 
@@ -389,17 +389,17 @@ int nft_run_cmd_from_buffer(struct nft_ctx *nft, const char *buf)
 	sprintf(nlbuf, "%s\n", buf);
 
 	if (nft_output_json(&nft->output))
-		rc = nft_parse_json_buffer(nft, nlbuf, &msgs, &cmds);
+		rc = nft_parse_json_buffer(nft, nlbuf, &msgs, &nft->cmds);
 	if (rc == -EINVAL)
-		rc = nft_parse_bison_buffer(nft, nlbuf, &msgs, &cmds);
+		rc = nft_parse_bison_buffer(nft, nlbuf, &msgs, &nft->cmds);
 	if (rc)
 		goto err;
 
-	if (nft_netlink(nft, &cmds, &msgs, nft->nf_sock) != 0)
+	if (nft_netlink(nft, &nft->cmds, &msgs, nft->nf_sock) != 0)
 		rc = -1;
 err:
 	erec_print_list(&nft->output, &msgs, nft->debug_mask);
-	list_for_each_entry_safe(cmd, next, &cmds, list) {
+	list_for_each_entry_safe(cmd, next, &nft->cmds, list) {
 		list_del(&cmd->list);
 		cmd_free(cmd);
 	}
@@ -421,7 +421,6 @@ int nft_run_cmd_from_filename(struct nft_ctx *nft, const char *filename)
 {
 	struct cmd *cmd, *next;
 	LIST_HEAD(msgs);
-	LIST_HEAD(cmds);
 	int rc;
 
 	rc = cache_update(nft, CMD_INVALID, &msgs);
@@ -433,17 +432,17 @@ int nft_run_cmd_from_filename(struct nft_ctx *nft, const char *filename)
 
 	rc = -EINVAL;
 	if (nft_output_json(&nft->output))
-		rc = nft_parse_json_filename(nft, filename, &msgs, &cmds);
+		rc = nft_parse_json_filename(nft, filename, &msgs, &nft->cmds);
 	if (rc == -EINVAL)
-		rc = nft_parse_bison_filename(nft, filename, &msgs, &cmds);
+		rc = nft_parse_bison_filename(nft, filename, &msgs, &nft->cmds);
 	if (rc)
 		goto err;
 
-	if (nft_netlink(nft, &cmds, &msgs, nft->nf_sock) != 0)
+	if (nft_netlink(nft, &nft->cmds, &msgs, nft->nf_sock) != 0)
 		rc = -1;
 err:
 	erec_print_list(&nft->output, &msgs, nft->debug_mask);
-	list_for_each_entry_safe(cmd, next, &cmds, list) {
+	list_for_each_entry_safe(cmd, next, &nft->cmds, list) {
 		list_del(&cmd->list);
 		cmd_free(cmd);
 	}
-- 
2.21.0

[nft PATCH v4 3/7] src: Make {table,chain}_not_found() public

[Phil Sutter]

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/rule.h | 3 +++
 src/evaluate.c | 4 ++--
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/include/rule.h b/include/rule.h
index 8e70c129fcce0..61aa040a2e891 100644
--- a/include/rule.h
+++ b/include/rule.h
@@ -646,4 +646,7 @@ struct timeout_protocol {
 extern struct timeout_protocol timeout_protocol[IPPROTO_MAX];
 extern int timeout_str2num(uint16_t l4proto, struct timeout_state *ts);
 
+int table_not_found(struct eval_ctx *ctx);
+int chain_not_found(struct eval_ctx *ctx);
+
 #endif /* NFTABLES_RULE_H */
diff --git a/src/evaluate.c b/src/evaluate.c
index 55fb3b6131e04..09bb1fd37a301 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -166,7 +166,7 @@ static struct table *table_lookup_global(struct eval_ctx *ctx)
 	return table;
 }
 
-static int table_not_found(struct eval_ctx *ctx)
+int table_not_found(struct eval_ctx *ctx)
 {
 	struct table *table;
 
@@ -181,7 +181,7 @@ static int table_not_found(struct eval_ctx *ctx)
 			 family2str(table->handle.family));
 }
 
-static int chain_not_found(struct eval_ctx *ctx)
+int chain_not_found(struct eval_ctx *ctx)
 {
 	const struct table *table;
 	struct chain *chain;
-- 
2.21.0

[nft PATCH v4 4/7] src: Restore local entries after cache update

[Phil Sutter]

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

[nft PATCH v4 5/7] rule: Introduce rule_lookup_by_index()

[Phil Sutter]

In contrast to rule_lookup(), this function returns a chain's rule at a
given index instead of by handle.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/rule.h |  2 ++
 src/rule.c     | 11 +++++++++++
 2 files changed, 13 insertions(+)

diff --git a/include/rule.h b/include/rule.h
index 61aa040a2e891..a7dd042d60e3f 100644
--- a/include/rule.h
+++ b/include/rule.h
@@ -260,6 +260,8 @@ extern struct rule *rule_get(struct rule *rule);
 extern void rule_free(struct rule *rule);
 extern void rule_print(const struct rule *rule, struct output_ctx *octx);
 extern struct rule *rule_lookup(const struct chain *chain, uint64_t handle);
+extern struct rule *rule_lookup_by_index(const struct chain *chain,
+					 uint64_t index);
 
 /**
  * struct set - nftables set
diff --git a/src/rule.c b/src/rule.c
index 78e0388e41e93..dc2dd3628b17f 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -716,6 +716,17 @@ struct rule *rule_lookup(const struct chain *chain, uint64_t handle)
 	return NULL;
 }
 
+struct rule *rule_lookup_by_index(const struct chain *chain, uint64_t index)
+{
+	struct rule *rule;
+
+	list_for_each_entry(rule, &chain->rules, list) {
+		if (!--index)
+			return rule;
+	}
+	return NULL;
+}
+
 struct scope *scope_init(struct scope *scope, const struct scope *parent)
 {
 	scope->parent = parent;
-- 
2.21.0

[nft PATCH v4 6/7] src: Make cache_is_complete() public

[Phil Sutter]

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/rule.h | 1 +
 src/rule.c     | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/rule.h b/include/rule.h
index a7dd042d60e3f..aa8881d375b96 100644
--- a/include/rule.h
+++ b/include/rule.h
@@ -638,6 +638,7 @@ extern int cache_update(struct nft_ctx *ctx, enum cmd_ops cmd,
 extern void cache_flush(struct nft_ctx *ctx, enum cmd_ops cmd,
 			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);
 
 struct timeout_protocol {
 	uint32_t array_size;
diff --git a/src/rule.c b/src/rule.c
index dc2dd3628b17f..b00161e0e4350 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -232,7 +232,7 @@ static int cache_completeness(enum cmd_ops cmd)
 	return 1;
 }
 
-static bool cache_is_complete(struct nft_cache *cache, enum cmd_ops cmd)
+bool cache_is_complete(struct nft_cache *cache, enum cmd_ops cmd)
 {
 	return cache_completeness(cache->cmd) >= cache_completeness(cmd);
 }
-- 
2.21.0

[nft PATCH v4 7/7] src: Support intra-transaction rule references

[Phil Sutter]

A rule may be added before or after another one using index keyword. To
support for the other rule being added within the same batch, one has to
make use of NFTNL_RULE_ID and NFTNL_RULE_POSITION_ID attributes. This
patch does just that among a few more crucial things:

* Fetch full kernel ruleset upon encountering a rule which references
  another one. Any earlier rule add/insert commands are then restored by
  cache_add_commands().

* Avoid cache updates for rules not referencing another one, but make
  sure cache is not treated as complete afterwards so a later rule with
  reference will cause cache update and repopulation from command list.

* Reduce rule_translate_index() to its core code which is the actual
  linking of rules and consequently rename the function. The removed
  bits are pulled into the calling rule_evaluate() to reduce code
  duplication in between cache inserts with and without rule reference.

* Pass the current command op to rule_evaluate() as indicator whether to
  insert before or after a referenced rule or at beginning or end of
  chain in cache. Exploit this from chain_evaluate() to avoid adding
  the chain's rules a second time.

Light casts shadow though: It has been possible to reference another
rule of the same transaction via its *guessed* handle - this patch
removes that possibility.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
Changes since v1:
- Move rule list manipulation into a dedicated function
  rule_cache_update().
- Restore old performance for simple commands by fetching a full rule
  cache only if the currently evaluated rule references another one.
- Extend 0024rule_0 test a bit to make sure things work with interactive
  nft also.
---
 include/rule.h                                |   6 +
 src/evaluate.c                                | 103 +++++++++++-------
 src/mnl.c                                     |   4 +
 src/rule.c                                    |  53 +++++++++
 .../shell/testcases/cache/0003_cache_update_0 |   7 ++
 .../shell/testcases/nft-f/0006action_object_0 |   2 +-
 tests/shell/testcases/transactions/0024rule_0 |  17 +++
 .../transactions/dumps/0024rule_0.nft         |   8 ++
 8 files changed, 162 insertions(+), 38 deletions(-)
 create mode 100755 tests/shell/testcases/transactions/0024rule_0
 create mode 100644 tests/shell/testcases/transactions/dumps/0024rule_0.nft

diff --git a/include/rule.h b/include/rule.h
index aa8881d375b96..e2bbdf3696bfe 100644
--- a/include/rule.h
+++ b/include/rule.h
@@ -73,6 +73,8 @@ struct handle {
 	struct position_spec	position;
 	struct position_spec	index;
 	uint32_t		set_id;
+	uint32_t		rule_id;
+	uint32_t		position_id;
 };
 
 extern void handle_merge(struct handle *dst, const struct handle *src);
@@ -263,6 +265,10 @@ extern struct rule *rule_lookup(const struct chain *chain, uint64_t handle);
 extern struct rule *rule_lookup_by_index(const struct chain *chain,
 					 uint64_t index);
 
+enum cmd_ops;
+extern void rule_cache_update(enum cmd_ops op, struct chain *chain,
+			      struct rule *rule, struct rule *ref);
+
 /**
  * struct set - nftables set
  *
diff --git a/src/evaluate.c b/src/evaluate.c
index 09bb1fd37a301..ef781ad2c8712 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -3184,46 +3184,32 @@ static int flowtable_evaluate(struct eval_ctx *ctx, struct flowtable *ft)
 	return 0;
 }
 
-/* Convert rule's handle.index into handle.position. */
-static int rule_translate_index(struct eval_ctx *ctx, struct rule *rule)
+/* make src point at dst, either via handle.position or handle.position_id */
+static void link_rules(struct rule *src, struct rule *dst)
 {
-	struct table *table;
-	struct chain *chain;
-	uint64_t index = 0;
-	struct rule *r;
-	int ret;
+	static uint32_t ref_id = 0;
 
-	/* update cache with CMD_LIST so that rules are fetched, too */
-	ret = cache_update(ctx->nft, CMD_LIST, ctx->msgs);
-	if (ret < 0)
-		return ret;
-
-	table = table_lookup(&rule->handle, &ctx->nft->cache);
-	if (!table)
-		return table_not_found(ctx);
-
-	chain = chain_lookup(table, &rule->handle);
-	if (!chain)
-		return chain_not_found(ctx);
-
-	list_for_each_entry(r, &chain->rules, list) {
-		if (++index < rule->handle.index.id)
-			continue;
-		rule->handle.position.id = r->handle.handle.id;
-		rule->handle.position.location = rule->handle.index.location;
-		break;
+	if (dst->handle.handle.id) {
+		/* dst is in kernel, make src reference it by handle */
+		src->handle.position.id = dst->handle.handle.id;
+		src->handle.position.location = src->handle.index.location;
+		return;
 	}
-	if (!rule->handle.position.id)
-		return cmd_error(ctx, &rule->handle.index.location,
-				"Could not process rule: %s",
-				strerror(ENOENT));
-	return 0;
+
+	/* dst is not in kernel, make src reference it by per-transaction ID */
+	if (!dst->handle.rule_id)
+		dst->handle.rule_id = ++ref_id;
+	src->handle.position_id = dst->handle.rule_id;
 }
 
-static int rule_evaluate(struct eval_ctx *ctx, struct rule *rule)
+static int rule_evaluate(struct eval_ctx *ctx, struct rule *rule, enum cmd_ops op)
 {
 	struct stmt *stmt, *tstmt = NULL;
 	struct error_record *erec;
+	struct rule *ref = NULL;
+	struct table *table;
+	struct chain *chain;
+	int ret;
 
 	proto_ctx_init(&ctx->pctx, rule->handle.family, ctx->nft->debug_mask);
 	memset(&ctx->ectx, 0, sizeof(ctx->ectx));
@@ -3248,10 +3234,53 @@ static int rule_evaluate(struct eval_ctx *ctx, struct rule *rule)
 		return -1;
 	}
 
-	if (rule->handle.index.id &&
-	    rule_translate_index(ctx, rule))
-		return -1;
+	if (!rule->handle.index.id &&
+	    !rule->handle.handle.id &&
+	    !rule->handle.position.id) {
+		/* No reference to another rule in this one, no need to update
+		 * cache for it. Just mark the cache as incomplete so it is not
+		 * left out by accident. */
+		if (cache_is_complete(&ctx->nft->cache, CMD_LIST))
+			ctx->nft->cache.cmd = CMD_RESET;
+		return 0;
+	}
+
+	/* update cache with CMD_LIST so that rules are fetched, too */
+	ret = cache_update(ctx->nft, CMD_LIST, ctx->msgs);
+	if (ret < 0)
+		return ret;
+
+	table = table_lookup(&rule->handle, &ctx->nft->cache);
+	if (!table)
+		return table_not_found(ctx);
+
+	chain = chain_lookup(table, &rule->handle);
+	if (!chain)
+		return chain_not_found(ctx);
+
+	if (rule->handle.index.id) {
+		ref = rule_lookup_by_index(chain, rule->handle.index.id);
+		if (!ref)
+			return cmd_error(ctx, &rule->handle.index.location,
+					 "Could not process rule: %s",
+					 strerror(ENOENT));
+
+		link_rules(rule, ref);
+	} else if (rule->handle.handle.id) {
+		ref = rule_lookup(chain, rule->handle.handle.id);
+		if (!ref)
+			return cmd_error(ctx, &rule->handle.handle.location,
+					 "Could not process rule: %s",
+					 strerror(ENOENT));
+	} else if (rule->handle.position.id) {
+		ref = rule_lookup(chain, rule->handle.position.id);
+		if (!ref)
+			return cmd_error(ctx, &rule->handle.position.location,
+					 "Could not process rule: %s",
+					 strerror(ENOENT));
+	}
 
+	rule_cache_update(op, chain, rule, ref);
 	return 0;
 }
 
@@ -3333,7 +3362,7 @@ static int chain_evaluate(struct eval_ctx *ctx, struct chain *chain)
 
 	list_for_each_entry(rule, &chain->rules, list) {
 		handle_merge(&rule->handle, &chain->handle);
-		if (rule_evaluate(ctx, rule) < 0)
+		if (rule_evaluate(ctx, rule, CMD_INVALID) < 0)
 			return -1;
 	}
 	return 0;
@@ -3430,7 +3459,7 @@ static int cmd_evaluate_add(struct eval_ctx *ctx, struct cmd *cmd)
 		return set_evaluate(ctx, cmd->set);
 	case CMD_OBJ_RULE:
 		handle_merge(&cmd->rule->handle, &cmd->handle);
-		return rule_evaluate(ctx, cmd->rule);
+		return rule_evaluate(ctx, cmd->rule, cmd->op);
 	case CMD_OBJ_CHAIN:
 		ret = cache_update(ctx->nft, cmd->op, ctx->msgs);
 		if (ret < 0)
diff --git a/src/mnl.c b/src/mnl.c
index f6363560721c1..9bb712adfa3b5 100644
--- a/src/mnl.c
+++ b/src/mnl.c
@@ -327,6 +327,10 @@ int mnl_nft_rule_add(struct netlink_ctx *ctx, const struct cmd *cmd,
 	nftnl_rule_set_str(nlr, NFTNL_RULE_CHAIN, h->chain.name);
 	if (h->position.id)
 		nftnl_rule_set_u64(nlr, NFTNL_RULE_POSITION, h->position.id);
+	if (h->rule_id)
+		nftnl_rule_set_u32(nlr, NFTNL_RULE_ID, h->rule_id);
+	if (h->position_id)
+		nftnl_rule_set_u32(nlr, NFTNL_RULE_POSITION_ID, h->position_id);
 
 	netlink_linearize_rule(ctx, nlr, rule);
 	nlh = nftnl_nlmsg_build_hdr(nftnl_batch_buffer(ctx->batch),
diff --git a/src/rule.c b/src/rule.c
index b00161e0e4350..0048a8e064523 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -293,6 +293,23 @@ static int cache_add_set_cmd(struct eval_ctx *ectx)
 	return 0;
 }
 
+static int cache_add_rule_cmd(struct eval_ctx *ectx)
+{
+	struct table *table;
+	struct chain *chain;
+
+	table = table_lookup(&ectx->cmd->rule->handle, &ectx->nft->cache);
+	if (!table)
+		return table_not_found(ectx);
+
+	chain = chain_lookup(table, &ectx->cmd->rule->handle);
+	if (!chain)
+		return chain_not_found(ectx);
+
+	rule_cache_update(ectx->cmd->op, chain, ectx->cmd->rule, NULL);
+	return 0;
+}
+
 static int cache_add_commands(struct nft_ctx *nft, struct list_head *msgs)
 {
 	struct eval_ctx ectx = {
@@ -314,6 +331,11 @@ static int cache_add_commands(struct nft_ctx *nft, struct list_head *msgs)
 				continue;
 			ret = cache_add_set_cmd(&ectx);
 			break;
+		case CMD_OBJ_RULE:
+			if (!cache_is_complete(&nft->cache, CMD_LIST))
+				continue;
+			ret = cache_add_rule_cmd(&ectx);
+			break;
 		default:
 			break;
 		}
@@ -727,6 +749,37 @@ struct rule *rule_lookup_by_index(const struct chain *chain, uint64_t index)
 	return NULL;
 }
 
+void rule_cache_update(enum cmd_ops op, struct chain *chain,
+		       struct rule *rule, struct rule *ref)
+{
+	switch (op) {
+	case CMD_INSERT:
+		rule_get(rule);
+		if (ref)
+			list_add_tail(&rule->list, &ref->list);
+		else
+			list_add(&rule->list, &chain->rules);
+		break;
+	case CMD_ADD:
+		rule_get(rule);
+		if (ref)
+			list_add(&rule->list, &ref->list);
+		else
+			list_add_tail(&rule->list, &chain->rules);
+		break;
+	case CMD_REPLACE:
+		rule_get(rule);
+		list_add(&rule->list, &ref->list);
+		/* fall through */
+	case CMD_DELETE:
+		list_del(&ref->list);
+		rule_free(ref);
+		break;
+	default:
+		break;
+	}
+}
+
 struct scope *scope_init(struct scope *scope, const struct scope *parent)
 {
 	scope->parent = parent;
diff --git a/tests/shell/testcases/cache/0003_cache_update_0 b/tests/shell/testcases/cache/0003_cache_update_0
index fa9b5df380a41..05edc9c7c33eb 100755
--- a/tests/shell/testcases/cache/0003_cache_update_0
+++ b/tests/shell/testcases/cache/0003_cache_update_0
@@ -34,6 +34,9 @@ EOF
 # add rule ip t4 c meta l4proto icmp accept -> rule to reference in next step
 # add rule ip t4 c index 0 drop -> index 0 is not found due to rule cache not
 #                                  being updated
+# add rule ip t4 c index 2 drop -> index 2 is not found due to igmp rule being
+#                                  in same transaction and therefore not having
+#                                  an allocated handle
 $NFT -i >/dev/null <<EOF
 add table ip t4; add chain ip t4 c
 add rule ip t4 c meta l4proto icmp accept
@@ -41,3 +44,7 @@ EOF
 $NFT -f - >/dev/null <<EOF
 add rule ip t4 c index 0 drop
 EOF
+$NFT -f - >/dev/null <<EOF
+add rule ip t4 c meta l4proto igmp accept
+add rule ip t4 c index 2 drop
+EOF
diff --git a/tests/shell/testcases/nft-f/0006action_object_0 b/tests/shell/testcases/nft-f/0006action_object_0
index b9766f2dbb721..742e0bdec69f7 100755
--- a/tests/shell/testcases/nft-f/0006action_object_0
+++ b/tests/shell/testcases/nft-f/0006action_object_0
@@ -16,7 +16,6 @@ generate1()
 	add set $family t s {type inet_service;}
 	add element $family t s {8080}
 	insert rule $family t c meta l4proto tcp tcp dport @s accept
-	replace rule $family t c handle 2 meta l4proto tcp tcp dport {9090}
 	add map $family t m {type inet_service:verdict;}
 	add element $family t m {10080:drop}
 	insert rule $family t c meta l4proto tcp tcp dport vmap @m
@@ -28,6 +27,7 @@ generate2()
 {
 	local family=$1
 	echo "
+	replace rule $family t c handle 2 meta l4proto tcp tcp dport {9090}
 	flush chain $family t c
 	delete element $family t m {10080:drop}
 	delete element $family t s {8080}
diff --git a/tests/shell/testcases/transactions/0024rule_0 b/tests/shell/testcases/transactions/0024rule_0
new file mode 100755
index 0000000000000..4c1ac41db3b47
--- /dev/null
+++ b/tests/shell/testcases/transactions/0024rule_0
@@ -0,0 +1,17 @@
+#!/bin/bash
+
+RULESET="flush ruleset
+add table x
+add chain x y
+add rule x y accept comment rule1
+add rule x y accept comment rule4
+add rule x y index 0 accept comment rule2
+insert rule x y index 2 accept comment rule3"
+
+$NFT -f - <<< "$RULESET" && \
+	$NFT -f - <<< "$RULESET" && \
+	echo "$RULESET" | tr '\n' ';' | $NFT -i >/dev/null && \
+	exit 0
+echo "E: intra-transaction rule reference failed"
+exit 1
+
diff --git a/tests/shell/testcases/transactions/dumps/0024rule_0.nft b/tests/shell/testcases/transactions/dumps/0024rule_0.nft
new file mode 100644
index 0000000000000..7860ff654c5e2
--- /dev/null
+++ b/tests/shell/testcases/transactions/dumps/0024rule_0.nft
@@ -0,0 +1,8 @@
+table ip x {
+	chain y {
+		accept comment "rule1"
+		accept comment "rule2"
+		accept comment "rule3"
+		accept comment "rule4"
+	}
+}
-- 
2.21.0

Re: [nft PATCH v4 7/7] src: Support intra-transaction rule references

[Eric Garver]

Hi Phil,

This series is close. I see one more issue below.

On Tue, May 28, 2019 at 11:03:23PM +0200, Phil Sutter wrote:
> A rule may be added before or after another one using index keyword. To
> support for the other rule being added within the same batch, one has to
> make use of NFTNL_RULE_ID and NFTNL_RULE_POSITION_ID attributes. This
> patch does just that among a few more crucial things:
>
> * Fetch full kernel ruleset upon encountering a rule which references
>   another one. Any earlier rule add/insert commands are then restored by
>   cache_add_commands().
>
> * Avoid cache updates for rules not referencing another one, but make
>   sure cache is not treated as complete afterwards so a later rule with
>   reference will cause cache update and repopulation from command list.
>
> * Reduce rule_translate_index() to its core code which is the actual
>   linking of rules and consequently rename the function. The removed
>   bits are pulled into the calling rule_evaluate() to reduce code
>   duplication in between cache inserts with and without rule reference.
>
> * Pass the current command op to rule_evaluate() as indicator whether to
>   insert before or after a referenced rule or at beginning or end of
>   chain in cache. Exploit this from chain_evaluate() to avoid adding
>   the chain's rules a second time.
>
> Light casts shadow though: It has been possible to reference another
> rule of the same transaction via its *guessed* handle - this patch
> removes that possibility.
>
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
[..]
> diff --git a/src/rule.c b/src/rule.c
> index b00161e0e4350..0048a8e064523 100644
> --- a/src/rule.c
> +++ b/src/rule.c
> @@ -293,6 +293,23 @@ static int cache_add_set_cmd(struct eval_ctx *ectx)
>  	return 0;
>  }
>
> +static int cache_add_rule_cmd(struct eval_ctx *ectx)
> +{
> +	struct table *table;
> +	struct chain *chain;
> +
> +	table = table_lookup(&ectx->cmd->rule->handle, &ectx->nft->cache);
> +	if (!table)
> +		return table_not_found(ectx);
> +
> +	chain = chain_lookup(table, &ectx->cmd->rule->handle);
> +	if (!chain)
> +		return chain_not_found(ectx);
> +
> +	rule_cache_update(ectx->cmd->op, chain, ectx->cmd->rule, NULL);
> +	return 0;
> +}
> +
>  static int cache_add_commands(struct nft_ctx *nft, struct list_head *msgs)
>  {
>  	struct eval_ctx ectx = {
> @@ -314,6 +331,11 @@ static int cache_add_commands(struct nft_ctx *nft, struct list_head *msgs)
>  				continue;
>  			ret = cache_add_set_cmd(&ectx);
>  			break;
> +		case CMD_OBJ_RULE:
> +			if (!cache_is_complete(&nft->cache, CMD_LIST))
> +				continue;
> +			ret = cache_add_rule_cmd(&ectx);
> +			break;
>  		default:
>  			break;
>  		}
> @@ -727,6 +749,37 @@ struct rule *rule_lookup_by_index(const struct chain *chain, uint64_t index)
>  	return NULL;
>  }
>
> +void rule_cache_update(enum cmd_ops op, struct chain *chain,
> +		       struct rule *rule, struct rule *ref)
> +{
> +	switch (op) {
> +	case CMD_INSERT:
> +		rule_get(rule);
> +		if (ref)
> +			list_add_tail(&rule->list, &ref->list);
> +		else
> +			list_add(&rule->list, &chain->rules);
> +		break;
> +	case CMD_ADD:
> +		rule_get(rule);
> +		if (ref)
> +			list_add(&rule->list, &ref->list);
> +		else
> +			list_add_tail(&rule->list, &chain->rules);
> +		break;
> +	case CMD_REPLACE:
> +		rule_get(rule);
> +		list_add(&rule->list, &ref->list);
> +		/* fall through */
> +	case CMD_DELETE:
> +		list_del(&ref->list);
> +		rule_free(ref);
> +		break;
> +	default:
> +		break;
> +	}
> +}

I'm seeing a NULL pointer dereferenced here. It occurs when we delete a rule
and add a new rule using the "index" keyword in the same transaction/batch.

FWIW, I've also got Pablo's recv buffer size patches applied.

# nft --handle list table inet foobar
table inet foobar { # handle 4004
        chain foo_chain { # handle 1
                accept # handle 2
                log # handle 3
        }
}

# nft -f -
delete rule inet foobar foo_chain handle 3
add rule inet foobar foo_chain index 0 drop
Segmentation fault (core dumped)

# nft --handle list table inet foobar
table inet foobar { # handle 4004
        chain foo_chain { # handle 1
                accept # handle 2
                log # handle 3
        }
}


(gdb) bt
#0  0x00007f76d3d6b9b2 in rule_cache_update (op=CMD_DELETE, chain=0x1865d70, rule=0x0, ref=0x0) at rule.c:755
#1  0x00007f76d3d6d16b in cache_add_rule_cmd (ectx=0x7fff51d96c00) at rule.c:309
#2  cache_add_commands (msgs=0x7fff51dab4e0, nft=0x17fba20) at rule.c:337
#3  cache_update (nft=0x17fba20, cmd=cmd@entry=CMD_LIST, msgs=0x7fff51dab4e0) at rule.c:381
#4  0x00007f76d3d7a261 in rule_evaluate (ctx=0x17fc160, rule=0x180df30, op=CMD_ADD) at evaluate.c:3249
#5  0x00007f76d3da423a in nft_parse (nft=nft@entry=0x17fba20, scanner=0x1824060, state=0x17fbb80) at parser_bison.y:799
#6  0x00007f76d3d91324 in nft_parse_bison_filename (cmds=0x17fbae0, msgs=0x7fff51dab4e0, filename=0x7f76d3db8385 "/dev/stdin", nft=0x17fba20) at libnftables.c:380
#7  nft_run_cmd_from_filename (nft=0x17fba20, filename=0x7f76d3db8385 "/dev/stdin", filename@entry=0x7fff51dac67d "-") at libnftables.c:446
#8  0x0000000000401698 in main (argc=3, argv=0x7fff51dab658) at main.c:310

(gdb) frame 1
#1  0x00007f76d3d6d16b in cache_add_rule_cmd (ectx=0x7fff51d96c00) at rule.c:309
309             rule_cache_update(ectx->cmd->op, chain, ectx->cmd->rule, NULL);

(gdb) print *ectx
$1 = {nft = 0x17fba20, msgs = 0x7fff51dab4e0, cmd = 0x1801730, table = 0x0, rule = 0x0, set = 0x0, stmt = 0x0, ectx = {dtype = 0x0, byteorder = BYTEORDER_INVALID, len = 0, maxval = 0}, pctx = {debug_mask = 0, family = 0, protocol = {{location = {indesc = 0x0, {{
              token_offset = 0, line_offset = 0, first_line = 0, last_line = 0, first_column = 0, last_column = 0}, {nle = 0x0}}}, desc = 0x0, offset = 0}, {location = {indesc = 0x0, {{token_offset = 0, line_offset = 0, first_line = 0, last_line = 0, first_column = 0,
              last_column = 0}, {nle = 0x0}}}, desc = 0x0, offset = 0}, {location = {indesc = 0x0, {{token_offset = 0, line_offset = 0, first_line = 0, last_line = 0, first_column = 0, last_column = 0}, {nle = 0x0}}}, desc = 0x0, offset = 0}, {location = {indesc = 0x0, {{
              token_offset = 0, line_offset = 0, first_line = 0, last_line = 0, first_column = 0, last_column = 0}, {nle = 0x0}}}, desc = 0x0, offset = 0}}}}

(gdb) print *ectx->cmd
$2 = {list = {next = 0x17fbae0, prev = 0x17fbae0}, location = {indesc = 0x17fbb88, {{token_offset = 6, line_offset = 0, first_line = 1, last_line = 1, first_column = 1, last_column = 43}, {nle = 0x6}}}, op = CMD_DELETE, obj = CMD_OBJ_RULE, handle = {family = 1, table = {
      location = {indesc = 0x17fbb88, {{token_offset = 23, line_offset = 0, first_line = 1, last_line = 1, first_column = 18, last_column = 23}, {nle = 0x17}}}, name = 0x18014c0 "foobar"}, chain = {location = {indesc = 0x17fbb88, {{token_offset = 33, line_offset = 0,
            first_line = 1, last_line = 1, first_column = 25, last_column = 33}, {nle = 0x21}}}, name = 0x180a740 "foo_chain"}, set = {location = {indesc = 0x0, {{token_offset = 0, line_offset = 0, first_line = 0, last_line = 0, first_column = 0, last_column = 0}, {
            nle = 0x0}}}, name = 0x0}, obj = {location = {indesc = 0x0, {{token_offset = 0, line_offset = 0, first_line = 0, last_line = 0, first_column = 0, last_column = 0}, {nle = 0x0}}}, name = 0x0}, flowtable = 0x0, handle = {location = {indesc = 0x17fbb88, {{
            token_offset = 40, line_offset = 0, first_line = 1, last_line = 1, first_column = 35, last_column = 42}, {nle = 0x28}}}, id = 3}, position = {location = {indesc = 0x0, {{token_offset = 0, line_offset = 0, first_line = 0, last_line = 0, first_column = 0,
            last_column = 0}, {nle = 0x0}}}, id = 0}, index = {location = {indesc = 0x0, {{token_offset = 0, line_offset = 0, first_line = 0, last_line = 0, first_column = 0, last_column = 0}, {nle = 0x0}}}, id = 0}, set_id = 0, rule_id = 0, position_id = 0}, seqnum = 0,
  {data = 0x0, expr = 0x0, set = 0x0, rule = 0x0, chain = 0x0, table = 0x0, flowtable = 0x0, monitor = 0x0, markup = 0x0, object = 0x0}, arg = 0x0}

(gdb) print ectx->cmd->rule
$3 = (struct rule *) 0x0

Re: [nft PATCH v4 7/7] src: Support intra-transaction rule references

[Pablo Neira Ayuso]

On Fri, May 31, 2019 at 12:56:25PM -0400, Eric Garver wrote:
> [..]
> > diff --git a/src/rule.c b/src/rule.c
> > index b00161e0e4350..0048a8e064523 100644
> > --- a/src/rule.c
> > +++ b/src/rule.c
> > @@ -293,6 +293,23 @@ static int cache_add_set_cmd(struct eval_ctx *ectx)
> >  	return 0;
> >  }
> >
> > +static int cache_add_rule_cmd(struct eval_ctx *ectx)
> > +{
> > +	struct table *table;
> > +	struct chain *chain;
> > +
> > +	table = table_lookup(&ectx->cmd->rule->handle, &ectx->nft->cache);
> > +	if (!table)
> > +		return table_not_found(ectx);
> > +
> > +	chain = chain_lookup(table, &ectx->cmd->rule->handle);
> > +	if (!chain)
> > +		return chain_not_found(ectx);
> > +
> > +	rule_cache_update(ectx->cmd->op, chain, ectx->cmd->rule, NULL);
> > +	return 0;
> > +}
> > +
> >  static int cache_add_commands(struct nft_ctx *nft, struct list_head *msgs)
> >  {
> >  	struct eval_ctx ectx = {
> > @@ -314,6 +331,11 @@ static int cache_add_commands(struct nft_ctx *nft, struct list_head *msgs)
> >  				continue;
> >  			ret = cache_add_set_cmd(&ectx);
> >  			break;
> > +		case CMD_OBJ_RULE:
> > +			if (!cache_is_complete(&nft->cache, CMD_LIST))
> > +				continue;
> > +			ret = cache_add_rule_cmd(&ectx);
> > +			break;
> >  		default:
> >  			break;
> >  		}
> > @@ -727,6 +749,37 @@ struct rule *rule_lookup_by_index(const struct chain *chain, uint64_t index)
> >  	return NULL;
> >  }
> >
> > +void rule_cache_update(enum cmd_ops op, struct chain *chain,
> > +		       struct rule *rule, struct rule *ref)
> > +{
> > +	switch (op) {
> > +	case CMD_INSERT:
> > +		rule_get(rule);
> > +		if (ref)
> > +			list_add_tail(&rule->list, &ref->list);
> > +		else
> > +			list_add(&rule->list, &chain->rules);
> > +		break;
> > +	case CMD_ADD:
> > +		rule_get(rule);
> > +		if (ref)
> > +			list_add(&rule->list, &ref->list);
> > +		else
> > +			list_add_tail(&rule->list, &chain->rules);
> > +		break;
> > +	case CMD_REPLACE:
> > +		rule_get(rule);
> > +		list_add(&rule->list, &ref->list);
> > +		/* fall through */
> > +	case CMD_DELETE:
> > +		list_del(&ref->list);
> > +		rule_free(ref);
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +}
> 
> I'm seeing a NULL pointer dereferenced here. It occurs when we delete a rule
> and add a new rule using the "index" keyword in the same transaction/batch.

I think we need two new things here:

#1 We need a new initial step, before evalution, to calculate the cache
   completeness level. This means, we interate over the batch to see what
   kind of completeness is needed. Then, cache is fetched only once, at
   the beginning of the batch processing. Ensure that cache is
   consistent from that step.

#2 Update the cache incrementally: Add new objects from the evaluation
   phase. If RESTART is hit, then release the cache, and restart the
   evaluation. Probably we don't need to restart the evaluation, just
   a function to refresh the batch, ie. check if several objects are
   there.

Re: [nft PATCH v4 7/7] src: Support intra-transaction rule references

[Phil Sutter]

Hi,

On Mon, Jun 03, 2019 at 06:59:17PM +0200, Pablo Neira Ayuso wrote:
> On Fri, May 31, 2019 at 12:56:25PM -0400, Eric Garver wrote:
[...]
> > I'm seeing a NULL pointer dereferenced here. It occurs when we delete a rule
> > and add a new rule using the "index" keyword in the same transaction/batch.

Yes, cache population for rule delete command was completely broken. I
missed that cmd->rule is NULL in that case, sorry for the mess.

> I think we need two new things here:
> 
> #1 We need a new initial step, before evalution, to calculate the cache
>    completeness level. This means, we interate over the batch to see what
>    kind of completeness is needed. Then, cache is fetched only once, at
>    the beginning of the batch processing. Ensure that cache is
>    consistent from that step.
> 
> #2 Update the cache incrementally: Add new objects from the evaluation
>    phase. If RESTART is hit, then release the cache, and restart the
>    evaluation. Probably we don't need to restart the evaluation, just
>    a function to refresh the batch, ie. check if several objects are
>    there.

I don't understand this but please wait a day or two before jumping in.
I'm currently working on fixing the problem above and some more I found
along the way.

Cheers, Phil