Re: [RFC PATCH net-next 2/9] net/sched: cls_flower: prepare fl_{set,dump}_key_flags() for ENC_FLAGS

[Davide Caratti <dcaratti@redhat.com>]

hello Asbjørn,

some update on this work: I tested your patch after adapting iproute2
bits (e.g. using TCA_FLOWER_KEY_FLAGS_TUNNEL_<CSUM|DONT_FRAGMENT|OAM|CRIT>

from

https://lore.kernel.org/netdev/20240611235355.177667-2-ast@fiberby.net/

Now: functional tests on TCA_FLOWER_KEY_ENC_FLAGS systematically fail. I must
admit that I didn't complete 100% of the analysis, but IMO there is at least an
endianness problem here. See below:

On Tue, Jun 11, 2024 at 11:53:35PM +0000, Asbjørn Sloth Tønnesen wrote:
> Prepare fl_set_key_flags/fl_dump_key_flags() for use with
> TCA_FLOWER_KEY_ENC_FLAGS{,_MASK}.
> 
> This patch adds an encap argument, similar to fl_set_key_ip/
> fl_dump_key_ip(), and determine the flower keys based on the
> encap argument, and use them in the rest of the two functions.
> 
> Since these functions are so far, only called with encap set false,
> then there is no functional change.
> 
> Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net>
> ---
>  net/sched/cls_flower.c | 40 ++++++++++++++++++++++++++++++----------
>  1 file changed, 30 insertions(+), 10 deletions(-)
> 
> diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
> index eef570c577ac7..6a5cecfd95619 100644
> --- a/net/sched/cls_flower.c
> +++ b/net/sched/cls_flower.c
> @@ -1166,19 +1166,28 @@ static void fl_set_key_flag(u32 flower_key, u32 flower_mask,
>  	}
>  }
>  
> -static int fl_set_key_flags(struct nlattr **tb, u32 *flags_key,
> +static int fl_set_key_flags(struct nlattr **tb, bool encap, u32 *flags_key,
>  			    u32 *flags_mask, struct netlink_ext_ack *extack)
>  {
> +	int fl_key, fl_mask;
>  	u32 key, mask;
>  
> +	if (encap) {
> +		fl_key = TCA_FLOWER_KEY_ENC_FLAGS;
> +		fl_mask = TCA_FLOWER_KEY_ENC_FLAGS_MASK;
> +	} else {
> +		fl_key = TCA_FLOWER_KEY_FLAGS;
> +		fl_mask = TCA_FLOWER_KEY_FLAGS_MASK;
> +	}
> +
>  	/* mask is mandatory for flags */
> -	if (!tb[TCA_FLOWER_KEY_FLAGS_MASK]) {
> +	if (NL_REQ_ATTR_CHECK(extack, NULL, tb, fl_mask)) {
>  		NL_SET_ERR_MSG(extack, "Missing flags mask");
>  		return -EINVAL;
>  	}
>  
> -	key = be32_to_cpu(nla_get_be32(tb[TCA_FLOWER_KEY_FLAGS]));
> -	mask = be32_to_cpu(nla_get_be32(tb[TCA_FLOWER_KEY_FLAGS_MASK]));
> +	key = be32_to_cpu(nla_get_be32(tb[fl_key]));
> +	mask = be32_to_cpu(nla_get_be32(tb[fl_mask]));


I think that (at least) the above hunk is wrong - or at least, it is a
functional discontinuity that causes failure in my test. While the
previous bitmask storing tunnel control flags was in host byte ordering,
the information on IP fragmentation are stored in network byte ordering.

So, if we want to use this enum

--- a/include/uapi/linux/pkt_cls.h
+++ b/include/uapi/linux/pkt_cls.h
@@ -677,6 +677,11 @@ enum {
 enum {
 	TCA_FLOWER_KEY_FLAGS_IS_FRAGMENT = (1 << 0),
 	TCA_FLOWER_KEY_FLAGS_FRAG_IS_FIRST = (1 << 1),
+	/* FLOW_DIS_ENCAPSULATION (1 << 2) is not exposed to userspace */
+	TCA_FLOWER_KEY_FLAGS_TUNNEL_CSUM = (1 << 3),
+	TCA_FLOWER_KEY_FLAGS_TUNNEL_DONT_FRAGMENT = (1 << 4),
+	TCA_FLOWER_KEY_FLAGS_TUNNEL_OAM = (1 << 5),
+	TCA_FLOWER_KEY_FLAGS_TUNNEL_CRIT_OPT = (1 << 6),
 };
 
consistently, we should keep using network byte ordering for
TCA_FLOWER_KEY_FLAGS_TUNNEL_* flags (for a reason that I don't understand,
because metadata are not transmitted on wire. But maybe I'm missing something).

Shall I convert iproute2 to flip those bits like it happens for
TCA_FLOWER_KEY_FLAGS ? thanks!

-- 
davide