[ovs-dev] [PATCH] netdev-offload-tc: Revert tunnel src/dst port masks handling

Roi Dayan roid at mellanox.com
Wed Jun 17 07:20:22 UTC 2020



On 2020-06-17 8:48 AM, Tonghao Zhang wrote:
> On Tue, Jun 16, 2020 at 9:05 PM Roi Dayan <roid at mellanox.com> wrote:
>>
>> The cited commit intended to add tc support for masking tunnel src/dst
>> ips and ports. It's not possible to do tunnel ports masking with
>> openflow rules and the default mask for tunnel ports set to 0 in
>> tnl_wc_init(), unlike tunnel ports default mask which is full mask.
>> So instead of never passing tunnel ports to tc, revert the changes
>> to tunnel ports to always pass the tunnel port.
>> In sw classification is done by the kernel, but for hw we must match
>> the tunnel dst port.
> Hi Roi
> I use the "ovs-appctl dpctl/add-flow" to install rules with tunnel
> src/dst port masked to tc dp.
> And I didn't find that feature affects openflow layer. "make check"
> all test suite were passed.
> If hw we must match tunnel dst port, can we change the tnl_wc_init. I
> test it ok.

dpctl used to debug and not strict like openflows. you can do a lot of stuff
in it to break ovs normal operation, e.g. not aging rules, not adding new rules.
But with openflow rules, you cannot actually control masking of tunnel ports.
"make check" is more of unit testing. its spawning new ovs process for a
single test/check so I don't think it's valid check for this case.

also, I'm not sure about changing tnl_wc_init(), because of the specific comment
about not to change it. also src port should stay 0.
otherwise you will get a lot of rules because the src port is random port opened
for a connection. 

from the original commit "8b7ea2d Extend OVS IPFIX exporter to export tunnel headers"
I see there is another comment in other place in the code expecting default 0 for ports.
* The protocol identifier of DPIF_IPFIX_TUNNEL_IPSEC_GRE is IPPROTO_GRE,
* and both tp_src and tp_dst are zero.

> 
> diff --git a/ofproto/tunnel.c b/ofproto/tunnel.c
> index 03f0ab76562a..5145a6d54e80 100644
> --- a/ofproto/tunnel.c
> +++ b/ofproto/tunnel.c
> @@ -381,8 +381,8 @@ tnl_wc_init(struct flow *flow, struct flow_wildcards *wc)
>          wc->masks.tunnel.ip_ttl = 0;
>          /* The tp_src and tp_dst members in flow_tnl are set to be always
>           * wildcarded, not to unwildcard them here. */
> -        wc->masks.tunnel.tp_src = 0;
> -        wc->masks.tunnel.tp_dst = 0;
> +        wc->masks.tunnel.tp_src = OVS_BE16_MAX;
> +        wc->masks.tunnel.tp_dst = OVS_BE16_MAX;
> 
>          if (is_ip_any(flow)
>              && IP_ECN_is_ce(flow->tunnel.ip_tos)) {
> 
>> Fixes: 5f568d049130 ("netdev-offload-tc: Allow to match the IP and port mask of tunnel")
>> Signed-off-by: Roi Dayan <roid at mellanox.com>
>> Reviewed-by: Eli Britstein <elibr at mellanox.com>
>> ---
>>  NEWS                        |  2 --
>>  include/openvswitch/match.h |  3 ---
>>  lib/match.c                 | 13 -------------
>>  lib/netdev-offload-tc.c     | 13 ++-----------
>>  lib/tc.c                    | 28 ++--------------------------
>>  tests/tunnel.at             |  4 ++--
>>  6 files changed, 6 insertions(+), 57 deletions(-)
>>
>> diff --git a/NEWS b/NEWS
>> index 88b273a0af89..22cacda20ac7 100644
>> --- a/NEWS
>> +++ b/NEWS
>> @@ -19,8 +19,6 @@ Post-v2.13.0
>>     - Tunnels: TC Flower offload
>>       * Tunnel Local endpoint address masked match are supported.
>>       * Tunnel Romte endpoint address masked match are supported.
>> -     * Tunnel Local endpoint ports masked match are supported.
>> -     * Tunnel Romte endpoint ports masked match are supported.
>>
>>
>>  v2.13.0 - 14 Feb 2020
>> diff --git a/include/openvswitch/match.h b/include/openvswitch/match.h
>> index 9e480318e2b3..2e8812048e86 100644
>> --- a/include/openvswitch/match.h
>> +++ b/include/openvswitch/match.h
>> @@ -105,9 +105,6 @@ void match_set_tun_flags(struct match *match, uint16_t flags);
>>  void match_set_tun_flags_masked(struct match *match, uint16_t flags, uint16_t mask);
>>  void match_set_tun_tp_dst(struct match *match, ovs_be16 tp_dst);
>>  void match_set_tun_tp_dst_masked(struct match *match, ovs_be16 port, ovs_be16 mask);
>> -void match_set_tun_tp_src(struct match *match, ovs_be16 tp_src);
>> -void match_set_tun_tp_src_masked(struct match *match,
>> -                                 ovs_be16 port, ovs_be16 mask);
>>  void match_set_tun_gbp_id_masked(struct match *match, ovs_be16 gbp_id, ovs_be16 mask);
>>  void match_set_tun_gbp_id(struct match *match, ovs_be16 gbp_id);
>>  void match_set_tun_gbp_flags_masked(struct match *match, uint8_t flags, uint8_t mask);
>> diff --git a/lib/match.c b/lib/match.c
>> index a77554851146..ba716579d8ec 100644
>> --- a/lib/match.c
>> +++ b/lib/match.c
>> @@ -294,19 +294,6 @@ match_set_tun_tp_dst_masked(struct match *match, ovs_be16 port, ovs_be16 mask)
>>  }
>>
>>  void
>> -match_set_tun_tp_src(struct match *match, ovs_be16 tp_src)
>> -{
>> -    match_set_tun_tp_src_masked(match, tp_src, OVS_BE16_MAX);
>> -}
>> -
>> -void
>> -match_set_tun_tp_src_masked(struct match *match, ovs_be16 port, ovs_be16 mask)
>> -{
>> -    match->wc.masks.tunnel.tp_src = mask;
>> -    match->flow.tunnel.tp_src = port & mask;
>> -}
>> -
>> -void
>>  match_set_tun_gbp_id_masked(struct match *match, ovs_be16 gbp_id, ovs_be16 mask)
>>  {
>>      match->wc.masks.tunnel.gbp_id = mask;
>> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
>> index aa6d22e74f29..258d31f54b08 100644
>> --- a/lib/netdev-offload-tc.c
>> +++ b/lib/netdev-offload-tc.c
>> @@ -712,15 +712,8 @@ parse_tc_flower_to_match(struct tc_flower *flower,
>>              match_set_tun_ttl_masked(match, flower->key.tunnel.ttl,
>>                                       flower->mask.tunnel.ttl);
>>          }
>> -        if (flower->mask.tunnel.tp_dst) {
>> -            match_set_tun_tp_dst_masked(match,
>> -                                        flower->key.tunnel.tp_dst,
>> -                                        flower->mask.tunnel.tp_dst);
>> -        }
>> -        if (flower->mask.tunnel.tp_src) {
>> -            match_set_tun_tp_src_masked(match,
>> -                                        flower->key.tunnel.tp_src,
>> -                                        flower->mask.tunnel.tp_src);
>> +        if (flower->key.tunnel.tp_dst) {
>> +            match_set_tun_tp_dst(match, flower->key.tunnel.tp_dst);
>>          }
>>          if (flower->key.tunnel.metadata.present.len) {
>>              flower_tun_opt_to_match(match, flower);
>> @@ -1470,8 +1463,6 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match,
>>          flower.mask.tunnel.ipv6.ipv6_dst = tnl_mask->ipv6_dst;
>>          flower.mask.tunnel.tos = tnl_mask->ip_tos;
>>          flower.mask.tunnel.ttl = tnl_mask->ip_ttl;
>> -        flower.mask.tunnel.tp_src = tnl_mask->tp_src;
>> -        flower.mask.tunnel.tp_dst = tnl_mask->tp_dst;
>>          flower.mask.tunnel.id = (tnl->flags & FLOW_TNL_F_KEY) ? tnl_mask->tun_id : 0;
>>          flower_match_to_tun_opt(&flower, tnl, tnl_mask);
>>          flower.tunnel = true;
>> diff --git a/lib/tc.c b/lib/tc.c
>> index 29b4328d8bfc..c2ab77553306 100644
>> --- a/lib/tc.c
>> +++ b/lib/tc.c
>> @@ -395,12 +395,6 @@ static const struct nl_policy tca_flower_policy[] = {
>>                                             .optional = true, },
>>      [TCA_FLOWER_KEY_ENC_UDP_DST_PORT] = { .type = NL_A_U16,
>>                                            .optional = true, },
>> -    [TCA_FLOWER_KEY_ENC_UDP_SRC_PORT] = { .type = NL_A_U16,
>> -                                          .optional = true, },
>> -    [TCA_FLOWER_KEY_ENC_UDP_DST_PORT_MASK] = { .type = NL_A_U16,
>> -                                               .optional = true, },
>> -    [TCA_FLOWER_KEY_ENC_UDP_SRC_PORT_MASK] = { .type = NL_A_U16,
>> -                                               .optional = true, },
>>      [TCA_FLOWER_KEY_FLAGS] = { .type = NL_A_BE32, .optional = true, },
>>      [TCA_FLOWER_KEY_FLAGS_MASK] = { .type = NL_A_BE32, .optional = true, },
>>      [TCA_FLOWER_KEY_IP_TTL] = { .type = NL_A_U8,
>> @@ -746,15 +740,7 @@ nl_parse_flower_tunnel(struct nlattr **attrs, struct tc_flower *flower)
>>          flower->key.tunnel.ipv6.ipv6_dst =
>>              nl_attr_get_in6_addr(attrs[TCA_FLOWER_KEY_ENC_IPV6_DST]);
>>      }
>> -    if (attrs[TCA_FLOWER_KEY_ENC_UDP_SRC_PORT_MASK]) {
>> -        flower->mask.tunnel.tp_src =
>> -            nl_attr_get_be16(attrs[TCA_FLOWER_KEY_ENC_UDP_SRC_PORT_MASK]);
>> -        flower->key.tunnel.tp_src =
>> -            nl_attr_get_be16(attrs[TCA_FLOWER_KEY_ENC_UDP_SRC_PORT]);
>> -    }
>> -    if (attrs[TCA_FLOWER_KEY_ENC_UDP_DST_PORT_MASK]) {
>> -        flower->mask.tunnel.tp_dst =
>> -            nl_attr_get_be16(attrs[TCA_FLOWER_KEY_ENC_UDP_DST_PORT_MASK]);
>> +    if (attrs[TCA_FLOWER_KEY_ENC_UDP_DST_PORT]) {
>>          flower->key.tunnel.tp_dst =
>>              nl_attr_get_be16(attrs[TCA_FLOWER_KEY_ENC_UDP_DST_PORT]);
>>      }
>> @@ -2713,10 +2699,7 @@ nl_msg_put_flower_tunnel(struct ofpbuf *request, struct tc_flower *flower)
>>      struct in6_addr *ipv6_dst_mask = &flower->mask.tunnel.ipv6.ipv6_dst;
>>      struct in6_addr *ipv6_src = &flower->key.tunnel.ipv6.ipv6_src;
>>      struct in6_addr *ipv6_dst = &flower->key.tunnel.ipv6.ipv6_dst;
>> -    ovs_be16 tp_dst_mask = flower->mask.tunnel.tp_dst;
>> -    ovs_be16 tp_src_mask = flower->mask.tunnel.tp_src;
>>      ovs_be16 tp_dst = flower->key.tunnel.tp_dst;
>> -    ovs_be16 tp_src = flower->key.tunnel.tp_src;
>>      ovs_be32 id = be64_to_be32(flower->key.tunnel.id);
>>      uint8_t tos = flower->key.tunnel.tos;
>>      uint8_t ttl = flower->key.tunnel.ttl;
>> @@ -2748,16 +2731,9 @@ nl_msg_put_flower_tunnel(struct ofpbuf *request, struct tc_flower *flower)
>>          nl_msg_put_u8(request, TCA_FLOWER_KEY_ENC_IP_TTL, ttl);
>>          nl_msg_put_u8(request, TCA_FLOWER_KEY_ENC_IP_TTL_MASK, ttl_mask);
>>      }
>> -    if (tp_dst_mask) {
>> -        nl_msg_put_be16(request, TCA_FLOWER_KEY_ENC_UDP_DST_PORT_MASK,
>> -                        tp_dst_mask);
>> +    if (tp_dst) {
>>          nl_msg_put_be16(request, TCA_FLOWER_KEY_ENC_UDP_DST_PORT, tp_dst);
>>      }
>> -    if (tp_src_mask) {
>> -        nl_msg_put_be16(request, TCA_FLOWER_KEY_ENC_UDP_SRC_PORT_MASK,
>> -                        tp_src_mask);
>> -        nl_msg_put_be16(request, TCA_FLOWER_KEY_ENC_UDP_SRC_PORT, tp_src);
>> -    }
>>      if (id_mask) {
>>          nl_msg_put_be32(request, TCA_FLOWER_KEY_ENC_KEY_ID, id);
>>      }
>> diff --git a/tests/tunnel.at b/tests/tunnel.at
>> index a74a67aa8123..e08fd1e048f8 100644
>> --- a/tests/tunnel.at
>> +++ b/tests/tunnel.at
>> @@ -123,10 +123,10 @@ AT_CHECK([ovs-appctl dpif/show | tail -n +3], [0], [dnl
>>      p2 2/2: (dummy)
>>  ])
>>
>> -AT_CHECK([ovs-appctl dpctl/add-flow "tunnel(dst=1.1.1.1,src=3.3.3.200/255.255.255.0,tp_dst=123,tp_src=1/0xf,ttl=64),recirc_id(0),in_port(1),eth(),eth_type(0x0800),ipv4()" "2"])
>> +AT_CHECK([ovs-appctl dpctl/add-flow "tunnel(dst=1.1.1.1,src=3.3.3.200/255.255.255.0,tp_dst=123,tp_src=1,ttl=64),recirc_id(0),in_port(1),eth(),eth_type(0x0800),ipv4()" "2"])
>>
>>  AT_CHECK([ovs-appctl dpctl/dump-flows | tail -1], [0], [dnl
>> -tunnel(src=3.3.3.200/255.255.255.0,dst=1.1.1.1,ttl=64,tp_src=1/0xf,tp_dst=123),recirc_id(0),in_port(1),eth_type(0x0800), packets:0, bytes:0, used:never, actions:2
>> +tunnel(src=3.3.3.200/255.255.255.0,dst=1.1.1.1,ttl=64,tp_src=1,tp_dst=123),recirc_id(0),in_port(1),eth_type(0x0800), packets:0, bytes:0, used:never, actions:2
>>  ])
>>
>>  OVS_VSWITCHD_STOP
>> --
>> 2.8.4
>>
> 
> 


More information about the dev mailing list