On Tue, Sep 09, 2025 at 01:48:44AM +0200, Phil Sutter wrote:
> On Mon, Sep 08, 2025 at 01:01:00PM +0200, Pablo Neira Ayuso wrote:
> > 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@xxxxxx>
> > > > ---
> > > > 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()?
>
> I agree, side-effects of reusing chain_evaluate() for deletion are not
> worth it.
>
> > > 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.
>
> Since we only need a proper value in chain->hook.num, a more minimal
> version is fine:
>
> diff --git a/src/evaluate.c b/src/evaluate.c
> index b7e4f71fdfbc9..8cecbe09de01c 100644
> --- a/src/evaluate.c
> +++ b/src/evaluate.c
> @@ -5992,6 +5992,22 @@ static void chain_del_cache(struct eval_ctx *ctx, struct cmd *cmd)
> chain_free(chain);
> }
>
> +static int chain_del_evaluate(struct eval_ctx *ctx, struct cmd *cmd)
> +{
> + struct chain *chain = cmd->chain;
> +
> + if (chain && chain->flags & CHAIN_F_BASECHAIN && chain->hook.name) {
> + chain->hook.num = str2hooknum(chain->handle.family,
> + chain->hook.name);
> + if (chain->hook.num == NF_INET_NUMHOOKS)
> + return __stmt_binary_error(ctx, &chain->hook.loc, NULL,
> + "The %s family does not support this hook",
> + family2str(chain->handle.family));
> + }
> + chain_del_cache(ctx, cmd);
> + return 0;
> +}
> +
> static void set_del_cache(struct eval_ctx *ctx, struct cmd *cmd)
> {
> struct table *table;
> @@ -6069,8 +6085,7 @@ static int cmd_evaluate_delete(struct eval_ctx *ctx, struct cmd *cmd)
> case CMD_OBJ_RULE:
> return 0;
> case CMD_OBJ_CHAIN:
> - chain_del_cache(ctx, cmd);
> - return 0;
> + return chain_del_evaluate(ctx, cmd);
> case CMD_OBJ_TABLE:
> table_del_cache(ctx, cmd);
> return 0;
>
> Fine with you?
Yes, thanks!
