The ethernet addresses of two ICMP request packets are indeed different . One
is original packet and the other is modified. It is an expected behavior
according to the code.
Actually, when a packet sent by port A is changed by flow table and then is
sent to itself, we expect to capture this packet. However, when this packet is
changed and is sent to another port, should we still capture the packet on port
A?
[root@localhost infiniband]# ovs-tcpdump -i tapVm71 -nnvve
11.11.70.1 > 1.1.70.2: ICMP echo request, id 15498, seq 17, length 64
09:36:52.822232 68:05:ca:21:d6:e5 > 52:54:00:67:d5:61, ethertype IPv4 (0x0800),
length 98: (tos 0x0, ttl 63, id 22101, offset 0, flags [none], proto ICMP (1),
length 84)
1.1.70.2 > 11.11.70.1: ICMP echo reply, id 15498, seq 17, length 64
09:36:53.862137 52:54:00:67:d5:61 > 68:05:ca:21:d6:e5, ethertype IPv4 (0x0800),
length 98: (tos 0x0, ttl 64, id 26518, offset 0, flags [DF], proto ICMP (1),
length 84)
11.11.70.1 > 1.1.70.2: ICMP echo request, id 15498, seq 18, length 64
09:36:53.862139 68:05:ca:21:d6:e5 > 52:54:00:9a:bf:ed, ethertype IPv4 (0x0800),
length 98: (tos 0x0, ttl 63, id 26518, offset 0, flags [DF], proto ICMP (1),
length 84)
11.11.70.1 > 1.1.70.2: ICMP echo request, id 15498, seq 18, length 64
09:36:53.862230 68:05:ca:21:d6:e5 > 52:54:00:67:d5:61, ethertype IPv4 (0x0800),
length 98: (tos 0x0, ttl 63, id 22176, offset 0, flags [none], proto ICMP (1),
length 84)
1.1.70.2 > 11.11.70.1: ICMP echo reply, id 15498, seq 18, length 64
-----邮件原件-----
发件人: Mike Pattrick [mailto:[email protected]]
发送时间: 2024年3月25日 22:26
收件人: zhangweiwei (RD) <[email protected]>
抄送: [email protected]
主题: Re: [PATCH] ofproto-dpif-upcall: Don't mirror packets that aren't modified.
On Mon, Mar 25, 2024 at 3:48 AM Zhangweiwei <[email protected]> wrote:
>
> Hi,
> I have tried this patch, however, there are still some issues when the
> packets contents are changed across recirculation. On the follow example,
> packets are modified in recirc_id(0) after mirror, the mirror context reset.
> Therefore, there are two ICMP request packets are mirrored on port mitapVm71.
>
> In the following example, ICMP packets ared sent from port(11) to
> port(14), [root@localhost ~]# ovs-appctl dpif/dump-flows vds1-br
> ct_state(-new-est-rel-rpl-inv-trk),recirc_id(0),in_port(11),packet_typ
> e(ns=0,id=0),eth(src=52:54:00:67:d5:61,dst=68:05:ca:21:d6:e5),eth_type
> (0x0800),ipv4(src=11.11.70.1,dst=1.1.70.2,proto=1,ttl=64,frag=no),
> packets:431, bytes:42238, used:0.574s,
> actions:10,set(eth(src=68:05:ca:21:d6:e5,dst=52:54:00:9a:bf:ed)),set(i
> pv4(ttl=63)),ct(zone=6),recirc(0x3e8)
> ct_state(+est-rel-rpl),recirc_id(0x3e8),in_port(11),packet_type(ns=0,i
> d=0),eth_type(0x0800),ipv4(frag=no), packets:430, bytes:42140,
> used:0.574s, actions:10,14
>
> ct_state(-new+est-rel+rpl-inv+trk),recirc_id(0x3e9),in_port(14),packet
> _type(ns=0,id=0),eth(src=52:54:00:9a:bf:ed,dst=68:05:ca:21:d6:e5),eth_
> type(0x0800),ipv4(dst=11.11.70.1,proto=1,ttl=64,frag=no), packets:431,
> bytes:42238, used:0.574s,
> actions:set(eth(src=68:05:ca:21:d6:e5,dst=52:54:00:67:d5:61)),set(ipv4
> (ttl=63)),11,10
> ct_state(-trk),recirc_id(0),in_port(14),packet_type(ns=0,id=0),eth(src
> =52:54:00:9a:bf:ed),eth_type(0x0800),ipv4(src=1.1.70.2,proto=1,frag=no
> ), packets:431, bytes:42238, used:0.574s,
> actions:ct(zone=6),recirc(0x3e9)
>
> [root@localhost ~]# ovs-appctl dpif/show
> netdev@ovs-netdev: hit:2552 missed:3019
> vds1-br:
> mitapVm71 14/10: (system)
> tapVm71 5/11: (dpdkvhostuserclient: configured_rx_queues=1,
> configured_tx_queues=1, mtu=1500, requested_rx_queues=1,
> requested_tx_queues=1)
> tapVm72 6/14: (dpdkvhostuserclient: configured_rx_queues=1,
> configured_tx_queues=1, mtu=1500, requested_rx_queues=1,
> requested_tx_queues=1)
>
> [root@localhost ~]# ovs-tcpdump -i tapVm71
> 14:38:53.702142 IP 11.11.70.1 > 1.1.70.2: ICMP echo request, id 13483,
> seq 2014, length 64
> 14:38:53.702143 IP 11.11.70.1 > 1.1.70.2: ICMP echo request, id 13483,
> seq 2014, length 64
> 14:38:53.702185 IP 1.1.70.2 > 11.11.70.1: ICMP echo reply, id 13483,
> seq 2014, length 64
> 14:38:54.742141 IP 11.11.70.1 > 1.1.70.2: ICMP echo request, id 13483,
> seq 2015, length 64
> 14:38:54.742143 IP 11.11.70.1 > 1.1.70.2: ICMP echo request, id 13483,
> seq 2015, length 64
> 14:38:54.742183 IP 1.1.70.2 > 11.11.70.1: ICMP echo reply, id 13483,
> seq 2015, length 64
> 14:38:55.782142 IP 11.11.70.1 > 1.1.70.2: ICMP echo request, id 13483,
> seq 2016, length 64
> 14:38:55.782144 IP 11.11.70.1 > 1.1.70.2: ICMP echo request, id 13483,
> seq 2016, length 64
> 14:38:55.782186 IP 1.1.70.2 > 11.11.70.1: ICMP echo reply, id 13483,
> seq 2016, length 64
Hello, thanks for the report. Is it possible to run the command "ovs-tcpdump -i
tapVm71 -ennvv" ? I ask because I see your actions reset the ethernet address.
If the ethernet address is different then this would be the expected behavior,
the collection of the packet as it enters, and then as it exists modified.
Thank you,
Mike
>
> -----邮件原件-----
> 发件人: Mike Pattrick [mailto:[email protected]]
> 发送时间: 2024年3月13日 1:37
> 收件人: [email protected]
> 抄送: Mike Pattrick <[email protected]>; zhangweiwei (RD)
> <[email protected]>
> 主题: [PATCH] ofproto-dpif-upcall: Don't mirror packets that aren't modified.
>
> Previously OVS reset the mirror contents when a packet is modified in such a
> way that the packets contents changes. However, this change incorrectly reset
> that mirror context when only metadata changes as well.
>
> Now we check for all metadata fields, instead of just tunnel metadata, before
> resetting the mirror context.
>
> Fixes: feed7f677505 ("ofproto-dpif-upcall: Mirror packets that are
> modified.")
> Reported-by: Zhangweiwei <[email protected]>
> Signed-off-by: Mike Pattrick <[email protected]>
> ---
> include/openvswitch/meta-flow.h | 1 +
> lib/meta-flow.c | 109 ++++++++++++++++++++++++++++++++
> ofproto/ofproto-dpif-xlate.c | 2 +-
> tests/ofproto-dpif.at | 5 +-
> 4 files changed, 114 insertions(+), 3 deletions(-)
>
> diff --git a/include/openvswitch/meta-flow.h
> b/include/openvswitch/meta-flow.h index 3b0220aaa..96aad3933 100644
> --- a/include/openvswitch/meta-flow.h
> +++ b/include/openvswitch/meta-flow.h
> @@ -2305,6 +2305,7 @@ void mf_set_flow_value_masked(const struct mf_field *,
> const union mf_value *mask,
> struct flow *); bool
> mf_is_tun_metadata(const struct mf_field *);
> +bool mf_is_metadata(const struct mf_field *);
> bool mf_is_frozen_metadata(const struct mf_field *); bool
> mf_is_pipeline_field(const struct mf_field *); bool mf_is_set(const
> struct mf_field *, const struct flow *); diff --git a/lib/meta-flow.c
> b/lib/meta-flow.c index aa7cf1fcb..7ecec334e 100644
> --- a/lib/meta-flow.c
> +++ b/lib/meta-flow.c
> @@ -1788,6 +1788,115 @@ mf_is_tun_metadata(const struct mf_field *mf)
> mf->id < MFF_TUN_METADATA0 + TUN_METADATA_NUM_OPTS; }
>
> +bool
> +mf_is_metadata(const struct mf_field *mf) {
> + switch (mf->id) {
> + CASE_MFF_TUN_METADATA:
> + case MFF_METADATA:
> + case MFF_IN_PORT:
> + case MFF_IN_PORT_OXM:
> + CASE_MFF_REGS:
> + CASE_MFF_XREGS:
> + CASE_MFF_XXREGS:
> + case MFF_PACKET_TYPE:
> + case MFF_DP_HASH:
> + case MFF_RECIRC_ID:
> + case MFF_CONJ_ID:
> + case MFF_ACTSET_OUTPUT:
> + case MFF_SKB_PRIORITY:
> + case MFF_PKT_MARK:
> + case MFF_CT_STATE:
> + case MFF_CT_ZONE:
> + case MFF_CT_MARK:
> + case MFF_CT_LABEL:
> + case MFF_CT_NW_PROTO:
> + case MFF_CT_NW_SRC:
> + case MFF_CT_NW_DST:
> + case MFF_CT_IPV6_SRC:
> + case MFF_CT_IPV6_DST:
> + case MFF_CT_TP_SRC:
> + case MFF_CT_TP_DST:
> + case MFF_N_IDS:
> + return true;
> +
> + case MFF_TUN_ID:
> + case MFF_TUN_SRC:
> + case MFF_TUN_DST:
> + case MFF_TUN_IPV6_SRC:
> + case MFF_TUN_IPV6_DST:
> + case MFF_TUN_FLAGS:
> + case MFF_TUN_GBP_ID:
> + case MFF_TUN_GBP_FLAGS:
> + case MFF_TUN_ERSPAN_VER:
> + case MFF_TUN_ERSPAN_IDX:
> + case MFF_TUN_ERSPAN_DIR:
> + case MFF_TUN_ERSPAN_HWID:
> + case MFF_TUN_GTPU_FLAGS:
> + case MFF_TUN_GTPU_MSGTYPE:
> + case MFF_TUN_TTL:
> + case MFF_TUN_TOS:
> + case MFF_ETH_SRC:
> + case MFF_ETH_DST:
> + case MFF_ETH_TYPE:
> + case MFF_VLAN_TCI:
> + case MFF_DL_VLAN:
> + case MFF_VLAN_VID:
> + case MFF_DL_VLAN_PCP:
> + case MFF_VLAN_PCP:
> + case MFF_MPLS_LABEL:
> + case MFF_MPLS_TC:
> + case MFF_MPLS_BOS:
> + case MFF_MPLS_TTL:
> + case MFF_IPV4_SRC:
> + case MFF_IPV4_DST:
> + case MFF_IPV6_SRC:
> + case MFF_IPV6_DST:
> + case MFF_IPV6_LABEL:
> + case MFF_IP_PROTO:
> + case MFF_IP_DSCP:
> + case MFF_IP_DSCP_SHIFTED:
> + case MFF_IP_ECN:
> + case MFF_IP_TTL:
> + case MFF_IP_FRAG:
> + case MFF_ARP_OP:
> + case MFF_ARP_SPA:
> + case MFF_ARP_TPA:
> + case MFF_ARP_SHA:
> + case MFF_ARP_THA:
> + case MFF_TCP_SRC:
> + case MFF_TCP_DST:
> + case MFF_TCP_FLAGS:
> + case MFF_UDP_SRC:
> + case MFF_UDP_DST:
> + case MFF_SCTP_SRC:
> + case MFF_SCTP_DST:
> + case MFF_ICMPV4_TYPE:
> + case MFF_ICMPV4_CODE:
> + case MFF_ICMPV6_TYPE:
> + case MFF_ICMPV6_CODE:
> + case MFF_ND_TARGET:
> + case MFF_ND_SLL:
> + case MFF_ND_TLL:
> + case MFF_ND_RESERVED:
> + case MFF_ND_OPTIONS_TYPE:
> + case MFF_NSH_FLAGS:
> + case MFF_NSH_TTL:
> + case MFF_NSH_MDTYPE:
> + case MFF_NSH_NP:
> + case MFF_NSH_SPI:
> + case MFF_NSH_SI:
> + case MFF_NSH_C1:
> + case MFF_NSH_C2:
> + case MFF_NSH_C3:
> + case MFF_NSH_C4:
> + return false;
> +
> + default:
> + OVS_NOT_REACHED();
> + }
> +}
> +
> bool
> mf_is_frozen_metadata(const struct mf_field *mf) { diff --git
> a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index
> 89f183182..faa364ec8 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -7141,7 +7141,7 @@ reset_mirror_ctx(struct xlate_ctx *ctx, const
> struct flow *flow,
>
> set_field = ofpact_get_SET_FIELD(a);
> mf = set_field->field;
> - if (mf_are_prereqs_ok(mf, flow, NULL) && !mf_is_tun_metadata(mf)) {
> + if (mf_are_prereqs_ok(mf, flow, NULL) && !mf_is_metadata(mf))
> + {
> ctx->mirrors = 0;
> }
> return;
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at index
> a1393f7f8..245e209c3 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -5443,7 +5443,8 @@ AT_CLEANUP
> # This test verifies that mirror state is preserved across recirculation.
> #
> # Otherwise, post-recirculation the ingress and the output to port 4 -#
> would cause the packet to be mirrored to port 3 a second time.
> +# would cause the packet to be mirrored to port 3 a second time. A
> +register # is also modified to verify that this doesn't reset the mirror
> context.
> AT_SETUP([ofproto-dpif - mirroring with recirculation]) AT_KEYWORDS([mirror
> mirrors mirroring]) OVS_VSWITCHD_START @@ -5454,7 +5455,7 @@ ovs-vsctl \
> --id=@m create Mirror name=mymirror select_all=true
> output_port=@p3
>
> AT_DATA([flows.txt], [dnl
> -in_port=1 actions=2,debug_recirc,4
> +in_port=1 actions=2,debug_recirc,set_field:1->reg4,4
> ])
> AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
>
> --
> 2.39.3
>
> ----------------------------------------------------------------------
> ---------------------------------------------------------------
> 本邮件及其附件含有新华三集团的保密信息，仅限于发送给上面地址中列出
> 的个人或群组。禁止任何其他人以任何形式使用（包括但不限于全部或部分地泄露、复制、
> 或散发）本邮件中的信息。如果您错收了本邮件，请您立即电话或邮件通知发件人并删除本
> 邮件！
> This e-mail and its attachments contain confidential information from
> New H3C, which is intended only for the person or entity whose address
> is listed above. Any use of the information contained herein in any
> way (including, but not limited to, total or partial disclosure,
> reproduction, or dissemination) by persons other than the intended
> recipient(s) is prohibited. If you receive this e-mail in error,
> please notify the sender by phone or email immediately and delete it!
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
