[ovs-dev] [PATCH v2 2/5] ofproto-dpif-xlate: Adjust generated mask for fragments.

[Jarno Rajahalme]

Acked-by: Jarno Rajahalme <jarno at ovn.org>

  Jarno

> On Aug 30, 2016, at 6:47 PM, Daniele Di Proietto <diproiettod at 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 <diproiettod at 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 at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev