On 7/2/23 17:19, wushao...@chinatelecom.cn wrote:
> 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
Hi. Thanks for the new version!
I didn't review the code, but see some comments inline.
>
> 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)
There should be an empty line here.
> Signed-off-by: Shaohua Wu <wushao...@chinatelecom.cn>
> ---
Please, add here the version log, i.e. what changed between
versions of the patch.
> 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;
> +
> + 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) {
> + 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);
> }
>
> 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);
Indentation is off here. One too many spaces.
> +}
> +
> +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 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])
Not sure why you sent the patch twice, but the test in this version
is always failing with:
./system-traffic.at:3241: ovs-ofctl --bundle add-flows br0 flows.txt
--- /dev/null 2023-06-06 14:05:49.440000000 +0000
+++ /root/ovs/tests/system-userspace-testsuite.dir/at-groups/67/stderr
2023-07-04 13:47:17.748000000 +0000
@@ -0,0 +1 @@
+ovs-ofctl: flows.txt:3: actions are invalid with specified match
(OFPBAC_MATCH_INCONSISTENT)
Please, fix that and send v3.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
