$subject says 'hairpin' reply lookup - what does that mean? Eli Britstein <[email protected]> writes:
> Reject stale ct_orig_tuple on forward lookups so a second NAT flow > gets its own connection instead of aliasing the first reply tuple. This does way more than the commit implies. The nat_action_reverse code allows simultaneous SRC|DST, and the checksum recompute are all part of this patch. The checksum recompute is especially invasive - it happens on every packet that we NAT, and I'm curious why it is needed. Do the address updates miss updating the checksum in some case? > 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) { These changes seem like they are similar to 5d50aa83e2c8e9 - IE: to provide support for an asymmetric case where we needed both SNAT and DNAT manipulation. But it is important that we make sure this can be fixed on both fwd and rev direction. And it needs to be documented, and part of a separate patch, I think. > 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); > + } Won't these walk the entire packet and throw away the device csum state? That seems very slow. Is there a bug in the packet_set_* code that this is hiding? > + } > + } 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; > + } > + } > + This change to ct_orig_tuple doesn't have much to do with other stuff going on here. This is a guard against a misbehavior with (afaict) stale metadata (is it even reachable from a PAT manip path that changes these details). I also think the comment is a bit confusing. And the rev dir check below is probably doing the same thing. It also isn't done the same way kernel works. Kernel uses the actual packet data, from what I remember. > 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 > + These tests only do a rejection case. Can you also show the hairpin reply case? > 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) build_packet and build_eth_ip_packet do much of this work. Is it not possible to reuse them? > +{ > + 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}, > }; _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
