I'm just shocked that we didn't notice for 5 years.
On Wed, Jan 24, 2018 at 02:24:50PM -0800, Ethan J. Jackson wrote: > Nice! I, for one, never understood the TOO_LITTLE stuff ... glad to see it > clarified and fixed. > > Ethan > > On Wed, Jan 24, 2018 at 11:40 AM, Ben Pfaff <[email protected]> wrote: > > > 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 <[email protected]> > > Fixes: e79a6c833e0d ("ofproto: Handle flow installation and eviction in > > upcall.") > > Reported-by: Huanle Han <[email protected]> > > Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2018- > > January/343665.html > > Signed-off-by: Ben Pfaff <[email protected]> > > --- > > 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 > > > > > > > -- > Ethan J. Jackson > ejj.sh _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
