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,

On Wed, Jun 26, 2024 at 11:55:31AM +0000, Asbjørn Sloth Tønnesen wrote:
> Hi Davide,
> 
> On 6/26/24 10:01 AM, Davide Caratti wrote:
> > On Wed, Jun 26, 2024 at 11:49 AM Davide Caratti <dcaratti@redhat.com> wrote:
> > > 
> > > So, we must htonl() the policy mask in the second hunk in patch 7,something like:
> 
> Good catch.
> 
> > or maybe better (but still untested), use NLA_BE32, like netfilter does in [1]
> > 
> > [1] https://elixir.bootlin.com/linux/latest/A/ident/NF_NAT_RANGE_MASK
> 
> Yes, that is better. It should work, as it triggers a htonl() in nla_validate_mask().

NLA_BE32 proved to fix the byte ordering problem:

 - it allows to set TCA_FLOWER_KEY_ENC_FLAGS_MASK and read it back consistently
 - it sets correct FLOW_DIS_F_* bits in 'enc_control'

FTR, I used this hunk on top of your RFC series:

-- >8 --
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -679,9 +679,9 @@ static const struct nla_policy fl_policy[TCA_FLOWER_MAX + 1] = {
        [TCA_FLOWER_KEY_ENC_UDP_SRC_PORT_MASK]  = { .type = NLA_U16 },
        [TCA_FLOWER_KEY_ENC_UDP_DST_PORT]       = { .type = NLA_U16 },
        [TCA_FLOWER_KEY_ENC_UDP_DST_PORT_MASK]  = { .type = NLA_U16 },
-       [TCA_FLOWER_KEY_FLAGS]          = NLA_POLICY_MASK(NLA_U32,
+       [TCA_FLOWER_KEY_FLAGS]          = NLA_POLICY_MASK(NLA_BE32,
                                                          TCA_FLOWER_KEY_FLAGS_POLICY_MASK),
-       [TCA_FLOWER_KEY_FLAGS_MASK]     = NLA_POLICY_MASK(NLA_U32,
+       [TCA_FLOWER_KEY_FLAGS_MASK]     = NLA_POLICY_MASK(NLA_BE32,
                                                          TCA_FLOWER_KEY_FLAGS_POLICY_MASK),
        [TCA_FLOWER_KEY_ICMPV4_TYPE]    = { .type = NLA_U8 },
        [TCA_FLOWER_KEY_ICMPV4_TYPE_MASK] = { .type = NLA_U8 },
@@ -744,9 +744,9 @@ static const struct nla_policy fl_policy[TCA_FLOWER_MAX + 1] = {
        [TCA_FLOWER_KEY_SPI_MASK]       = { .type = NLA_U32 },
        [TCA_FLOWER_L2_MISS]            = NLA_POLICY_MAX(NLA_U8, 1),
        [TCA_FLOWER_KEY_CFM]            = { .type = NLA_NESTED },
-       [TCA_FLOWER_KEY_ENC_FLAGS]      = NLA_POLICY_MASK(NLA_U32,
+       [TCA_FLOWER_KEY_ENC_FLAGS]      = NLA_POLICY_MASK(NLA_BE32,
                                                          TCA_FLOWER_KEY_ENC_FLAGS_POLICY_MASK),
-       [TCA_FLOWER_KEY_ENC_FLAGS_MASK] = NLA_POLICY_MASK(NLA_U32,
+       [TCA_FLOWER_KEY_ENC_FLAGS_MASK] = NLA_POLICY_MASK(NLA_BE32,
                                                          TCA_FLOWER_KEY_ENC_FLAGS_POLICY_MASK),
 };

-- >8 --

but I think I found another small problem. You removed FLOW_DISSECTOR_KEY_ENC_FLAGS
from TC flower, re-using 'enc_control' instead; however, the FLOW_DISSECTOR_KEY_ENC_CONTROL
bit is set only if flower tries to match 'enc_ipv4' or 'enc_ipv6'. We don't notice
the problem with 'ip_flags' because AFAIS flow dissector copies those bits even with
no relevant FLOW_DISSECTOR_KEY* requested. When matching tunnel flags instead, we
will end up in skb_flow_dissect_tunne_info() with 


	/* A quick check to see if there might be something to do. */
	if (!dissector_uses_key(flow_dissector,
				FLOW_DISSECTOR_KEY_ENC_KEYID) &&
	    !dissector_uses_key(flow_dissector,
				FLOW_DISSECTOR_KEY_ENC_IPV4_ADDRS) &&
	    !dissector_uses_key(flow_dissector,
				FLOW_DISSECTOR_KEY_ENC_IPV6_ADDRS) &&
	    !dissector_uses_key(flow_dissector,
				FLOW_DISSECTOR_KEY_ENC_CONTROL) &&
	    !dissector_uses_key(flow_dissector,
				FLOW_DISSECTOR_KEY_ENC_PORTS) &&
	    !dissector_uses_key(flow_dissector,
				FLOW_DISSECTOR_KEY_ENC_IP) &&
	    !dissector_uses_key(flow_dissector,
				FLOW_DISSECTOR_KEY_ENC_OPTS))
		return;

 
^^ a kernel that returns without loading tunnel info, because "there is nothing
to do". So, the attempt to put a valid value in patch9 regardless of the address
family is not sufficient. IMO it can be fixed with the following hunk:

-- >8 --
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -2199,7 +2199,8 @@ static void fl_init_dissector(struct flow_dissector *dissector,
        FL_KEY_SET_IF_MASKED(mask, keys, cnt,
                             FLOW_DISSECTOR_KEY_ENC_IPV6_ADDRS, enc_ipv6);
        if (FL_KEY_IS_MASKED(mask, enc_ipv4) ||
-           FL_KEY_IS_MASKED(mask, enc_ipv6))
+           FL_KEY_IS_MASKED(mask, enc_ipv6) ||
+           FL_KEY_IS_MASKED(mask, enc_control))
                FL_KEY_SET(keys, cnt, FLOW_DISSECTOR_KEY_ENC_CONTROL,
                           enc_control);
        FL_KEY_SET_IF_MASKED(mask, keys, cnt,
-- >8 --

at least it passes my functional test (that I didn't send yet, together with
iproute bits :(  promise will do that now)

-- 
davide