[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <ZnxP_IHSJWg8FhfO@dcaratti.users.ipa.redhat.com>
Date: Wed, 26 Jun 2024 19:29:32 +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,
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@...hat.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
Powered by blists - more mailing lists