diff mbox series

[nft] make cache persistent if local entries were added

Message ID 20181020102406.17346-1-phil@nwl.cc
State RFC
Delegated to: Pablo Neira
Headers show
Series [nft] make cache persistent if local entries were added | expand

Commit Message

Phil Sutter Oct. 20, 2018, 10:24 a.m. UTC 
JSON API as well as nft CLI allow to run multiple commands within the
same batch. Depending on the local cache state, a later command may
trigger a cache update which removes the local entry added by an earlier
command.

To overcome this, introduce a special genid value to set when local
entries are added to the cache which blocks all cache updates until the
batch has been committed to the kernel.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/nftables.h |  2 ++
 include/rule.h     | 12 ++++++++----
 src/evaluate.c     |  6 +++---
 src/libnftables.c  |  2 ++
 src/monitor.c      |  4 ++--
 src/rule.c         | 17 +++++++++++++----
 6 files changed, 30 insertions(+), 13 deletions(-)

Comments

Pablo Neira Ayuso Oct. 20, 2018, 10:35 a.m. UTC | #1 
On Sat, Oct 20, 2018 at 12:24:06PM +0200, Phil Sutter wrote:
> JSON API as well as nft CLI allow to run multiple commands within the
> same batch. Depending on the local cache state, a later command may
> trigger a cache update which removes the local entry added by an earlier
> command.
> 
> To overcome this, introduce a special genid value to set when local
> entries are added to the cache which blocks all cache updates until the
> batch has been committed to the kernel.

Probably we can make sure we issue a cache_update() by when we call
chain_add_hash(), before adding the local object to the cache, then
lock it? Or add assert() to _add_hash() functions to make sure cache
is up to date? We need a valid cache before we can lock it, right?

Do you have several examples that are triggering the problem that we
can place in the test regression infrastructure?

Thanks!

> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  include/nftables.h |  2 ++
>  include/rule.h     | 12 ++++++++----
>  src/evaluate.c     |  6 +++---
>  src/libnftables.c  |  2 ++
>  src/monitor.c      |  4 ++--
>  src/rule.c         | 17 +++++++++++++----
>  6 files changed, 30 insertions(+), 13 deletions(-)
> 
> diff --git a/include/nftables.h b/include/nftables.h
> index 25e78c80df7e0..47d2c2bb036cc 100644
> --- a/include/nftables.h
> +++ b/include/nftables.h
> @@ -37,6 +37,8 @@ struct nft_cache {
>  	struct list_head	list;
>  	uint32_t		seqnum;
>  };
> +#define CACHE_LOCK_GENID	(uint16_t)-1

This one is a valid cache number in the kernel, right? IIRC, it's zero
the one that is never used.
Phil Sutter Oct. 20, 2018, 11:15 a.m. UTC | #2 
On Sat, Oct 20, 2018 at 12:35:11PM +0200, Pablo Neira Ayuso wrote:
> On Sat, Oct 20, 2018 at 12:24:06PM +0200, Phil Sutter wrote:
> > JSON API as well as nft CLI allow to run multiple commands within the
> > same batch. Depending on the local cache state, a later command may
> > trigger a cache update which removes the local entry added by an earlier
> > command.
> > 
> > To overcome this, introduce a special genid value to set when local
> > entries are added to the cache which blocks all cache updates until the
> > batch has been committed to the kernel.
> 
> Probably we can make sure we issue a cache_update() by when we call
> chain_add_hash(), before adding the local object to the cache, then
> lock it? Or add assert() to _add_hash() functions to make sure cache
> is up to date? We need a valid cache before we can lock it, right?

The problem is that a batch commit outdates the local cache. An example
showing the problem is:

| % sudo nft -i
| nft> list ruleset
| nft> add table ip t
| nft> add table ip t2; add chain ip t2 c
| Error: Could not process rule: No such file or directory
| add table ip t2; add chain ip t2 c
|                               ^^

With 'list ruleset', I just ensure cache->genid is not zero. The first
'add table' command increments kernel's genid. The 'add chain' command
triggers a cache update which removes table t2 from it.

> Do you have several examples that are triggering the problem that we
> can place in the test regression infrastructure?

I'll try to collect a few and will send a test case so we have something
to validate against.

Thanks, Phil
diff mbox series

Patch

diff --git a/include/nftables.h b/include/nftables.h
index 25e78c80df7e0..47d2c2bb036cc 100644
--- a/include/nftables.h
+++ b/include/nftables.h
@@ -37,6 +37,8 @@  struct nft_cache {
 	struct list_head	list;
 	uint32_t		seqnum;
 };
+#define CACHE_LOCK_GENID	(uint16_t)-1
+#define CACHE_UNLOCK_GENID	1
 
 struct mnl_socket;
 struct parser_state;
diff --git a/include/rule.h b/include/rule.h
index 9e029899513fd..d5ddc78a7d3c4 100644
--- a/include/rule.h
+++ b/include/rule.h
@@ -216,7 +216,8 @@  extern const char *chain_hookname_lookup(const char *name);
 extern struct chain *chain_alloc(const char *name);
 extern struct chain *chain_get(struct chain *chain);
 extern void chain_free(struct chain *chain);
-extern void chain_add_hash(struct chain *chain, struct table *table);
+extern void chain_add_hash(struct chain *chain, struct table *table,
+			   struct nft_cache *cache);
 extern struct chain *chain_lookup(const struct table *table,
 				  const struct handle *h);
 
@@ -299,7 +300,8 @@  extern struct set *set_alloc(const struct location *loc);
 extern struct set *set_get(struct set *set);
 extern void set_free(struct set *set);
 extern struct set *set_clone(const struct set *set);
-extern void set_add_hash(struct set *set, struct table *table);
+extern void set_add_hash(struct set *set, struct table *table,
+			 struct nft_cache *cache);
 extern struct set *set_lookup(const struct table *table, const char *name);
 extern struct set *set_lookup_global(uint32_t family, const char *table,
 				     const char *name, struct nft_cache *cache);
@@ -381,7 +383,8 @@  struct obj {
 struct obj *obj_alloc(const struct location *loc);
 extern struct obj *obj_get(struct obj *obj);
 void obj_free(struct obj *obj);
-void obj_add_hash(struct obj *obj, struct table *table);
+void obj_add_hash(struct obj *obj, struct table *table,
+		  struct nft_cache *cache);
 struct obj *obj_lookup(const struct table *table, const char *name,
 		       uint32_t type);
 void obj_print(const struct obj *n, struct output_ctx *octx);
@@ -406,7 +409,8 @@  struct flowtable {
 extern struct flowtable *flowtable_alloc(const struct location *loc);
 extern struct flowtable *flowtable_get(struct flowtable *flowtable);
 extern void flowtable_free(struct flowtable *flowtable);
-extern void flowtable_add_hash(struct flowtable *flowtable, struct table *table);
+extern void flowtable_add_hash(struct flowtable *flowtable,
+			       struct table *table, struct nft_cache *cache);
 
 void flowtable_print(const struct flowtable *n, struct output_ctx *octx);
 
diff --git a/src/evaluate.c b/src/evaluate.c
index db49a18d0150c..f2448296be0c0 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -3014,7 +3014,7 @@  static int set_evaluate(struct eval_ctx *ctx, struct set *set)
 	ctx->set = NULL;
 
 	if (set_lookup(table, set->handle.set.name) == NULL)
-		set_add_hash(set_get(set), table);
+		set_add_hash(set_get(set), table, ctx->cache);
 
 	/* Default timeout value implies timeout support */
 	if (set->timeout)
@@ -3198,12 +3198,12 @@  static int chain_evaluate(struct eval_ctx *ctx, struct chain *chain)
 		if (chain_lookup(table, &ctx->cmd->handle) == NULL) {
 			chain = chain_alloc(NULL);
 			handle_merge(&chain->handle, &ctx->cmd->handle);
-			chain_add_hash(chain, table);
+			chain_add_hash(chain, table, ctx->cache);
 		}
 		return 0;
 	} else {
 		if (chain_lookup(table, &chain->handle) == NULL)
-			chain_add_hash(chain_get(chain), table);
+			chain_add_hash(chain_get(chain), table, ctx->cache);
 	}
 
 	if (chain->flags & CHAIN_F_BASECHAIN) {
diff --git a/src/libnftables.c b/src/libnftables.c
index 977763793e768..1726b0ee19b54 100644
--- a/src/libnftables.c
+++ b/src/libnftables.c
@@ -464,6 +464,7 @@  err:
 	}
 	erec_print_list(&nft->output, &msgs, nft->debug_mask);
 	iface_cache_release();
+	nft->cache.genid = CACHE_UNLOCK_GENID;
 	if (nft->scanner) {
 		scanner_destroy(nft->scanner);
 		nft->scanner = NULL;
@@ -505,6 +506,7 @@  err:
 	}
 	erec_print_list(&nft->output, &msgs, nft->debug_mask);
 	iface_cache_release();
+	nft->cache.genid = CACHE_UNLOCK_GENID;
 	if (nft->scanner) {
 		scanner_destroy(nft->scanner);
 		nft->scanner = NULL;
diff --git a/src/monitor.c b/src/monitor.c
index 213c40d119b4c..bab298f8352f7 100644
--- a/src/monitor.c
+++ b/src/monitor.c
@@ -608,7 +608,7 @@  static void netlink_events_cache_addset(struct netlink_mon_handler *monh,
 		goto out;
 	}
 
-	set_add_hash(s, t);
+	set_add_hash(s, t, monh->cache);
 out:
 	nftnl_set_free(nls);
 }
@@ -698,7 +698,7 @@  static void netlink_events_cache_addobj(struct netlink_mon_handler *monh,
 		goto out;
 	}
 
-	obj_add_hash(obj, t);
+	obj_add_hash(obj, t, monh->cache);
 out:
 	nftnl_obj_free(nlo);
 }
diff --git a/src/rule.c b/src/rule.c
index 8f78a36ca1f91..35a4de5d9b8c5 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -234,6 +234,8 @@  int cache_update(struct mnl_socket *nf_sock, struct nft_cache *cache,
 		.octx		= octx,
 	};
 
+	if (cache->genid == CACHE_LOCK_GENID)
+		return 0;
 replay:
 	ctx.seqnum = cache->seqnum++;
 	genid = mnl_genid_get(&ctx);
@@ -341,9 +343,10 @@  void set_free(struct set *set)
 	xfree(set);
 }
 
-void set_add_hash(struct set *set, struct table *table)
+void set_add_hash(struct set *set, struct table *table, struct nft_cache *cache)
 {
 	list_add_tail(&set->list, &table->sets);
+	cache->genid = CACHE_LOCK_GENID;
 }
 
 struct set *set_lookup(const struct table *table, const char *name)
@@ -753,9 +756,11 @@  void chain_free(struct chain *chain)
 	xfree(chain);
 }
 
-void chain_add_hash(struct chain *chain, struct table *table)
+void chain_add_hash(struct chain *chain, struct table *table,
+		    struct nft_cache *cache)
 {
 	list_add_tail(&chain->list, &table->chains);
+	cache->genid = CACHE_LOCK_GENID;
 }
 
 struct chain *chain_lookup(const struct table *table, const struct handle *h)
@@ -1094,6 +1099,7 @@  struct table *table_get(struct table *table)
 void table_add_hash(struct table *table, struct nft_cache *cache)
 {
 	list_add_tail(&table->list, &cache->list);
+	cache->genid = CACHE_LOCK_GENID;
 }
 
 struct table *table_lookup(const struct handle *h,
@@ -1634,9 +1640,10 @@  void obj_free(struct obj *obj)
 	xfree(obj);
 }
 
-void obj_add_hash(struct obj *obj, struct table *table)
+void obj_add_hash(struct obj *obj, struct table *table, struct nft_cache *cache)
 {
 	list_add_tail(&obj->list, &table->objs);
+	cache->genid = CACHE_LOCK_GENID;
 }
 
 struct obj *obj_lookup(const struct table *table, const char *name,
@@ -1935,9 +1942,11 @@  void flowtable_free(struct flowtable *flowtable)
 	xfree(flowtable);
 }
 
-void flowtable_add_hash(struct flowtable *flowtable, struct table *table)
+void flowtable_add_hash(struct flowtable *flowtable, struct table *table,
+			struct nft_cache *cache)
 {
 	list_add_tail(&flowtable->list, &table->flowtables);
+	cache->genid = CACHE_LOCK_GENID;
 }
 
 static void flowtable_print_declaration(const struct flowtable *flowtable,