[PATCH nf-next,v2 1/2] netfilter: nf_tables: honor EINTR in ruleset validation from commit/abort path

[PATCH nf-next,v2 1/2] netfilter: nf_tables: honor EINTR in ruleset validation from commit/abort path

[Pablo Neira Ayuso]

Do not return EAGAIN to replay the transaction if table validation
reports EINTR. Abort the transaction and report EINTR error instead.

Fixes: 169384fbe851 ("netfilter: nf_tables: allow loop termination for pending fatal signal")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
v2: fix check for EINTR (Phil Sutter)

 net/netfilter/nf_tables_api.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index b28f6730e26d..b57ef8f4834f 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -9885,6 +9885,7 @@ static int nf_tables_validate(struct net *net)
 {
 	struct nftables_pernet *nft_net = nft_pernet(net);
 	struct nft_table *table;
+	int err;
 
 	list_for_each_entry(table, &nft_net->tables, list) {
 		switch (table->validate_state) {
@@ -9894,15 +9895,24 @@ static int nf_tables_validate(struct net *net)
 			nft_validate_state_update(table, NFT_VALIDATE_DO);
 			fallthrough;
 		case NFT_VALIDATE_DO:
-			if (nft_table_validate(net, table) < 0)
-				return -EAGAIN;
+			err = nft_table_validate(net, table);
+			if (err < 0) {
+				if (err == -EINTR)
+					goto err_eintr;
 
+				return -EAGAIN;
+			}
 			nft_validate_state_update(table, NFT_VALIDATE_SKIP);
 			break;
 		}
 	}
 
 	return 0;
+err_eintr:
+	list_for_each_entry(table, &nft_net->tables, list)
+		nft_validate_state_update(table, NFT_VALIDATE_SKIP);
+
+	return -EINTR;
 }
 
 /* a drop policy has to be deferred until all rules have been activated,
@@ -10710,7 +10720,11 @@ static int nf_tables_commit(struct net *net, struct sk_buff *skb)
 	}
 
 	/* 0. Validate ruleset, otherwise roll back for error reporting. */
-	if (nf_tables_validate(net) < 0) {
+	err = nf_tables_validate(net);
+	if (err < 0) {
+		if (err == -EINTR)
+			return -EINTR;
+
 		nft_net->validate_state = NFT_VALIDATE_DO;
 		return -EAGAIN;
 	}
@@ -11054,9 +11068,11 @@ static int __nf_tables_abort(struct net *net, enum nfnl_abort_action action)
 	};
 	int err = 0;
 
-	if (action == NFNL_ABORT_VALIDATE &&
-	    nf_tables_validate(net) < 0)
-		err = -EAGAIN;
+	if (action == NFNL_ABORT_VALIDATE) {
+		err = nf_tables_validate(net);
+		if (err < 0 && err != -EINTR)
+			err = -EAGAIN;
+	}
 
 	list_for_each_entry_safe_reverse(trans, next, &nft_net->commit_list,
 					 list) {
-- 
2.30.2

[PATCH nf-next,v2 2/2] netfilter: nf_tables: honor validation state in preparation phase

[Pablo Neira Ayuso]

There are three states for table validation:

- SKIP, this is the initial state, skip validation from the preparation
  phase and the commit/abort path.
- NEED, this state is entered from the preparation phase, to validate
  the table from the commit/abort path. In case that validation fails,
  this enters the DO state and the transaction is replayed.
- DO, this state validates updates incremental from the preparation
  step for error reporting.

Currently the NEED state is set on if:
- A new rule contains expressions that require validation, this
  includes jump/goto chain.
- A new set element with jump/goto chain.

However, there are two issues:

- Validation is performed incrementally with new rules and elements
  regardless the states. This patch updates the logic to perform the
  validation only in the DO state.

- Reset validate state in case the transaction is finally aborted with
  with NFNL_ABORT_NONE. Otherwise, the next batch can observe the DO
  state and it performs the validation incrementally.

Fixes: a654de8fdc18 ("netfilter: nf_tables: fix chain dependency validation")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
v2: no changes

 net/netfilter/nf_tables_api.c | 50 ++++++++++++++++++++++++++---------
 1 file changed, 38 insertions(+), 12 deletions(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index b57ef8f4834f..8ef9abac4579 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -3997,16 +3997,8 @@ static void nf_tables_rule_release(const struct nft_ctx *ctx, struct nft_rule *r
 	nf_tables_rule_destroy(ctx, rule);
 }
 
-/** nft_chain_validate - loop detection and hook validation
- *
- * @ctx: context containing call depth and base chain
- * @chain: chain to validate
- *
- * Walk through the rules of the given chain and chase all jumps/gotos
- * and set lookups until either the jump limit is hit or all reachable
- * chains have been validated.
- */
-int nft_chain_validate(const struct nft_ctx *ctx, const struct nft_chain *chain)
+static int __nft_chain_validate(const struct nft_ctx *ctx,
+				const struct nft_chain *chain)
 {
 	struct nft_expr *expr, *last;
 	struct nft_rule *rule;
@@ -4037,6 +4029,30 @@ int nft_chain_validate(const struct nft_ctx *ctx, const struct nft_chain *chain)
 
 	return 0;
 }
+
+/** nft_chain_validate - loop detection and hook validation
+ *
+ * @ctx: context containing call depth and base chain
+ * @chain: chain to validate
+ *
+ * Walk through the rules of the given chain and chase all jumps/gotos and set
+ * lookups until either the jump limit is hit or all reachable chains have been
+ * validated.
+ *
+ * This function is called from preparation phase: initial state is SKIP which
+ * means that validation can be skipped entirely. However, in case of a new rule
+ * or set element needs validation, then the NEED state is entered and the
+ * validation is performed from the commit/abort phase. In case this fails, the
+ * transaction is aborted and it is replayed in the DO validation state, then
+ * incremental validation is performed for error reporting.
+ */
+int nft_chain_validate(const struct nft_ctx *ctx, const struct nft_chain *chain)
+{
+	if (ctx->table->validate_state != NFT_VALIDATE_DO)
+		return 0;
+
+	return __nft_chain_validate(ctx, chain);
+}
 EXPORT_SYMBOL_GPL(nft_chain_validate);
 
 static int nft_table_validate(struct net *net, const struct nft_table *table)
@@ -4044,6 +4060,7 @@ static int nft_table_validate(struct net *net, const struct nft_table *table)
 	struct nft_chain *chain;
 	struct nft_ctx ctx = {
 		.net	= net,
+		.table	= (struct nft_table *)table,
 		.family	= table->family,
 	};
 	int err;
@@ -4053,7 +4070,7 @@ static int nft_table_validate(struct net *net, const struct nft_table *table)
 			continue;
 
 		ctx.chain = chain;
-		err = nft_chain_validate(&ctx, chain);
+		err = __nft_chain_validate(&ctx, chain);
 		if (err < 0)
 			return err;
 
@@ -11063,15 +11080,24 @@ static int __nf_tables_abort(struct net *net, enum nfnl_abort_action action)
 	struct nft_trans *trans, *next;
 	LIST_HEAD(set_update_list);
 	struct nft_trans_elem *te;
+	struct nft_table *table;
 	struct nft_ctx ctx = {
 		.net = net,
 	};
 	int err = 0;
 
-	if (action == NFNL_ABORT_VALIDATE) {
+	switch (action) {
+	case NFNL_ABORT_NONE:
+		list_for_each_entry(table, &nft_net->tables, list)
+			nft_validate_state_update(table, NFT_VALIDATE_SKIP);
+		break;
+	case NFNL_ABORT_AUTOLOAD:
+		break;
+	case NFNL_ABORT_VALIDATE:
 		err = nf_tables_validate(net);
 		if (err < 0 && err != -EINTR)
 			err = -EAGAIN;
+		break;
 	}
 
 	list_for_each_entry_safe_reverse(trans, next, &nft_net->commit_list,
-- 
2.30.2

Re: [PATCH nf-next,v2 1/2] netfilter: nf_tables: honor EINTR in ruleset validation from commit/abort path

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> Do not return EAGAIN to replay the transaction if table validation
> reports EINTR. Abort the transaction and report EINTR error instead.

Acked-by: Florian Westphal <fw@strlen.de>

Re: [PATCH nf-next,v2 2/2] netfilter: nf_tables: honor validation state in preparation phase

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>  static int nft_table_validate(struct net *net, const struct nft_table *table)
> @@ -4044,6 +4060,7 @@ static int nft_table_validate(struct net *net, const struct nft_table *table)
>  	struct nft_chain *chain;
>  	struct nft_ctx ctx = {
>  		.net	= net,
> +		.table	= (struct nft_table *)table,

Patch LGTM but I think it would make more sense to remove the const
qualifier from nft_table_validate() rather than this un-do.

Aside from that:
Reviewed-by: Florian Westphal <fw@strlen.de>