[PATCH nf,v2] netfilter: nftables: accept all dummy chain when table is dormant

[PATCH nf,v2] netfilter: nftables: accept all dummy chain when table is dormant

[Pablo Neira Ayuso]

The dormant flag need to be updated from the preparation phase,
otherwise, two consecutive requests to dorm a table in the same batch
might try to remove the same hooks twice, resulting in the following
warning:

 hook not found, pf 3 num 0
 WARNING: CPU: 0 PID: 334 at net/netfilter/core.c:480 __nf_unregister_net_hook+0x1eb/0x610 net/netfilter/core.c:480
 Modules linked in:
 CPU: 0 PID: 334 Comm: kworker/u4:5 Not tainted 5.12.0-syzkaller #0
 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
 Workqueue: netns cleanup_net
 RIP: 0010:__nf_unregister_net_hook+0x1eb/0x610 net/netfilter/core.c:480

This patch is a partial revert of 0ce7cf4127f1 ("netfilter: nftables:
update table flags from the commit phase") to restore the previous
behaviour, which updates the dormant flag from the preparation phase
to address this issue.

However, there is still another problem: A batch containing a series of
dorm-wakeup-dorm table and vice-versa also trigger the warning above
since hook unregistration happens from the preparation phase, while hook
registration occurs from the commit phase.

To fix this problem, this patch adds a dummy accept-all-rule that is
used in case the table enters dormant state. This patch removes
nf_tables_table_enable() and nf_tables_table_disable() since chain hooks
are always registered, therefore, they are not needed anymore.

The new __NFT_TABLE_F_UPDATE internal flag is used to signal the commit
phase that the dormant flag has been updated.

Reported-by: syzbot+7ad5cd1615f2d89c6e7e@syzkaller.appspotmail.com
Fixes: 0ce7cf4127f1 ("netfilter: nftables: update table flags from the commit phase")
Fixes: 9ddf63235749 ("netfilter: nf_tables: add support for dormant tables")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
v2: untoggle __NFT_TABLE_F_UPDATE from the commit transaction loop instead of
    doing this right after nf_tables_commit_chain_prepare().

 include/net/netfilter/nf_tables.h |   3 -
 net/netfilter/nf_tables_api.c     | 148 +++++++++++++++---------------
 2 files changed, 75 insertions(+), 76 deletions(-)

diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index 27eeb613bb4e..8ad2839c4dd2 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -1507,15 +1507,12 @@ struct nft_trans_chain {
 struct nft_trans_table {
 	bool				update;
 	u8				state;
-	u32				flags;
 };
 
 #define nft_trans_table_update(trans)	\
 	(((struct nft_trans_table *)trans->data)->update)
 #define nft_trans_table_state(trans)	\
 	(((struct nft_trans_table *)trans->data)->state)
-#define nft_trans_table_flags(trans)	\
-	(((struct nft_trans_table *)trans->data)->flags)
 
 struct nft_trans_elem {
 	struct nft_set			*set;
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index d63d2d8f769c..104b9a47be5f 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -32,6 +32,7 @@ static LIST_HEAD(nf_tables_objects);
 static LIST_HEAD(nf_tables_flowtables);
 static LIST_HEAD(nf_tables_destroy_list);
 static DEFINE_SPINLOCK(nf_tables_destroy_list_lock);
+static struct nft_rule *nft_accept_all_rule;
 static u64 table_handle;
 
 enum {
@@ -237,8 +238,7 @@ static int nf_tables_register_hook(struct net *net,
 	struct nft_base_chain *basechain;
 	const struct nf_hook_ops *ops;
 
-	if (table->flags & NFT_TABLE_F_DORMANT ||
-	    !nft_is_base_chain(chain))
+	if (!nft_is_base_chain(chain))
 		return 0;
 
 	basechain = nft_base_chain(chain);
@@ -260,8 +260,7 @@ static void nf_tables_unregister_hook(struct net *net,
 	struct nft_base_chain *basechain;
 	const struct nf_hook_ops *ops;
 
-	if (table->flags & NFT_TABLE_F_DORMANT ||
-	    !nft_is_base_chain(chain))
+	if (!nft_is_base_chain(chain))
 		return;
 	basechain = nft_base_chain(chain);
 	ops = &basechain->ops;
@@ -736,7 +735,8 @@ static int nf_tables_fill_table_info(struct sk_buff *skb, struct net *net,
 		goto nla_put_failure;
 
 	if (nla_put_string(skb, NFTA_TABLE_NAME, table->name) ||
-	    nla_put_be32(skb, NFTA_TABLE_FLAGS, htonl(table->flags)) ||
+	    nla_put_be32(skb, NFTA_TABLE_FLAGS,
+			 htonl(table->flags & NFT_TABLE_F_MASK)) ||
 	    nla_put_be32(skb, NFTA_TABLE_USE, htonl(table->use)) ||
 	    nla_put_be64(skb, NFTA_TABLE_HANDLE, cpu_to_be64(table->handle),
 			 NFTA_TABLE_PAD))
@@ -902,65 +902,18 @@ static int nf_tables_gettable(struct sk_buff *skb, const struct nfnl_info *info,
 	return err;
 }
 
-static void nft_table_disable(struct net *net, struct nft_table *table, u32 cnt)
-{
-	struct nft_chain *chain;
-	u32 i = 0;
-
-	list_for_each_entry(chain, &table->chains, list) {
-		if (!nft_is_active_next(net, chain))
-			continue;
-		if (!nft_is_base_chain(chain))
-			continue;
-
-		if (cnt && i++ == cnt)
-			break;
-
-		nf_tables_unregister_hook(net, table, chain);
-	}
-}
-
-static int nf_tables_table_enable(struct net *net, struct nft_table *table)
-{
-	struct nft_chain *chain;
-	int err, i = 0;
-
-	list_for_each_entry(chain, &table->chains, list) {
-		if (!nft_is_active_next(net, chain))
-			continue;
-		if (!nft_is_base_chain(chain))
-			continue;
-
-		err = nf_tables_register_hook(net, table, chain);
-		if (err < 0)
-			goto err_register_hooks;
-
-		i++;
-	}
-	return 0;
-
-err_register_hooks:
-	if (i)
-		nft_table_disable(net, table, i);
-	return err;
-}
-
-static void nf_tables_table_disable(struct net *net, struct nft_table *table)
-{
-	nft_table_disable(net, table, 0);
-}
-
 enum {
 	NFT_TABLE_STATE_UNCHANGED	= 0,
 	NFT_TABLE_STATE_DORMANT,
 	NFT_TABLE_STATE_WAKEUP
 };
 
+#define __NFT_TABLE_F_UPDATE	(NFT_TABLE_F_MASK + 1)
+
 static int nf_tables_updtable(struct nft_ctx *ctx)
 {
 	struct nft_trans *trans;
 	u32 flags;
-	int ret = 0;
 
 	if (!ctx->nla[NFTA_TABLE_FLAGS])
 		return 0;
@@ -985,23 +938,19 @@ static int nf_tables_updtable(struct nft_ctx *ctx)
 
 	if ((flags & NFT_TABLE_F_DORMANT) &&
 	    !(ctx->table->flags & NFT_TABLE_F_DORMANT)) {
+		ctx->table->flags |= NFT_TABLE_F_DORMANT | __NFT_TABLE_F_UPDATE;
 		nft_trans_table_state(trans) = NFT_TABLE_STATE_DORMANT;
 	} else if (!(flags & NFT_TABLE_F_DORMANT) &&
 		   ctx->table->flags & NFT_TABLE_F_DORMANT) {
-		ret = nf_tables_table_enable(ctx->net, ctx->table);
-		if (ret >= 0)
-			nft_trans_table_state(trans) = NFT_TABLE_STATE_WAKEUP;
+		ctx->table->flags &= ~NFT_TABLE_F_DORMANT;
+		ctx->table->flags |= __NFT_TABLE_F_UPDATE;
+		nft_trans_table_state(trans) = NFT_TABLE_STATE_WAKEUP;
 	}
-	if (ret < 0)
-		goto err;
 
-	nft_trans_table_flags(trans) = flags;
 	nft_trans_table_update(trans) = true;
 	nft_trans_commit_list_add_tail(ctx->net, trans);
+
 	return 0;
-err:
-	nft_trans_destroy(trans);
-	return ret;
 }
 
 static u32 nft_chain_hash(const void *data, u32 len, u32 seed)
@@ -8214,6 +8163,17 @@ static int nf_tables_commit_chain_prepare(struct net *net, struct nft_chain *cha
 	if (chain->rules_next || !nft_is_active_next(net, chain))
 		return 0;
 
+	if (chain->table->flags & NFT_TABLE_F_DORMANT) {
+		chain->rules_next = nf_tables_chain_alloc_rules(chain, 1);
+		if (!chain->rules_next)
+			return -ENOMEM;
+
+		chain->rules_next[0] = nft_accept_all_rule;
+		chain->rules_next[1] = NULL;
+
+		return 0;
+	}
+
 	rule = list_entry(&chain->rules, struct nft_rule, list);
 	i = 0;
 
@@ -8513,8 +8473,21 @@ static int nf_tables_commit(struct net *net, struct sk_buff *skb)
 			nf_tables_commit_chain_prepare_cancel(net);
 			return ret;
 		}
-		if (trans->msg_type == NFT_MSG_NEWRULE ||
-		    trans->msg_type == NFT_MSG_DELRULE) {
+		switch (trans->msg_type) {
+		case NFT_MSG_NEWTABLE:
+			if (!(trans->ctx.table->flags & __NFT_TABLE_F_UPDATE))
+				break;
+
+			list_for_each_entry(chain, &trans->ctx.table->chains, list) {
+				ret = nf_tables_commit_chain_prepare(net, chain);
+				if (ret < 0) {
+					nf_tables_commit_chain_prepare_cancel(net);
+					return ret;
+				}
+			}
+			break;
+		case NFT_MSG_NEWRULE:
+		case NFT_MSG_DELRULE:
 			chain = trans->ctx.chain;
 
 			ret = nf_tables_commit_chain_prepare(net, chain);
@@ -8522,6 +8495,7 @@ static int nf_tables_commit(struct net *net, struct sk_buff *skb)
 				nf_tables_commit_chain_prepare_cancel(net);
 				return ret;
 			}
+			break;
 		}
 	}
 
@@ -8546,14 +8520,11 @@ static int nf_tables_commit(struct net *net, struct sk_buff *skb)
 					       trans->msg_type);
 		switch (trans->msg_type) {
 		case NFT_MSG_NEWTABLE:
-			if (nft_trans_table_update(trans)) {
-				if (nft_trans_table_state(trans) == NFT_TABLE_STATE_DORMANT)
-					nf_tables_table_disable(net, trans->ctx.table);
-
-				trans->ctx.table->flags = nft_trans_table_flags(trans);
-			} else {
+			if (nft_trans_table_update(trans))
+				trans->ctx.table->flags &= ~__NFT_TABLE_F_UPDATE;
+			else
 				nft_clear(net, trans->ctx.table);
-			}
+
 			nf_tables_table_notify(&trans->ctx, NFT_MSG_NEWTABLE);
 			nft_trans_destroy(trans);
 			break;
@@ -8769,8 +8740,11 @@ static int __nf_tables_abort(struct net *net, enum nfnl_abort_action action)
 		case NFT_MSG_NEWTABLE:
 			if (nft_trans_table_update(trans)) {
 				if (nft_trans_table_state(trans) == NFT_TABLE_STATE_WAKEUP)
-					nf_tables_table_disable(net, trans->ctx.table);
+					trans->ctx.table->flags |= NFT_TABLE_F_DORMANT;
+				else if (nft_trans_table_state(trans) == NFT_TABLE_STATE_DORMANT)
+					trans->ctx.table->flags &= ~NFT_TABLE_F_DORMANT;
 
+				trans->ctx.table->flags &= ~__NFT_TABLE_F_UPDATE;
 				nft_trans_destroy(trans);
 			} else {
 				list_del_rcu(&trans->ctx.table->list);
@@ -9669,6 +9643,27 @@ static struct pernet_operations nf_tables_net_ops = {
 	.size		= sizeof(struct nftables_pernet),
 };
 
+static int nft_rule_accept_all_init(void)
+{
+	struct nft_immediate_expr *priv;
+	struct nft_rule *rule;
+	struct nft_expr *expr;
+
+	rule = kzalloc(sizeof(*rule) +
+		       NFT_EXPR_SIZE(sizeof(struct nft_immediate_expr)),
+		       GFP_KERNEL);
+	if (!rule)
+		return -ENOMEM;
+
+	expr = nft_expr_first(rule);
+	priv = nft_expr_priv(expr);
+	priv->dreg = NFT_REG_VERDICT;
+	priv->data.verdict.code = NF_ACCEPT;
+	nft_accept_all_rule = rule;
+
+	return 0;
+}
+
 static int __init nf_tables_module_init(void)
 {
 	int err;
@@ -9697,6 +9692,10 @@ static int __init nf_tables_module_init(void)
 	if (err < 0)
 		goto err_offload;
 
+	err = nft_rule_accept_all_init();
+	if (err < 0)
+		goto err_accept_all_rule;
+
 	err = netlink_register_notifier(&nft_nl_notifier);
 	if (err < 0)
 		goto err_netlink_notifier;
@@ -9713,6 +9712,8 @@ static int __init nf_tables_module_init(void)
 err_nfnl_subsys:
 	netlink_unregister_notifier(&nft_nl_notifier);
 err_netlink_notifier:
+	kfree(nft_accept_all_rule);
+err_accept_all_rule:
 	nft_offload_exit();
 err_offload:
 	rhltable_destroy(&nft_objname_ht);
@@ -9731,6 +9732,7 @@ static void __exit nf_tables_module_exit(void)
 {
 	nfnetlink_subsys_unregister(&nf_tables_subsys);
 	netlink_unregister_notifier(&nft_nl_notifier);
+	kfree(nft_accept_all_rule);
 	nft_offload_exit();
 	unregister_netdevice_notifier(&nf_tables_flowtable_notifier);
 	nft_chain_filter_fini();
-- 
2.30.2

Re: [PATCH nf,v2] netfilter: nftables: accept all dummy chain when table is dormant

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> The dormant flag need to be updated from the preparation phase,
> otherwise, two consecutive requests to dorm a table in the same batch
> might try to remove the same hooks twice, resulting in the following
> warning:
> 
>  hook not found, pf 3 num 0
>  WARNING: CPU: 0 PID: 334 at net/netfilter/core.c:480 __nf_unregister_net_hook+0x1eb/0x610 net/netfilter/core.c:480
>  Modules linked in:
>  CPU: 0 PID: 334 Comm: kworker/u4:5 Not tainted 5.12.0-syzkaller #0
>  Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
>  Workqueue: netns cleanup_net
>  RIP: 0010:__nf_unregister_net_hook+0x1eb/0x610 net/netfilter/core.c:480
> 
> This patch is a partial revert of 0ce7cf4127f1 ("netfilter: nftables:
> update table flags from the commit phase") to restore the previous
> behaviour, which updates the dormant flag from the preparation phase
> to address this issue.
> 
> However, there is still another problem: A batch containing a series of
> dorm-wakeup-dorm table and vice-versa also trigger the warning above
> since hook unregistration happens from the preparation phase, while hook
> registration occurs from the commit phase.

You could add nf_unregister_net_hook_try() or somesuch that elides
the WARN().

AFAIU this would not be needed at all if the WARN would not exist.
We could also just remove the WARN but it did catch the earlier
NFPROTO_ARP bug, so I would refer to keep it.

Re: [PATCH nf,v2] netfilter: nftables: accept all dummy chain when table is dormant

[Pablo Neira Ayuso]

On Wed, May 19, 2021 at 02:15:33PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > The dormant flag need to be updated from the preparation phase,
> > otherwise, two consecutive requests to dorm a table in the same batch
> > might try to remove the same hooks twice, resulting in the following
> > warning:
> > 
> >  hook not found, pf 3 num 0
> >  WARNING: CPU: 0 PID: 334 at net/netfilter/core.c:480 __nf_unregister_net_hook+0x1eb/0x610 net/netfilter/core.c:480
> >  Modules linked in:
> >  CPU: 0 PID: 334 Comm: kworker/u4:5 Not tainted 5.12.0-syzkaller #0
> >  Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> >  Workqueue: netns cleanup_net
> >  RIP: 0010:__nf_unregister_net_hook+0x1eb/0x610 net/netfilter/core.c:480
> > 
> > This patch is a partial revert of 0ce7cf4127f1 ("netfilter: nftables:
> > update table flags from the commit phase") to restore the previous
> > behaviour, which updates the dormant flag from the preparation phase
> > to address this issue.
> > 
> > However, there is still another problem: A batch containing a series of
> > dorm-wakeup-dorm table and vice-versa also trigger the warning above
> > since hook unregistration happens from the preparation phase, while hook
> > registration occurs from the commit phase.
> 
> You could add nf_unregister_net_hook_try() or somesuch that elides
> the WARN().

Let me elaborate a bit more the problem here.

The hook registration happens from the preparation phase (because it
might fail for several reasons) and hook unregistration happens from
the commit phase (because registration from the abort phase is not
possible, since registration might fail). Hence, nf_register_net_hook()
cannot be called from the commit and abort phases, only from the
preparation phase.

I'm assuming that looking at dormant flags from commit/abort phase
to decide what to do.

Let's have a look a several possible scenarios:

Scenario A) batch containing two commands: dorm + wake up

From preparation phase.

- dorm: preparation phase sets the dormant flag (hooks are
        still registered).
- wake up: unset the dormant flag. This needs to skip hook
           registration, because they are already registered.
           (it needs a way to check that hooks are registered).

From commit phase.

- dorm: dormant flag is unset, no-op.
- wake-up: dormant flag is unset, no-op.

From abort phase (reversed), it undoes preparation phase.

- wake-up: set the dormant flag, unregister hooks.
- dorm: unset the dormant flag, register hooks (not possible)

Problems: Needs a function to check if hooks are present.
          abort phase needs to register hooks.

Scenario B) batch containing two commands: wake up + dorm

From preparation phase.

- wake up: unset the dormant flag. This needs to register hooks.
- dorm: preparation phase sets the dormant flag (hooks are
        still registered).

From commit phase.

- wake-up: dormant flag is set, unregister hooks.
- dorm: dormant flag is set, unregister hooks (again).

From abort phase (reversed), it undoes preparation phase.

- dorm: unset the dormant flag, register hooks (not possible)
- wake-up: set dormant flag, unregister hooks.

Problems: commit phase needs try_unregister hook function.
          abort phase needs to unregister hooks.

I can also describe dorm + wake up + dorm, and wake up + dorm + wake
up scenarios.

I also tried looking at the transaction state instead of the dormant
flags, similar problems.

I also tried adding more internal flags to annotate context. I looked
at adding fields to nft_table to count for the number of pending hook
registration / unregistration. It's all tricky.

The patch I posted is addressing the issue by skipping hook
registration / unregistration for dormant flags updates.

What's your concern with the approach I'm proposing?

Re: [PATCH nf,v2] netfilter: nftables: accept all dummy chain when table is dormant

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Wed, May 19, 2021 at 02:15:33PM +0200, Florian Westphal wrote:
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > The dormant flag need to be updated from the preparation phase,
> > > otherwise, two consecutive requests to dorm a table in the same batch
> > > might try to remove the same hooks twice, resulting in the following
> > > warning:
> > > 
> > >  hook not found, pf 3 num 0
> > >  WARNING: CPU: 0 PID: 334 at net/netfilter/core.c:480 __nf_unregister_net_hook+0x1eb/0x610 net/netfilter/core.c:480
> > >  Modules linked in:
> > >  CPU: 0 PID: 334 Comm: kworker/u4:5 Not tainted 5.12.0-syzkaller #0
> > >  Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> > >  Workqueue: netns cleanup_net
> > >  RIP: 0010:__nf_unregister_net_hook+0x1eb/0x610 net/netfilter/core.c:480
> > > 
> > > This patch is a partial revert of 0ce7cf4127f1 ("netfilter: nftables:
> > > update table flags from the commit phase") to restore the previous
> > > behaviour, which updates the dormant flag from the preparation phase
> > > to address this issue.
> > > 
> > > However, there is still another problem: A batch containing a series of
> > > dorm-wakeup-dorm table and vice-versa also trigger the warning above
> > > since hook unregistration happens from the preparation phase, while hook
> > > registration occurs from the commit phase.
> > 
> > You could add nf_unregister_net_hook_try() or somesuch that elides
> > the WARN().
> 
> Let me elaborate a bit more the problem here.

Thanks, that makes it much clearer.

> The hook registration happens from the preparation phase (because it
> might fail for several reasons) and hook unregistration happens from
> the commit phase (because registration from the abort phase is not
> possible, since registration might fail). Hence, nf_register_net_hook()
> cannot be called from the commit and abort phases, only from the
> preparation phase.
> 
> I'm assuming that looking at dormant flags from commit/abort phase
> to decide what to do.
> 
> Let's have a look a several possible scenarios:
> 
> Scenario A) batch containing two commands: dorm + wake up
> 
> From preparation phase.
> 
> - dorm: preparation phase sets the dormant flag (hooks are
>         still registered).
> - wake up: unset the dormant flag. This needs to skip hook
>            registration, because they are already registered.
>            (it needs a way to check that hooks are registered).
> 
> From commit phase.
> 
> - dorm: dormant flag is unset, no-op.
> - wake-up: dormant flag is unset, no-op.
> 
> From abort phase (reversed), it undoes preparation phase.
> 
> - wake-up: set the dormant flag, unregister hooks.
> - dorm: unset the dormant flag, register hooks (not possible)
> 
> Problems: Needs a function to check if hooks are present.
>           abort phase needs to register hooks.

I agree that abort and/or commit phases cannot register hooks.

> Scenario B) batch containing two commands: wake up + dorm
> 
> From preparation phase.
> 
> - wake up: unset the dormant flag. This needs to register hooks.
> - dorm: preparation phase sets the dormant flag (hooks are
>         still registered).
> 
> From commit phase.
> 
> - wake-up: dormant flag is set, unregister hooks.
> - dorm: dormant flag is set, unregister hooks (again).
> 
> From abort phase (reversed), it undoes preparation phase.
> 
> - dorm: unset the dormant flag, register hooks (not possible)
> - wake-up: set dormant flag, unregister hooks.
> 
> Problems: commit phase needs try_unregister hook function.
>           abort phase needs to unregister hooks.

... but that is doable in the sense that unregister can't fail.

> I also tried looking at the transaction state instead of the dormant
> flags, similar problems.
> 
> I also tried adding more internal flags to annotate context. I looked
> at adding fields to nft_table to count for the number of pending hook
> registration / unregistration. It's all tricky.

Ok, too bad.

> The patch I posted is addressing the issue by skipping hook
> registration / unregistration for dormant flags updates.
> 
> What's your concern with the approach I'm proposing?

No concern, I did not understand the problem with hook register in
abort/commit.

I also dislike that dormat tables now retrain the hook overhead, but
I guess dormat tables are an exception and not the norm, so maybe
unfounded concern.

Re: [PATCH nf,v2] netfilter: nftables: accept all dummy chain when table is dormant

[Pablo Neira Ayuso]

On Wed, May 19, 2021 at 08:34:04PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
[...]
> > Let's have a look a several possible scenarios:
> > 
> > Scenario A) batch containing two commands: dorm + wake up
> > 
> > From preparation phase.
> > 
> > - dorm: preparation phase sets the dormant flag (hooks are
> >         still registered).
> > - wake up: unset the dormant flag. This needs to skip hook
> >            registration, because they are already registered.
> >            (it needs a way to check that hooks are registered).
> > 
> > From commit phase.
> > 
> > - dorm: dormant flag is unset, no-op.
> > - wake-up: dormant flag is unset, no-op.
> > 
> > From abort phase (reversed), it undoes preparation phase.
> > 
> > - wake-up: set the dormant flag, unregister hooks.
> > - dorm: unset the dormant flag, register hooks (not possible)
> > 
> > Problems: Needs a function to check if hooks are present.
> >           abort phase needs to register hooks.
> 
> I agree that abort and/or commit phases cannot register hooks.
> 
> > Scenario B) batch containing two commands: wake up + dorm
> > 
> > From preparation phase.
> > 
> > - wake up: unset the dormant flag. This needs to register hooks.
> > - dorm: preparation phase sets the dormant flag (hooks are
> >         still registered).
> > 
> > From commit phase.
> > 
> > - wake-up: dormant flag is set, unregister hooks.
> > - dorm: dormant flag is set, unregister hooks (again).
> > 
> > From abort phase (reversed), it undoes preparation phase.
> > 
> > - dorm: unset the dormant flag, register hooks (not possible)
> > - wake-up: set dormant flag, unregister hooks.
> > 
> > Problems: commit phase needs try_unregister hook function.
> >           abort phase needs to unregister hooks.
> 
> ... but that is doable in the sense that unregister can't fail.

Right, but adding "unregister hooks" to the abort path breaks a
different scenario. This might unregister a hook that, because of a later
wake-up action, needs to stay there, because you cannot call register
a hook from the abort path, it's a bit of a whac-a-mole game.

> > I also tried looking at the transaction state instead of the dormant
> > flags, similar problems.
> > 
> > I also tried adding more internal flags to annotate context. I looked
> > at adding fields to nft_table to count for the number of pending hook
> > registration / unregistration. It's all tricky.
> 
> Ok, too bad.
> 
> > The patch I posted is addressing the issue by skipping hook
> > registration / unregistration for dormant flags updates.
> > 
> > What's your concern with the approach I'm proposing?
> 
> No concern, I did not understand the problem with hook register in
> abort/commit.
> 
> I also dislike that dormat tables now retrain the hook overhead, but
> I guess dormat tables are an exception and not the norm, so maybe
> unfounded concern.

You are right that this approach incurs in the hook evaluation penalty
from the packet path. But I don't think there's a need to optimize
this feature at this stage. If it turns out that this needs to be
optimized, maybe it should be possible to add a core feature to
disable hook while leaving in registered (ie. hook "dormant" state).

So I'm just inclined to keep it simple while making sure that any
possible (silly) robot-generated sequence with this toggle works fine.

Re: [PATCH nf,v2] netfilter: nftables: accept all dummy chain when table is dormant

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Wed, May 19, 2021 at 08:34:04PM +0200, Florian Westphal wrote:
> > ... but that is doable in the sense that unregister can't fail.
> 
> Right, but adding "unregister hooks" to the abort path breaks a
> different scenario. This might unregister a hook that, because of a later
> wake-up action, needs to stay there, because you cannot call register
> a hook from the abort path, it's a bit of a whac-a-mole game.

Argh, indeed.  We'd have to re-scan the transaction log during
preparation phase for each dormant on/off and chain add/delete to
see if the action un-does an earlier pending one, then remove both
if they cancel each other.

> > I guess dormat tables are an exception and not the norm, so maybe
> > unfounded concern.
> 
> You are right that this approach incurs in the hook evaluation penalty
> from the packet path. But I don't think there's a need to optimize
> this feature at this stage

Ok, I dislike optimizing too early as well.

> So I'm just inclined to keep it simple while making sure that any
> possible (silly) robot-generated sequence with this toggle works fine.

Ok, lets use your approach then.