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

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

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?