Reject stale ct_orig_tuple on forward lookups so a second NAT flow gets its own connection instead of aliasing the first reply tuple.
Signed-off-by: Eli Britstein <[email protected]> --- lib/conntrack.c | 130 +++++++++++++++++++++++++++---- tests/system-kmod-macros.at | 45 +++++++++++ tests/system-traffic.at | 6 ++ tests/system-userspace-macros.at | 9 +++ tests/test-conntrack.c | 117 ++++++++++++++++++++++++++++ 5 files changed, 293 insertions(+), 14 deletions(-) diff --git a/lib/conntrack.c b/lib/conntrack.c index 2ea70dd7d..66ece8f5c 100644 --- a/lib/conntrack.c +++ b/lib/conntrack.c @@ -867,14 +867,17 @@ pat_packet(struct dp_packet *pkt, const struct conn_key *key) static uint16_t nat_action_reverse(uint16_t nat_action) { + uint16_t rev = nat_action & (NAT_ACTION_SRC_PORT | NAT_ACTION_DST_PORT); + if (nat_action & NAT_ACTION_SRC) { - nat_action ^= NAT_ACTION_SRC; - nat_action |= NAT_ACTION_DST; - } else if (nat_action & NAT_ACTION_DST) { - nat_action ^= NAT_ACTION_DST; - nat_action |= NAT_ACTION_SRC; + rev |= NAT_ACTION_DST; + } + + if (nat_action & NAT_ACTION_DST) { + rev |= NAT_ACTION_SRC; } - return nat_action; + + return rev; } static void @@ -885,7 +888,9 @@ nat_packet_ipv4(struct dp_packet *pkt, const struct conn_key *key, if (nat_action & NAT_ACTION_SRC) { packet_set_ipv4_addr(pkt, &nh->ip_src, key->dst.addr.ipv4); - } else if (nat_action & NAT_ACTION_DST) { + } + + if (nat_action & NAT_ACTION_DST) { packet_set_ipv4_addr(pkt, &nh->ip_dst, key->src.addr.ipv4); } } @@ -899,7 +904,9 @@ nat_packet_ipv6(struct dp_packet *pkt, const struct conn_key *key, if (nat_action & NAT_ACTION_SRC) { packet_set_ipv6_addr(pkt, key->nw_proto, nh6->ip6_src.be32, &key->dst.addr.ipv6, true); - } else if (nat_action & NAT_ACTION_DST) { + } + + if (nat_action & NAT_ACTION_DST) { packet_set_ipv6_addr(pkt, key->nw_proto, nh6->ip6_dst.be32, &key->src.addr.ipv6, true); } @@ -966,6 +973,65 @@ nat_inner_packet(struct dp_packet *pkt, struct conn_key *key, pkt->offloads = orig_offloads; } +static void +nat_packet_fix_checksum(struct dp_packet *pkt, ovs_be16 dl_type) +{ + size_t l4_size; + + if (!dp_packet_l3(pkt) || dp_packet_l4_checksum_good(pkt)) { + return; + } + + l4_size = dp_packet_l4_size(pkt); + if (dl_type == htons(ETH_TYPE_IP)) { + struct ip_header *nh = dp_packet_l3(pkt); + + if (nh->ip_proto == IPPROTO_TCP && l4_size >= TCP_HEADER_LEN) { + uint32_t tcp_csum = packet_csum_pseudoheader(nh); + struct tcp_header *th = dp_packet_l4(pkt); + + th->tcp_csum = 0; + th->tcp_csum = csum_finish(csum_continue(tcp_csum, th, l4_size)); + dp_packet_l4_checksum_set_good(pkt); + } else if (nh->ip_proto == IPPROTO_UDP && l4_size >= UDP_HEADER_LEN) { + struct udp_header *uh = dp_packet_l4(pkt); + + if (uh->udp_csum) { + uh->udp_csum = 0; + uh->udp_csum = csum_finish(csum_continue( + packet_csum_pseudoheader(nh), uh, l4_size)); + if (!uh->udp_csum) { + uh->udp_csum = htons(0xffff); + } + + dp_packet_l4_checksum_set_good(pkt); + } + } + } else { + struct ovs_16aligned_ip6_hdr *nh6 = dp_packet_l3(pkt); + + if (nh6->ip6_nxt == IPPROTO_TCP && l4_size >= TCP_HEADER_LEN) { + struct tcp_header *th = dp_packet_l4(pkt); + + th->tcp_csum = 0; + th->tcp_csum = packet_csum_upperlayer6(nh6, th, nh6->ip6_nxt, + l4_size); + dp_packet_l4_checksum_set_good(pkt); + } else if (nh6->ip6_nxt == IPPROTO_UDP && l4_size >= UDP_HEADER_LEN) { + struct udp_header *uh = dp_packet_l4(pkt); + + uh->udp_csum = 0; + uh->udp_csum = packet_csum_upperlayer6(nh6, uh, nh6->ip6_nxt, + l4_size); + if (!uh->udp_csum) { + uh->udp_csum = htons(0xffff); + } + + dp_packet_l4_checksum_set_good(pkt); + } + } +} + static void nat_packet(struct dp_packet *pkt, struct conn *conn, bool reply, bool related) { @@ -977,7 +1043,9 @@ nat_packet(struct dp_packet *pkt, struct conn *conn, bool reply, bool related) /* Update ct_state. */ if (nat_action & NAT_ACTION_SRC) { pkt->md.ct_state |= CS_SRC_NAT; - } else if (nat_action & NAT_ACTION_DST) { + } + + if (nat_action & NAT_ACTION_DST) { pkt->md.ct_state |= CS_DST_NAT; } @@ -995,6 +1063,8 @@ nat_packet(struct dp_packet *pkt, struct conn *conn, bool reply, bool related) } else { pat_packet(pkt, key); } + + nat_packet_fix_checksum(pkt, key->dl_type); } } @@ -1278,8 +1348,8 @@ handle_nat(struct dp_packet *pkt, struct conn *conn, static bool check_orig_tuple(struct conntrack *ct, struct dp_packet *pkt, - struct conn_lookup_ctx *ctx_in, long long now, - struct conn **conn, + struct conn_lookup_ctx *ctx_in, uint16_t zone, + long long now, struct conn **conn, const struct nat_action_info_t *nat_action_info) { if (!(pkt->md.ct_state & (CS_SRC_NAT | CS_DST_NAT)) || @@ -1327,8 +1397,27 @@ check_orig_tuple(struct conntrack *ct, struct dp_packet *pkt, } key.dl_type = ctx_in->key.dl_type; - key.zone = pkt->md.ct_zone; + key.zone = zone; + + /* Orig-tuple lookup is only valid for reply packets: the wire tuple + * must be the reverse of the pre-NAT forward tuple. Without this, + * stale ct_orig_tuple metadata can match the wrong connection when a + * new forward flow reuses overlapping state across zones. */ + if (key.nw_proto == IPPROTO_UDP || key.nw_proto == IPPROTO_TCP + || key.nw_proto == IPPROTO_SCTP) { + if (ctx_in->key.src.port != key.dst.port + || ctx_in->key.dst.port != key.src.port) { + return false; + } + } + conn_lookup(ct, &key, now, conn, NULL); + if (*conn + && conn_key_cmp(&(*conn)->key_node[CT_DIR_REV].key, &ctx_in->key)) { + *conn = NULL; + return false; + } + return *conn ? true : false; } @@ -1481,8 +1570,21 @@ process_one(struct conntrack *ct, struct dp_packet *pkt, nat_action_info); } - } else if (check_orig_tuple(ct, pkt, ctx, now, &conn, nat_action_info)) { - create_new_conn = conn_update_state(ct, pkt, ctx, conn, now); + } else if (check_orig_tuple(ct, pkt, ctx, zone, now, &conn, + nat_action_info)) { + /* Matched the orig (pre-NAT forward) tuple: this is a reply. */ + ctx->reply = true; + if (OVS_LIKELY(!conn_update_state_alg(ct, pkt, ctx, conn, + nat_action_info, + ct_alg_ctl, now, + &create_new_conn))) { + create_new_conn = conn_update_state(ct, pkt, ctx, conn, now); + } + + if (nat_action_info && !create_new_conn) { + handle_nat(pkt, conn, zone, ctx->reply, ctx->icmp_related, + nat_action_info); + } } else { if (ctx->icmp_related) { /* An icmp related conn should always be found; no new diff --git a/tests/system-kmod-macros.at b/tests/system-kmod-macros.at index 75ef7642d..1756db3b3 100644 --- a/tests/system-kmod-macros.at +++ b/tests/system-kmod-macros.at @@ -279,6 +279,51 @@ m4_define([CHECK_NO_DPDK_OFFLOAD]) # The kernel module tests do not use TC offload. m4_define([CHECK_NO_TC_OFFLOAD]) +# OVS_CONNTRACK_ORIG_TUPLE_STALE_FORWARD() +# +# Kernel datapath smoke test: two distinct UDP flows in zone 1 must create +# two conntrack entries through the OpenFlow NAT commit path. +m4_define([OVS_CONNTRACK_ORIG_TUPLE_STALE_FORWARD], +[ +OVS_TRAFFIC_VSWITCHD_START() + +ADD_NAMESPACES(at_ns0) +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24") + +AT_DATA([flows.txt], [dnl +table=0,priority=100,in_port=1,udp,actions=ct(zone=1,table=1,nat) +table=1,cookie=0x1,priority=200,udp,tp_src=45112,ct_state=+new+trk,actions=ct(commit,zone=1,nat(src),table=2) +table=1,cookie=0x2,priority=200,udp,tp_src=37319,ct_state=+new+trk,actions=ct(commit,zone=1,nat(src),table=2) +table=1,priority=0,actions=drop +table=2,priority=0,actions=drop +]) + +AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt]) +AT_CHECK([ovs-appctl dpctl/flush-conntrack]) + +flow_l3="eth_src=50:54:00:00:00:09,eth_dst=50:54:00:00:00:0a,dl_type=0x0800,nw_src=10.1.1.1,nw_dst=8.8.0.10,nw_proto=17,nw_ttl=64,nw_frag=no" + +AT_CHECK([first_pkt=$(ovs-ofctl compose-packet --bare "$flow_l3, udp_src=45112,udp_dst=53"); dnl + ovs-ofctl -O OpenFlow13 packet-out br0 dnl + "in_port=1,packet=${first_pkt},actions=resubmit(,0)"]) + +OVS_WAIT_UNTIL([ovs-appctl dpctl/dump-conntrack zone=1 | grep -q "sport=45112,dport=53"]) + +AT_CHECK([second_pkt=$(ovs-ofctl compose-packet --bare "$flow_l3, udp_src=37319,udp_dst=53"); dnl + ovs-ofctl -O OpenFlow13 packet-out br0 dnl + "in_port=1,packet=${second_pkt},actions=resubmit(,0)"]) + +OVS_WAIT_UNTIL([ovs-appctl dpctl/dump-conntrack zone=1 | grep -q "sport=37319,dport=53"]) + +AT_CHECK([ovs-appctl dpctl/dump-conntrack zone=1 | grep "sport=37319,dport=53"], [0], [dnl +udp,orig=(src=10.1.1.1,dst=8.8.0.10,sport=37319,dport=53),reply=(src=8.8.0.10,dst=10.1.1.1,sport=53,dport=37319),zone=1 +]) + +AT_CHECK([sh -c 'test $(ovs-appctl dpctl/dump-conntrack zone=1 | grep -c "orig=.src=10\.1\.1\.1,dst=8\.8\.0\.10,") -eq 2']) + +OVS_TRAFFIC_VSWITCHD_STOP +]) + # OVS_CHECK_BAREUDP() # # The feature needs to be enabled in the kernel configuration (CONFIG_BAREUDP) diff --git a/tests/system-traffic.at b/tests/system-traffic.at index b34da49f9..6b1b13f6f 100644 --- a/tests/system-traffic.at +++ b/tests/system-traffic.at @@ -4848,6 +4848,12 @@ OVS_TRAFFIC_VSWITCHD_STOP(["dnl /execute ct.*Invalid argument/d"]) AT_CLEANUP +AT_SETUP([conntrack - orig tuple rejects stale forward query]) +CHECK_CONNTRACK() +CHECK_CONNTRACK_NAT() +OVS_CONNTRACK_ORIG_TUPLE_STALE_FORWARD() +AT_CLEANUP + AT_SETUP([conntrack - generic IP protocol]) CHECK_CONNTRACK() OVS_TRAFFIC_VSWITCHD_START() diff --git a/tests/system-userspace-macros.at b/tests/system-userspace-macros.at index f0d9121e3..e9ef86e82 100644 --- a/tests/system-userspace-macros.at +++ b/tests/system-userspace-macros.at @@ -384,6 +384,15 @@ m4_define([CHECK_EXTERNAL_CT], AT_SKIP_IF([:]) ]) +# OVS_CONNTRACK_ORIG_TUPLE_STALE_FORWARD() +# +# Userspace regression for stale ct_orig_tuple on a forward query. The +# kernel datapath uses a separate smoke test in system-kmod-macros.at. +m4_define([OVS_CONNTRACK_ORIG_TUPLE_STALE_FORWARD], +[ + AT_CHECK([ovstest test-conntrack orig-tuple-rejects-stale-forward]) +]) + # ADD_EXTERNAL_CT() # # The userspace datapath does not support external ct. diff --git a/tests/test-conntrack.c b/tests/test-conntrack.c index 2babe989c..b080321bc 100644 --- a/tests/test-conntrack.c +++ b/tests/test-conntrack.c @@ -17,6 +17,12 @@ #include <config.h> #include "conntrack.h" +#include "packets.h" + +#include "ct-dpif.h" + +#include <arpa/inet.h> + #include "dp-packet.h" #include "fatal-signal.h" #include "flow.h" @@ -148,6 +154,72 @@ build_tcp_packet(struct dp_packet *pkt, uint16_t tcp_src, uint16_t tcp_dst, return pkt; } +static struct dp_packet * +build_udp_packet(struct dp_packet *pkt, uint16_t udp_src, uint16_t udp_dst, + const char *udp_payload, size_t payload_len) +{ + struct udp_header *udph; + struct ip_header *iph; + uint16_t ip_tot_len; + uint32_t udp_csum; + struct flow flow; + + ovs_assert(pkt); + udph = dp_packet_l4(pkt); + ovs_assert(udph); + + udph->udp_src = htons(udp_src); + udph->udp_dst = htons(udp_dst); + udph->udp_len = htons(UDP_HEADER_LEN + payload_len); + udph->udp_csum = 0; + + if (udp_payload && payload_len > 0) { + memcpy((char *) udph + UDP_HEADER_LEN, udp_payload, payload_len); + } + + iph = dp_packet_l3(pkt); + ip_tot_len = IP_HEADER_LEN + UDP_HEADER_LEN + payload_len; + iph->ip_tot_len = htons(ip_tot_len); + iph->ip_csum = 0; + iph->ip_csum = csum(iph, IP_HEADER_LEN); + + udp_csum = packet_csum_pseudoheader(iph); + udph->udp_csum = csum_finish( + csum_continue(udp_csum, udph, UDP_HEADER_LEN + payload_len)); + + flow_extract(pkt, &flow); + return pkt; +} + +static void +set_ct_orig_tuple_ipv4(struct dp_packet *pkt, ovs_be32 src, ovs_be32 dst, + uint16_t src_port, uint16_t dst_port, uint8_t proto) +{ + pkt->md.ct_orig_tuple_ipv6 = false; + pkt->md.ct_orig_tuple.ipv4 = (struct ovs_key_ct_tuple_ipv4) { + src, dst, htons(src_port), htons(dst_port), proto, + }; +} + +static unsigned int +ct_zone_conn_count(struct conntrack *tracker, uint16_t zone) +{ + struct conntrack_dump dump; + struct ct_dpif_entry entry; + unsigned int count = 0; + int tot_bkts; + + conntrack_dump_start(tracker, &dump, &zone, &tot_bkts); + + while (conntrack_dump_next(&dump, &entry) != EOF) { + count++; + ct_dpif_entry_uninit(&entry); + } + + conntrack_dump_done(&dump); + return count; +} + static struct dp_packet_batch * prepare_packets(size_t n, bool change, unsigned tid, ovs_be16 *dl_type) { @@ -576,6 +648,49 @@ test_ftp_alg_large_payload(struct ovs_cmdl_context *ctx OVS_UNUSED) conntrack_destroy(ct); } +static void +test_orig_tuple_rejects_stale_forward(struct ovs_cmdl_context *ctx OVS_UNUSED) +{ + struct eth_addr eth_src = ETH_ADDR_C(50, 54, 00, 00, 00, 09); + struct eth_addr eth_dst = ETH_ADDR_C(50, 54, 00, 00, 00, 0a); + ovs_be32 ip_src = inet_addr("12.12.12.11"); + ovs_be32 ip_dst = inet_addr("8.8.0.10"); + struct nat_action_info_t src_only; + struct dp_packet_batch batch; + long long now = time_msec(); + struct dp_packet *pkt; + + ct = conntrack_init(); + + memset(&src_only, 0, sizeof src_only); + src_only.nat_action = NAT_ACTION_SRC; + + pkt = build_eth_ip_packet(NULL, eth_src, eth_dst, ip_src, ip_dst, + IPPROTO_UDP, 0); + build_udp_packet(pkt, 45112, 53, NULL, 0); + + dp_packet_batch_init_packet(&batch, pkt); + conntrack_execute(ct, &batch, htons(ETH_TYPE_IP), false, true, 1, + NULL, NULL, NULL, &src_only, now, 0); + dp_packet_delete_batch(&batch, false); + + ovs_assert(ct_zone_conn_count(ct, 1) == 1); + + build_udp_packet(pkt, 37319, 53, NULL, 0); + pkt->md.ct_state = CS_SRC_NAT; + pkt->md.ct_zone = 1; + set_ct_orig_tuple_ipv4(pkt, ip_src, ip_dst, 45112, 53, IPPROTO_UDP); + + dp_packet_batch_init_packet(&batch, pkt); + conntrack_execute(ct, &batch, htons(ETH_TYPE_IP), false, true, 1, + NULL, NULL, NULL, &src_only, now, 0); + ovs_assert(pkt->md.ct_state & CS_NEW); + ovs_assert(!(pkt->md.ct_state & CS_REPLY_DIR)); + dp_packet_delete_batch(&batch, true); + + ovs_assert(ct_zone_conn_count(ct, 1) == 2); + conntrack_destroy(ct); +} static const struct ovs_cmdl_command commands[] = { /* Connection tracker tests. */ @@ -601,6 +716,8 @@ static const struct ovs_cmdl_command commands[] = { * is rewritten to the SNAT target rather than causing a crash. */ {"ftp-alg-large-payload", "", 0, 0, test_ftp_alg_large_payload, OVS_RO}, + {"orig-tuple-rejects-stale-forward", "", 0, 0, + test_orig_tuple_rejects_stale_forward, OVS_RO}, {NULL, NULL, 0, 0, NULL, OVS_RO}, }; -- 2.43.0 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
