| Message ID | 20250512030252.3329782-1-jianqi.ren.cn@windriver.com |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | [6.1.y] netfilter: nf_tables: fix memleak in map from abort path | expand |
Hi,
NACK.
This patch requires:
e79b47a8615d ("netfilter: nf_tables: restore set elements when delete set fails")
which you have skipped for some reason.
On Mon, May 12, 2025 at 11:02:52AM +0800, jianqi.ren.cn@windriver.com wrote:
> From: Pablo Neira Ayuso <pablo@netfilter.org>
>
> [ Upstream commit 86a1471d7cde792941109b93b558b5dc078b9ee9 ]
>
> The delete set command does not rely on the transaction object for
> element removal, therefore, a combination of delete element + delete set
> from the abort path could result in restoring twice the refcount of the
> mapping.
>
> Check for inactive element in the next generation for the delete element
> command in the abort path, skip restoring state if next generation bit
> has been already cleared. This is similar to the activate logic using
> the set walk iterator.
>
> [ 6170.286929] ------------[ cut here ]------------
> [ 6170.286939] WARNING: CPU: 6 PID: 790302 at net/netfilter/nf_tables_api.c:2086 nf_tables_chain_destroy+0x1f7/0x220 [nf_tables]
> [ 6170.287071] Modules linked in: [...]
> [ 6170.287633] CPU: 6 PID: 790302 Comm: kworker/6:2 Not tainted 6.9.0-rc3+ #365
> [ 6170.287768] RIP: 0010:nf_tables_chain_destroy+0x1f7/0x220 [nf_tables]
> [ 6170.287886] Code: df 48 8d 7d 58 e8 69 2e 3b df 48 8b 7d 58 e8 80 1b 37 df 48 8d 7d 68 e8 57 2e 3b df 48 8b 7d 68 e8 6e 1b 37 df 48 89 ef eb c4 <0f> 0b 48 83 c4 08 5b 5d 41 5c 41 5d 41 5e 41 5f c3 cc cc cc cc 0f
> [ 6170.287895] RSP: 0018:ffff888134b8fd08 EFLAGS: 00010202
> [ 6170.287904] RAX: 0000000000000001 RBX: ffff888125bffb28 RCX: dffffc0000000000
> [ 6170.287912] RDX: 0000000000000003 RSI: ffffffffa20298ab RDI: ffff88811ebe4750
> [ 6170.287919] RBP: ffff88811ebe4700 R08: ffff88838e812650 R09: fffffbfff0623a55
> [ 6170.287926] R10: ffffffff8311d2af R11: 0000000000000001 R12: ffff888125bffb10
> [ 6170.287933] R13: ffff888125bffb10 R14: dead000000000122 R15: dead000000000100
> [ 6170.287940] FS: 0000000000000000(0000) GS:ffff888390b00000(0000) knlGS:0000000000000000
> [ 6170.287948] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 6170.287955] CR2: 00007fd31fc00710 CR3: 0000000133f60004 CR4: 00000000001706f0
> [ 6170.287962] Call Trace:
> [ 6170.287967] <TASK>
> [ 6170.287973] ? __warn+0x9f/0x1a0
> [ 6170.287986] ? nf_tables_chain_destroy+0x1f7/0x220 [nf_tables]
> [ 6170.288092] ? report_bug+0x1b1/0x1e0
> [ 6170.287986] ? nf_tables_chain_destroy+0x1f7/0x220 [nf_tables]
> [ 6170.288092] ? report_bug+0x1b1/0x1e0
> [ 6170.288104] ? handle_bug+0x3c/0x70
> [ 6170.288112] ? exc_invalid_op+0x17/0x40
> [ 6170.288120] ? asm_exc_invalid_op+0x1a/0x20
> [ 6170.288132] ? nf_tables_chain_destroy+0x2b/0x220 [nf_tables]
> [ 6170.288243] ? nf_tables_chain_destroy+0x1f7/0x220 [nf_tables]
> [ 6170.288366] ? nf_tables_chain_destroy+0x2b/0x220 [nf_tables]
> [ 6170.288483] nf_tables_trans_destroy_work+0x588/0x590 [nf_tables]
>
> Fixes: 591054469b3e ("netfilter: nf_tables: revisit chain/object refcounting from elements")
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> [fixed conflicts due to missing commits
> 0e1ea651c9717ddcd8e0648d8468477a31867b0a ("netfilter: nf_tables: shrink
> memory consumption of set elements") and
> 9dad402b89e81a0516bad5e0ac009b7a0a80898f ("netfilter: nf_tables: expose
> opaque set element as struct nft_elem_priv") so we pass the correct types
> and values to nft_setelem_active_next() + nft_set_elem_ext()]
> Signed-off-by: Jianqi Ren <jianqi.ren.cn@windriver.com>
> Signed-off-by: He Zhe <zhe.he@windriver.com>
> ---
> Verified the build test
> ---
> net/netfilter/nf_tables_api.c | 16 ++++++++++++++--
> 1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index 656c4fb76773..1d4d77d21d61 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -6772,6 +6772,16 @@ void nft_data_hold(const struct nft_data *data, enum nft_data_types type)
> }
> }
>
> +static int nft_setelem_active_next(const struct net *net,
> + const struct nft_set *set,
> + struct nft_set_elem *elem)
> +{
> + const struct nft_set_ext *ext = nft_set_elem_ext(set, elem->priv);
> + u8 genmask = nft_genmask_next(net);
> +
> + return nft_set_elem_active(ext, genmask);
> +}
> +
> static void nft_setelem_data_activate(const struct net *net,
> const struct nft_set *set,
> struct nft_set_elem *elem)
> @@ -10115,8 +10125,10 @@ static int __nf_tables_abort(struct net *net, enum nfnl_abort_action action)
> case NFT_MSG_DELSETELEM:
> te = (struct nft_trans_elem *)trans->data;
>
> - nft_setelem_data_activate(net, te->set, &te->elem);
> - nft_setelem_activate(net, te->set, &te->elem);
> + if (!nft_setelem_active_next(net, te->set, &te->elem)) {
> + nft_setelem_data_activate(net, te->set, &te->elem);
> + nft_setelem_activate(net, te->set, &te->elem);
> + }
> if (!nft_setelem_is_catchall(te->set, &te->elem))
> te->set->ndeact--;
>
> --
> 2.34.1
>
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c index 656c4fb76773..1d4d77d21d61 100644 --- a/net/netfilter/nf_tables_api.c +++ b/net/netfilter/nf_tables_api.c @@ -6772,6 +6772,16 @@ void nft_data_hold(const struct nft_data *data, enum nft_data_types type) } } +static int nft_setelem_active_next(const struct net *net, + const struct nft_set *set, + struct nft_set_elem *elem) +{ + const struct nft_set_ext *ext = nft_set_elem_ext(set, elem->priv); + u8 genmask = nft_genmask_next(net); + + return nft_set_elem_active(ext, genmask); +} + static void nft_setelem_data_activate(const struct net *net, const struct nft_set *set, struct nft_set_elem *elem) @@ -10115,8 +10125,10 @@ static int __nf_tables_abort(struct net *net, enum nfnl_abort_action action) case NFT_MSG_DELSETELEM: te = (struct nft_trans_elem *)trans->data; - nft_setelem_data_activate(net, te->set, &te->elem); - nft_setelem_activate(net, te->set, &te->elem); + if (!nft_setelem_active_next(net, te->set, &te->elem)) { + nft_setelem_data_activate(net, te->set, &te->elem); + nft_setelem_activate(net, te->set, &te->elem); + } if (!nft_setelem_is_catchall(te->set, &te->elem)) te->set->ndeact--;