Re: [nft PATCH 3/5] mnl: Allow for updating devices on existing inet ingress hook chains

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

On Fri, Aug 29, 2025 at 04:25:11PM +0200, Phil Sutter wrote:
> Complete commit a66b5ad9540dd ("src: allow for updating devices on
> existing netdev chain") in supporting inet family ingress hook chains as
> well. The kernel does already but nft has to add a proper hooknum
> attribute to pass the checks.
> 
> The hook.num field has to be initialized from hook.name using
> str2hooknum(), which is part of chain evaluation. Calling
> chain_evaluate() just for that purpose is a bit over the top, but the
> hook name lookup may fail and performing chain evaluation for delete
> command as well fits more into the code layout than duplicating parts of
> it in mnl_nft_chain_del() or elsewhere. Just avoid the
> chain_cache_find() call as its assert() triggers when deleting by
> handle and also don't add to be deleted chains to cache.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  src/evaluate.c | 6 ++++--
>  src/mnl.c      | 2 ++
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/src/evaluate.c b/src/evaluate.c
> index b7e4f71fdfbc9..db4ac18f1dc9f 100644
> --- a/src/evaluate.c
> +++ b/src/evaluate.c
> @@ -5758,7 +5758,9 @@ static int chain_evaluate(struct eval_ctx *ctx, struct chain *chain)
>  		return table_not_found(ctx);
>  
>  	if (chain == NULL) {
> -		if (!chain_cache_find(table, ctx->cmd->handle.chain.name)) {
> +		if (ctx->cmd->op != CMD_DELETE &&
> +		    ctx->cmd->op != CMD_DESTROY &&
> +		    !chain_cache_find(table, ctx->cmd->handle.chain.name)) {
>  			chain = chain_alloc();
>  			handle_merge(&chain->handle, &ctx->cmd->handle);
>  			chain_cache_add(chain, table);
> @@ -6070,7 +6072,7 @@ static int cmd_evaluate_delete(struct eval_ctx *ctx, struct cmd *cmd)
>  		return 0;
>  	case CMD_OBJ_CHAIN:
>  		chain_del_cache(ctx, cmd);
> -		return 0;
> +		return chain_evaluate(ctx, cmd->chain);

Maybe fix this to perform chain_del_cache() after chain_evaluate()?
ie.

                if (chain_evaluate(ctx, cmd->chain) < 0)
                        return -1;

                chain_del_cache(ctx, cmd);
                return 0;

>  	case CMD_OBJ_TABLE:
>  		table_del_cache(ctx, cmd);
>  		return 0;
> diff --git a/src/mnl.c b/src/mnl.c
> index 984dcac27b1cf..d1402c0fcb9f4 100644
> --- a/src/mnl.c
> +++ b/src/mnl.c
> @@ -994,6 +994,8 @@ int mnl_nft_chain_del(struct netlink_ctx *ctx, struct cmd *cmd)
>  		struct nlattr *nest;
>  
>  		nest = mnl_attr_nest_start(nlh, NFTA_CHAIN_HOOK);
> +		mnl_attr_put_u32(nlh, NFTA_HOOK_HOOKNUM,
> +				 htonl(cmd->chain->hook.num));
>  		mnl_nft_chain_devs_build(nlh, cmd);
>  		mnl_attr_nest_end(nlh, nest);
>  	}
> -- 
> 2.51.0
>