diff mbox series

[ovs-dev] ofproto-dpif-xlate: ovs-tcpdump cannot capture incomming vxlan packets

Message ID 20200522090234.35857-1-hepeng.0320@bytedance.com
State New
Headers show
Series [ovs-dev] ofproto-dpif-xlate: ovs-tcpdump cannot capture incomming vxlan packets | expand

Commit Message

hepeng.0320 May 22, 2020, 9:02 a.m. UTC
when running ovs-tcpdump -i ethX and the port is used as the incomming port for a vxlan port.

The callstack for the upcall:

mirror_ingress_packet
mirror_packet
output_normal
compose_output_action
compose_output_action__
terminate_native_tunnel

will xlate the action into a tnl_pop action, not an output action to the
mirror port. So eventually the translated actions will be 'tnl_pop(x), tnl_pop(x)'.
However, the right action should be '(mirror port), tnl_pop(x)'

This patch adds a flag in xlate_ctx indicating the current output_normal
is used by mirroring. Note that we cannot use ctx->mirrors as the
indicator as in the mirror code, the ctx->mirrors will not be cleared
after mirror action finished.
---
 ofproto/ofproto-dpif-xlate.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

hepeng.0320 May 22, 2020, 9:11 a.m. UTC | #1
ignore this patch, forget to sign-off the patch.

hepeng.0320 <xnhp0320@gmail.com> 于2020年5月22日周五 下午5:02写道:
>
> when running ovs-tcpdump -i ethX and the port is used as the incomming port for a vxlan port.
>
> The callstack for the upcall:
>
> mirror_ingress_packet
> mirror_packet
> output_normal
> compose_output_action
> compose_output_action__
> terminate_native_tunnel
>
> will xlate the action into a tnl_pop action, not an output action to the
> mirror port. So eventually the translated actions will be 'tnl_pop(x), tnl_pop(x)'.
> However, the right action should be '(mirror port), tnl_pop(x)'
>
> This patch adds a flag in xlate_ctx indicating the current output_normal
> is used by mirroring. Note that we cannot use ctx->mirrors as the
> indicator as in the mirror code, the ctx->mirrors will not be cleared
> after mirror action finished.
> ---
>  ofproto/ofproto-dpif-xlate.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 80fba84cb..03f370a0a 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -269,6 +269,7 @@ struct xlate_ctx {
>      bool exit;                  /* No further actions should be processed. */
>      mirror_mask_t mirrors;      /* Bitmap of associated mirrors. */
>      int mirror_snaplen;         /* Max size of a mirror packet in byte. */
> +    bool in_mirror_output;
>
>     /* Freezing Translation
>      * ====================
> @@ -2154,7 +2155,9 @@ mirror_packet(struct xlate_ctx *ctx, struct xbundle *xbundle,
>          if (out) {
>              struct xbundle *out_xbundle = xbundle_lookup(ctx->xcfg, out);
>              if (out_xbundle) {
> +                ctx->in_mirror_output = true;
>                  output_normal(ctx, out_xbundle, &xvlan);
> +                ctx->in_mirror_output = false;
>              }
>          } else if (xvlan.v[0].vid != out_vlan
>                     && !eth_addr_is_reserved(ctx->xin->flow.dl_dst)) {
> @@ -2165,7 +2168,9 @@ mirror_packet(struct xlate_ctx *ctx, struct xbundle *xbundle,
>              LIST_FOR_EACH (xb, list_node, &xbridge->xbundles) {
>                  if (xbundle_includes_vlan(xb, &xvlan)
>                      && !xbundle_mirror_out(xbridge, xb)) {
> +                    ctx->in_mirror_output = true;
>                      output_normal(ctx, xb, &xvlan);
> +                    ctx->in_mirror_output = false;
>                  }
>              }
>              xvlan.v[0].vid = old_vid;
> @@ -4231,7 +4236,7 @@ compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port,
>              native_tunnel_output(ctx, xport, flow, odp_port, truncate);
>              flow->tunnel = flow_tnl; /* Restore tunnel metadata */
>
> -        } else if (terminate_native_tunnel(ctx, flow, wc,
> +        } else if (!ctx->in_mirror_output && terminate_native_tunnel(ctx, flow, wc,
>                                             &odp_tnl_port)) {
>              /* Intercept packet to be received on native tunnel port. */
>              nl_msg_put_odp_port(ctx->odp_actions, OVS_ACTION_ATTR_TUNNEL_POP,
> @@ -7492,6 +7497,7 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
>          .exit = false,
>          .error = XLATE_OK,
>          .mirrors = 0,
> +        .in_mirror_output = false,
>
>          .freezing = false,
>          .recirc_update_dp_hash = false,
> --
> 2.20.1
>
0-day Robot May 22, 2020, 10:02 a.m. UTC | #2
Bleep bloop.  Greetings hepeng.0320, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
ERROR: Author hepeng.0320 <xnhp0320@gmail.com> needs to sign off.
WARNING: Line is 83 characters long (recommended limit is 79)
#67 FILE: ofproto/ofproto-dpif-xlate.c:4239:
        } else if (!ctx->in_mirror_output && terminate_native_tunnel(ctx, flow, wc,

Lines checked: 81, Warnings: 1, Errors: 1


Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Thanks,
0-day Robot
diff mbox series

Patch

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 80fba84cb..03f370a0a 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -269,6 +269,7 @@  struct xlate_ctx {
     bool exit;                  /* No further actions should be processed. */
     mirror_mask_t mirrors;      /* Bitmap of associated mirrors. */
     int mirror_snaplen;         /* Max size of a mirror packet in byte. */
+    bool in_mirror_output;
 
    /* Freezing Translation
     * ====================
@@ -2154,7 +2155,9 @@  mirror_packet(struct xlate_ctx *ctx, struct xbundle *xbundle,
         if (out) {
             struct xbundle *out_xbundle = xbundle_lookup(ctx->xcfg, out);
             if (out_xbundle) {
+                ctx->in_mirror_output = true;
                 output_normal(ctx, out_xbundle, &xvlan);
+                ctx->in_mirror_output = false;
             }
         } else if (xvlan.v[0].vid != out_vlan
                    && !eth_addr_is_reserved(ctx->xin->flow.dl_dst)) {
@@ -2165,7 +2168,9 @@  mirror_packet(struct xlate_ctx *ctx, struct xbundle *xbundle,
             LIST_FOR_EACH (xb, list_node, &xbridge->xbundles) {
                 if (xbundle_includes_vlan(xb, &xvlan)
                     && !xbundle_mirror_out(xbridge, xb)) {
+                    ctx->in_mirror_output = true;
                     output_normal(ctx, xb, &xvlan);
+                    ctx->in_mirror_output = false;
                 }
             }
             xvlan.v[0].vid = old_vid;
@@ -4231,7 +4236,7 @@  compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port,
             native_tunnel_output(ctx, xport, flow, odp_port, truncate);
             flow->tunnel = flow_tnl; /* Restore tunnel metadata */
 
-        } else if (terminate_native_tunnel(ctx, flow, wc,
+        } else if (!ctx->in_mirror_output && terminate_native_tunnel(ctx, flow, wc,
                                            &odp_tnl_port)) {
             /* Intercept packet to be received on native tunnel port. */
             nl_msg_put_odp_port(ctx->odp_actions, OVS_ACTION_ATTR_TUNNEL_POP,
@@ -7492,6 +7497,7 @@  xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
         .exit = false,
         .error = XLATE_OK,
         .mirrors = 0,
+        .in_mirror_output = false,
 
         .freezing = false,
         .recirc_update_dp_hash = false,