Re: [PATCH nft 2/2] src: propagate key datatype for anonymous sets

[Florian Westphal <fw@strlen.de>]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Thu, Dec 09, 2021 at 04:11:31PM +0100, Florian Westphal wrote:
> > set s10 {
> >   typeof tcp option mptcp subtype
> >   elements = { mp-join, dss }
> > }
> > 
> > is listed correctly: typeof provides the 'mptcpopt_subtype'
> > datatype, so listing will print the elements with their sybolic types.
> > 
> > In anon case this doesn't work:
> > tcp option mptcp subtype { mp-join, dss }
> > 
> > gets shown as 'tcp option mptcp subtype { 1,  2}' because the anon
> > set has integer type.
> > 
> > This change propagates the datatype to the individual members
> > of the anon set.
> > 
> > After this change, multiple existing data types such as
> > TYPE_ICMP_TYPE could be replaced by integer-type aliases, but those
> > data types are already exposed to userspace via the 'set type'
> > directive so doing this may break existing set definitions.
> > 
> > Signed-off-by: Florian Westphal <fw@strlen.de>
> > ---
> >  src/expression.c              | 34 ++++++++++++++++++++++++++++++++++
> >  tests/py/any/tcpopt.t         |  2 +-
> >  tests/py/any/tcpopt.t.payload | 10 +++++-----
> >  3 files changed, 40 insertions(+), 6 deletions(-)
> > 
> > diff --git a/src/expression.c b/src/expression.c
> > index f1cca8845376..9de70c6cc1a4 100644
> > --- a/src/expression.c
> > +++ b/src/expression.c
> > @@ -1249,6 +1249,31 @@ static void set_ref_expr_destroy(struct expr *expr)
> >  	set_free(expr->set);
> >  }
> >  
> > +static void set_ref_expr_set_type(const struct expr *expr,
> > +				  const struct datatype *dtype,
> > +				  enum byteorder byteorder)
> > +{
> > +	const struct set *s = expr->set;
> > +
> > +	/* normal sets already have a precise datatype that is given in
> > +	 * the set definition via type foo.
> > +	 *
> > +	 * Anon sets do not have this, and need to rely on type info
> > +	 * generated at rule creation time.
> > +	 *
> > +	 * For most cases, the type info is correct.
> > +	 * In some cases however, the kernel only stores TYPE_INTEGER.
> > +	 *
> > +	 * This happens with expressions that only use an integer alias
> > +	 * type, such as mptcp_suboption.
> > +	 *
> > +	 * In this case nft would print '1', '2', etc. instead of symbolic
> > +	 * names because the base type lacks ->sym_tbl information.
> > +	 */
> > +	if (s->init && set_is_anonymous(s->flags))
> > +		expr_set_type(s->init, dtype, byteorder);
> 
> Will this work with concatenations?

No.  But concatenations won't work for any of these, e.g.

ip version . ip daddr
tcp option mptcp subtype . tcp dport

... and so on because integer type (unspecific length) can't be
used with concat types so far.

So I think the problem is related but not added in this patch.

However, I think I need to withdraw this propsed patch for a different
reason.

If we'd try to expose mptcp option matching for specific sub fields,
the symbol expression path might not be ideal, for example consider:

   mptcp option subtype mp-join address-id gt 4 drop

(illustrative example with made-up syntax).

From parser perspective it might be better to make this

MPTCP OPTION SUBTYPE mptcp_option_fields : { ...


mptcp_option_fields : MP_JOIN mptcp_option_mpjoin_fields ...
		    | MP_CAPABLE mptcp_option_capable_fields ...


rather than

mptcp option subtype STRING STRING
... where parser calls helpers to 'translate' $4 and $5 to
the desired needed offset-length values (plus dependencies
to avoid bogus match on different suboption).

What do you think?