Re: [PATCH nft v2] evaluate: don't update cache for anonymous chains

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Fri, Mar 21, 2025 at 03:30:05PM +0100, Florian Westphal wrote:
> Chain lookup needs a name, not a numerical id.

Thanks for this v2.

> After patch, loading bogon gives following errors:
> 
> Error: No symbol type information a b index 1 10.1.26.a
> 
> v2: Don't return an error, just make it a no-op (Pablo Neira Ayuso)

Fixes: c330152b7f77 ("src: support for implicit chain bindings")

I suggest to modify the comment below before you apply:

> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  src/evaluate.c                                            | 7 +++++++
>  .../bogons/nft-f/null_deref_on_anon_chain_update_crash    | 8 ++++++++
>  2 files changed, 15 insertions(+)
>  create mode 100644 tests/shell/testcases/bogons/nft-f/null_deref_on_anon_chain_update_crash
> 
> diff --git a/src/evaluate.c b/src/evaluate.c
> index a27961193da5..26aa0ef53241 100644
> --- a/src/evaluate.c
> +++ b/src/evaluate.c
> @@ -5371,6 +5371,13 @@ static int rule_cache_update(struct eval_ctx *ctx, enum cmd_ops op)
>  	if (!table)
>  		return table_not_found(ctx);
>  
> +	/* chain is anonymous: no update needed, rules cannot be added
> +	 * or removed from anon chains after the chain was committed to
> +	 * kernel.
> +	 */

I suggest this comment:

        /* chain is anonymous, adding new rules via index is not supported. */

note that this bogon test you provide is actually putting everything
in the same transaction. Your comment regarding kernel behaviour is
correct, but it does not apply to rule_cache_update(). The issue is
that rule_cache_update() does not need to update the cache if this
chain is anonymous.

Thanks.

> +	if (!rule->handle.chain.name)
> +		return 0;
> +
>  	chain = chain_cache_find(table, rule->handle.chain.name);
>  	if (!chain)
>  		return chain_not_found(ctx);
> diff --git a/tests/shell/testcases/bogons/nft-f/null_deref_on_anon_chain_update_crash b/tests/shell/testcases/bogons/nft-f/null_deref_on_anon_chain_update_crash
> new file mode 100644
> index 000000000000..310486c59ee0
> --- /dev/null
> +++ b/tests/shell/testcases/bogons/nft-f/null_deref_on_anon_chain_update_crash
> @@ -0,0 +1,8 @@
> +table ip f {
> +        chain c {
> +                jump {
> +                        accept
> +                }
> +        }
> +}
> +a b index 1 10.1.26.a
> -- 
> 2.48.1
> 
>