[ovs-dev] [PATCH v2 4/5] ofproto-dpif-upcall: Slow path flows that datapath can't fully match.

[Ben Pfaff]

In the OVS architecture, when a datapath doesn't have a match for a packet,
it sends the packet and the flow that it extracted from it to userspace.
Userspace then examines the packet and the flow and compares them.
Commonly, the flow is the same as what userspace expects, given the packet,
but there are two other possibilities:

    - The flow lacks one or more fields that userspace expects to be there,
      that is, the datapath doesn't understand or parse them but userspace
      does.  This is, for example, what would happen if current OVS
      userspace, which understands and extracts TCP flags, were to be
      paired with an older OVS kernel module, which does not.  Internally
      OVS uses the name ODP_FIT_TOO_LITTLE for this situation.

    - The flow includes fields that userspace does not know about, that is,
      the datapath understands and parses them but userspace does not.
      This is, for example, what would happen if an old OVS userspace that
      does not understand or extract TCP flags, were to be paired with a
      recent OVS kernel module that does.  Internally, OVS uses the name
      ODP_FIT_TOO_MUCH for this situation.

The latter is not a big deal and OVS doesn't have to do much to cope with
it.

The former is more of a problem.  When the datapath can't match on all the
fields that OVS supports, it means that OVS can't safely install a flow at
all, other than one that directs packets to the slow path.  Otherwise, if
OVS did install a flow, it could match a packet that does not match the
flow that OVS intended to match and could cause the wrong behavior.

Somehow, this nuance was lost a long time.  From about 2013 until today,
it seems that OVS has ignored ODP_FIT_TOO_LITTLE.  Instead, it happily
installs a flow regardless of whether the datapath can actually fully match
it.  I imagine that this is rarely a problem because most of the time
the datapath and userspace are well matched, but it is still an important
problem to fix.  This commit fixes it, by forcing flows into the slow path
when the datapath cannot match specifically enough.

CC: Ethan Jackson <ejj at eecs.berkeley.edu>
Fixes: e79a6c833e0d ("ofproto: Handle flow installation and eviction in upcall.")
Reported-by: Huanle Han <hanxueluo at gmail.com>
Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2018-January/343665.html
Signed-off-by: Ben Pfaff <blp at ovn.org>
---
 lib/odp-util.h                |  5 +++--
 ofproto/ofproto-dpif-upcall.c | 16 +++++++++++++---
 2 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/lib/odp-util.h b/lib/odp-util.h
index 0fcc593d2ace..1fad159db9fb 100644
--- a/lib/odp-util.h
+++ b/lib/odp-util.h
@@ -37,14 +37,15 @@ struct simap;
 struct pkt_metadata;
 
 #define SLOW_PATH_REASONS                                               \
-    /* These reasons are mutually exclusive. */                         \
     SPR(SLOW_CFM,        "cfm",        "Consists of CFM packets")       \
     SPR(SLOW_BFD,        "bfd",        "Consists of BFD packets")       \
     SPR(SLOW_LACP,       "lacp",       "Consists of LACP packets")      \
     SPR(SLOW_STP,        "stp",        "Consists of STP packets")       \
     SPR(SLOW_LLDP,       "lldp",       "Consists of LLDP packets")      \
     SPR(SLOW_ACTION,     "action",                                      \
-        "Uses action(s) not supported by datapath")
+        "Uses action(s) not supported by datapath")                     \
+    SPR(SLOW_MATCH,      "match",                                       \
+        "Datapath can't match specifically enough")
 
 /* Indexes for slow-path reasons.  Client code uses "enum slow_path_reason"
  * values instead of these, these are just a way to construct those. */
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 035233d5adc8..5eb20f7cc236 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -206,6 +206,7 @@ struct upcall {
      * dpif-netdev.  If a modification is absolutely necessary, a const cast
      * may be used with other datapaths. */
     const struct flow *flow;       /* Parsed representation of the packet. */
+    enum odp_key_fitness fitness;  /* Fitness of 'flow' relative to ODP key. */
     const ovs_u128 *ufid;          /* Unique identifier for 'flow'. */
     unsigned pmd_id;               /* Datapath poll mode driver id. */
     const struct dp_packet *packet;   /* Packet associated with this upcall. */
@@ -787,8 +788,9 @@ recv_upcalls(struct handler *handler)
             break;
         }
 
-        if (odp_flow_key_to_flow(dupcall->key, dupcall->key_len, flow)
-            == ODP_FIT_ERROR) {
+        upcall->fitness = odp_flow_key_to_flow(dupcall->key, dupcall->key_len,
+                                               flow);
+        if (upcall->fitness == ODP_FIT_ERROR) {
             goto free_dupcall;
         }
 
@@ -1174,6 +1176,9 @@ upcall_xlate(struct udpif *udpif, struct upcall *upcall,
 
     upcall->xout_initialized = true;
 
+    if (upcall->fitness == ODP_FIT_TOO_LITTLE) {
+        upcall->xout.slow |= SLOW_MATCH;
+    }
     if (!upcall->xout.slow) {
         ofpbuf_use_const(&upcall->put_actions,
                          odp_actions->data, odp_actions->size);
@@ -2029,10 +2034,12 @@ xlate_key(struct udpif *udpif, const struct nlattr *key, unsigned int len,
 {
     struct ofproto_dpif *ofproto;
     ofp_port_t ofp_in_port;
+    enum odp_key_fitness fitness;
     struct xlate_in xin;
     int error;
 
-    if (odp_flow_key_to_flow(key, len, &ctx->flow) == ODP_FIT_ERROR) {
+    fitness = odp_flow_key_to_flow(key, len, &ctx->flow);
+    if (fitness == ODP_FIT_ERROR) {
         return EINVAL;
     }
 
@@ -2051,6 +2058,9 @@ xlate_key(struct udpif *udpif, const struct nlattr *key, unsigned int len,
     }
     xin.xcache = ctx->xcache;
     xlate_actions(&xin, &ctx->xout);
+    if (fitness == ODP_FIT_TOO_LITTLE) {
+        ctx->xout.slow |= SLOW_MATCH;
+    }
 
     return 0;
 }
-- 
2.10.2