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,

thanks for your patience!

On Fri, Jun 21, 2024 at 02:45:28PM +0000, Asbjørn Sloth Tønnesen wrote:
> 
> Could you please post your iproute2 code?

sure, will clean it up and share it today in ML.
 
> > 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:

[...]
 
> It is always preferred to have a well-defined endianness for binary protocols, even
> if it might only be used locally for now.

given the implementation of fl_set_key_flags() in patch 2,

	key = be32_to_cpu(nla_get_be32(tb[fl_key]));
	mask = be32_to_cpu(nla_get_be32(tb[fl_mask]));

when fl_key and fl_mask are TCA_FLOWER_KEY_ENC_FLAGS and TCA_FLOWER_KEY_ENC_FLAGS_MASK,
I assume that we want to turn them to network ordering, like it's already being done for
TCA_FLOWER_KEY_FLAGS and TCA_FLOWER_KEY_FLAGS_MASK.

So, we must htonl() the policy mask in the second hunk in patch 7,something like:

@@ -746,9 +746,9 @@ static const struct nla_policy fl_policy[TCA_FLOWER_MAX + 1] = {
 	[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,
-							  TUNNEL_FLAGS_PRESENT),
+							  htonl(TCA_FLOWER_KEY_ENC_FLAGS_POLICY_MASK)),
 	[TCA_FLOWER_KEY_ENC_FLAGS_MASK]	= NLA_POLICY_MASK(NLA_U32,
-							  TUNNEL_FLAGS_PRESENT),
+							  htonl(TCA_FLOWER_KEY_ENC_FLAGS_POLICY_MASK)),
 };

And for the same reason, the flower code in patch 3 needs to be changed as follows:

@@ -676,8 +680,10 @@ 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]		= { .type = NLA_U32 },
-	[TCA_FLOWER_KEY_FLAGS_MASK]	= { .type = NLA_U32 },
+	[TCA_FLOWER_KEY_FLAGS]		= NLA_POLICY_MASK(NLA_U32,
+							  ntohl(TCA_FLOWER_KEY_FLAGS_POLICY_MASK)),
+	[TCA_FLOWER_KEY_FLAGS_MASK]	= NLA_POLICY_MASK(NLA_U32,
+							  ntohl(TCA_FLOWER_KEY_FLAGS_POLICY_MASK)),
 	[TCA_FLOWER_KEY_ICMPV4_TYPE]	= { .type = NLA_U8 },
 	[TCA_FLOWER_KEY_ICMPV4_TYPE_MASK] = { .type = NLA_U8 },
 	[TCA_FLOWER_KEY_ICMPV4_CODE]	= { .type = NLA_U8 },

Otherwise it will break the following use case (taken from tc_flower.sh kselftest):

# tc qdisc add dev lo clsact
# tc filter add dev lo ingress protocol ip pref 1 handle 101 flower ip_flags frag action continue
RTNETLINK answers: Invalid argument
We have an error talking to the kernel

because TCA_FLOWER_KEY_FLAGS_POLICY_MASK and TCA_FLOWER_KEY_ENC_FLAGS_POLICY_MASK
are in host byte order _ so netlink policy mask validation will fail unless we turn
the mask to network byte order.

(And I see we don't have a tdc selftest  for 'ip_flags', this might be a
good chance to add it :-) )

-- 
davide