On Wed, Jun 17, 2020 at 09:21:57AM +0200, Dumitru Ceara wrote: > 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.
I've forgotten what I had in mind! I do remember discussing this before; maybe some of my suggestions from then would still be applicable. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
