During translation of OF actions on a bridge, we can store the last valid state of translated actions while iterating over the OF actions and revert to it in case of error. This can be performed in the do_xlate_actions() funtion.
Signed-off-by: Zoltan Balogh <[email protected]> Signed-off-by: Sugesh Chandran <[email protected]> Co-authored-by: Sugesh Chandran <[email protected]> Tested-by: Sugesh Chandran <[email protected]> --- ofproto/ofproto-dpif-xlate.c | 119 ++++++++++++++++++++++++++++ tests/ofproto-dpif.at | 179 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 298 insertions(+) diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index ddcaf05..0cc59e7 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -6147,6 +6147,107 @@ xlate_ofpact_unroll_xlate(struct xlate_ctx *ctx, "cookie=%#"PRIx64, a->rule_table_id, a->rule_cookie); } +struct xlate_backup_data { + size_t odp_actions_size; + struct flow wc_masks; + struct flow flow; + struct flow base_flow; +}; + +static void +store_ctx_xlate_data(struct xlate_backup_data *data, + const struct xlate_ctx *ctx) +{ + data->odp_actions_size = ctx->odp_actions->size; + data->wc_masks = ctx->wc->masks; + data->flow = ctx->xin->flow; + data->base_flow = ctx->base_flow; +} + +static void +restore_ctx_xlate_data(struct xlate_ctx *ctx, + const struct xlate_backup_data *data) +{ + ctx->odp_actions->size = data->odp_actions_size; + ctx->wc->masks = data->wc_masks; + ctx->xin->flow = data->flow; + ctx->base_flow = data->base_flow; +} + +static bool +ofpact_validates_dp_actions(const struct ofpact *a) +{ + switch (a->type) { + /* Output. */ + case OFPACT_OUTPUT: + case OFPACT_GROUP: + case OFPACT_CONTROLLER: + case OFPACT_ENQUEUE: + case OFPACT_OUTPUT_REG: + case OFPACT_BUNDLE: + /* Metadata. */ + case OFPACT_SET_TUNNEL: + case OFPACT_SET_QUEUE: + case OFPACT_POP_QUEUE: + case OFPACT_FIN_TIMEOUT: + /* Flow table interaction. */ + case OFPACT_RESUBMIT: + case OFPACT_LEARN: + case OFPACT_CONJUNCTION: + /* Arithmetic. */ + case OFPACT_MULTIPATH: + /* Other. */ + case OFPACT_NOTE: + case OFPACT_EXIT: + case OFPACT_SAMPLE: + case OFPACT_UNROLL_XLATE: + case OFPACT_CT: + case OFPACT_CT_CLEAR: + case OFPACT_NAT: + case OFPACT_OUTPUT_TRUNC: + case OFPACT_CLONE: + /* Instructions. */ + case OFPACT_METER: + case OFPACT_CLEAR_ACTIONS: + case OFPACT_WRITE_ACTIONS: + case OFPACT_WRITE_METADATA: + case OFPACT_GOTO_TABLE: + return true; + /* Header changes. */ + case OFPACT_SET_FIELD: + case OFPACT_SET_VLAN_VID: + case OFPACT_SET_VLAN_PCP: + case OFPACT_STRIP_VLAN: + case OFPACT_PUSH_VLAN: + case OFPACT_SET_ETH_SRC: + case OFPACT_SET_ETH_DST: + case OFPACT_SET_IPV4_SRC: + case OFPACT_SET_IPV4_DST: + case OFPACT_SET_IP_DSCP: + case OFPACT_SET_IP_ECN: + case OFPACT_SET_IP_TTL: + case OFPACT_SET_L4_SRC_PORT: + case OFPACT_SET_L4_DST_PORT: + case OFPACT_REG_MOVE: + case OFPACT_STACK_PUSH: + case OFPACT_STACK_POP: + case OFPACT_DEC_TTL: + case OFPACT_SET_MPLS_LABEL: + case OFPACT_SET_MPLS_TC: + case OFPACT_SET_MPLS_TTL: + case OFPACT_DEC_MPLS_TTL: + case OFPACT_PUSH_MPLS: + case OFPACT_POP_MPLS: + /* Generic encap & decap */ + case OFPACT_ENCAP: + case OFPACT_DECAP: + /* Debugging actions. */ + case OFPACT_DEBUG_RECIRC: + default: + return false; + } +} + static void do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len, struct xlate_ctx *ctx, bool is_last_action) @@ -6154,6 +6255,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len, struct flow_wildcards *wc = ctx->wc; struct flow *flow = &ctx->xin->flow; const struct ofpact *a; + struct xlate_backup_data xlate_data; if (ovs_native_tunneling_is_on(ctx->xbridge->ofproto)) { tnl_neigh_snoop(flow, wc, ctx->xbridge->name); @@ -6165,6 +6267,9 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len, return; } + /* Initialize xlate_data. */ + store_ctx_xlate_data(&xlate_data, ctx); + OFPACT_FOR_EACH (a, ofpacts, ofpacts_len) { struct ofpact_controller *controller; const struct ofpact_metadata *metadata; @@ -6172,8 +6277,11 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len, const struct mf_field *mf; bool last = is_last_action && ofpact_last(a, ofpacts, ofpacts_len) && ctx->action_set.size; + size_t old_size; if (ctx->error) { + /* Restore xlate data. */ + restore_ctx_xlate_data(ctx, &xlate_data); break; } @@ -6196,6 +6304,8 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len, ds_destroy(&s); } + old_size = ctx->odp_actions->size; + switch (a->type) { case OFPACT_OUTPUT: xlate_output_action(ctx, ofpact_get_OUTPUT(a)->port, @@ -6518,6 +6628,9 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len, case OFPACT_CLONE: compose_clone(ctx, ofpact_get_CLONE(a), last); + if (!ctx->error) { + store_ctx_xlate_data(&xlate_data, ctx); + } break; case OFPACT_ENCAP: @@ -6555,6 +6668,12 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len, break; } + /* Store xlate data. */ + if (ofpact_validates_dp_actions(a) && !ctx->error + && old_size != ctx->odp_actions->size) { + store_ctx_xlate_data(&xlate_data, ctx); + } + /* Check if need to store this and the remaining actions for later * execution. */ if (!ctx->error && ctx->exit && ctx_first_frozen_action(ctx)) { diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at index c75a1ac..1124f0e 100644 --- a/tests/ofproto-dpif.at +++ b/tests/ofproto-dpif.at @@ -9954,3 +9954,182 @@ AT_CHECK([grep flow_del ovs-vswitchd.log], [1]) OVS_VSWITCHD_STOP AT_CLEANUP + +AT_SETUP([ofproto-dpif - xlate error - rollback]) + +# ->-+ +# | p1 LOCAL +# +-o-------+ +---------+ +---------+ +-------o-+ +# | br0 | | br1 | | br2 | | br3 | +# +-------o-+ +-o-----o-+ +-o-----o-+ +-o-------+ +# p01 | p10 | | p12 p21 | | p23 p32 | +# +-----------+ +-----------+ +-----------+ + +OVS_VSWITCHD_START([dnl + -- add-br br1 -- set bridge br1 datapath_type=dummy \ + -- add-br br2 -- set bridge br2 datapath_type=dummy \ + -- add-br br3 -- set bridge br3 datapath_type=dummy \ + -- add-port br0 p01 \ + -- set interface p01 type=patch options:peer=p10 ofport_request=10 \ + -- add-port br1 p10 \ + -- set interface p10 type=patch options:peer=p01 ofport_request=20 \ + -- add-port br1 p12 \ + -- set interface p12 type=patch options:peer=p21 ofport_request=30 \ + -- add-port br2 p21 \ + -- set interface p21 type=patch options:peer=p12 ofport_request=40 \ + -- add-port br2 p23 \ + -- set interface p23 type=patch options:peer=p32 ofport_request=50 \ + -- add-port br3 p32 \ + -- set interface p32 type=patch options:peer=p23 ofport_request=60 +]) + +AT_CHECK([ + ovs-vsctl add-port br0 p1 -- set interface p1 type=dummy ofport_request=1 +]) + +AT_CHECK([ + ovs-appctl dpif/show | grep -v hit | sed "s/$(printf \\t)/ /g" | sed 's./[[0-9]]\{1,\}..' +], [0], [dnl + br0: + br0 65534: (dummy-internal) + p01 10/none: (patch: peer=p10) + p1 1: (dummy) + br1: + br1 65534: (dummy-internal) + p10 20/none: (patch: peer=p01) + p12 30/none: (patch: peer=p21) + br2: + br2 65534: (dummy-internal) + p21 40/none: (patch: peer=p12) + p23 50/none: (patch: peer=p32) + br3: + br3 65534: (dummy-internal) + p32 60/none: (patch: peer=p23) +]) + +AT_CHECK([ + ovs-ofctl del-flows br0 + ovs-ofctl del-flows br1 + ovs-ofctl del-flows br2 + ovs-ofctl del-flows br3 +], [0]) + +# Error due to pushing too many MPLS labels. +AT_CHECK([ + ovs-ofctl add-flow -OOpenFlow13 br0 "in_port=1,ip,actions=dec_ttl,push_mpls:0x8847,output:p01" + ovs-ofctl add-flow -OOpenFlow13 br1 "in_port=p10,mpls,actions=mod_dl_src:aa:aa:aa:bb:bb:f0,push_mpls:0x8847,output:p12" + ovs-ofctl add-flow -OOpenFlow13 br2 "in_port=p21,mpls,actions=mod_dl_dst:aa:aa:aa:00:00:01,push_mpls:0x8847,output:p23" + ovs-ofctl add-flow -OOpenFlow13 br3 "in_port=p32,mpls,actions=push_mpls:0x8847,output:LOCAL" +], [0]) + +AT_CHECK([ovs-appctl ofproto/trace br0 'in_port=1,ip,nw_ttl=255'], [0], [stdout]) +AT_CHECK([tail -1 stdout], [0], + [Datapath actions: drop +]) + +AT_CHECK([ + ovs-ofctl del-flows br0 + ovs-ofctl del-flows br1 + ovs-ofctl del-flows br2 + ovs-ofctl del-flows br3 +], [0]) + +# Error due to applying encap(Ethernet) to an Ethernet packet. +AT_CHECK([ +ovs-ofctl add-flow br0 "in_port=1,actions=dec_ttl,output:p01" +ovs-ofctl add-flow br1 in_port=p10,actions=p12 +ovs-ofctl add-flow br2 -OOpenFlow13 "in_port=p21,ip,actions=dec_ttl,dec_ttl,dec_ttl,dec_ttl,output:LOCAL,dec_ttl,mod_dl_src:aa:aa:aa:bb:bb:ff,encap(ethernet),dec_ttl,output:p23" +ovs-ofctl add-flow br3 "in_port=p32,actions=LOCAL" +], [0]) + +AT_CHECK([ovs-appctl ofproto/trace br0 'in_port=1,ip,nw_ttl=255'], [0], [stdout]) +AT_CHECK([tail -1 stdout], [0], + [Datapath actions: 102 +]) + +OVS_VSWITCHD_STOP +AT_CLEANUP + +AT_SETUP([ofproto-dpif - xlate error - rollback2]) + +# ->-+ +# | p1 LOCAL LOCAL +# +-o-------+ +-------o-+ +---------+ +-------o-+ +# | br0 | | br1 | | br2 | | br3 | +# +-------o-+ +-o----o-o+ +-o-----o-+ +-o-o-----+ +# p01 | p10 | p13| |p12 p21 | | p23 p32 | | p31 +# +-----------+ | +----------+ +-----------+ | +# +--------------------------------+ + +OVS_VSWITCHD_START([dnl + -- add-br br1 -- set bridge br1 datapath_type=dummy \ + -- add-br br2 -- set bridge br2 datapath_type=dummy \ + -- add-br br3 -- set bridge br3 datapath_type=dummy \ + -- add-port br0 p01 \ + -- set interface p01 type=patch options:peer=p10 ofport_request=10 \ + -- add-port br1 p10 \ + -- set interface p10 type=patch options:peer=p01 ofport_request=20 \ + -- add-port br1 p12 \ + -- set interface p12 type=patch options:peer=p21 ofport_request=30 \ + -- add-port br2 p21 \ + -- set interface p21 type=patch options:peer=p12 ofport_request=40 \ + -- add-port br2 p23 \ + -- set interface p23 type=patch options:peer=p32 ofport_request=50 \ + -- add-port br3 p32 \ + -- set interface p32 type=patch options:peer=p23 ofport_request=60 \ + -- add-port br1 p13 \ + -- set interface p13 type=patch options:peer=p31 ofport_request=70 \ + -- add-port br3 p31 \ + -- set interface p31 type=patch options:peer=p13 ofport_request=80 +]) + +AT_CHECK([ + ovs-vsctl add-port br0 p1 -- set interface p1 type=dummy ofport_request=1 +]) + +AT_CHECK([ + ovs-appctl dpif/show | grep -v hit | sed "s/$(printf \\t)/ /g" | sed 's./[[0-9]]\{1,\}..' +], [0], [dnl + br0: + br0 65534: (dummy-internal) + p01 10/none: (patch: peer=p10) + p1 1: (dummy) + br1: + br1 65534: (dummy-internal) + p10 20/none: (patch: peer=p01) + p12 30/none: (patch: peer=p21) + p13 70/none: (patch: peer=p31) + br2: + br2 65534: (dummy-internal) + p21 40/none: (patch: peer=p12) + p23 50/none: (patch: peer=p32) + br3: + br3 65534: (dummy-internal) + p31 80/none: (patch: peer=p13) + p32 60/none: (patch: peer=p23) +]) + + +AT_CHECK([ + ovs-ofctl del-flows br0 + ovs-ofctl del-flows br1 + ovs-ofctl del-flows br2 + ovs-ofctl del-flows br3 +], [0]) + +# Drop packet due to xlate error on one path (br0-br1-br2-b3) but forward it via another one (br0-br1-br3). +AT_CHECK([ + ovs-ofctl add-flow br0 "in_port=1,actions=dec_ttl,output:p01" + ovs-ofctl add-flow br1 in_port=p10,ip,actions=dec_ttl,dec_ttl,dec_ttl,dec_ttl,output:LOCAL,mod_dl_dst:ee:ee:ee:ff:ff:01,output:p12,p13 + ovs-ofctl add-flow br2 -OOpenFlow13 "in_port=p21,ip,actions=dec_ttl,dec_ttl,dec_ttl,dec_ttl,dec_ttl,mod_dl_src:aa:aa:aa:bb:bb:ff,encap(ethernet),dec_ttl,output:p23" + ovs-ofctl add-flow br3 "in_port=p32,actions=LOCAL" + ovs-ofctl add-flow br3 "in_port=p31,actions=LOCAL" +], [0]) + +AT_CHECK([ovs-appctl ofproto/trace br0 'in_port=1,ip,nw_ttl=255'], [0], [stdout]) +AT_CHECK([grep "Datapath actions" stdout], [0], + [Datapath actions: 101,set(eth(dst=ee:ee:ee:ff:ff:01)),103 +]) + +OVS_VSWITCHD_STOP +AT_CLEANUP -- 1.9.1 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
