Hi Shaohua, Some nits as well (before you send v3). I will review the code in v3.
wushao...@chinatelecom.cn writes: > From: Shaohua Wu <wushao...@chinatelecom.cn> > > 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: Shaohua Wu <wushao...@chinatelecom.cn> > --- > lib/conntrack.c | 39 +++++++++++++++++++++++++++++++++++---- > lib/packets.c | 22 ++++++++++++++++++++++ > lib/packets.h | 2 ++ > tests/system-traffic.at | 40 ++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 99 insertions(+), 4 deletions(-) > > diff --git a/lib/conntrack.c b/lib/conntrack.c > index 4375c03e2..07c6da6be 100644 > --- a/lib/conntrack.c > +++ b/lib/conntrack.c > @@ -698,6 +698,25 @@ 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; Space after the comma. > + > + 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) > { > @@ -771,6 +790,9 @@ pat_packet(struct dp_packet *pkt, const struct conn_key > *key) > packet_set_tcp_port(pkt, key->dst.port, key->src.port); > } else if (key->nw_proto == IPPROTO_UDP) { > packet_set_udp_port(pkt, key->dst.port, key->src.port); > + } else if (packet_is_icmpv4_info_message(pkt) && > + key->nw_proto == IPPROTO_ICMP) { Maybe invert this test here. > + packet_set_icmp_id(pkt, key->src.icmp_id); > } > } > > @@ -2306,8 +2328,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; > @@ -2328,6 +2350,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; > @@ -2341,6 +2366,9 @@ another_round: > } > > *port = htons(orig); > + if (peer_port) { > + *peer_port = htons(orig); > + } > > return false; > } > @@ -2374,7 +2402,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; > > @@ -2409,11 +2438,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); Space after the comma. > } > > 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 462b51f92..f8e33cd68 100644 > --- a/lib/packets.c > +++ b/lib/packets.c > @@ -1492,6 +1492,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; > +} There should be an empty line here. > /* 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 200b25cf0..57b433f73 100644 > --- a/lib/packets.h > +++ b/lib/packets.h > @@ -1628,6 +1628,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, > diff --git a/tests/system-traffic.at b/tests/system-traffic.at > index a05ca311c..0a918c669 100644 > --- a/tests/system-traffic.at > +++ b/tests/system-traffic.at > @@ -3219,6 +3219,46 @@ > 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.0.0.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.0.0.2/24") > + > +dnl Allow any traffic from ns0->ns1. Only allow nd, return traffic from > ns1->ns0. > +AT_DATA([flows.txt], [dnl > +table=0,in_port=2,ct_state=-trk,icmp,action=ct(table=1, nat) > +table=0,in_port=1,ct_state=-trk,icmp,action=ct(table=1, nat) > +table=1,in_port=1,ct_state=+trk+new,action=ct(commit, > nat(src=10.0.0.201)),ovs-p1 > +table=1,in_port=1,ct_state=+trk+est,action=2 > +table=1,in_port=2,ct_state=+trk+est,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=62cf3bde230262cf3bde230108004500001c00010000400166de0a0000010a0000020800f60b01f40000 > +actions=resubmit(,0)" > +ovs-ofctl packet-out br0 > +"in_port=ovs-p0,packet=62cf3bde230262cf3bde230108004500001c00010000400166dc0a0000030a0000020800f60b01f40000 > +actions=resubmit(,0)" > +ovs-ofctl packet-out br0 > +"in_port=ovs-p0,packet=62cf3bde230262cf3bde230108004500001c00010000400166db0a0000040a0000020800f60b01f40000 > +actions=resubmit(,0)" > + > +AT_CHECK([ovs-appctl dpctl/dump-conntrack | sort], [0], [dnl > +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.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.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) > +]) > + > +OVS_TRAFFIC_VSWITCHD_STOP > +AT_CLEANUP > + > AT_SETUP([conntrack - multiple namespaces, internal ports]) > CHECK_CONNTRACK() > CHECK_CONNTRACK_LOCAL_STACK() _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev