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

[Jarno Rajahalme]

Acked-by: Jarno Rajahalme 

  Jarno

> On Aug 30, 2016, at 6:47 PM, Daniele Di Proietto  
> 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 
> ---
> 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(>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(>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=1])
> +
> +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

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

[Daniele Di Proietto]

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 
---
 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(>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(>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=1])
+
+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