[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZnvkIHCsqnDLlVa9@dcaratti.users.ipa.redhat.com>
Date: Wed, 26 Jun 2024 11:49:20 +0200
From: Davide Caratti <dcaratti@...hat.com>
To: Asbjørn Sloth Tønnesen <ast@...erby.net>
Cc: Ilya Maximets <i.maximets@....org>, Jamal Hadi Salim <jhs@...atatu.com>,
Cong Wang <xiyou.wangcong@...il.com>, Jiri Pirko <jiri@...nulli.us>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
David Ahern <dsahern@...nel.org>, Simon Horman <horms@...nel.org>,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH net-next 2/9] net/sched: cls_flower: prepare
fl_{set,dump}_key_flags() for ENC_FLAGS
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
Powered by blists - more mailing lists