| Message ID | 20210518224730.317215-1-pablo@netfilter.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [nf] netfilter: nftables: accept all dummy chain when table is dormant | expand |
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 Would it be possible to reject such a batch instead of having to add rely on dummy hooking instead? I don't think we should try to be clever with nonsensical yes-no-yes-yes-no type commits.
On Wed, May 19, 2021 at 12:56:19AM +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 > > Would it be possible to reject such a batch instead of having to add > rely on dummy hooking instead? That's a simple way to fix it, yes, ie. hit EBUSY. > I don't think we should try to be clever with nonsensical yes-no-yes-yes-no > type commits. Note that no such EBUSY limitation exists so far in the transaction semantics that I know [*]. We already discussed that robots might do non-sensical stuff when creating a batches, and reporting EBUSY for this add-del-add case might just break them. This also removes the conditional hook registration, so hooks are registered once at chain creation. This simplifies interaction with the netfilter core at the cost of adding complexity to nf_tables_commit_chain_prepare() path. [*] Well, you might still hit EBUSY from a batch, but that happens if the object is really in use, not because of the add-del-add sequence.
Pablo Neira Ayuso <pablo@netfilter.org> wrote: > On Wed, May 19, 2021 at 12:56:19AM +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 > > > > Would it be possible to reject such a batch instead of having to add > > rely on dummy hooking instead? > > That's a simple way to fix it, yes, ie. hit EBUSY. > > > I don't think we should try to be clever with nonsensical yes-no-yes-yes-no > > type commits. > > Note that no such EBUSY limitation exists so far in the transaction > semantics that I know [*]. We already discussed that robots might do > non-sensical stuff when creating a batches, and reporting EBUSY for > this add-del-add case might just break them. I don't think this breaks existing users, noone except syzbot reported such WARN splat so far. > This also removes the conditional hook registration, so hooks are > registered once at chain creation. This simplifies interaction with > the netfilter core at the cost of adding complexity to > nf_tables_commit_chain_prepare() path. It also adds side effect (hook registration) during preparation phase. I think its similar to add table foo delete table foo delete table foo ... and that gives -ENOENT.
On Wed, May 19, 2021 at 10:30:28AM +0200, Florian Westphal wrote: > Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > On Wed, May 19, 2021 at 12:56:19AM +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 > > > > > > Would it be possible to reject such a batch instead of having to add > > > rely on dummy hooking instead? > > > > That's a simple way to fix it, yes, ie. hit EBUSY. > > > > > I don't think we should try to be clever with nonsensical yes-no-yes-yes-no > > > type commits. > > > > Note that no such EBUSY limitation exists so far in the transaction > > semantics that I know [*]. We already discussed that robots might do > > non-sensical stuff when creating a batches, and reporting EBUSY for > > this add-del-add case might just break them. > > I don't think this breaks existing users, noone except syzbot > reported such WARN splat so far. > > > This also removes the conditional hook registration, so hooks are > > registered once at chain creation. This simplifies interaction with > > the netfilter core at the cost of adding complexity to > > nf_tables_commit_chain_prepare() path. > > It also adds side effect (hook registration) during preparation phase. Chain hook registration always happened from preparation phase before this patch. > I think its similar to > > add table foo > delete table foo > delete table foo > > ... and that gives -ENOENT. This is the preparation phase that is rejecting it with -ENOENT. The sequence that this patch handles is similar to: add table foo delete table foo add table foo which does _not_ hit EBUSY. The existing transaction semantics handles similar sequences for the existing objects. This patch ensures that: add table x add chain x y { type filter hook input priority 0; } add table x { flags dormant; } add table x { ; } in a batch file works fine. A robot could generate such sequence above.
On Wed, May 19, 2021 at 11:01:50AM +0200, Pablo Neira Ayuso wrote: > On Wed, May 19, 2021 at 10:30:28AM +0200, Florian Westphal wrote: > > Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > > On Wed, May 19, 2021 at 12:56:19AM +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 > > > > > > > > Would it be possible to reject such a batch instead of having to add > > > > rely on dummy hooking instead? > > > > > > That's a simple way to fix it, yes, ie. hit EBUSY. > > > > > > > I don't think we should try to be clever with nonsensical yes-no-yes-yes-no > > > > type commits. > > > > > > Note that no such EBUSY limitation exists so far in the transaction > > > semantics that I know [*]. We already discussed that robots might do > > > non-sensical stuff when creating a batches, and reporting EBUSY for > > > this add-del-add case might just break them. > > > > I don't think this breaks existing users, noone except syzbot > > reported such WARN splat so far. > > > > > This also removes the conditional hook registration, so hooks are > > > registered once at chain creation. This simplifies interaction with > > > the netfilter core at the cost of adding complexity to > > > nf_tables_commit_chain_prepare() path. > > > > It also adds side effect (hook registration) during preparation phase. > > Chain hook registration always happened from preparation phase before > this patch. > > > I think its similar to > > > > add table foo > > delete table foo > > delete table foo > > > > ... and that gives -ENOENT. > > This is the preparation phase that is rejecting it with -ENOENT. > > The sequence that this patch handles is similar to: > > add table foo > delete table foo > add table foo > > which does _not_ hit EBUSY. > > The existing transaction semantics handles similar sequences for the > existing objects. > > This patch ensures that: > > add table x > add chain x y { type filter hook input priority 0; } > add table x { flags dormant; } > add table x { ; } > > in a batch file works fine. Actually, the sequence this handle is: add table x add chain x y { type filter hook input priority 0; } add table x { flags dormant; } add table x { ; } add table x { flags dormant; } which is similar to: add table x delete table x add table x > A robot could generate such sequence above.
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..c65f1dd3d148 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,22 @@ 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; + } + } + trans->ctx.table->flags &= ~__NFT_TABLE_F_UPDATE; + break; + case NFT_MSG_NEWRULE: + case NFT_MSG_DELRULE: chain = trans->ctx.chain; ret = nf_tables_commit_chain_prepare(net, chain); @@ -8522,6 +8496,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 +8521,9 @@ 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)) nft_clear(net, trans->ctx.table); - } + nf_tables_table_notify(&trans->ctx, NFT_MSG_NEWTABLE); nft_trans_destroy(trans); break; @@ -8769,8 +8739,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 +9642,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 +9691,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 +9711,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 +9731,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();
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> --- include/net/netfilter/nf_tables.h | 3 - net/netfilter/nf_tables_api.c | 147 +++++++++++++++--------------- 2 files changed, 74 insertions(+), 76 deletions(-)