On Wed, Dec 14, 2022 at 10:50 PM <[email protected]> wrote:
>
> From: wushaohua <[email protected]>
>
> The icmp_id maybe conflicts in snat
> Description:
> If multiple devices send icmp packets with the same icmp_id,
> the sip of the packets changes to the same source ip address after the snat
> operation,
> and the packets are sent to the same destination ip address.
> Only one ct entry is created.The data packets sent by other devices cannot be
> sent to the peer device
>
> ovs-appctl dpctl/dump-conntrack
> icmp,orig=(src=10.0.0.4,dst=10.0.0.2,id=500,type=8,code=0),reply=(src=10.0.0.2,dst=10.0.0.201,id=500,type=0,code=0)
>
> After fixing the problem:
> ovs-appctl dpctl/dump-conntrack
> icmp,orig=(src=10.0.0.3,dst=10.0.0.2,id=500,type=8,code=0),reply=(src=10.0.0.2,dst=10.0.0.201,id=501,type=0,code=0)
> icmp,orig=(src=10.0.0.1,dst=10.0.0.2,id=500,type=8,code=0),reply=(src=10.0.0.2,dst=10.0.0.201,id=500,type=0,code=0)
> icmp,orig=(src=10.0.0.7,dst=10.0.0.2,id=500,type=8,code=0),reply=(src=10.0.0.2,dst=10.0.0.201,id=505,type=0,code=0)
> icmp,orig=(src=10.0.0.4,dst=10.0.0.2,id=500,type=8,code=0),reply=(src=10.0.0.2,dst=10.0.0.201,id=502,type=0,code=0)
> icmp,orig=(src=10.0.0.5,dst=10.0.0.2,id=500,type=8,code=0),reply=(src=10.0.0.2,dst=10.0.0.201,id=503,type=0,code=0)
> icmp,orig=(src=10.0.0.6,dst=10.0.0.2,id=500,type=8,code=0),reply=(src=10.0.0.2,dst=10.0.0.201,id=504,type=0,code=0)
>
> Signed-off-by: wushaohua <[email protected]>
Hello wushaohua,
Unfortunately, this patch no longer applies cleanly to the master
branch due to other changes that have been applied. But It's quite
simple to rebase with a 3-way merge.
This patch does resolve the issue you identified. I think it could be
improved by adding a unit tests, I've included an example below. Other
than that, it looks good to me.
Cheers,
M
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 4c378e1d0..abcd1d8fe 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -5497,6 +5497,41 @@
tcp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=<cleared>,dport=<cleared>),reply=(src=
OVS_TRAFFIC_VSWITCHD_STOP
AT_CLEANUP
+AT_SETUP([conntrack - SNAT with icmp_id])
+CHECK_CONNTRACK()
+CHECK_CONNTRACK_NAT()
+OVS_TRAFFIC_VSWITCHD_START()
+
+ADD_NAMESPACES(at_ns0, at_ns1)
+
+ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
+NS_CHECK_EXEC([at_ns0], [ip link set dev p0 address 80:88:88:88:88:88])
+ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24")
+
+dnl Allow any traffic from ns0->ns1. Only allow nd, return traffic
from ns1->ns0.
+AT_DATA([flows.txt], [dnl
+in_port=1,ip,action=ct(commit,zone=1,nat(src=10.1.1.240)),2
+in_port=2,ct_state=-trk,ip,action=ct(table=0,zone=1,nat)
+in_port=2,ct_state=+trk,ct_zone=1,ip,action=1
+])
+
+AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt])
+
+ovs-appctl dpctl/flush-conntrack
+
+ovs-ofctl packet-out br0
"in_port=ovs-p0,packet=e2b2c2a8b8c8e2b2c2a9b9c908004500001c00010000400164dc0a0101020a0101010800f60a01f40001
actions=resubmit(,0)"
+ovs-ofctl packet-out br0
"in_port=ovs-p0,packet=e2b2c2a8b8c8e2b2c2a9b9c908004500001c00010000400164db0a0101030a0101010800f60a01f40001
actions=resubmit(,0)"
+ovs-ofctl packet-out br0
"in_port=ovs-p0,packet=e2b2c2a8b8c8e2b2c2a9b9c908004500001c00010000400164da0a0101040a0101010800f60a01f40001
actions=resubmit(,0)"
+
+AT_CHECK([ovs-appctl dpctl/dump-conntrack | sort], [0], [dnl
+icmp,orig=(src=10.1.1.2,dst=10.1.1.1,id=500,type=8,code=0),reply=(src=10.1.1.1,dst=10.1.1.240,id=500,type=0,code=0),zone=1
+icmp,orig=(src=10.1.1.3,dst=10.1.1.1,id=500,type=8,code=0),reply=(src=10.1.1.1,dst=10.1.1.240,id=501,type=0,code=0),zone=1
+icmp,orig=(src=10.1.1.4,dst=10.1.1.1,id=500,type=8,code=0),reply=(src=10.1.1.1,dst=10.1.1.240,id=502,type=0,code=0),zone=1
+])
+
+OVS_TRAFFIC_VSWITCHD_STOP
+AT_CLEANUP
+
> ---
> lib/conntrack.c | 51 +++++++++++++++++++++++++++++++++++++++++++------
> lib/packets.c | 22 +++++++++++++++++++++
> lib/packets.h | 2 ++
> 3 files changed, 69 insertions(+), 6 deletions(-)
>
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 550b2be9b..a5d3e6a3f 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -697,6 +697,24 @@ get_ip_proto(const struct dp_packet *pkt)
> return ip_proto;
> }
>
> +static bool
> +packet_is_icmpv4_info_message(const struct dp_packet *pkt)
> +{
> + uint8_t ip_proto,icmp_type;
> +
> + ip_proto = get_ip_proto(pkt);
> + if (ip_proto == IPPROTO_ICMP) {
> + icmp_type = packet_get_icmp_type(pkt);
> + if (icmp_type == ICMP4_ECHO_REQUEST ||
> + icmp_type == ICMP4_ECHO_REPLY ||
> + icmp_type == ICMP4_TIMESTAMP ||
> + icmp_type == ICMP4_TIMESTAMPREPLY ||
> + icmp_type == ICMP4_INFOREQUEST ||
> + icmp_type == ICMP4_INFOREPLY)
> + return true;
> + }
> + return false;
> +}
> static bool
> is_ftp_ctl(const enum ct_alg_ctl_type ct_alg_ctl)
> {
> @@ -773,6 +791,9 @@ pat_packet(struct dp_packet *pkt, const struct conn *conn)
> } else if (conn->key.nw_proto == IPPROTO_UDP) {
> struct udp_header *uh = dp_packet_l4(pkt);
> packet_set_udp_port(pkt, conn->rev_key.dst.port, uh->udp_dst);
> + } else if (packet_is_icmpv4_info_message(pkt) &&
> + conn->key.nw_proto == IPPROTO_ICMP) {
> + packet_set_icmp_id(pkt, conn->rev_key.dst.icmp_id);
> }
> } else if (conn->nat_action & NAT_ACTION_DST) {
> if (conn->key.nw_proto == IPPROTO_TCP) {
> @@ -781,6 +802,9 @@ pat_packet(struct dp_packet *pkt, const struct conn *conn)
> } else if (conn->key.nw_proto == IPPROTO_UDP) {
> packet_set_udp_port(pkt, conn->rev_key.dst.port,
> conn->rev_key.src.port);
> + } else if (packet_is_icmpv4_info_message(pkt) &&
> + conn->key.nw_proto == IPPROTO_ICMP) {
> + packet_set_icmp_id(pkt, conn->rev_key.src.icmp_id);
> }
> }
> }
> @@ -831,13 +855,19 @@ un_pat_packet(struct dp_packet *pkt, const struct conn
> *conn)
> } else if (conn->key.nw_proto == IPPROTO_UDP) {
> struct udp_header *uh = dp_packet_l4(pkt);
> packet_set_udp_port(pkt, uh->udp_src, conn->key.src.port);
> - }
> + } else if (packet_is_icmpv4_info_message(pkt) &&
> + conn->key.nw_proto == IPPROTO_ICMP) {
> + packet_set_icmp_id(pkt, conn->key.src.icmp_id);
> + }
> } else if (conn->nat_action & NAT_ACTION_DST) {
> if (conn->key.nw_proto == IPPROTO_TCP) {
> packet_set_tcp_port(pkt, conn->key.dst.port, conn->key.src.port);
> } else if (conn->key.nw_proto == IPPROTO_UDP) {
> packet_set_udp_port(pkt, conn->key.dst.port, conn->key.src.port);
> - }
> + } else if (packet_is_icmpv4_info_message(pkt) &&
> + conn->key.nw_proto == IPPROTO_ICMP) {
> + packet_set_icmp_id(pkt, conn->key.dst.icmp_id);
> + }
> }
> }
>
> @@ -2366,8 +2396,8 @@ store_addr_to_key(union ct_addr *addr, struct conn_key
> *key,
>
> static bool
> nat_get_unique_l4(struct conntrack *ct, struct conn *nat_conn,
> - ovs_be16 *port, uint16_t curr, uint16_t min,
> - uint16_t max)
> + ovs_be16 *port, ovs_be16 *peer_port,
> + uint16_t curr, uint16_t min, uint16_t max)
> {
> static const unsigned int max_attempts = 128;
> uint16_t range = max - min + 1;
> @@ -2388,6 +2418,9 @@ another_round:
> }
>
> *port = htons(curr);
> + if (peer_port) {
> + *peer_port = htons(curr);
> + }
> if (!conn_lookup(ct, &nat_conn->rev_key,
> time_msec(), NULL, NULL)) {
> return true;
> @@ -2401,6 +2434,9 @@ another_round:
> }
>
> *port = htons(orig);
> + if (peer_port) {
> + *peer_port = htons(orig);
> + }
>
> return false;
> }
> @@ -2434,7 +2470,8 @@ nat_get_unique_tuple(struct conntrack *ct, const struct
> conn *conn,
> uint32_t hash = nat_range_hash(conn, ct->hash_basis, nat_info);
> union ct_addr min_addr = {0}, max_addr = {0}, addr = {0};
> bool pat_proto = conn->key.nw_proto == IPPROTO_TCP ||
> - conn->key.nw_proto == IPPROTO_UDP;
> + conn->key.nw_proto == IPPROTO_UDP ||
> + conn->key.nw_proto == IPPROTO_ICMP;
> uint16_t min_dport, max_dport, curr_dport;
> uint16_t min_sport, max_sport, curr_sport;
>
> @@ -2469,11 +2506,13 @@ nat_get_unique_tuple(struct conntrack *ct, const
> struct conn *conn,
> bool found = false;
> if (nat_info->nat_action & NAT_ACTION_DST_PORT) {
> found = nat_get_unique_l4(ct, nat_conn, &nat_conn->rev_key.src.port,
> - curr_dport, min_dport, max_dport);
> + NULL,curr_dport, min_dport, max_dport);
> }
>
> if (!found) {
> found = nat_get_unique_l4(ct, nat_conn, &nat_conn->rev_key.dst.port,
> + nat_conn->rev_key.nw_proto == IPPROTO_ICMP
> ?
> + &nat_conn->rev_key.src.port : NULL,
> curr_sport, min_sport, max_sport);
> }
>
> diff --git a/lib/packets.c b/lib/packets.c
> index 1dcd4a6fc..11d21d8c3 100644
> --- a/lib/packets.c
> +++ b/lib/packets.c
> @@ -1441,6 +1441,28 @@ packet_set_icmp(struct dp_packet *packet, uint8_t
> type, uint8_t code)
> pkt_metadata_init_conn(&packet->md);
> }
>
> +/* Sets the ICMP id of the ICMP header contained in 'packet'.
> + * 'packet' must be a valid ICMP packet with its l4 offset properly
> + * populated. */
> +void
> +packet_set_icmp_id(struct dp_packet *packet, ovs_be16 icmp_id)
> +{
> + struct icmp_header *ih = dp_packet_l4(packet);
> + ovs_be16 orig_ic = ih->icmp_fields.echo.id;
> +
> + if (icmp_id != orig_ic) {
> + ih->icmp_fields.echo.id = icmp_id;
> + ih->icmp_csum = recalc_csum16(ih->icmp_csum, orig_ic, icmp_id);
> + }
> + pkt_metadata_init_conn(&packet->md);
> +}
> +
> +uint8_t
> +packet_get_icmp_type(const struct dp_packet *packet)
> +{
> + struct icmp_header *ih = dp_packet_l4(packet);
> + return ih->icmp_type;
> +}
> /* Sets the IGMP type to IGMP_HOST_MEMBERSHIP_QUERY and populates the
> * v3 query header fields in 'packet'. 'packet' must be a valid IGMPv3
> * query packet with its l4 offset properly populated.
> diff --git a/lib/packets.h b/lib/packets.h
> index 5bdf6e4bb..f30ace119 100644
> --- a/lib/packets.h
> +++ b/lib/packets.h
> @@ -1602,6 +1602,8 @@ void packet_set_tcp_port(struct dp_packet *, ovs_be16
> src, ovs_be16 dst);
> void packet_set_udp_port(struct dp_packet *, ovs_be16 src, ovs_be16 dst);
> void packet_set_sctp_port(struct dp_packet *, ovs_be16 src, ovs_be16 dst);
> void packet_set_icmp(struct dp_packet *, uint8_t type, uint8_t code);
> +void packet_set_icmp_id(struct dp_packet *, ovs_be16 icmp_id);
> +uint8_t packet_get_icmp_type(const struct dp_packet *packet);
> void packet_set_nd(struct dp_packet *, const struct in6_addr *target,
> const struct eth_addr sll, const struct eth_addr tll);
> void packet_set_nd_ext(struct dp_packet *packet,
> --
> 2.30.2
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
