On 6/17/20 12:07 AM, Ben Pfaff wrote: > On Fri, Jan 10, 2020 at 10:34:43AM +0100, Dumitru Ceara wrote: >> When ofproto/trace detects a recirc action it resumes execution at the >> specified next table. However, if the ct action performs SNAT/DNAT, >> e.g., ct(commit,nat(src=1.1.1.1:4000),table=42), the src/dst IPs and >> ports in the oftrace_recirc_node->flow field are not updated. This leads >> to misleading outputs from ofproto/trace as real packets would actually >> first get NATed and might match different flows when recirculated. >> >> Assume the first IP/port from the NAT src/dst action will be used by >> conntrack for the translation and update the oftrace_recirc_node->flow >> accordingly. This is not entirely correct as conntrack might choose a >> different IP/port but the result is more realistic than before. >> >> This fix covers new connections. However, for reply traffic that executes >> actions of the form ct(nat, table=42) we still don't update the flow as >> we don't have any information about conntrack state when tracing. >> >> Also move the oftrace_recirc_node processing out of ofproto_trace() >> and to its own function, ofproto_trace_recirc_node() for better >> readability/ >> >> Signed-off-by: Dumitru Ceara <[email protected]> > > There are ways to make this better, but it's a good improvement already! > > I applied this to master. > > I folded in the following minor changes: > > diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c > index 5f1a05c3a6cc..78a54c715dc7 100644 > --- a/ofproto/ofproto-dpif-trace.c > +++ b/ofproto/ofproto-dpif-trace.c > @@ -693,15 +693,14 @@ ofproto_trace_recirc_node(struct oftrace_recirc_node > *node, > const struct ofpact_nat *ofn = node->nat_act; > > ds_put_cstr(output, "Replacing src/dst IP/ports to simulate NAT:\n"); > - ds_put_cstr(output, " Initial flow: "); > + ds_put_cstr(output, " Initial flow: "); > oftrace_print_ip_flow(&node->flow, ofn->range_af, output); > > if (ofn->flags & NX_NAT_F_SRC) { > if (ofn->range_af == AF_INET) { > node->flow.nw_src = ofn->range.addr.ipv4.min; > } else if (ofn->range_af == AF_INET6) { > - memcpy(&node->flow.ipv6_src, &ofn->range.addr.ipv6.min, > - sizeof node->flow.ipv6_src); > + node->flow.ipv6_src = ofn->range.addr.ipv6.min; > } > > if (ofn->range_af != AF_UNSPEC && ofn->range.proto.min) { > @@ -712,8 +711,7 @@ ofproto_trace_recirc_node(struct oftrace_recirc_node > *node, > if (ofn->range_af == AF_INET) { > node->flow.nw_dst = ofn->range.addr.ipv4.min; > } else if (ofn->range_af == AF_INET6) { > - memcpy(&node->flow.ipv6_dst, &ofn->range.addr.ipv6.min, > - sizeof node->flow.ipv6_dst); > + node->flow.ipv6_dst = ofn->range.addr.ipv6.min; > } > > if (ofn->range_af != AF_UNSPEC && ofn->range.proto.min) { > > Thanks, > > Ben. >
Thanks Ben for following up on this! Looks good to me. You mentioned there are ways to make this better, I can work on follow up patches if you already have suggestions. Thanks, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
