* [PATCH nf] netfilter: nf_tables: skip immediate deactivate in _PREPARE_ERROR and _COMMIT
@ 2023-07-20 7:07 Pablo Neira Ayuso
2023-07-20 21:04 ` Florian Westphal
0 siblings, 1 reply; 2+ messages in thread
From: Pablo Neira Ayuso @ 2023-07-20 7:07 UTC (permalink / raw)
To: netfilter-devel
On error when building the rule, the immediate expression unbinds the
chain, hence objects can be deactivated by the transaction records.
Similarly, commit path also does not require deactivate because this is
already done from _PREPARE.
Otherwise, it is possible to trigger the following warning:
WARNING: CPU: 3 PID: 915 at net/netfilter/nf_tables_api.c:2013 nf_tables_chain_destroy+0x1f7/0x210 [nf_tables]
CPU: 3 PID: 915 Comm: chain-bind-err- Not tainted 6.1.39 #1
RIP: 0010:nf_tables_chain_destroy+0x1f7/0x210 [nf_tables]
Reported-by: Kevin Rich <kevinrich1337@gmail.com>
Fixes: 4bedf9eee016 ("netfilter: nf_tables: fix chain binding transaction logic")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
net/netfilter/nft_immediate.c | 30 +++++++++++++++++++++---------
1 file changed, 21 insertions(+), 9 deletions(-)
diff --git a/net/netfilter/nft_immediate.c b/net/netfilter/nft_immediate.c
index 407d7197f75b..a46f872a31cd 100644
--- a/net/netfilter/nft_immediate.c
+++ b/net/netfilter/nft_immediate.c
@@ -125,15 +125,27 @@ static void nft_immediate_activate(const struct nft_ctx *ctx,
return nft_data_hold(&priv->data, nft_dreg_to_type(priv->dreg));
}
+static void nft_immediate_chain_deactivate(const struct nft_ctx *ctx,
+ struct nft_chain *chain,
+ enum nft_trans_phase phase)
+{
+ struct nft_ctx chain_ctx;
+ struct nft_rule *rule;
+
+ chain_ctx = *ctx;
+ chain_ctx.chain = chain;
+
+ list_for_each_entry(rule, &chain->rules, list)
+ nft_rule_expr_deactivate(&chain_ctx, rule, phase);
+}
+
static void nft_immediate_deactivate(const struct nft_ctx *ctx,
const struct nft_expr *expr,
enum nft_trans_phase phase)
{
const struct nft_immediate_expr *priv = nft_expr_priv(expr);
const struct nft_data *data = &priv->data;
- struct nft_ctx chain_ctx;
struct nft_chain *chain;
- struct nft_rule *rule;
if (priv->dreg == NFT_REG_VERDICT) {
switch (data->verdict.code) {
@@ -143,19 +155,19 @@ static void nft_immediate_deactivate(const struct nft_ctx *ctx,
if (!nft_chain_binding(chain))
break;
- chain_ctx = *ctx;
- chain_ctx.chain = chain;
-
- list_for_each_entry(rule, &chain->rules, list)
- nft_rule_expr_deactivate(&chain_ctx, rule, phase);
-
switch (phase) {
case NFT_TRANS_PREPARE_ERROR:
nf_tables_unbind_chain(ctx, chain);
- fallthrough;
+ nft_deactivate_next(ctx->net, chain);
+ break;
case NFT_TRANS_PREPARE:
+ nft_immediate_chain_deactivate(ctx, chain, phase);
nft_deactivate_next(ctx->net, chain);
break;
+ case NFT_TRANS_ABORT:
+ case NFT_TRANS_RELEASE:
+ nft_immediate_chain_deactivate(ctx, chain, phase);
+ fallthrough;
default:
nft_chain_del(chain);
chain->bound = false;
--
2.30.2
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH nf] netfilter: nf_tables: skip immediate deactivate in _PREPARE_ERROR and _COMMIT
2023-07-20 7:07 [PATCH nf] netfilter: nf_tables: skip immediate deactivate in _PREPARE_ERROR and _COMMIT Pablo Neira Ayuso
@ 2023-07-20 21:04 ` Florian Westphal
0 siblings, 0 replies; 2+ messages in thread
From: Florian Westphal @ 2023-07-20 21:04 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: netfilter-devel
Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On error when building the rule, the immediate expression unbinds the
> chain, hence objects can be deactivated by the transaction records.
> Similarly, commit path also does not require deactivate because this is
> already done from _PREPARE.
>
> Otherwise, it is possible to trigger the following warning:
>
> WARNING: CPU: 3 PID: 915 at net/netfilter/nf_tables_api.c:2013 nf_tables_chain_destroy+0x1f7/0x210 [nf_tables]
> CPU: 3 PID: 915 Comm: chain-bind-err- Not tainted 6.1.39 #1
> RIP: 0010:nf_tables_chain_destroy+0x1f7/0x210 [nf_tables]
>
> Reported-by: Kevin Rich <kevinrich1337@gmail.com>
> Fixes: 4bedf9eee016 ("netfilter: nf_tables: fix chain binding transaction logic")
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
> net/netfilter/nft_immediate.c | 30 +++++++++++++++++++++---------
> 1 file changed, 21 insertions(+), 9 deletions(-)
>
> diff --git a/net/netfilter/nft_immediate.c b/net/netfilter/nft_immediate.c
> index 407d7197f75b..a46f872a31cd 100644
> --- a/net/netfilter/nft_immediate.c
> +++ b/net/netfilter/nft_immediate.c
> @@ -125,15 +125,27 @@ static void nft_immediate_activate(const struct nft_ctx *ctx,
> return nft_data_hold(&priv->data, nft_dreg_to_type(priv->dreg));
> }
>
> +static void nft_immediate_chain_deactivate(const struct nft_ctx *ctx,
> + struct nft_chain *chain,
> + enum nft_trans_phase phase)
> +{
> + struct nft_ctx chain_ctx;
> + struct nft_rule *rule;
> +
> + chain_ctx = *ctx;
> + chain_ctx.chain = chain;
> +
> + list_for_each_entry(rule, &chain->rules, list)
> + nft_rule_expr_deactivate(&chain_ctx, rule, phase);
> +}
> +
> static void nft_immediate_deactivate(const struct nft_ctx *ctx,
> const struct nft_expr *expr,
> enum nft_trans_phase phase)
> {
> const struct nft_immediate_expr *priv = nft_expr_priv(expr);
> const struct nft_data *data = &priv->data;
> - struct nft_ctx chain_ctx;
> struct nft_chain *chain;
> - struct nft_rule *rule;
>
> if (priv->dreg == NFT_REG_VERDICT) {
> switch (data->verdict.code) {
> @@ -143,19 +155,19 @@ static void nft_immediate_deactivate(const struct nft_ctx *ctx,
> if (!nft_chain_binding(chain))
> break;
>
> - chain_ctx = *ctx;
> - chain_ctx.chain = chain;
> -
> - list_for_each_entry(rule, &chain->rules, list)
> - nft_rule_expr_deactivate(&chain_ctx, rule, phase);
> -
> switch (phase) {
> case NFT_TRANS_PREPARE_ERROR:
> nf_tables_unbind_chain(ctx, chain);
> - fallthrough;
> + nft_deactivate_next(ctx->net, chain);
> + break;
> case NFT_TRANS_PREPARE:
> + nft_immediate_chain_deactivate(ctx, chain, phase);
> nft_deactivate_next(ctx->net, chain);
> break;
So far so good, don't do deactivate from PREPARE_ERROR.
> + case NFT_TRANS_ABORT:
> + case NFT_TRANS_RELEASE:
> + nft_immediate_chain_deactivate(ctx, chain, phase);
> + fallthrough;
> default:
> nft_chain_del(chain);
But this one I don't understand, this introduces a memory leak.
After this, COMMIT no longer calls deactivate which means
nft_lookup ->deactivate won't call nf_tables_deactivate_set()
which then won't call into nf_tables_unbind_set().
I've removed the entire hunk above to do:
> case NFT_TRANS_PREPARE:
> + nft_immediate_chain_deactivate(ctx, chain, phase);
> nft_deactivate_next(ctx->net, chain);
> break;
> default:
> + nft_immediate_chain_deactivate(ctx, chain, phase);
> nft_chain_del(chain);
Which means nft_immediate_chain_deactivate() is always called except
for PREPARE_ERROR. Does that make sense to you?
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2023-07-20 21:05 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-07-20 7:07 [PATCH nf] netfilter: nf_tables: skip immediate deactivate in _PREPARE_ERROR and _COMMIT Pablo Neira Ayuso
2023-07-20 21:04 ` Florian Westphal
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).