Re: [PATCH nft 3/3] netlink_linearize: skip set element expression in map statement key

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

On Tue, Sep 26, 2023 at 05:49:10PM +0200, Phil Sutter wrote:
> On Tue, Sep 26, 2023 at 05:25:00PM +0200, Pablo Neira Ayuso wrote:
> > This fix is similar to 22d201010919 ("netlink_linearize: skip set element
> > expression in set statement key") to fix map statement.
> > 
> > netlink_gen_map_stmt() relies on the map key, that is expressed as a set
> > element. Use the set element key instead to skip the set element wrap,
> > otherwise get_register() abort execution:
> > 
> >   nft: netlink_linearize.c:650: netlink_gen_expr: Assertion `dreg < ctx->reg_low' failed.
> > 
> > Reported-by: Luci Stanescu <luci@cnix.ro>
> > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> 
> This patch adds map stmt printing and parsing support to JSON, but not a
> word about that in the commit message. Did this slip in by accident
> (e.g.  'git commit -a')? Anyway, I think it should go into a separate
> patch.

Not accidental.

I just added complete JSON, it has been time consuming, I initially
thought about fixing this issue only and leave JSON alone unfixed, but
looking at recent stuff, that will backfire sooner or later.

I took the existing set statement support and extend it to have a
"data" field to store the data mapping.

> [...]
> > diff --git a/src/netlink_linearize.c b/src/netlink_linearize.c
> > index 53a318aa2e62..99ed9f387a81 100644
> > --- a/src/netlink_linearize.c
> > +++ b/src/netlink_linearize.c
> > @@ -1585,13 +1585,13 @@ static void netlink_gen_map_stmt(struct netlink_linearize_ctx *ctx,
> >  	int num_stmts = 0;
> >  	struct stmt *this;
> >  
> > -	sreg_key = get_register(ctx, stmt->map.key);
> > -	netlink_gen_expr(ctx, stmt->map.key, sreg_key);
> > +	sreg_key = get_register(ctx, stmt->set.key->key);
> > +	netlink_gen_expr(ctx, stmt->set.key->key, sreg_key);
> >  
> >  	sreg_data = get_register(ctx, stmt->map.data);
> >  	netlink_gen_expr(ctx, stmt->map.data, sreg_data);
> >  
> > -	release_register(ctx, stmt->map.key);
> > +	release_register(ctx, stmt->set.key->key);
> >  	release_register(ctx, stmt->map.data);
> >  
> >  	nle = alloc_nft_expr("dynset");
> 
> Any particular reason why this doesn't just use stmt->map.key->key? The

It should use map.key->key, yes.

> first two fields in structs set_stmt and map_stmt are identical, so the
> above works "by accident".

Exactly.

I will fix and send a v2.