[ovs-dev] [PATCH v2] ofproto-dpif-trace: Improve NAT tracing.

Dumitru Ceara dceara at redhat.com
Wed Jun 17 07:21:57 UTC 2020


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 <dceara at redhat.com>
> 
> 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



More information about the dev mailing list