Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> 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@xxxxxxxxx>
> Fixes: 4bedf9eee016 ("netfilter: nf_tables: fix chain binding transaction logic")
> Signed-off-by: Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx>
> ---
> 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?
