Message ID | AM2PR07MB1042A433389ABD5D8D77CF358A1E0@AM2PR07MB1042.eurprd07.prod.outlook.com |
---|---|
State | Changes Requested |
Headers | show |
On Tue, Apr 25, 2017 at 04:30:23PM +0000, Zoltán Balogh wrote: > From: Jan Scheurich <jan.scheurich@ericsson.com> > > Add support for actions push_eth and pop_eth to the netdev datapath and > the supporting libraries. This patch relies on the support for these actions > in the kernel datapath to be present. > > Signed-off-by: Lorand Jakab <lojakab@cisco.com> > Signed-off-by: Simon Horman <simon.horman@netronome.com> > Signed-off-by: Jiri Benc <jbenc@redhat.com> > Signed-off-by: Yi Yang <yi.y.yang@intel.com> > Signed-off-by: Jean Tourrilhes <jt@labs.hpe.com> > Signed-off-by: Jan Scheurich <jan.scheurich@ericsson.com> > Co-authored-by: Zoltan Balogh <zoltan.balogh@ericsson.com> Hi, can you help me to understand this part here? I believe that this will lead to userspace sending a OVS_ACTION_ATTR_PUSH_ETH action argument to the kernel that is different from what the kernel actually wants. What's the idea, and how is that problem avoided? struct ovs_action_push_eth { struct ovs_key_ethernet addresses; #ifndef __KERNEL__ __be16 eth_type; #endif }; Thanks, Ben.
> On Tue, Apr 25, 2017 at 04:30:23PM +0000, Zoltán Balogh wrote: > > From: Jan Scheurich <jan.scheurich@ericsson.com> > > > > Add support for actions push_eth and pop_eth to the netdev datapath and > > the supporting libraries. This patch relies on the support for these actions > > in the kernel datapath to be present. > > Hi, can you help me to understand this part here? I believe that this > will lead to userspace sending a OVS_ACTION_ATTR_PUSH_ETH action > argument to the kernel that is different from what the kernel actually > wants. What's the idea, and how is that problem avoided? > > struct ovs_action_push_eth { > struct ovs_key_ethernet addresses; > #ifndef __KERNEL__ > __be16 eth_type; > #endif > }; > > Thanks, > > Ben. Hi, I'm afraid the kernel would not accept that OVS_ACTION_ATTR_PUSH_ETH action in this case. Jiri, could you confirm, please? We could remove the eth_type from struct ovs_action_push_eth, then put an additional "set action" after putting "push_eth" in the odp_put_push_eth_action() function in order to set the ethertype of the packet. void odp_put_push_eth_action(struct ofpbuf *odp_actions, const struct eth_addr *eth_src, const struct eth_addr *eth_dst, const ovs_be16 eth_type) { struct ovs_action_push_eth eth; memset(ð, 0, sizeof eth); if (eth_src) { eth.addresses.eth_src = *eth_src; } if (eth_dst) { eth.addresses.eth_dst = *eth_dst; } nl_msg_put_unspec(odp_actions, OVS_ACTION_ATTR_PUSH_ETH, ð, sizeof eth); commit_set_action(odp_actions, OVS_KEY_ATTR_ETHERTYPE, eth_type); } That way we would split the push_eth action into a simpler push_eth and a set_field. However this would lead to modify odp_execute_set_action()as well, since changing of ethertype with set_field is not allowed: static void odp_execute_set_action(struct dp_packet *packet, const struct nlattr *a) { enum ovs_key_attr type = nl_attr_type(a); const struct ovs_key_ipv4 *ipv4_key; const struct ovs_key_ipv6 *ipv6_key; struct pkt_metadata *md = &packet->md; switch (type) { ... case OVS_KEY_ATTR_ETHERTYPE: case OVS_KEY_ATTR_IN_PORT: case OVS_KEY_ATTR_VLAN: case OVS_KEY_ATTR_TCP_FLAGS: case OVS_KEY_ATTR_CT_STATE: case OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV4: case OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV6: case OVS_KEY_ATTR_CT_ZONE: case OVS_KEY_ATTR_CT_MARK: case OVS_KEY_ATTR_CT_LABELS: case __OVS_KEY_ATTR_MAX: default: OVS_NOT_REACHED(); } } I guess something similar should be done at kernel side too, since the kernel module would not accept set_field ethertype either. Another option would be to convert OVS_ACTION_ATTR_PUSH_ETH action argument between userspace and kernel. Do you have any other proposal? Btw, the original comment message of struct ovs_action_push_eth indicates eth_type as a second member. /* * struct ovs_action_push_eth - %OVS_ACTION_ATTR_PUSH_ETH action argument. * @addresses: Source and destination MAC addresses. * @eth_type: Ethernet type */ struct ovs_action_push_eth { struct ovs_key_ethernet addresses; }; Best regards, Zoltan
Zoltan, original l3 userspace patch set gets ether_type from packet->md. eh->eth_type = packet->md.packet_ethertype; I think we should keep struct ovs_action_push_eth consistent in both kernel and userspace, we can use the way in original l3 userspace patch set to get ether_type. -----Original Message----- From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-bounces@openvswitch.org] On Behalf Of Zoltán Balogh Sent: Thursday, May 4, 2017 6:02 PM To: Ben Pfaff <blp@ovn.org>; Jiri Benc (jbenc@redhat.com) <jbenc@redhat.com> Cc: 'dev@openvswitch.org' <dev@openvswitch.org> Subject: Re: [ovs-dev] [PATCH v4 2/7] userspace: Support for push_eth and pop_eth actions > On Tue, Apr 25, 2017 at 04:30:23PM +0000, Zoltán Balogh wrote: > > From: Jan Scheurich <jan.scheurich@ericsson.com> > > > > Add support for actions push_eth and pop_eth to the netdev datapath > > and the supporting libraries. This patch relies on the support for > > these actions in the kernel datapath to be present. > > Hi, can you help me to understand this part here? I believe that this > will lead to userspace sending a OVS_ACTION_ATTR_PUSH_ETH action > argument to the kernel that is different from what the kernel actually > wants. What's the idea, and how is that problem avoided? > > struct ovs_action_push_eth { > struct ovs_key_ethernet addresses; > #ifndef __KERNEL__ > __be16 eth_type; > #endif > }; > > Thanks, > > Ben. Hi, I'm afraid the kernel would not accept that OVS_ACTION_ATTR_PUSH_ETH action in this case. Jiri, could you confirm, please? We could remove the eth_type from struct ovs_action_push_eth, then put an additional "set action" after putting "push_eth" in the odp_put_push_eth_action() function in order to set the ethertype of the packet. void odp_put_push_eth_action(struct ofpbuf *odp_actions, const struct eth_addr *eth_src, const struct eth_addr *eth_dst, const ovs_be16 eth_type) { struct ovs_action_push_eth eth; memset(ð, 0, sizeof eth); if (eth_src) { eth.addresses.eth_src = *eth_src; } if (eth_dst) { eth.addresses.eth_dst = *eth_dst; } nl_msg_put_unspec(odp_actions, OVS_ACTION_ATTR_PUSH_ETH, ð, sizeof eth); commit_set_action(odp_actions, OVS_KEY_ATTR_ETHERTYPE, eth_type); } That way we would split the push_eth action into a simpler push_eth and a set_field. However this would lead to modify odp_execute_set_action()as well, since changing of ethertype with set_field is not allowed: static void odp_execute_set_action(struct dp_packet *packet, const struct nlattr *a) { enum ovs_key_attr type = nl_attr_type(a); const struct ovs_key_ipv4 *ipv4_key; const struct ovs_key_ipv6 *ipv6_key; struct pkt_metadata *md = &packet->md; switch (type) { ... case OVS_KEY_ATTR_ETHERTYPE: case OVS_KEY_ATTR_IN_PORT: case OVS_KEY_ATTR_VLAN: case OVS_KEY_ATTR_TCP_FLAGS: case OVS_KEY_ATTR_CT_STATE: case OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV4: case OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV6: case OVS_KEY_ATTR_CT_ZONE: case OVS_KEY_ATTR_CT_MARK: case OVS_KEY_ATTR_CT_LABELS: case __OVS_KEY_ATTR_MAX: default: OVS_NOT_REACHED(); } } I guess something similar should be done at kernel side too, since the kernel module would not accept set_field ethertype either. Another option would be to convert OVS_ACTION_ATTR_PUSH_ETH action argument between userspace and kernel. Do you have any other proposal? Btw, the original comment message of struct ovs_action_push_eth indicates eth_type as a second member. /* * struct ovs_action_push_eth - %OVS_ACTION_ATTR_PUSH_ETH action argument. * @addresses: Source and destination MAC addresses. * @eth_type: Ethernet type */ struct ovs_action_push_eth { struct ovs_key_ethernet addresses; }; Best regards, Zoltan
On Thu, 4 May 2017 10:02:22 +0000, Zoltán Balogh wrote: > I'm afraid the kernel would not accept that OVS_ACTION_ATTR_PUSH_ETH > action in this case. > > Jiri, could you confirm, please? It would accept it and it would ignore the eth_type field. The kernel does not verify whether netlink attributes are longer than they should be. It just accepts such attributes and silently ignores the excessive data. I would discourage doing this, though. The data structures between the kernel and the user space should match. (As a side note, we're currently having a discussion that this treatment of excessively long attributes should be changed. Of course this would require the user space asking for such new behavior by setsockopt to keep compatibility with existing programs.) > We could remove the eth_type from struct ovs_action_push_eth, then > put an additional "set action" after putting "push_eth" in the > odp_put_push_eth_action() function in order to set the ethertype of > the packet. This seems weird, though. Under which circumstances does ovs not know what ethertype the packet has? The kernel knows about it (via skb->protocol) all the time, with or without the Ethernet header. No flow matching could work otherwise. > That way we would split the push_eth action into a simpler push_eth > and a set_field. However this would lead to modify > odp_execute_set_action()as well, since changing of ethertype with > set_field is not allowed: Disallowing changing of ethertype is a sensible thing to do. > I guess something similar should be done at kernel side too, since > the kernel module would not accept set_field ethertype either. It does not need it. The kernel knows what the ethertype is. > Another option would be to convert OVS_ACTION_ATTR_PUSH_ETH action > argument between userspace and kernel. > > Do you have any other proposal? > > Btw, the original comment message of struct ovs_action_push_eth > indicates eth_type as a second member. Thanks, this is a (documentation) bug. A leftover from one of the previous versions that nobody noticed. I'll remove it upstream once net-next reopens. Jiri
On 4 May 2017 at 11:13, Jiri Benc <jbenc@redhat.com> wrote: > On Thu, 4 May 2017 10:02:22 +0000, Zoltán Balogh wrote: >> I'm afraid the kernel would not accept that OVS_ACTION_ATTR_PUSH_ETH >> action in this case. >> >> Jiri, could you confirm, please? > > It would accept it and it would ignore the eth_type field. The kernel > does not verify whether netlink attributes are longer than they should > be. It just accepts such attributes and silently ignores the excessive > data. Really? Previously the OVS usage of netlink always validated that netlink attributes are exactly as long as expected and that there's no trailing data. I recognise this is a little different from most kernel netlink usage, but that's important to how backward/forward compatibility works with OVS userspace.
On 4 May 2017 at 12:51, Joe Stringer <joe@ovn.org> wrote: > On 4 May 2017 at 11:13, Jiri Benc <jbenc@redhat.com> wrote: >> On Thu, 4 May 2017 10:02:22 +0000, Zoltán Balogh wrote: >>> I'm afraid the kernel would not accept that OVS_ACTION_ATTR_PUSH_ETH >>> action in this case. >>> >>> Jiri, could you confirm, please? >> >> It would accept it and it would ignore the eth_type field. The kernel >> does not verify whether netlink attributes are longer than they should >> be. It just accepts such attributes and silently ignores the excessive >> data. > > Really? Previously the OVS usage of netlink always validated that > netlink attributes are exactly as long as expected and that there's no > trailing data. With a glance at __ovs_nla_copy_actions(), this is also true for OVS_ACTION_ATTR_PUSH_ETH.
On Thu, 4 May 2017 12:53:25 -0700, Joe Stringer wrote: > On 4 May 2017 at 12:51, Joe Stringer <joe@ovn.org> wrote: > > Really? Previously the OVS usage of netlink always validated that > > netlink attributes are exactly as long as expected and that there's no > > trailing data. > > With a glance at __ovs_nla_copy_actions(), this is also true for > OVS_ACTION_ATTR_PUSH_ETH. Right, sorry. I completely forgot about the special netlink handling of ovs. Jiri
It is indeed possible to remove the eth_type field from struct ovs_action_push_eth and let the netdev datapath determine the Ethertype of the resulting frame automatically based on the packet_type of the current packet as you suggest in the patch below and is done in the kernel datapath. /Jan > > Hello Jan, > > I talked with Yi over lync to clarify what he meant with his mail below. He suggested to remove eth_type from struct ovs_action_push_eth, > do not specify it when putting the OVS_ACTION_ATTR_PUSH_ETH action argument. Then in push_eth() in lib/packet.c, the eth_type could > be derived from packet->packet_type. > > /* Push Ethernet header onto 'packet' assuming it is layer 3 */ > void > push_eth(struct dp_packet *packet, const struct eth_addr *dst, > - const struct eth_addr *src, ovs_be16 type) > + const struct eth_addr *src) > { > struct eth_header *eh; > > ovs_assert(packet->packet_type != htonl(PT_ETH)); > eh = dp_packet_resize_l2(packet, ETH_HEADER_LEN); > eh->eth_dst = *dst; > eh->eth_src = *src; > - eh->eth_type = type; > + eh->eth_type = pt_ns_type_be(packet->packet_type); > packet->packet_type = htonl(PT_ETH); > } > > That way, the new ethertype would be set automatically, like in the kernel module. >
Hello Jan, I'm going to do this and create a new v5 branch in gitlab. Best regards, Zoltan Jan Scheurich <jan.scheurich@ericsson.com> wrote: It is indeed possible to remove the eth_type field from struct ovs_action_push_eth and let the netdev datapath determine the Ethertype of the resulting frame automatically based on the packet_type of the current packet as you suggest in the patch below and is done in the kernel datapath. /Jan > > Hello Jan, > > I talked with Yi over lync to clarify what he meant with his mail below. He suggested to remove eth_type from struct ovs_action_push_eth, > do not specify it when putting the OVS_ACTION_ATTR_PUSH_ETH action argument. Then in push_eth() in lib/packet.c, the eth_type could > be derived from packet->packet_type. > > /* Push Ethernet header onto 'packet' assuming it is layer 3 */ > void > push_eth(struct dp_packet *packet, const struct eth_addr *dst, > - const struct eth_addr *src, ovs_be16 type) > + const struct eth_addr *src) > { > struct eth_header *eh; > > ovs_assert(packet->packet_type != htonl(PT_ETH)); > eh = dp_packet_resize_l2(packet, ETH_HEADER_LEN); > eh->eth_dst = *dst; > eh->eth_src = *src; > - eh->eth_type = type; > + eh->eth_type = pt_ns_type_be(packet->packet_type); > packet->packet_type = htonl(PT_ETH); > } > > That way, the new ethertype would be set automatically, like in the kernel module. >
> > With a glance at __ovs_nla_copy_actions(), this is also true for > > OVS_ACTION_ATTR_PUSH_ETH. > > Right, sorry. I completely forgot about the special netlink handling of > ovs. Thank you for your comments! I'm going to remove the eth_type from struct ovs_action_push_eth, then set eth_type of a packet based on its packet_type when pushing ethernet header in lib/packets.c as it was suggested by Yi. /* Push Ethernet header onto 'packet' assuming it is layer 3 */ void push_eth(struct dp_packet *packet, const struct eth_addr *dst, - const struct eth_addr *src, ovs_be16 type) + const struct eth_addr *src) { struct eth_header *eh; ovs_assert(packet->packet_type != htonl(PT_ETH)); eh = dp_packet_resize_l2(packet, ETH_HEADER_LEN); eh->eth_dst = *dst; eh->eth_src = *src; - eh->eth_type = type; + eh->eth_type = pt_ns_type_be(packet->packet_type); packet->packet_type = htonl(PT_ETH); } Ben, will you continue reviewing v4 or should I send out v5 with this correction? Best regards, Zoltan
Hi Ben,
I sent v5 to the dev list.
https://mail.openvswitch.org/pipermail/ovs-dev/2017-May/332151.html
Best regards
Zoltan
From: Ben Pfaff [mailto:blp@ovn.org]
Sent: Friday, May 05, 2017 4:37 PM
To: Zoltán Balogh <zoltan.balogh@ericsson.com>
Cc: dev@openvswitch.org; Jiri Benc <jbenc@redhat.com>; Yang, Yi Y <yi.y.yang@intel.com>; Joe Stringer <joe@ovn.org>
Subject: RE: [ovs-dev] [PATCH v4 2/7] userspace: Support for push_eth and pop_eth actions
Please do send v5.
diff --git a/datapath/linux/compat/include/linux/openvswitch.h b/datapath/linux/compat/include/linux/openvswitch.h index 8a6b729..3a8fba1 100644 --- a/datapath/linux/compat/include/linux/openvswitch.h +++ b/datapath/linux/compat/include/linux/openvswitch.h @@ -762,6 +762,9 @@ enum ovs_ct_attr { */ struct ovs_action_push_eth { struct ovs_key_ethernet addresses; +#ifndef __KERNEL__ + __be16 eth_type; +#endif }; /** @@ -838,8 +841,7 @@ enum ovs_nat_attr { * entries in the flow key. * @OVS_ACTION_ATTR_PUSH_ETH: Push a new outermost Ethernet header onto the * packet. - * @OVS_ACTION_ATTR_POP_ETH: Pop the outermost Ethernet header off the - * packet. + * @OVS_ACTION_ATTR_POP_ETH: Pop the outermost Ethernet header off the packet. * * Only a single header can be set with a single %OVS_ACTION_ATTR_SET. Not all * fields within a header are modifiable, e.g. the IPv4 protocol and fragment diff --git a/lib/odp-execute.c b/lib/odp-execute.c index c4563b1..8e090c8 100644 --- a/lib/odp-execute.c +++ b/lib/odp-execute.c @@ -732,6 +732,21 @@ odp_execute_actions(void *dp, struct dp_packet_batch *batch, bool steal, case OVS_ACTION_ATTR_METER: /* Not implemented yet. */ break; + case OVS_ACTION_ATTR_PUSH_ETH: { + const struct ovs_action_push_eth *eth = nl_attr_get(a); + + DP_PACKET_BATCH_FOR_EACH (packet, batch) { + push_eth(packet, ð->addresses.eth_dst, + ð->addresses.eth_src, eth->eth_type); + } + break; + } + + case OVS_ACTION_ATTR_POP_ETH: + DP_PACKET_BATCH_FOR_EACH (packet, batch) { + pop_eth(packet); + } + break; case OVS_ACTION_ATTR_OUTPUT: case OVS_ACTION_ATTR_TUNNEL_PUSH: @@ -739,8 +754,6 @@ odp_execute_actions(void *dp, struct dp_packet_batch *batch, bool steal, case OVS_ACTION_ATTR_USERSPACE: case OVS_ACTION_ATTR_RECIRC: case OVS_ACTION_ATTR_CT: - case OVS_ACTION_ATTR_PUSH_ETH: - case OVS_ACTION_ATTR_POP_ETH: case OVS_ACTION_ATTR_UNSPEC: case __OVS_ACTION_ATTR_MAX: OVS_NOT_REACHED(); diff --git a/lib/odp-util.c b/lib/odp-util.c index ebb572d..d2adc22 100644 --- a/lib/odp-util.c +++ b/lib/odp-util.c @@ -865,9 +865,11 @@ format_odp_action(struct ds *ds, const struct nlattr *a) break; case OVS_ACTION_ATTR_PUSH_ETH: { const struct ovs_action_push_eth *eth = nl_attr_get(a); - ds_put_format(ds, "push_eth(src="ETH_ADDR_FMT",dst="ETH_ADDR_FMT")", + ds_put_format(ds, "push_eth(src="ETH_ADDR_FMT",dst="ETH_ADDR_FMT + ",type=0x%04"PRIx16")", ETH_ADDR_ARGS(eth->addresses.eth_src), - ETH_ADDR_ARGS(eth->addresses.eth_dst)); + ETH_ADDR_ARGS(eth->addresses.eth_dst), + ntohs(eth->eth_type)); break; } case OVS_ACTION_ATTR_POP_ETH: @@ -1078,14 +1080,41 @@ parse_odp_userspace_action(const char *s, struct ofpbuf *actions) odp_put_userspace_action(pid, user_data, user_data_size, tunnel_out_port, include_actions, actions); res = n + n1; + goto out; } else if (s[n] == ')') { odp_put_userspace_action(pid, user_data, user_data_size, ODPP_NONE, include_actions, actions); res = n + 1; - } else { - res = -EINVAL; + goto out; + } + } + + { + struct ovs_action_push_eth push; + int eth_type = 0; + int n1 = -1; + + if (ovs_scan(&s[n], "push_eth(src="ETH_ADDR_SCAN_FMT"," + "dst="ETH_ADDR_SCAN_FMT",type=%i)%n", + ETH_ADDR_SCAN_ARGS(push.addresses.eth_src), + ETH_ADDR_SCAN_ARGS(push.addresses.eth_dst), + ð_type, &n1)) { + + nl_msg_put_unspec(actions, OVS_ACTION_ATTR_PUSH_ETH, + &push, sizeof push); + + res = n + n1; + goto out; } } + + if (!strncmp(&s[n], "pop_eth", 7)) { + nl_msg_put_flag(actions, OVS_ACTION_ATTR_POP_ETH); + res = 7; + goto out; + } + + res = -EINVAL; out: ofpbuf_uninit(&buf); return res; @@ -5528,6 +5557,33 @@ odp_put_userspace_action(uint32_t pid, } void +odp_put_pop_eth_action(struct ofpbuf *odp_actions) +{ + nl_msg_put_flag(odp_actions, OVS_ACTION_ATTR_POP_ETH); +} + +void +odp_put_push_eth_action(struct ofpbuf *odp_actions, + const struct eth_addr *eth_src, + const struct eth_addr *eth_dst, + const ovs_be16 eth_type) +{ + struct ovs_action_push_eth eth; + + memset(ð, 0, sizeof eth); + if (eth_src) { + eth.addresses.eth_src = *eth_src; + } + if (eth_dst) { + eth.addresses.eth_dst = *eth_dst; + } + eth.eth_type = eth_type; + + nl_msg_put_unspec(odp_actions, OVS_ACTION_ATTR_PUSH_ETH, + ð, sizeof eth); +} + +void odp_put_tunnel_action(const struct flow_tnl *tunnel, struct ofpbuf *odp_actions) { diff --git a/lib/odp-util.h b/lib/odp-util.h index 45686d0..8306388 100644 --- a/lib/odp-util.h +++ b/lib/odp-util.h @@ -328,4 +328,11 @@ void odp_put_tunnel_action(const struct flow_tnl *tunnel, void odp_put_tnl_push_action(struct ofpbuf *odp_actions, struct ovs_action_push_tnl *data); + +void odp_put_pop_eth_action(struct ofpbuf *odp_actions); +void odp_put_push_eth_action(struct ofpbuf *odp_actions, + const struct eth_addr *eth_src, + const struct eth_addr *eth_dst, + const ovs_be16 eth_type); + #endif /* odp-util.h */ diff --git a/lib/packets.c b/lib/packets.c index 26781b0..35a41a5 100644 --- a/lib/packets.c +++ b/lib/packets.c @@ -226,6 +226,47 @@ eth_pop_vlan(struct dp_packet *packet) } } +/* Push Ethernet header onto 'packet' assuming it is layer 3 */ +void +push_eth(struct dp_packet *packet, const struct eth_addr *dst, + const struct eth_addr *src, ovs_be16 type) +{ + struct eth_header *eh; + + ovs_assert(packet->packet_type != htonl(PT_ETH)); + eh = dp_packet_resize_l2(packet, ETH_HEADER_LEN); + eh->eth_dst = *dst; + eh->eth_src = *src; + eh->eth_type = type; + packet->packet_type = htonl(PT_ETH); +} + +/* Removes Ethernet header, including VLAN header, from 'packet'. + * + * Previous to calling this function, 'ofpbuf_l3(packet)' must not be NULL */ +void +pop_eth(struct dp_packet *packet) +{ + char *l2_5 = dp_packet_l2_5(packet); + char *l3 = dp_packet_l3(packet); + ovs_be16 ethertype; + int increment; + + ovs_assert(packet->packet_type == htonl(PT_ETH)); + ovs_assert(l3 != NULL); + + if (l2_5) { + increment = packet->l2_5_ofs; + ethertype = *(ALIGNED_CAST(ovs_be16 *, (l2_5 - 2))); + } else { + increment = packet->l3_ofs; + ethertype = *(ALIGNED_CAST(ovs_be16 *, (l3 - 2))); + } + + dp_packet_resize_l2(packet, -increment); + packet->packet_type = PACKET_TYPE_BE(OFPHTN_ETHERTYPE, ntohs(ethertype)); +} + /* Set ethertype of the packet. */ static void set_ethertype(struct dp_packet *packet, ovs_be16 eth_type) diff --git a/lib/packets.h b/lib/packets.h index e127471..03c90f8 100644 --- a/lib/packets.h +++ b/lib/packets.h @@ -403,6 +403,10 @@ struct eth_header { }); BUILD_ASSERT_DECL(ETH_HEADER_LEN == sizeof(struct eth_header)); +void push_eth(struct dp_packet *packet, const struct eth_addr *dst, + const struct eth_addr *src, ovs_be16 type); +void pop_eth(struct dp_packet *packet); + #define LLC_DSAP_SNAP 0xaa #define LLC_SSAP_SNAP 0xaa #define LLC_CNTL_SNAP 3 diff --git a/ofproto/ofproto-dpif-sflow.c b/ofproto/ofproto-dpif-sflow.c index 59fafa1..43483d7 100644 --- a/ofproto/ofproto-dpif-sflow.c +++ b/ofproto/ofproto-dpif-sflow.c @@ -1188,13 +1188,13 @@ dpif_sflow_read_actions(const struct flow *flow, dpif_sflow_pop_mpls_lse(sflow_actions); break; } - case OVS_ACTION_ATTR_PUSH_ETH: - case OVS_ACTION_ATTR_POP_ETH: - /* TODO: SFlow does not currently define a MAC-in-MAC - * encapsulation structure. We could use an extension - * structure to report this. - */ - break; + case OVS_ACTION_ATTR_PUSH_ETH: + case OVS_ACTION_ATTR_POP_ETH: + /* TODO: SFlow does not currently define a MAC-in-MAC + * encapsulation structure. We could use an extension + * structure to report this. + */ + break; case OVS_ACTION_ATTR_SAMPLE: case OVS_ACTION_ATTR_CLONE: case OVS_ACTION_ATTR_UNSPEC: