[ovs-dev] [PATCH] ofproto-dpif: remove checking nd_ext for datapath
Aaron Conole
aconole at redhat.com
Tue Aug 25 15:13:38 UTC 2020
范开喜 <fankaixi.li at bytedance.com> writes:
> We expect ICMPv6 Neighbor Discovery to be the same as ARP procedure. But
> now OVS checks the datapath whether it supports setting "ND Option Type"
> and "ND Reserved" fields. If not, ovs would reject openflow rules setting
> these fields. So controller could not add this type of rules to support
> ICMPv6 Neighbor Discovery Responder.
>
> Signed-off-by: fankaixi.li <fankaixi.li at bytedance.com>
> ---
Your patch is malformatted - please resbumit.
Additionally, the kernel doesn't have support for these attributes and
datastructures, so the current approach of checking per datapath seems
correct. Did I miss something? What happens when you use the kernel
tree ovs module with these actions and attempt to have flows installed.
Do they actually get installed (check with 'ovs-appctl dpctl/show')?
> datapath/linux/compat/include/linux/openvswitch.h | 6 +--
> lib/odp-util.c | 20 ++++----
> lib/odp-util.h | 4 --
> ofproto/ofproto-dpif.c | 57
> -----------------------
> tests/test-odp.c | 3 +-
> vswitchd/vswitch.xml | 5 --
> 6 files changed, 11 insertions(+), 84 deletions(-)
>
> diff --git a/datapath/linux/compat/include/linux/openvswitch.h
> b/datapath/linux/compat/include/linux/openvswitch.h
> index cc41bbea4..2c42e7b97 100644
> --- a/datapath/linux/compat/include/linux/openvswitch.h
> +++ b/datapath/linux/compat/include/linux/openvswitch.h
> @@ -378,11 +378,11 @@ enum ovs_key_attr {
> #endif
>
> #ifndef __KERNEL__
> - /* Only used within userspace data path. */
> OVS_KEY_ATTR_PACKET_TYPE, /* be32 packet type */
> - OVS_KEY_ATTR_ND_EXTENSIONS, /* struct ovs_key_nd_extensions */
> #endif
>
> + OVS_KEY_ATTR_ND_EXTENSIONS, /* struct ovs_key_nd_extensions */
> +
> __OVS_KEY_ATTR_MAX
> };
>
> @@ -518,12 +518,10 @@ struct ovs_key_nd {
> __u8 nd_tll[ETH_ALEN];
> };
>
> -#ifndef __KERNEL__
> struct ovs_key_nd_extensions {
> __be32 nd_reserved;
> __u8 nd_options_type;
> };
> -#endif
>
> #define OVS_CT_LABELS_LEN_32 4
> #define OVS_CT_LABELS_LEN (OVS_CT_LABELS_LEN_32 * sizeof(__u32))
> diff --git a/lib/odp-util.c b/lib/odp-util.c
> index 5989381e9..49fbc145f 100644
> --- a/lib/odp-util.c
> +++ b/lib/odp-util.c
> @@ -6353,18 +6353,14 @@ odp_flow_key_from_flow__(const struct
> odp_flow_key_parms *parms,
>
> /* Add ND Extensions Attr only if supported and reserved
> field
> * or options type is set. */
> - if (parms->support.nd_ext) {
> - struct ovs_key_nd_extensions *nd_ext_key;
> -
> - if (data->igmp_group_ip4 != 0 || data->tcp_flags != 0)
> {
> - /* 'struct ovs_key_nd_extensions' has padding,
> - * clear it. */
> - nd_ext_key = nl_msg_put_unspec_zero(buf,
> - OVS_KEY_ATTR_ND_EXTENSIONS,
> - sizeof *nd_ext_key);
> - nd_ext_key->nd_reserved = data->igmp_group_ip4;
> - nd_ext_key->nd_options_type =
> ntohs(data->tcp_flags);
> - }
> + struct ovs_key_nd_extensions *nd_ext_key;
> +
> + if (data->igmp_group_ip4 != 0 || data->tcp_flags != 0) {
> + nd_ext_key = nl_msg_put_unspec_uninit(buf,
> + OVS_KEY_ATTR_ND_EXTENSIONS,
> + sizeof *nd_ext_key);
> + nd_ext_key->nd_reserved = data->igmp_group_ip4;
> + nd_ext_key->nd_options_type = ntohs(data->tcp_flags);
> }
> }
> }
> diff --git a/lib/odp-util.h b/lib/odp-util.h
> index 623a66aa2..b9700291c 100644
> --- a/lib/odp-util.h
> +++ b/lib/odp-util.h
> @@ -204,10 +204,6 @@ int odp_flow_from_string(const char *s, const struct
> simap *port_names,
> /* Conntrack original direction tuple matching * supported. */
> \
> ODP_SUPPORT_FIELD(bool, ct_orig_tuple, "CT orig tuple")
> \
> ODP_SUPPORT_FIELD(bool, ct_orig_tuple6, "CT orig tuple for IPv6")
> \
> -
> \
> - /* If true, it means that the datapath supports the IPv6 Neigh
> \
> - * Discovery Extension bits. */
> \
> - ODP_SUPPORT_FIELD(bool, nd_ext, "IPv6 ND Extension")
>
> /* Indicates support for various fields. This defines how flows will be
> * serialised. */
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 4f0638f23..b8a3a1003 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -1474,53 +1474,6 @@ check_max_dp_hash_alg(struct dpif_backer *backer)
> return max_alg;
> }
>
> -/* Tests whether 'backer''s datapath supports IPv6 ND extensions.
> - * Only userspace datapath support OVS_KEY_ATTR_ND_EXTENSIONS in keys.
> - *
> - * Returns false if 'backer' definitely does not support matching and
> - * setting reserved and options type, true if it seems to support. */
> -static bool
> -check_nd_extensions(struct dpif_backer *backer)
> -{
> - struct eth_header *eth;
> - struct ofpbuf actions;
> - struct dp_packet packet;
> - struct flow flow;
> - int error;
> - struct ovs_key_nd_extensions key, mask;
> -
> - ofpbuf_init(&actions, 64);
> - memset(&key, 0x53, sizeof key);
> - memset(&mask, 0x7f, sizeof mask);
> - commit_masked_set_action(&actions, OVS_KEY_ATTR_ND_EXTENSIONS, &key,
> &mask,
> - sizeof key);
> -
> - /* Compose a dummy ethernet packet. */
> - dp_packet_init(&packet, ETH_HEADER_LEN);
> - eth = dp_packet_put_zeros(&packet, ETH_HEADER_LEN);
> - eth->eth_type = htons(0x1234);
> -
> - flow_extract(&packet, &flow);
> -
> - /* Execute the actions. On datapaths without support fails with
> EINVAL. */
> - struct dpif_execute execute = {
> - .actions = actions.data,
> - .actions_len = actions.size,
> - .packet = &packet,
> - .flow = &flow,
> - .probe = true,
> - };
> - error = dpif_execute(backer->dpif, &execute);
> -
> - dp_packet_uninit(&packet);
> - ofpbuf_uninit(&actions);
> -
> - VLOG_INFO("%s: Datapath %s IPv6 ND Extensions",
> dpif_name(backer->dpif),
> - error ? "does not support" : "supports");
> -
> - return !error;
> -}
> -
> #define CHECK_FEATURE__(NAME, SUPPORT, FIELD, VALUE, ETHTYPE)
> \
> static bool
> \
> check_##NAME(struct dpif_backer *backer)
> \
> @@ -1599,7 +1552,6 @@ check_support(struct dpif_backer *backer)
> backer->rt_support.odp.ct_state_nat = check_ct_state_nat(backer);
> backer->rt_support.odp.ct_orig_tuple = check_ct_orig_tuple(backer);
> backer->rt_support.odp.ct_orig_tuple6 = check_ct_orig_tuple6(backer);
> - backer->rt_support.odp.nd_ext = check_nd_extensions(backer);
> }
>
> static int
> @@ -4637,14 +4589,6 @@ check_actions(const struct ofproto_dpif *ofproto,
> "ct original direction tuple");
> return OFPERR_NXBAC_CT_DATAPATH_SUPPORT;
> }
> - } else if (!support->nd_ext && ofpact->type == OFPACT_SET_FIELD) {
> - const struct mf_field *dst = ofpact_get_mf_dst(ofpact);
> -
> - if (dst->id == MFF_ND_RESERVED || dst->id ==
> MFF_ND_OPTIONS_TYPE) {
> - report_unsupported_act("set field",
> - "setting IPv6 ND Extensions
> fields");
> - return OFPERR_OFPBAC_BAD_SET_ARGUMENT;
> - }
> }
> }
>
> @@ -5585,7 +5529,6 @@ get_datapath_cap(const char *datapath_type, struct
> smap *cap)
> smap_add(cap, "ct_state_nat", odp.ct_state_nat ? "true" : "false");
> smap_add(cap, "ct_orig_tuple", odp.ct_orig_tuple ? "true" : "false");
> smap_add(cap, "ct_orig_tuple6", odp.ct_orig_tuple6 ? "true" : "false");
> - smap_add(cap, "nd_ext", odp.nd_ext ? "true" : "false");
>
> /* DPIF_SUPPORT_FIELDS */
> smap_add(cap, "masked_set_action", s.masked_set_action ? "true" :
> "false");
> diff --git a/tests/test-odp.c b/tests/test-odp.c
> index 0ddfd4070..644c51a89 100644
> --- a/tests/test-odp.c
> +++ b/tests/test-odp.c
> @@ -64,8 +64,7 @@ parse_keys(bool wc_keys)
> .ct_zone = true,
> .ct_mark = true,
> .ct_label = true,
> - .max_vlan_headers = SIZE_MAX,
> - .nd_ext = true,
> + .max_vlan_headers = SIZE_MAX
> },
> };
>
> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> index 81c84927f..37cb7a685 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -5955,11 +5955,6 @@ ovs-vsctl add-port br0 p0 -- set Interface p0
> type=patch options:peer=p1 \
> packet to be sent to the Open vSwitch slow path, which is likely to
> make it too slow for mirroring traffic in bulk.
> </column>
> - <column name="capabilities" key="nd_ext" type='{"type": "boolean"}'>
> - True if the datapath supports OVS_KEY_ATTR_ND_EXTENSIONS to match
> on
> - ICMPv6 "ND reserved" and "ND option type" header fields. If false,
> - the datapath reports error if the feature is used.
> - </column>
> <group title="Clone Actions">
> <p>
> When Open vSwitch translates actions from OpenFlow into the
> datapath
> --
> 2.11.0
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
More information about the dev
mailing list