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

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

[-- Attachment #1: Type: text/plain, Size: 2322 bytes --]

On Mon, Sep 08, 2025 at 12:29:03PM +0200, Pablo Neira Ayuso wrote:
> 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);

My suggestion won't work.

Maybe add a specific chain_del_evaluate(), see untested patch attached.

[-- Attachment #2: x.patch --]
[-- Type: text/x-diff, Size: 1191 bytes --]

diff --git a/src/evaluate.c b/src/evaluate.c
index d4971228b61f..7c72a453d0f3 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -5722,6 +5722,37 @@ static uint32_t str2hooknum(uint32_t family, const char *hook)
 	return NF_INET_NUMHOOKS;
 }
 
+static int chain_del_evaluate(struct eval_ctx *ctx, struct chain *chain)
+{
+	struct table *table;
+
+	table = table_cache_find(&ctx->nft->cache.table_cache,
+				 ctx->cmd->handle.table.name,
+				 ctx->cmd->handle.family);
+	if (table == NULL)
+		return table_not_found(ctx);
+
+	if (chain->dev_expr) {
+		if (!(chain->flags & CHAIN_F_BASECHAIN))
+			chain->flags |= CHAIN_F_BASECHAIN;
+
+		if (chain->handle.family == NFPROTO_NETDEV ||
+		    (chain->handle.family == NFPROTO_INET &&
+		     chain->hook.num == NF_INET_INGRESS)) {
+			if (chain->dev_expr &&
+			    !evaluate_device_expr(ctx, &chain->dev_expr))
+				return -1;
+		} else if (chain->dev_expr) {
+			return __stmt_binary_error(ctx, &chain->dev_expr->location, NULL,
+						   "This chain type cannot be bound to device");
+		}
+	}
+
+	chain_del_cache(ctx, cmd);
+
+	return 0;
+}
+
 static int chain_evaluate(struct eval_ctx *ctx, struct chain *chain)
 {
 	struct table *table;