| Message ID | 1483669181-15311-1-git-send-email-jarno@ovn.org |
|---|---|
| State | Changes Requested |
| Headers | show |
On Thu, Jan 05, 2017 at 06:19:41PM -0800, Jarno Rajahalme wrote: > 'ctx->was_mpls' is used to flag when an MPLS packet has been popped to > a non-MPLS packet, but it was not set when the MPLS POP is implicit > due to the 'ctx->xin->flow' being restored after a patch port > traversal to group bucket execution. > > If MPLS push on a non-MPLS packet is followed by an MPLS POP in the > same actions list, a recirculation after the MPLS POP would not be > necessary, as the parsed L3/4 fields are still available. Even so, > the current action validation and execution code in the Linux kernel > datapath implementation would need to be enhanced to support this > case. For now we trigger recirculation instead. > > Reported-by: Thomas Morin <thomas.morin@orange.com> > Suggested-by: Takashi YAMAMOTO <yamamoto@ovn.org> > Fixes: 742c0ac3c0ab ("mpls: Fix MPLS restoration after patch port and group bucket.") > Signed-off-by: Jarno Rajahalme <jarno@ovn.org> This is complicated. This is a bug fix, I guess. Is this anything we can test? Should this be implemented as a helper function? Acked-by: Ben Pfaff <blp@ovn.org>
> On Jan 6, 2017, at 9:06 AM, Ben Pfaff <blp@ovn.org> wrote: > > On Thu, Jan 05, 2017 at 06:19:41PM -0800, Jarno Rajahalme wrote: >> 'ctx->was_mpls' is used to flag when an MPLS packet has been popped to >> a non-MPLS packet, but it was not set when the MPLS POP is implicit >> due to the 'ctx->xin->flow' being restored after a patch port >> traversal to group bucket execution. >> >> If MPLS push on a non-MPLS packet is followed by an MPLS POP in the >> same actions list, a recirculation after the MPLS POP would not be >> necessary, as the parsed L3/4 fields are still available. Even so, >> the current action validation and execution code in the Linux kernel >> datapath implementation would need to be enhanced to support this >> case. For now we trigger recirculation instead. >> >> Reported-by: Thomas Morin <thomas.morin@orange.com> >> Suggested-by: Takashi YAMAMOTO <yamamoto@ovn.org> >> Fixes: 742c0ac3c0ab ("mpls: Fix MPLS restoration after patch port and group bucket.") >> Signed-off-by: Jarno Rajahalme <jarno@ovn.org> > > This is complicated. > > This is a bug fix, I guess. Is this anything we can test? > Testing was there, but surprisingly did not need a change, so the patch did not have the desired effect. Sorry for the churn, will send a new version once the test cases actually work as they should. Jarno > Should this be implemented as a helper function? > Will do.
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 371ced4..27b64d9 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -3053,9 +3053,15 @@ compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port, * metadata table. */ ctx->wc->masks.tunnel = old_flow_tnl_wc; - /* The peer bridge popping MPLS should have no effect on the original - * bridge. */ - ctx->was_mpls = old_was_mpls; + /* An MPLS packet can be implicitly popped back to a non-MPLS packet + * when the flow key is restored, if the patch port peer pushed MPLS to + * a non-MPLS packet. In this case the now restored flow key is of + * non-MPLS type, while the base flow is of an MPLS type. Set the + * 'was_mpls' flag also in that case. */ + ctx->was_mpls = old_was_mpls || + (eth_type_mpls(ctx->base_flow.dl_type) + && !eth_type_mpls(ctx->xin->flow.dl_type) + && ctx->xbridge->support.odp.recirc); /* The peer bridge's conntrack execution should have no effect on the * original bridge. */ @@ -3379,9 +3385,15 @@ xlate_group_bucket(struct xlate_ctx *ctx, struct ofputil_bucket *bucket) * group buckets. */ ctx->xin->flow = old_flow; - /* The group bucket popping MPLS should have no effect after bucket - * execution. */ - ctx->was_mpls = old_was_mpls; + /* An MPLS packet can be implicitly popped back to a non-MPLS packet + * when the flow key is restored, if the group bucket pushed MPLS to + * a non-MPLS packet. In this case the now restored flow key is of + * non-MPLS type, while the base flow is of an MPLS type. Set the + * 'was_mpls' flag also in that case. */ + ctx->was_mpls = old_was_mpls || + (eth_type_mpls(ctx->base_flow.dl_type) + && !eth_type_mpls(ctx->xin->flow.dl_type) + && ctx->xbridge->support.odp.recirc); /* The fact that the group bucket exits (for any reason) does not mean that * the translation after the group action should exit. Specifically, if
'ctx->was_mpls' is used to flag when an MPLS packet has been popped to a non-MPLS packet, but it was not set when the MPLS POP is implicit due to the 'ctx->xin->flow' being restored after a patch port traversal to group bucket execution. If MPLS push on a non-MPLS packet is followed by an MPLS POP in the same actions list, a recirculation after the MPLS POP would not be necessary, as the parsed L3/4 fields are still available. Even so, the current action validation and execution code in the Linux kernel datapath implementation would need to be enhanced to support this case. For now we trigger recirculation instead. Reported-by: Thomas Morin <thomas.morin@orange.com> Suggested-by: Takashi YAMAMOTO <yamamoto@ovn.org> Fixes: 742c0ac3c0ab ("mpls: Fix MPLS restoration after patch port and group bucket.") Signed-off-by: Jarno Rajahalme <jarno@ovn.org> --- ofproto/ofproto-dpif-xlate.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-)