From: Pablo Neira Ayuso <pablo@netfilter.org>
To: netfilter-devel@vger.kernel.org
Cc: gregkh@linuxfoundation.org, sashal@kernel.org, stable@vger.kernel.org
Subject: [PATCH -stable,5.4 24/26] netfilter: nf_tables: fix table flag updates
Date: Tue, 21 Nov 2023 13:13:31 +0100 [thread overview]
Message-ID: <20231121121333.294238-25-pablo@netfilter.org> (raw)
In-Reply-To: <20231121121333.294238-1-pablo@netfilter.org>
commit 179d9ba5559a756f4322583388b3213fe4e391b0 upstream.
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.
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 two internal flags to annotate the
original dormant flag status which are __NFT_TABLE_F_WAS_DORMANT and
__NFT_TABLE_F_WAS_AWAKEN, to restore it from the abort path.
The __NFT_TABLE_F_UPDATE bitmask allows to handle the dormant flag update
with one single transaction.
Reported-by: syzbot+7ad5cd1615f2d89c6e7e@syzkaller.appspotmail.com
Fixes: 0ce7cf4127f1 ("netfilter: nftables: update table flags from the commit phase")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
include/net/netfilter/nf_tables.h | 6 ---
include/uapi/linux/netfilter/nf_tables.h | 1 +
net/netfilter/nf_tables_api.c | 59 ++++++++++++++++--------
3 files changed, 41 insertions(+), 25 deletions(-)
diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index 59dd0845605a..0a49d44ddb84 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -1392,16 +1392,10 @@ 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/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
index 0a995403172c..6a08c03a511d 100644
--- a/include/uapi/linux/netfilter/nf_tables.h
+++ b/include/uapi/linux/netfilter/nf_tables.h
@@ -162,6 +162,7 @@ enum nft_hook_attributes {
enum nft_table_flags {
NFT_TABLE_F_DORMANT = 0x1,
};
+#define NFT_TABLE_F_MASK (NFT_TABLE_F_DORMANT)
/**
* enum nft_table_attributes - nf_tables table netlink attributes
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index ffe586d8cdcb..f8685e1eb29e 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -701,7 +701,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))
@@ -890,20 +891,22 @@ static int nf_tables_table_enable(struct net *net, struct nft_table *table)
static void nf_tables_table_disable(struct net *net, struct nft_table *table)
{
+ table->flags &= ~NFT_TABLE_F_DORMANT;
nft_table_disable(net, table, 0);
+ table->flags |= NFT_TABLE_F_DORMANT;
}
-enum {
- NFT_TABLE_STATE_UNCHANGED = 0,
- NFT_TABLE_STATE_DORMANT,
- NFT_TABLE_STATE_WAKEUP
-};
+#define __NFT_TABLE_F_INTERNAL (NFT_TABLE_F_MASK + 1)
+#define __NFT_TABLE_F_WAS_DORMANT (__NFT_TABLE_F_INTERNAL << 0)
+#define __NFT_TABLE_F_WAS_AWAKEN (__NFT_TABLE_F_INTERNAL << 1)
+#define __NFT_TABLE_F_UPDATE (__NFT_TABLE_F_WAS_DORMANT | \
+ __NFT_TABLE_F_WAS_AWAKEN)
static int nf_tables_updtable(struct nft_ctx *ctx)
{
struct nft_trans *trans;
u32 flags;
- int ret = 0;
+ int ret;
if (!ctx->nla[NFTA_TABLE_FLAGS])
return 0;
@@ -922,21 +925,27 @@ static int nf_tables_updtable(struct nft_ctx *ctx)
if ((flags & NFT_TABLE_F_DORMANT) &&
!(ctx->table->flags & NFT_TABLE_F_DORMANT)) {
- nft_trans_table_state(trans) = NFT_TABLE_STATE_DORMANT;
+ ctx->table->flags |= NFT_TABLE_F_DORMANT;
+ if (!(ctx->table->flags & __NFT_TABLE_F_UPDATE))
+ ctx->table->flags |= __NFT_TABLE_F_WAS_AWAKEN;
} 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;
+ if (!(ctx->table->flags & __NFT_TABLE_F_UPDATE)) {
+ ret = nf_tables_table_enable(ctx->net, ctx->table);
+ if (ret < 0)
+ goto err_register_hooks;
+
+ ctx->table->flags |= __NFT_TABLE_F_WAS_DORMANT;
+ }
}
- 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:
+
+err_register_hooks:
nft_trans_destroy(trans);
return ret;
}
@@ -7302,10 +7311,14 @@ static int nf_tables_commit(struct net *net, struct sk_buff *skb)
switch (trans->msg_type) {
case NFT_MSG_NEWTABLE:
if (nft_trans_table_update(trans)) {
- if (nft_trans_table_state(trans) == NFT_TABLE_STATE_DORMANT)
+ if (!(trans->ctx.table->flags & __NFT_TABLE_F_UPDATE)) {
+ nft_trans_destroy(trans);
+ break;
+ }
+ if (trans->ctx.table->flags & NFT_TABLE_F_DORMANT)
nf_tables_table_disable(net, trans->ctx.table);
- trans->ctx.table->flags = nft_trans_table_flags(trans);
+ trans->ctx.table->flags &= ~__NFT_TABLE_F_UPDATE;
} else {
nft_clear(net, trans->ctx.table);
}
@@ -7500,9 +7513,17 @@ static int __nf_tables_abort(struct net *net, enum nfnl_abort_action action)
switch (trans->msg_type) {
case NFT_MSG_NEWTABLE:
if (nft_trans_table_update(trans)) {
- if (nft_trans_table_state(trans) == NFT_TABLE_STATE_WAKEUP)
+ if (!(trans->ctx.table->flags & __NFT_TABLE_F_UPDATE)) {
+ nft_trans_destroy(trans);
+ break;
+ }
+ if (trans->ctx.table->flags & __NFT_TABLE_F_WAS_DORMANT) {
nf_tables_table_disable(net, trans->ctx.table);
-
+ trans->ctx.table->flags |= NFT_TABLE_F_DORMANT;
+ } else if (trans->ctx.table->flags & __NFT_TABLE_F_WAS_AWAKEN) {
+ 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);
--
2.30.2
next prev parent reply other threads:[~2023-11-21 12:13 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-21 12:13 [PATCH -stable,5.4 00/26] Netfilter stable fixes for 5.4 Pablo Neira Ayuso
2023-11-21 12:13 ` [PATCH -stable,5.4 01/26] netfilter: nf_tables: pass context to nft_set_destroy() Pablo Neira Ayuso
2023-11-21 12:13 ` [PATCH -stable,5.4 02/26] netfilter: nftables: rename set element data activation/deactivation functions Pablo Neira Ayuso
2023-11-21 16:19 ` [PATCH 2/26] " kernel test robot
2023-11-21 12:13 ` [PATCH -stable,5.4 03/26] netfilter: nf_tables: drop map element references from preparation phase Pablo Neira Ayuso
2023-11-21 12:13 ` [PATCH -stable,5.4 04/26] netfilter: nft_set_rbtree: Switch to node list walk for overlap detection Pablo Neira Ayuso
2023-11-21 12:13 ` [PATCH -stable,5.4 05/26] netfilter: nft_set_rbtree: fix null deref on element insertion Pablo Neira Ayuso
2023-11-21 12:13 ` [PATCH -stable,5.4 06/26] netfilter: nft_set_rbtree: fix overlap expiration walk Pablo Neira Ayuso
2023-11-21 12:13 ` [PATCH -stable,5.4 07/26] netfilter: nf_tables: don't skip expired elements during walk Pablo Neira Ayuso
2023-11-21 12:13 ` [PATCH -stable,5.4 08/26] netfilter: nf_tables: GC transaction API to avoid race with control plane Pablo Neira Ayuso
2023-11-21 12:13 ` [PATCH -stable,5.4 09/26] netfilter: nf_tables: adapt set backend to use GC transaction API Pablo Neira Ayuso
2023-11-21 12:13 ` [PATCH -stable,5.4 10/26] netfilter: nft_set_hash: mark set element as dead when deleting from packet path Pablo Neira Ayuso
2023-11-21 12:13 ` [PATCH -stable,5.4 11/26] netfilter: nf_tables: remove busy mark and gc batch API Pablo Neira Ayuso
2023-11-21 12:13 ` [PATCH -stable,5.4 12/26] netfilter: nf_tables: fix GC transaction races with netns and netlink event exit path Pablo Neira Ayuso
2023-11-21 12:13 ` [PATCH -stable,5.4 13/26] netfilter: nf_tables: GC transaction race with netns dismantle Pablo Neira Ayuso
2023-11-21 12:13 ` [PATCH -stable,5.4 14/26] netfilter: nf_tables: GC transaction race with abort path Pablo Neira Ayuso
2023-11-21 12:13 ` [PATCH -stable,5.4 15/26] netfilter: nf_tables: use correct lock to protect gc_list Pablo Neira Ayuso
2023-11-21 12:13 ` [PATCH -stable,5.4 16/26] netfilter: nf_tables: defer gc run if previous batch is still pending Pablo Neira Ayuso
2023-11-21 12:13 ` [PATCH -stable,5.4 17/26] netfilter: nft_set_rbtree: skip sync GC for new elements in this transaction Pablo Neira Ayuso
2023-11-21 12:13 ` [PATCH -stable,5.4 18/26] netfilter: nft_set_rbtree: use read spinlock to avoid datapath contention Pablo Neira Ayuso
2023-11-21 12:13 ` [PATCH -stable,5.4 19/26] netfilter: nft_set_hash: try later when GC hits EAGAIN on iteration Pablo Neira Ayuso
2023-11-21 12:13 ` [PATCH -stable,5.4 20/26] netfilter: nf_tables: fix memleak when more than 255 elements expired Pablo Neira Ayuso
2023-11-21 12:13 ` [PATCH -stable,5.4 21/26] netfilter: nf_tables: unregister flowtable hooks on netns exit Pablo Neira Ayuso
2023-11-21 12:13 ` [PATCH -stable,5.4 22/26] netfilter: nf_tables: double hook unregistration in netns path Pablo Neira Ayuso
2023-11-21 12:13 ` [PATCH -stable,5.4 23/26] netfilter: nftables: update table flags from the commit phase Pablo Neira Ayuso
2023-11-22 16:21 ` Sasha Levin
2023-11-22 17:32 ` Pablo Neira Ayuso
2023-11-23 10:36 ` Pablo Neira Ayuso
2023-11-24 16:23 ` Greg KH
2023-11-21 12:13 ` Pablo Neira Ayuso [this message]
2023-11-21 12:13 ` [PATCH -stable,5.4 25/26] netfilter: nf_tables: disable toggling dormant table state more than once Pablo Neira Ayuso
2023-11-21 12:13 ` [PATCH -stable,5.4 26/26] netfilter: nf_tables: bogus EBUSY when deleting flowtable after flush (for 5.4) Pablo Neira Ayuso
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20231121121333.294238-25-pablo@netfilter.org \
--to=pablo@netfilter.org \
--cc=gregkh@linuxfoundation.org \
--cc=netfilter-devel@vger.kernel.org \
--cc=sashal@kernel.org \
--cc=stable@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox