[ovs-dev] [PATCH 8/8] ofproto-dpif: Do not add more MPLS labels to mask than the datapath can handle

Simon Horman horms at verge.net.au
Wed Jan 29 12:33:17 UTC 2014


On Fri, Jan 17, 2014 at 01:01:29PM -0800, Ben Pfaff wrote:
> On Fri, Jan 17, 2014 at 11:33:45AM +0900, Simon Horman wrote:
> > On Thu, Jan 16, 2014 at 05:28:53PM -0800, Ben Pfaff wrote:
> > > On Fri, Jan 17, 2014 at 09:59:49AM +0900, Simon Horman wrote:
> > > > On Thu, Jan 16, 2014 at 04:46:23PM -0800, Ben Pfaff wrote:
> > > > > On Wed, Jan 15, 2014 at 04:13:25PM +0900, Simon Horman wrote:
> > > > > > The key is sourced from the datapath so should not
> > > > > > have more labels than it can handle.
> > > > > > 
> > > > > > dpif-netdev supports as many LSEs as can fit in struct flow,
> > > > > > so it is safe to pass SIZE_MAX as the limit there.
> > > > > > 
> > > > > > This is an proposed enhancement to
> > > > > > "Implement OpenFlow support for MPLS, for up to 3 labels."
> > > > > > 
> > > > > > Signed-off-by: Simon Horman <horms at verge.net.au>
> > > > > 
> > > > > I don't think we'd ever try to install a flow that matches on more
> > > > > MPLS labels than we probed the datapath as supporting.  If we do, that
> > > > > sounds like a bug.  I think that this change would just make the bug
> > > > > harder to find, by making us suppress the error inside odp-util
> > > > > without actually fixing the bug at the higher layer.
> > > > 
> > > > The problem I was trying to address is as follows:
> > > > 
> > > > Suppose that the datapath supports one MPLS LSE and sends a that key with
> > > > one MPLS LSE which does not have the BoS bit set. I believe that the
> > > > current code would create a mask with three LSEs masked instead of one.
> > > > But perhaps this is not a problem.
> > > 
> > > I think that in that case the proper userspace behavior would be to
> > > parse the flow from the kernel as ODP_FIT_TOO_LITTLE, which means that
> > > userspace should never try to set up a flow.  Oh, except that it would
> > > set up a slow-path flow.  Ouch.  Hmm.
> > 
> > Just to clarify. I am talking about a situation where:
> > 1. The datapath supports fewer LSEs than ovs-vswitchd and;
> > 2. The packet has more LSEs than the datapath supports
> 
> My message above was concluding that we do need to do something, and I
> ran out of time at that point.  I'm going to reconsider the patch.  I
> don't really like it at first glance, so I'm going to see if I can
> think of a way I like better.

Hi Ben,

I think that the key here is that when the translation is made from
key attributes to a struct flow some information may be is lost: the number
of MPLS LSE attributes that were present in the key.

I think it is reasonable for the datapath to send fewer attributes
than ovs-vswitchd in two cases.

1) If there were fewer LSEs present in the packet than ovs-vswitchd can handle.

   I believe this already occurs when using the user-space datapath and
   I believe that your change to make odp_flow_key_to_flow__() return
   ODP_FIT_TOO_LITTLE in this case causes the test-suite to fail with
   dpif_netdev_mask_from_nlattrs() reporting
   "internal error parsing flow mask..."

   I wonder if odp_flow_key_to_flow__() should ODP_FIT_PERFECT in this case.

2) In the case where it supports fewer LSEs than ovs-vswtichd.
   This was the motivation for this patch.

   I have tried coding up an alternate approach to addressing this,
   passing a max_labels parameter to flow_count_mpls_labels();
   passing the probed maximum number of labels supported to the datapath
   to directly and indirectly via its callers; and limiting len
   in flow_count_mpls_labels() accordingly.

   The general idea is to limit the scope of setting of
   wc->masks.mpls_lse.

   This is a much more verbose change than the patch that started this
   thread and as a result I am much less comfortable about its
   correctness.  But for reference it is below.


   I think that a simpler but more hackish way to get the same effect would
   be to zero the trailing portion of wc->masks.mpls_lse at
   some point according the number of labels supported by the datapath.
   Perhaps temporarily before calling odp_flow_key_from_mask().


From: Simon Horman <horms at verge.net.au>

    Limit MPLS labels scanned by flow_count_mpls_labels
    
    Signed-off-by: Simon Horman <horms at verge.net.au>

diff --git a/lib/flow.c b/lib/flow.c
index e3d4221..33121a9 100644
--- a/lib/flow.c
+++ b/lib/flow.c
@@ -1054,14 +1054,15 @@ flow_set_vlan_pcp(struct flow *flow, uint8_t pcp)
  * the maximum number of LSEs that can be stored in 'flow' is returned.
  */
 int
-flow_count_mpls_labels(const struct flow *flow, struct flow_wildcards *wc)
+flow_count_mpls_labels(const struct flow *flow, struct flow_wildcards *wc,
+                       size_t max_labels)
 {
     if (wc) {
         wc->masks.dl_type = OVS_BE16_MAX;
     }
     if (eth_type_mpls(flow->dl_type)) {
         int i;
-        int len = ARRAY_SIZE(flow->mpls_lse);
+        int len = MIN(max_labels, ARRAY_SIZE(flow->mpls_lse));
 
         for (i = 0; i < len; i++) {
             if (wc) {
@@ -1084,10 +1085,11 @@ flow_count_mpls_labels(const struct flow *flow, struct flow_wildcards *wc)
 int
 flow_count_common_mpls_labels(const struct flow *flow_a,
                               const struct flow *flow_b,
-                              struct flow_wildcards *wc)
+                              struct flow_wildcards *wc,
+                              size_t max_labels)
 {
-    int flow_a_n = flow_count_mpls_labels(flow_a, wc);
-    int flow_b_n = flow_count_mpls_labels(flow_b, wc);
+    int flow_a_n = flow_count_mpls_labels(flow_a, wc, max_labels);
+    int flow_b_n = flow_count_mpls_labels(flow_b, wc, max_labels);
     int min_n = MIN(flow_a_n, flow_b_n);
 
     if (min_n == 0) {
@@ -1116,9 +1118,9 @@ flow_count_common_mpls_labels(const struct flow *flow_a,
 
 void
 flow_push_mpls(struct flow *flow, ovs_be16 mpls_eth_type,
-               struct flow_wildcards *wc)
+               struct flow_wildcards *wc, size_t max_labels)
 {
-    int n = flow_count_mpls_labels(flow, wc);
+    int n = flow_count_mpls_labels(flow, wc, max_labels);
 
     ovs_assert(n < ARRAY_SIZE(flow->mpls_lse));
 
@@ -1151,9 +1153,10 @@ flow_push_mpls(struct flow *flow, ovs_be16 mpls_eth_type,
 }
 
 bool
-flow_pop_mpls(struct flow *flow, ovs_be16 eth_type, struct flow_wildcards *wc)
+flow_pop_mpls(struct flow *flow, ovs_be16 eth_type, struct flow_wildcards *wc,
+              size_t max_labels)
 {
-    int n = flow_count_mpls_labels(flow, wc);
+    int n = flow_count_mpls_labels(flow, wc, max_labels);
     int i;
 
     if (n == 0) {
diff --git a/lib/flow.h b/lib/flow.h
index 69d406b..e7c10c5 100644
--- a/lib/flow.h
+++ b/lib/flow.h
@@ -195,13 +195,16 @@ void flow_set_dl_vlan(struct flow *, ovs_be16 vid);
 void flow_set_vlan_vid(struct flow *, ovs_be16 vid);
 void flow_set_vlan_pcp(struct flow *, uint8_t pcp);
 
-int flow_count_mpls_labels(const struct flow *, struct flow_wildcards *);
+int flow_count_mpls_labels(const struct flow *, struct flow_wildcards *,
+                           size_t max_labels);
 int flow_count_common_mpls_labels(const struct flow *flow_a,
                                   const struct flow *flow_b,
-                                  struct flow_wildcards *wc);
+                                  struct flow_wildcards *wc,
+                                  size_t max_labels);
 void flow_push_mpls(struct flow *, ovs_be16 mpls_eth_type,
-                    struct flow_wildcards *);
-bool flow_pop_mpls(struct flow *, ovs_be16 eth_type, struct flow_wildcards *);
+                    struct flow_wildcards *, size_t max_labels);
+bool flow_pop_mpls(struct flow *, ovs_be16 eth_type, struct flow_wildcards *,
+                   size_t max_labels);
 void flow_set_mpls_label(struct flow *, int idx, ovs_be32 label);
 void flow_set_mpls_ttl(struct flow *, int idx, uint8_t ttl);
 void flow_set_mpls_tc(struct flow *, int idx, uint8_t tc);
diff --git a/lib/odp-util.c b/lib/odp-util.c
index 520b314..c38f1be 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -2523,7 +2523,7 @@ odp_flow_key_from_flow__(struct ofpbuf *buf, const struct flow *data,
         struct ovs_key_mpls *mpls_key;
         int i, n;
 
-        n = flow_count_mpls_labels(flow, NULL);
+        n = flow_count_mpls_labels(flow, NULL, SIZE_MAX);
         mpls_key = nl_msg_put_unspec_uninit(buf, OVS_KEY_ATTR_MPLS,
                                             n * sizeof *mpls_key);
         for (i = 0; i < n; i++) {
@@ -3475,11 +3475,12 @@ commit_vlan_action(ovs_be16 vlan_tci, struct flow *base,
 
 static void
 commit_mpls_action(const struct flow *flow, struct flow *base,
-                   struct ofpbuf *odp_actions, struct flow_wildcards *wc)
+                   struct ofpbuf *odp_actions, struct flow_wildcards *wc,
+                   size_t max_labels)
 {
-    int base_n = flow_count_mpls_labels(base, wc);
-    int flow_n = flow_count_mpls_labels(flow, wc);
-    int common_n = flow_count_common_mpls_labels(flow, base, wc);
+    int base_n = flow_count_mpls_labels(base, wc, max_labels);
+    int flow_n = flow_count_mpls_labels(flow, wc, max_labels);
+    int common_n = flow_count_common_mpls_labels(flow, base, wc, max_labels);
 
     while (base_n > common_n) {
         if (base_n - 1 == common_n && flow_n > common_n) {
@@ -3516,7 +3517,7 @@ commit_mpls_action(const struct flow *flow, struct flow *base,
                 dl_type = flow->dl_type;
             }
             nl_msg_put_be16(odp_actions, OVS_ACTION_ATTR_POP_MPLS, dl_type);
-            popped = flow_pop_mpls(base, flow->dl_type, wc);
+            popped = flow_pop_mpls(base, flow->dl_type, wc, max_labels);
             ovs_assert(popped);
             base_n--;
         }
@@ -3536,7 +3537,7 @@ commit_mpls_action(const struct flow *flow, struct flow *base,
                                       sizeof *mpls);
         mpls->mpls_ethertype = flow->dl_type;
         mpls->mpls_lse = flow->mpls_lse[flow_n - base_n - 1];
-        flow_push_mpls(base, mpls->mpls_ethertype, wc);
+        flow_push_mpls(base, mpls->mpls_ethertype, wc, max_labels);
         flow_set_mpls_lse(base, 0, mpls->mpls_lse);
         base_n++;
     }
@@ -3760,14 +3761,15 @@ commit_set_pkt_mark_action(const struct flow *flow, struct flow *base,
  * slow path, if there is one, otherwise 0. */
 enum slow_path_reason
 commit_odp_actions(const struct flow *flow, struct flow *base,
-                   struct ofpbuf *odp_actions, struct flow_wildcards *wc)
+                   struct ofpbuf *odp_actions, struct flow_wildcards *wc,
+                   size_t max_mpls_labels)
 {
     enum slow_path_reason slow;
 
     commit_set_ether_addr_action(flow, base, odp_actions, wc);
     slow = commit_set_nw_action(flow, base, odp_actions, wc);
     commit_set_port_action(flow, base, odp_actions, wc);
-    commit_mpls_action(flow, base, odp_actions, wc);
+    commit_mpls_action(flow, base, odp_actions, wc, max_mpls_labels);
     commit_vlan_action(flow->vlan_tci, base, odp_actions, wc);
     commit_set_priority_action(flow, base, odp_actions, wc);
     commit_set_pkt_mark_action(flow, base, odp_actions, wc);
diff --git a/lib/odp-util.h b/lib/odp-util.h
index abb8789..60f8871 100644
--- a/lib/odp-util.h
+++ b/lib/odp-util.h
@@ -181,7 +181,8 @@ void commit_odp_tunnel_action(const struct flow *, struct flow *base,
 enum slow_path_reason commit_odp_actions(const struct flow *,
                                          struct flow *base,
                                          struct ofpbuf *odp_actions,
-                                         struct flow_wildcards *wc);
+                                         struct flow_wildcards *wc,
+                                         size_t max_mpls_labels);
 
 /* ofproto-dpif interface.
  *
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index f067456..b98e519 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -1799,7 +1799,8 @@ compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port,
     if (out_port != ODPP_NONE) {
         ctx->xout->slow |= commit_odp_actions(flow, &ctx->base_flow,
                                               &ctx->xout->odp_actions,
-                                              &ctx->xout->wc);
+                                              &ctx->xout->wc,
+                                              ctx->xbridge->max_mpls_depth);
         nl_msg_put_odp_port(&ctx->xout->odp_actions, OVS_ACTION_ATTR_OUTPUT,
                             out_port);
 
@@ -2074,7 +2075,8 @@ execute_controller_action(struct xlate_ctx *ctx, int len,
 
     ctx->xout->slow |= commit_odp_actions(&ctx->xin->flow, &ctx->base_flow,
                                           &ctx->xout->odp_actions,
-                                          &ctx->xout->wc);
+                                          &ctx->xout->wc,
+                                          ctx->xbridge->max_mpls_depth);
 
     odp_execute_actions(NULL, packet, &md, ctx->xout->odp_actions.data,
                         ctx->xout->odp_actions.size, NULL);
@@ -2108,7 +2110,7 @@ compose_mpls_push_action(struct xlate_ctx *ctx, struct ofpact_push_mpls *mpls)
 
     ovs_assert(eth_type_mpls(mpls->ethertype));
 
-    n = flow_count_mpls_labels(flow, wc);
+    n = flow_count_mpls_labels(flow, wc, ctx->xbridge->max_mpls_depth);
     if (!n) {
         if (mpls->position == OFPACT_MPLS_BEFORE_VLAN) {
             vlan_tci = 0;
@@ -2117,14 +2119,15 @@ compose_mpls_push_action(struct xlate_ctx *ctx, struct ofpact_push_mpls *mpls)
         }
         ctx->xout->slow |= commit_odp_actions(flow, &ctx->base_flow,
                                               &ctx->xout->odp_actions,
-                                              &ctx->xout->wc);
+                                              &ctx->xout->wc,
+                                              ctx->xbridge->max_mpls_depth);
     } else if (n >= ctx->xbridge->max_mpls_depth) {
         COVERAGE_INC(xlate_actions_mpls_overflow);
         ctx->xout->slow |= SLOW_ACTION;
         return;
     }
 
-    flow_push_mpls(flow, mpls->ethertype, wc);
+    flow_push_mpls(flow, mpls->ethertype, wc, ctx->xbridge->max_mpls_depth);
     flow->vlan_tci = vlan_tci;
 }
 
@@ -2134,8 +2137,9 @@ compose_mpls_pop_action(struct xlate_ctx *ctx, ovs_be16 eth_type)
     struct flow_wildcards *wc = &ctx->xout->wc;
     struct flow *flow = &ctx->xin->flow;
 
-    if (!flow_pop_mpls(flow, eth_type, wc)
-        && flow_count_mpls_labels(flow, wc) >= ARRAY_SIZE(flow->mpls_lse)) {
+    if (!flow_pop_mpls(flow, eth_type, wc, ctx->xbridge->max_mpls_depth)
+        && flow_count_mpls_labels(flow, wc, ctx->xbridge->max_mpls_depth) >=
+        ARRAY_SIZE(flow->mpls_lse)) {
         if (ctx->xin->packet != NULL) {
             static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
             VLOG_WARN_RL(&rl, "bridge %s: dropping packet on which an "
@@ -2433,7 +2437,8 @@ xlate_sample_action(struct xlate_ctx *ctx,
 
   ctx->xout->slow |= commit_odp_actions(&ctx->xin->flow, &ctx->base_flow,
                                         &ctx->xout->odp_actions,
-                                        &ctx->xout->wc);
+                                        &ctx->xout->wc,
+                                        ctx->xbridge->max_mpls_depth);
 
   compose_flow_sample_cookie(os->probability, os->collector_set_id,
                              os->obs_domain_id, os->obs_point_id, &cookie);



More information about the dev mailing list