Acked-by: Jarno Rajahalme <ja...@ovn.org> Jarno
> On Aug 30, 2016, at 6:47 PM, Daniele Di Proietto <diproiet...@vmware.com> > wrote: > > It's possible to install an OpenFlow flow that matches on udp source and > destination ports without matching on fragments. If the subtable where > such flow stays is visited during translation of a later fragment, the > generated mask will have incorrect prerequisited for the datapath and it > would be revalidated away at the first chance. > > This commit fixes it by adjusting the mask for later fragments after > translation. > > Other prerequisites of the mask are also prerequisites in OpenFlow, but > not the ip fragment bit, that's why we need a special case here. > > For completeness, this commits also fixes a related problem in bfd, > where we check the udp destination port without checking if the frame is > an ip fragment. It's not really necessary to address this separately, > given the adjustment that we perform. > > VMware-BZ: #1651589 > Signed-off-by: Daniele Di Proietto <diproiet...@vmware.com> > --- > lib/bfd.c | 2 +- > ofproto/ofproto-dpif-xlate.c | 11 +++++++++++ > tests/ofproto-dpif.at | 35 +++++++++++++++++++++++++++++++++++ > 3 files changed, 47 insertions(+), 1 deletion(-) > > diff --git a/lib/bfd.c b/lib/bfd.c > index fcb6f64..87f3322 100644 > --- a/lib/bfd.c > +++ b/lib/bfd.c > @@ -658,7 +658,7 @@ bfd_should_process_flow(const struct bfd *bfd_, const > struct flow *flow, > > if (flow->dl_type == htons(ETH_TYPE_IP)) { > memset(&wc->masks.nw_proto, 0xff, sizeof wc->masks.nw_proto); > - if (flow->nw_proto == IPPROTO_UDP) { > + if (flow->nw_proto == IPPROTO_UDP && !(flow->nw_frag & > FLOW_NW_FRAG_LATER)) { > memset(&wc->masks.tp_dst, 0xff, sizeof wc->masks.tp_dst); > if (flow->tp_dst == htons(BFD_DEST_PORT)) { > bool check_tnl_key; > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > index 0118d01..0403c98 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -5329,6 +5329,17 @@ xlate_wc_finish(struct xlate_ctx *ctx) > if (ctx->wc->masks.vlan_tci) { > ctx->wc->masks.vlan_tci |= htons(VLAN_CFI); > } > + > + /* The classifier might return masks that match on tp_src and tp_dst even > + * for later fragments. This happens because there might be flows that > + * match on tp_src or tp_dst without matching on the frag bits, because > + * it is not a prerequisite for OpenFlow. Since it is a prerequisite for > + * datapath flows and since tp_src and tp_dst are always going to be 0, > + * wildcard the fields here. */ > + if (ctx->xin->flow.nw_frag & FLOW_NW_FRAG_LATER) { > + ctx->wc->masks.tp_src = 0; > + ctx->wc->masks.tp_dst = 0; > + } > } > > /* Translates the flow, actions, or rule in 'xin' into datapath actions in > diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at > index 8e5fde6..e047c1c 100644 > --- a/tests/ofproto-dpif.at > +++ b/tests/ofproto-dpif.at > @@ -8903,3 +8903,38 @@ AT_CHECK([ovs-vsctl --timeout=10 wait-until Interface > int1 mtu=1600]) > > OVS_VSWITCHD_STOP > AT_CLEANUP > + > +AT_SETUP([ofproto - fragment prerequisites]) > +OVS_VSWITCHD_START > + > +AT_CHECK([ovs-appctl vlog/set dpif:dbg dpif_netdev:dbg]) > + > +add_of_ports br0 1 > + > +AT_DATA([flows.txt], [dnl > +priority=10,in_port=1,udp,tp_src=67,tp_dst=68,action=drop > +priority=1,in_port=1,udp,action=drop > +]) > + > +AT_CHECK([ovs-ofctl add-flows br0 flows.txt]) > + > +AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:max-idle=10000]) > + > +ovs-appctl time/stop > +AT_CHECK([ovs-appctl netdev-dummy/receive p1 > 'recirc_id(0),in_port(1),eth_type(0x0800),ipv4(proto=17,frag=later)']) > +ovs-appctl time/warp 5000 > + > +AT_CHECK([strip_ufid < ovs-vswitchd.log | filter_flow_install | strip_used], > [0], [dnl > +recirc_id(0),in_port(1),eth_type(0x0800),ipv4(proto=17,frag=later), > actions:drop > +]) > + > +dnl Change the flow table. This will trigger revalidation of all the flows. > +AT_CHECK([ovs-ofctl add-flow br0 priority=5,in_port=1,action=drop]) > +AT_CHECK([ovs-appctl revalidator/wait], [0]) > + > +dnl We don't want revalidators to delete any flow. If the flow has been > +dnl deleted it means that there's some inconsistency with the revalidation. > +AT_CHECK([grep flow_del ovs-vswitchd.log], [1]) > + > +OVS_VSWITCHD_STOP > +AT_CLEANUP > -- > 2.9.3 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev