On 11/10/25 7:57 PM, Reuven Plevinsky via dev wrote:
> From: Eli Britstein <[email protected]>
>
> Introduce eth-type field to tunnel metadata that is internally matched
> for tunnels, for robust detection of the IP type.
>
> Signed-off-by: Eli Britstein <[email protected]>
> ---
> include/openvswitch/match.h | 1 +
> include/openvswitch/meta-flow.h | 14 +++++++++++
> include/openvswitch/packets.h | 3 ++-
> lib/dpif.c | 6 +++++
> lib/dpif.h | 1 +
> lib/match.c | 10 ++++++++
> lib/meta-flow.c | 18 +++++++++++++++
> lib/meta-flow.xml | 1 +
> lib/netdev-native-tnl.c | 20 ++++++++++++++++
> lib/netdev-offload-dpdk.c | 41 +++++++++++++++++++++++++++++----
> lib/odp-util.h | 5 +++-
> ofproto/ofproto-dpif-xlate.c | 16 +++++++++++++
> ofproto/ofproto-dpif.c | 3 +++
> ofproto/tunnel.c | 1 +
> tests/bfd.at | 2 +-
> tests/tunnel.at | 40 ++++++++++++++++----------------
> 16 files changed, 154 insertions(+), 28 deletions(-)
Hi, Eli and Reuven.
Thanks for the update! This version is still mising a big part of the
change, which is the actual datapth flow attribute to match on, i.e.
the OVS_TUNNEL_KEY_ATTR_ETHERTYPE. This is necessary even for userpspace
datapath implementation, because netlink-formatted attributes is the only
way revalidators work. Wihtout it, each datapath flow will likely be
constantly removed from the datapath on a very next revalidation cycle.
>
> diff --git a/include/openvswitch/match.h b/include/openvswitch/match.h
> index 2e8812048e86..b962badfcfd3 100644
> --- a/include/openvswitch/match.h
> +++ b/include/openvswitch/match.h
> @@ -127,6 +127,7 @@ void match_set_tun_gtpu_flags_masked(struct match *match,
> uint8_t flags,
> void match_set_tun_gtpu_msgtype(struct match *match, uint8_t msgtype);
> void match_set_tun_gtpu_msgtype_masked(struct match *match, uint8_t msgtype,
> uint8_t mask);
> +void match_set_tun_eth_type(struct match *, ovs_be16);
> void match_set_in_port(struct match *, ofp_port_t ofp_port);
> void match_set_pkt_mark(struct match *, uint32_t pkt_mark);
> void match_set_pkt_mark_masked(struct match *, uint32_t pkt_mark, uint32_t
> mask);
> diff --git a/include/openvswitch/meta-flow.h b/include/openvswitch/meta-flow.h
> index 875f122c5f55..e644c684bbf0 100644
> --- a/include/openvswitch/meta-flow.h
> +++ b/include/openvswitch/meta-flow.h
> @@ -304,6 +304,20 @@ enum OVS_PACKED_ENUM mf_field_id {
> */
> MFF_TUN_ID,
>
> + /* "tun_eth_type".
> + *
> + * Packet's Tunnel Ethernet type.
> + *
> + * Type: be16.
> + * Maskable: no.
> + * Formatting: decimal.
Maybe hexadecimal?
> + * Prerequisites: none.
> + * Access: read-only.
> + * NXM: none.
> + * OXM: none.
> + */
> + MFF_TUN_ETH_TYPE,
> +
> /* "tun_src".
> *
> * The IPv4 source address in the outer IP header of a tunneled packet.
> diff --git a/include/openvswitch/packets.h b/include/openvswitch/packets.h
> index a65cb0d04e77..c9af1b6ceca6 100644
> --- a/include/openvswitch/packets.h
> +++ b/include/openvswitch/packets.h
> @@ -45,7 +45,8 @@ struct flow_tnl {
> uint8_t erspan_hwid;
> uint8_t gtpu_flags;
> uint8_t gtpu_msgtype;
> - uint8_t pad1[4]; /* Pad to 64 bits. */
> + ovs_be16 eth_type;
> + uint8_t pad1[2]; /* Pad to 64 bits. */
> struct tun_metadata metadata;
> };
>
> diff --git a/lib/dpif.c b/lib/dpif.c
> index 070fc0131ffb..dbec240cc9c8 100644
> --- a/lib/dpif.c
> +++ b/lib/dpif.c
> @@ -1945,6 +1945,12 @@ dpif_may_support_psample(const struct dpif *dpif)
> return !dpif_is_netdev(dpif);
> }
>
> +bool
> +dpif_supports_tun_eth_type(const struct dpif *dpif)
> +{
> + return dpif_is_netdev(dpif);
This should be a proper probing function that provides a key with
OVS_TUNNEL_KEY_ATTR_ETHERTYPE to the datapath and checks the result.
> +}
> +
> /* Meters */
> void
> dpif_meter_get_features(const struct dpif *dpif,
> diff --git a/lib/dpif.h b/lib/dpif.h
> index 0b685d8df133..0a410fca5bd2 100644
> --- a/lib/dpif.h
> +++ b/lib/dpif.h
> @@ -921,6 +921,7 @@ int dpif_bond_add(struct dpif *, uint32_t bond_id,
> odp_port_t *member_map);
> int dpif_bond_del(struct dpif *, uint32_t bond_id);
> int dpif_bond_stats_get(struct dpif *, uint32_t bond_id, uint64_t *n_bytes);
> bool dpif_supports_lb_output_action(const struct dpif *);
> +bool dpif_supports_tun_eth_type(const struct dpif *);
>
>
> /* Cache */
> diff --git a/lib/match.c b/lib/match.c
> index 9b7e06e0c7f8..bd8b21d592b1 100644
> --- a/lib/match.c
> +++ b/lib/match.c
> @@ -402,6 +402,13 @@ match_set_tun_gtpu_msgtype(struct match *match, uint8_t
> msgtype)
> match_set_tun_gtpu_msgtype_masked(match, msgtype, UINT8_MAX);
> }
>
> +void
> +match_set_tun_eth_type(struct match *match, ovs_be16 eth_type)
> +{
> + match->wc.masks.tunnel.eth_type = OVS_BE16_MAX;
> + match->flow.tunnel.eth_type = eth_type;
> +}
> +
> void
> match_set_in_port(struct match *match, ofp_port_t ofp_port)
> {
> @@ -1391,6 +1398,9 @@ format_flow_tunnel(struct ds *s, const struct match
> *match)
> if (wc->masks.tunnel.gtpu_msgtype) {
> ds_put_format(s, "gtpu_msgtype=%"PRIu8",", tnl->gtpu_msgtype);
> }
> + if (wc->masks.tunnel.eth_type) {
> + ds_put_format(s, "tun_eth_type=0x%04"PRIx16",",
> ntohs(tnl->eth_type));
> + }
> if (wc->masks.tunnel.flags & FLOW_TNL_F_MASK) {
> format_flags_masked(s, "tun_flags", flow_tun_flag_to_string,
> tnl->flags & FLOW_TNL_F_MASK,
> diff --git a/lib/meta-flow.c b/lib/meta-flow.c
> index 2595fd634b4d..2a6ae129a91a 100644
> --- a/lib/meta-flow.c
> +++ b/lib/meta-flow.c
> @@ -213,6 +213,8 @@ mf_is_all_wild(const struct mf_field *mf, const struct
> flow_wildcards *wc)
> return !wc->masks.packet_type;
> case MFF_CONJ_ID:
> return !wc->masks.conj_id;
> + case MFF_TUN_ETH_TYPE:
> + return !wc->masks.tunnel.eth_type;
> case MFF_TUN_SRC:
> return !wc->masks.tunnel.ip_src;
> case MFF_TUN_DST:
> @@ -524,6 +526,7 @@ mf_is_value_valid(const struct mf_field *mf, const union
> mf_value *value)
> case MFF_PACKET_TYPE:
> case MFF_CONJ_ID:
> case MFF_TUN_ID:
> + case MFF_TUN_ETH_TYPE:
> case MFF_TUN_SRC:
> case MFF_TUN_DST:
> case MFF_TUN_IPV6_SRC:
> @@ -680,6 +683,9 @@ mf_get_value(const struct mf_field *mf, const struct flow
> *flow,
> case MFF_TUN_ID:
> value->be64 = flow->tunnel.tun_id;
> break;
> + case MFF_TUN_ETH_TYPE:
> + value->be16 = flow->tunnel.eth_type;
> + break;
> case MFF_TUN_SRC:
> value->be32 = flow->tunnel.ip_src;
> break;
> @@ -1017,6 +1023,9 @@ mf_set_value(const struct mf_field *mf,
> case MFF_TUN_ID:
> match_set_tun_id(match, value->be64);
> break;
> + case MFF_TUN_ETH_TYPE:
> + match_set_tun_eth_type(match, value->be16);
> + break;
> case MFF_TUN_SRC:
> match_set_tun_src(match, value->be32);
> break;
> @@ -1439,6 +1448,9 @@ mf_set_flow_value(const struct mf_field *mf,
> case MFF_TUN_ID:
> flow->tunnel.tun_id = value->be64;
> break;
> + case MFF_TUN_ETH_TYPE:
> + flow->tunnel.eth_type = value->be16;
> + break;
> case MFF_TUN_SRC:
> flow->tunnel.ip_src = value->be32;
> break;
> @@ -1823,6 +1835,7 @@ mf_is_any_metadata(const struct mf_field *mf)
> return true;
>
> case MFF_TUN_ID:
> + case MFF_TUN_ETH_TYPE:
> case MFF_TUN_SRC:
> case MFF_TUN_DST:
> case MFF_TUN_IPV6_SRC:
> @@ -1917,6 +1930,7 @@ mf_is_pipeline_field(const struct mf_field *mf)
> {
> switch (mf->id) {
> case MFF_TUN_ID:
> + case MFF_TUN_ETH_TYPE:
> case MFF_TUN_SRC:
> case MFF_TUN_DST:
> case MFF_TUN_IPV6_SRC:
> @@ -2075,6 +2089,9 @@ mf_set_wild(const struct mf_field *mf, struct match
> *match, char **err_str)
> case MFF_TUN_ID:
> match_set_tun_id_masked(match, htonll(0), htonll(0));
> break;
> + case MFF_TUN_ETH_TYPE:
> + match->flow.tunnel.eth_type = 0;
> + break;
> case MFF_TUN_SRC:
> match_set_tun_src_masked(match, htonl(0), htonl(0));
> break;
> @@ -2479,6 +2496,7 @@ mf_set(const struct mf_field *mf,
> case MFF_ICMPV6_CODE:
> case MFF_ND_RESERVED:
> case MFF_ND_OPTIONS_TYPE:
> + case MFF_TUN_ETH_TYPE:
> return OFPUTIL_P_NONE;
>
> case MFF_DP_HASH:
> diff --git a/lib/meta-flow.xml b/lib/meta-flow.xml
> index 5c57ab08ff18..558298279b9e 100644
> --- a/lib/meta-flow.xml
> +++ b/lib/meta-flow.xml
> @@ -1508,6 +1508,7 @@ ovs-ofctl add-flow br-int
> 'in_port=3,tun_src=192.168.1.1,tun_id=5001 actions=1'
> </diagram>
> </field>
>
> + <field id="MFF_TUN_ETH_TYPE" title="Tunnel Ethernet type"
> internal="yes"/>
> <field id="MFF_TUN_SRC" title="Tunnel IPv4 Source">
> <p>
> When a packet is received from a tunnel, this field is the
> diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c
> index 008b452b8a6e..92bc1d08a813 100644
> --- a/lib/netdev-native-tnl.c
> +++ b/lib/netdev-native-tnl.c
> @@ -506,6 +506,7 @@ netdev_gre_pop_header(struct dp_packet *packet)
> struct pkt_metadata *md = &packet->md;
> struct flow_tnl *tnl = &md->tunnel;
> int hlen = sizeof(struct eth_header) + 4;
> + const struct eth_header *eth;
>
> ovs_assert(data_dp);
>
> @@ -522,6 +523,11 @@ netdev_gre_pop_header(struct dp_packet *packet)
> goto err;
> }
>
> + eth = dp_packet_eth(packet);
> + if (eth) {
> + tnl->eth_type = eth->eth_type;
> + }
> +
> tnl_ol_pop(packet, hlen);
>
> return packet;
> @@ -1102,6 +1108,7 @@ netdev_vxlan_pop_header(struct dp_packet *packet)
> unsigned int hlen;
> ovs_be32 vx_flags;
> enum packet_type next_pt = PT_ETH;
> + const struct eth_header *eth;
>
> ovs_assert(packet->l3_ofs > 0);
> ovs_assert(packet->l4_ofs > 0);
> @@ -1152,6 +1159,12 @@ netdev_vxlan_pop_header(struct dp_packet *packet)
> tnl->flags |= FLOW_TNL_F_KEY;
>
> packet->packet_type = htonl(next_pt);
> +
> + eth = dp_packet_eth(packet);
> + if (eth) {
> + tnl->eth_type = eth->eth_type;
> + }
> +
> tnl_ol_pop(packet, hlen + VXLAN_HLEN);
> if (next_pt != PT_ETH) {
> packet->l3_ofs = 0;
> @@ -1219,6 +1232,7 @@ netdev_geneve_pop_header(struct dp_packet *packet)
> struct flow_tnl *tnl = &md->tunnel;
> struct genevehdr *gnh;
> unsigned int hlen, opts_len, ulen;
> + const struct eth_header *eth;
>
> pkt_metadata_init_tnl(md);
> if (GENEVE_BASE_HLEN > dp_packet_l4_size(packet)) {
> @@ -1260,6 +1274,12 @@ netdev_geneve_pop_header(struct dp_packet *packet)
> tnl->flags |= FLOW_TNL_F_UDPIF;
>
> packet->packet_type = htonl(PT_ETH);
> +
> + eth = dp_packet_eth(packet);
> + if (eth) {
> + tnl->eth_type = eth->eth_type;
> + }
This should be done in ip_extract_tnl_md().
> +
> tnl_ol_pop(packet, hlen);
>
> return packet;
> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
> index ca1982ccd8fb..b8942294691f 100644
> --- a/lib/netdev-offload-dpdk.c
> +++ b/lib/netdev-offload-dpdk.c
> @@ -1184,11 +1184,32 @@ parse_tnl_ip_match(struct flow_patterns *patterns,
> struct match *match,
> uint8_t proto)
> {
> + ovs_be16 eth_type = match->flow.tunnel.eth_type;
> struct flow *consumed_masks;
> + bool is_ipv4, is_ipv6;
> +
> + /* Determine IP version - either explicitly set via eth_type
> + * or inferred from masks.
> + */
> + if (eth_type == htons(ETH_TYPE_IP)) {
> + is_ipv4 = true;
> + is_ipv6 = false;
> + } else if (eth_type == htons(ETH_TYPE_IPV6)) {
> + is_ipv4 = false;
> + is_ipv6 = true;
> + } else {
> + /* eth_type is 0 - infer from which address masks are set */
> + is_ipv4 = (match->wc.masks.tunnel.ip_src ||
> + match->wc.masks.tunnel.ip_dst);
> + is_ipv6 = (!is_all_zeros(&match->wc.masks.tunnel.ipv6_src,
> + sizeof(struct in6_addr)) ||
> + !is_all_zeros(&match->wc.masks.tunnel.ipv6_dst,
> + sizeof(struct in6_addr)));
> + }
>
> consumed_masks = &match->wc.masks;
> /* IP v4 */
> - if (match->wc.masks.tunnel.ip_src || match->wc.masks.tunnel.ip_dst) {
> + if (is_ipv4) {
> struct rte_flow_item_ipv4 *spec, *mask;
>
> spec = xzalloc(sizeof *spec);
> @@ -1212,10 +1233,7 @@ parse_tnl_ip_match(struct flow_patterns *patterns,
> consumed_masks->tunnel.ip_dst = 0;
>
> add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_IPV4, spec, mask,
> NULL);
> - } else if (!is_all_zeros(&match->wc.masks.tunnel.ipv6_src,
> - sizeof(struct in6_addr)) ||
> - !is_all_zeros(&match->wc.masks.tunnel.ipv6_dst,
> - sizeof(struct in6_addr))) {
> + } else if (is_ipv6) {
> /* IP v6 */
> struct rte_flow_item_ipv6 *spec, *mask;
>
> @@ -1378,6 +1396,19 @@ parse_flow_tnl_match(struct netdev *tnldev,
> return ret;
> }
>
> + if (match->wc.masks.tunnel.eth_type) {
> + struct rte_flow_item_eth *spec, *mask;
> +
> + spec = xzalloc(sizeof *spec);
> + mask = xzalloc(sizeof *mask);
> +
> + spec->type = match->flow.tunnel.eth_type;
> + mask->type = match->wc.masks.tunnel.eth_type;
> + match->wc.masks.tunnel.eth_type = 0;
> +
> + add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_ETH, spec, mask, NULL);
> + }
This should probbaly be in parse_tnl_ip_match().
> +
> if (!strcmp(netdev_get_type(tnldev), "vxlan")) {
> ret = parse_vxlan_match(patterns, match);
> }
> diff --git a/lib/odp-util.h b/lib/odp-util.h
> index 339ffc14f248..66b5339ed2ba 100644
> --- a/lib/odp-util.h
> +++ b/lib/odp-util.h
> @@ -207,7 +207,10 @@ int odp_flow_from_string(const char *s, const struct
> simap *port_names,
>
> \
> /* If true, it means that the datapath supports the IPv6 Neigh
> \
> * Discovery Extension bits. */
> \
> - ODP_SUPPORT_FIELD(bool, nd_ext, "IPv6 ND Extension")
> + ODP_SUPPORT_FIELD(bool, nd_ext, "IPv6 ND Extension")
> \
> +
> \
> + /* Tunnel ethernet type matching supported. */
> \
> + ODP_SUPPORT_FIELD(bool, tun_eth_type, "Tunnel Ethernet Type")
>
> /* Indicates support for various fields. This defines how flows will be
> * serialised. */
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 2c8197fb7307..7350d141c906 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -8081,6 +8081,8 @@ xlate_wc_init(struct xlate_ctx *ctx)
> static void
> xlate_wc_finish(struct xlate_ctx *ctx)
> {
> + struct ofproto_dpif *ofproto = ctx->xbridge->ofproto;
> + struct dpif_backer *backer = ofproto->backer;
> int i;
>
> /* Clear the metadata and register wildcard masks, because we won't
> @@ -8152,6 +8154,20 @@ xlate_wc_finish(struct xlate_ctx *ctx)
> if (!flow_tnl_dst_is_set(&ctx->xin->upcall_flow->tunnel)) {
> memset(&ctx->wc->masks.tunnel, 0, sizeof ctx->wc->masks.tunnel);
> }
> + /* Clear tunnel eth type if not supported. */
> + if (!backer->rt_support.odp.tun_eth_type) {
> + ctx->wc->masks.tunnel.eth_type = 0;
> + }
> + /* Clear IPv6 artifacts when eth_type explicitly indicates IPv4 */
> + if (ctx->wc->masks.tunnel.eth_type == htons(ETH_TYPE_IP)) {
We shouldn't compare the mask here. It should be the value in the
upcall_flow instead. And we should not clear anything if eth_type
is not exact matched.
> + ctx->wc->masks.tunnel.ipv6_src = in6addr_any;
> + ctx->wc->masks.tunnel.ipv6_dst = in6addr_any;
> + }
> + /* Clear IPv4 artifacts when eth_type explicitly indicates IPv6 */
> + if (ctx->wc->masks.tunnel.eth_type == htons(ETH_TYPE_IPV6)) {
Same here.
> + ctx->wc->masks.tunnel.ip_src = 0;
> + ctx->wc->masks.tunnel.ip_dst = 0;
> + }
> }
>
> /* This will tweak the odp actions generated. For now, it will:
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 4a200dd08828..c1b3474a1b56 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -1759,6 +1759,8 @@ check_support(struct dpif_backer *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);
> + backer->rt_support.odp.tun_eth_type =
> + dpif_supports_tun_eth_type(backer->dpif);
> }
>
> /* TC does not support offloading the explicit drop action. As such we need
> to
> @@ -5924,6 +5926,7 @@ get_datapath_cap(const char *datapath_type, struct smap
> *cap)
> smap_add(cap, "ct_orig_tuple", s->odp.ct_orig_tuple ? "true" : "false");
> smap_add(cap, "ct_orig_tuple6", s->odp.ct_orig_tuple6 ? "true" :
> "false");
> smap_add(cap, "nd_ext", s->odp.nd_ext ? "true" : "false");
> + smap_add(cap, "tun_eth_type", s->odp.tun_eth_type ? "true" : "false");
>
> /* DPIF_SUPPORT_FIELDS */
> smap_add(cap, "masked_set_action",
> diff --git a/ofproto/tunnel.c b/ofproto/tunnel.c
> index f067a6c26c19..4adc87b91baa 100644
> --- a/ofproto/tunnel.c
> +++ b/ofproto/tunnel.c
> @@ -381,6 +381,7 @@ tnl_wc_init(struct flow *flow, struct flow_wildcards *wc)
> * wildcarded, not to unwildcard them here. */
> wc->masks.tunnel.tp_src = 0;
> wc->masks.tunnel.tp_dst = 0;
> + wc->masks.tunnel.eth_type = OVS_BE16_MAX;
We should not match on this if it is not needed in the final flow. It's
better if we unwildcard it later when we know that we actually need to
match on this field, as it may not be supported. And if we have to match,
but it is not supported, then we'd need to do a SLOW_MATCH.
>
> if (is_ip_any(flow)
> && IP_ECN_is_ce(flow->tunnel.ip_tos)) {
> diff --git a/tests/bfd.at b/tests/bfd.at
> index f5c6409f6c65..aea0a3392325 100644
> --- a/tests/bfd.at
> +++ b/tests/bfd.at
> @@ -1127,7 +1127,7 @@ bridge("br0")
> -> no learned MAC for destination, flooding
>
> Final flow: unchanged
> -Megaflow:
> recirc_id=0,eth,udp,tun_id=0,tun_src=2.2.2.2,tun_dst=2.2.2.1,tun_tos=0,tun_flags=-df-csum+key,in_port=1,dl_src=00:11:22:33:44:66,dl_dst=00:23:20:00:00:77,nw_frag=no,tp_dst=3784
> +Megaflow:
> recirc_id=0,eth,udp,tun_id=0,tun_src=2.2.2.2,tun_dst=2.2.2.1,tun_tos=0,tun_eth_type=0x0000,tun_flags=-df-csum+key,in_port=1,dl_src=00:11:22:33:44:66,dl_dst=00:23:20:00:00:77,nw_frag=no,tp_dst=3784
Not sure why we're matching on tun_eth_type being zero here. It's clearly
incorrect, as the real type is IPv4. Hence this flow will never properly
match on the actual traffic. We need to either provide the value to the
call or infer it from the other data we have.
Same for all the tests below.
Also, would be good to see a few tests specific to the new functionality
introduced in this patch.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
