Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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?
