In ofproto/ofproto-dpif.c function rule_dpif_lookup_from_table the first fragment ports are _temporarily_ set to 0 for the purpose of flow lookup. However, the ports are then restored, and multipath code sees non-0 ports for the first fragment
We could skip the saving/restore of the original ports, and clear the ports permanently in ‘normal’ frag_mode. It does seem inconsistent to consider the ports 0 for flow lookup, but then use non-0 ports for the first fragment for OVS action processing. Regards, Jeroen From: Darrell Ball <[email protected]> Sent: Wednesday, June 5, 2019 8:33 PM To: Van Bemmel, Jeroen (Nokia - US) <[email protected]> Cc: [email protected] Subject: Re: [ovs-dev] [PATCH] lib/flow.c: Don't include ports of first fragments in hash There is a switch config, called 'frag_mode'; the default is 'normal' where tcp ports, udp ports and ICMP type/code are set to 0 for all fragments. http://www.openvswitch.org/support/dist-docs/ovs-ofctl.8.pdf This is referenced during datapath flow translation right now. It might be worth investigating whether this config can be referenced and folded in earlier. On Wed, Jun 5, 2019 at 3:36 PM Van Bemmel, Jeroen (Nokia - US) <[email protected]<mailto:[email protected]>> wrote: For a series of IP fragments, only the first packet includes the transport header (TCP/UDP/SCTP) and the src/dst ports. By including these port numbers in the hash, it may happen that a first fragment hashes to a different value than subsequent packets, causing different packets from the same flow to follow different paths. This in turn may result in out-of-order delivery or failed reassembly. This patch excludes port numbers from the hash calculation in case of IP fragmentation. Signed-off-by: Jeroen van Bemmel <[email protected]<mailto:[email protected]>> --- diff --git a/lib/flow.c b/lib/flow.c index f39b57f5b..0ff20a87a 100644 --- a/lib/flow.c +++ b/lib/flow.c @@ -2370,8 +2370,10 @@ flow_hash_symmetric_l3l4(const struct flow *flow, uint32_t basis, } hash = hash_add(hash, flow->nw_proto); - if (flow->nw_proto == IPPROTO_TCP || flow->nw_proto == IPPROTO_SCTP || - (inc_udp_ports && flow->nw_proto == IPPROTO_UDP)) { + if ((flow->nw_proto == IPPROTO_TCP || flow->nw_proto == IPPROTO_SCTP || + (inc_udp_ports && flow->nw_proto == IPPROTO_UDP)) + /* For first fragments, don't include ports in hash */ + && !(flow->nw_frag & FLOW_NW_FRAG_MASK) ) { hash = hash_add(hash, (OVS_FORCE uint16_t) (flow->tp_src ^ flow->tp_dst)); } @@ -2486,7 +2488,8 @@ flow_mask_hash_fields(const struct flow *flow, struct flow_wildcards *wc, break; case NX_HASH_FIELDS_SYMMETRIC_L3L4_UDP: - if (is_ip_any(flow) && flow->nw_proto == IPPROTO_UDP) { + if (is_ip_any(flow) && flow->nw_proto == IPPROTO_UDP + && !(flow->nw_frag & FLOW_NW_FRAG_MASK) ) { memset(&wc->masks.tp_src, 0xff, sizeof wc->masks.tp_src); memset(&wc->masks.tp_dst, 0xff, sizeof wc->masks.tp_dst); } @@ -2503,7 +2506,8 @@ flow_mask_hash_fields(const struct flow *flow, struct flow_wildcards *wc, } memset(&wc->masks.nw_proto, 0xff, sizeof wc->masks.nw_proto); - if (flow->nw_proto == IPPROTO_TCP || flow->nw_proto == IPPROTO_SCTP) { + if ((flow->nw_proto == IPPROTO_TCP || flow->nw_proto == IPPROTO_SCTP) + && OVS_LIKELY(!(flow->nw_frag & FLOW_NW_FRAG_MASK))) { memset(&wc->masks.tp_src, 0xff, sizeof wc->masks.tp_src); memset(&wc->masks.tp_dst, 0xff, sizeof wc->masks.tp_dst); } _______________________________________________ dev mailing list [email protected]<mailto:[email protected]> https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
