On Mon, Mar 18, 2013 at 08:54:49AM -0700, Jesse Gross wrote:
> On Mon, Mar 18, 2013 at 12:37 AM, Simon Horman <[email protected]> wrote:
> > * Update the order in which actions are committed and thus
> > appear in the datapath such that MPLS actions appear after
> > l3 and l4 (nw and port) actions.
> >
> > In the case where an mpls_push action is present it should ensure
> > that l3 and l4 actions occur first, which seems logical as
> > once a mpls_push has occur the frame will be MPLS rather
> > than IPv4 or IPv6.
> >
> > In the case where there is an mpls_pop action is present this should
> > not make any difference as the frame will have been MPLS to start with
> > and thus not satisfy the pre-requisites for l3 or l4 actions.
> >
> > * Update commit_set_nw_action() to use the base ethertype when considering
> > eligibility to commit l3 (nw) actions. This allows l3 actions to be
> > applied so long as the frame was originally IPv4 or IPv6, even if
> > an mpls_push action will be applied and thus flow indicates the
> > frame will be MPLS.
> >
> > Signed-off-by: Simon Horman <[email protected]>
>
> I think this is the right path although I'm not sure that it fully
> handles L4 ports correctly if you push an MPLS label, output, and then
> set a port number. We should ignore the last set but I think we will
> attempt to do it anyways since we originally had an unlabeled packet.
> Maybe we should sanitize the flow a little more when we push a label.
I think this can be handled by adding some constraints in
do_xlate_actions(). Something like this:
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 0b72013..8e1e94e 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -6449,10 +6449,16 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t
ofpacts_len,
break;
case OFPACT_SET_IPV4_SRC:
+ if (ctx->flow.dl_type != htons(ETH_TYPE_IP)) {
+ goto out;
+ }
ctx->flow.nw_src = ofpact_get_SET_IPV4_SRC(a)->ipv4;
break;
case OFPACT_SET_IPV4_DST:
+ if (ctx->flow.dl_type != htons(ETH_TYPE_IP)) {
+ goto out;
+ }
ctx->flow.nw_dst = ofpact_get_SET_IPV4_DST(a)->ipv4;
break;
@@ -6465,10 +6471,16 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t
ofpacts_len,
break;
case OFPACT_SET_L4_SRC_PORT:
+ if (!is_ip_any(&ctx->flow)) {
+ goto out;
+ }
ctx->flow.tp_src = htons(ofpact_get_SET_L4_SRC_PORT(a)->port);
break;
case OFPACT_SET_L4_DST_PORT:
+ if (!is_ip_any(&ctx->flow)) {
+ goto out;
+ }
ctx->flow.tp_dst = htons(ofpact_get_SET_L4_DST_PORT(a)->port);
break;
> It would also be good to add some comments since the ordering and
> checks were essentially arbitrary before but are now important.
Sure, will do.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
