[ovs-dev] [PATCH 1/2] xlate: rollback to valid known state in case of ctx->error on translation

Zoltan Balogh zoltan.balogh at ericsson.com
Mon Oct 30 11:22:49 UTC 2017


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 <zoltan.balogh at ericsson.com>
Signed-off-by: Sugesh Chandran <sugesh.chandran at intel.com>
Co-authored-by: Sugesh Chandran <sugesh.chandran at intel.com>
Tested-by: Sugesh Chandran <sugesh.chandran at intel.com>
---
 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



More information about the dev mailing list