| Message ID | 20201009121503.43893-1-hepeng.0320@bytedance.com |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [ovs-dev,v1] ofproto-dpif-xlate: ovs-tcpdump cannot capture incomming vxlan packets | expand |
On Fri, Oct 09, 2020 at 08:15:03PM +0800, hepeng.0320 wrote: > 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. > > Signed-off-by: hepeng.0320 <hepeng.0320@bytedance.com> Hi all, this patch appears to have gone stale - no response in the best part of three years. Accordingly, I am marking it as Changes Requested in patchwork (I'm still searching for the best status for such cases).
On Mon, Jun 26, 2023 at 10:57 PM Simon Horman <simon.horman@corigine.com> wrote: > > On Fri, Oct 09, 2020 at 08:15:03PM +0800, hepeng.0320 wrote: > > 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. > > > > Signed-off-by: hepeng.0320 <hepeng.0320@bytedance.com> > > Hi all, > > this patch appears to have gone stale - no response in the best part of > three years. > > Accordingly, I am marking it as Changes Requested in patchwork > (I'm still searching for the best status for such cases). IIRC, there is another patch which has already been merged. It solves the same problem through another approach. Although I am not sure its approach works in every situation, the problem stated in the comments should have been solved. I think you can drop the patch.
On Thu, Jun 29, 2023 at 10:06:33AM +0800, .贺鹏 wrote: > [You don't often get email from hepeng.0320@bytedance.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] > > On Mon, Jun 26, 2023 at 10:57 PM Simon Horman <simon.horman@corigine.com> wrote: > > > > On Fri, Oct 09, 2020 at 08:15:03PM +0800, hepeng.0320 wrote: > > > 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. > > > > > > Signed-off-by: hepeng.0320 <hepeng.0320@bytedance.com> > > > > Hi all, > > > > this patch appears to have gone stale - no response in the best part of > > three years. > > > > Accordingly, I am marking it as Changes Requested in patchwork > > (I'm still searching for the best status for such cases). > > IIRC, there is another patch which has already been merged. It solves > the same problem through another approach. > Although I am not sure its approach works in every situation, the > problem stated in the comments should have > been solved. > > I think you can drop the patch. Thanks, I've marked it as "Not Applicable". We can always revisit this if it turns out to be necessary.
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index e0ede2cab..169fd2761 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; @@ -4241,8 +4246,8 @@ 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, - &odp_tnl_port)) { + } 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, odp_tnl_port); @@ -7505,6 +7510,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,
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. Signed-off-by: hepeng.0320 <hepeng.0320@bytedance.com> --- ofproto/ofproto-dpif-xlate.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)