From: Justin Pettit Date: Sat, 3 Aug 2013 04:17:31 +0000 (-0700) Subject: ofproto-dpif: Always un-wildcard fields that are being set. X-Git-Tag: sliver-openvswitch-2.0.90-1~33^2~32 X-Git-Url: http://git.onelab.eu/?p=sliver-openvswitch.git;a=commitdiff_plain;h=f74e7df7450d6c31caaad66fe8b1dc923e86e9a7 ofproto-dpif: Always un-wildcard fields that are being set. The ODP library has an optimization to not set a header if the field was not changed, regardless of whether an action to set the field was present. That library is also responsible for un-wildcarding fields that are bieng modified. This leads to a problem where a packet matches a flow that updates a field, but that particular packet's field already has that value. As such, an overly loose megaflow will be generated that doesn't match on that field and the actions won't update it. A second packet that should have the field set will match that flow and will not be modified. This commit changes the behavior to always un-wildcard fields that are being modified. Since the ODP library updates the entire header if a field in it is modified, and all those fields will be un-wildcarded, the generated flows may be different. However, they should be correct. Bug #18946. Reported-by: Jesse Gross Signed-off-by: Justin Pettit Acked-by: Ben Pfaff --- diff --git a/lib/multipath.c b/lib/multipath.c index 6c0560d90..4b9e4af96 100644 --- a/lib/multipath.c +++ b/lib/multipath.c @@ -114,7 +114,7 @@ multipath_execute(const struct ofpact_multipath *mp, struct flow *flow, mp->max_link + 1, mp->arg); flow_mask_hash_fields(flow, wc, mp->fields); - nxm_reg_load(&mp->dst, link, flow); + nxm_reg_load(&mp->dst, link, flow, wc); } static uint16_t diff --git a/lib/nx-match.c b/lib/nx-match.c index bdb3a2ba7..940dd9a44 100644 --- a/lib/nx-match.c +++ b/lib/nx-match.c @@ -1304,6 +1304,7 @@ nxm_execute_reg_move(const struct ofpact_reg_move *move, union mf_value dst_value; memset(&mask_value, 0xff, sizeof mask_value); + mf_write_subfield_flow(&move->dst, &mask_value, &wc->masks); mf_write_subfield_flow(&move->src, &mask_value, &wc->masks); mf_get_value(move->dst.field, flow, &dst_value); @@ -1322,11 +1323,15 @@ nxm_execute_reg_load(const struct ofpact_reg_load *load, struct flow *flow) void nxm_reg_load(const struct mf_subfield *dst, uint64_t src_data, - struct flow *flow) + struct flow *flow, struct flow_wildcards *wc) { union mf_subvalue src_subvalue; + union mf_subvalue mask_value; ovs_be64 src_data_be = htonll(src_data); + memset(&mask_value, 0xff, sizeof mask_value); + mf_write_subfield_flow(dst, &mask_value, &wc->masks); + bitwise_copy(&src_data_be, sizeof src_data_be, 0, &src_subvalue, sizeof src_subvalue, 0, sizeof src_data_be * 8); @@ -1479,7 +1484,8 @@ nxm_execute_stack_push(const struct ofpact_stack *push, void nxm_execute_stack_pop(const struct ofpact_stack *pop, - struct flow *flow, struct ofpbuf *stack) + struct flow *flow, struct flow_wildcards *wc, + struct ofpbuf *stack) { union mf_subvalue *src_value; @@ -1487,6 +1493,10 @@ nxm_execute_stack_pop(const struct ofpact_stack *pop, /* Only pop if stack is not empty. Otherwise, give warning. */ if (src_value) { + union mf_subvalue mask_value; + + memset(&mask_value, 0xff, sizeof mask_value); + mf_write_subfield_flow(&pop->subfield, &mask_value, &wc->masks); mf_write_subfield_flow(&pop->subfield, src_value, flow); } else { if (!VLOG_DROP_WARN(&rl)) { diff --git a/lib/nx-match.h b/lib/nx-match.h index a6b7c5233..9dcc19a1d 100644 --- a/lib/nx-match.h +++ b/lib/nx-match.h @@ -87,7 +87,7 @@ void nxm_execute_reg_move(const struct ofpact_reg_move *, struct flow *, struct flow_wildcards *); void nxm_execute_reg_load(const struct ofpact_reg_load *, struct flow *); void nxm_reg_load(const struct mf_subfield *, uint64_t src_data, - struct flow *); + struct flow *, struct flow_wildcards *); char *nxm_parse_stack_action(struct ofpact_stack *, const char *) WARN_UNUSED_RESULT; @@ -113,7 +113,8 @@ void nxm_execute_stack_push(const struct ofpact_stack *, const struct flow *, struct flow_wildcards *, struct ofpbuf *); void nxm_execute_stack_pop(const struct ofpact_stack *, - struct flow *, struct ofpbuf *); + struct flow *, struct flow_wildcards *, + struct ofpbuf *); int nxm_field_bytes(uint32_t header); int nxm_field_bits(uint32_t header); diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index bba4355c1..91ec01a5a 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -1708,6 +1708,7 @@ compose_set_mpls_ttl_action(struct xlate_ctx *ctx, uint8_t ttl) return true; } + ctx->xout->wc.masks.mpls_lse |= htonl(MPLS_TTL_MASK); set_mpls_lse_ttl(&ctx->xin->flow.mpls_lse, ttl); return false; } @@ -1887,7 +1888,8 @@ xlate_bundle_action(struct xlate_ctx *ctx, slave_enabled_cb, CONST_CAST(struct xbridge *, ctx->xbridge)); if (bundle->dst.field) { - nxm_reg_load(&bundle->dst, ofp_to_u16(port), &ctx->xin->flow); + nxm_reg_load(&bundle->dst, ofp_to_u16(port), &ctx->xin->flow, + &ctx->xout->wc); } else { xlate_output_action(ctx, port, 0, false); } @@ -2038,12 +2040,14 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len, break; case OFPACT_SET_VLAN_VID: + wc->masks.vlan_tci |= htons(VLAN_VID_MASK | VLAN_CFI); flow->vlan_tci &= ~htons(VLAN_VID_MASK); flow->vlan_tci |= (htons(ofpact_get_SET_VLAN_VID(a)->vlan_vid) | htons(VLAN_CFI)); break; case OFPACT_SET_VLAN_PCP: + wc->masks.vlan_tci |= htons(VLAN_PCP_MASK | VLAN_CFI); flow->vlan_tci &= ~htons(VLAN_PCP_MASK); flow->vlan_tci |= htons((ofpact_get_SET_VLAN_PCP(a)->vlan_pcp << VLAN_PCP_SHIFT) @@ -2051,35 +2055,42 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len, break; case OFPACT_STRIP_VLAN: + memset(&wc->masks.vlan_tci, 0xff, sizeof wc->masks.vlan_tci); flow->vlan_tci = htons(0); break; case OFPACT_PUSH_VLAN: /* XXX 802.1AD(QinQ) */ + memset(&wc->masks.vlan_tci, 0xff, sizeof wc->masks.vlan_tci); flow->vlan_tci = htons(VLAN_CFI); break; case OFPACT_SET_ETH_SRC: + memset(&wc->masks.dl_src, 0xff, sizeof wc->masks.dl_src); memcpy(flow->dl_src, ofpact_get_SET_ETH_SRC(a)->mac, ETH_ADDR_LEN); break; case OFPACT_SET_ETH_DST: + memset(&wc->masks.dl_dst, 0xff, sizeof wc->masks.dl_dst); memcpy(flow->dl_dst, ofpact_get_SET_ETH_DST(a)->mac, ETH_ADDR_LEN); break; case OFPACT_SET_IPV4_SRC: + memset(&wc->masks.nw_src, 0xff, sizeof wc->masks.nw_src); if (flow->dl_type == htons(ETH_TYPE_IP)) { flow->nw_src = ofpact_get_SET_IPV4_SRC(a)->ipv4; } break; case OFPACT_SET_IPV4_DST: + memset(&wc->masks.nw_dst, 0xff, sizeof wc->masks.nw_dst); if (flow->dl_type == htons(ETH_TYPE_IP)) { flow->nw_dst = ofpact_get_SET_IPV4_DST(a)->ipv4; } break; case OFPACT_SET_IPV4_DSCP: + wc->masks.nw_tos |= IP_DSCP_MASK; /* OpenFlow 1.0 only supports IPv4. */ if (flow->dl_type == htons(ETH_TYPE_IP)) { flow->nw_tos &= ~IP_DSCP_MASK; @@ -2089,6 +2100,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len, case OFPACT_SET_L4_SRC_PORT: memset(&wc->masks.nw_proto, 0xff, sizeof wc->masks.nw_proto); + memset(&wc->masks.tp_src, 0xff, sizeof wc->masks.tp_src); if (is_ip_any(flow)) { flow->tp_src = htons(ofpact_get_SET_L4_SRC_PORT(a)->port); } @@ -2096,6 +2108,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len, case OFPACT_SET_L4_DST_PORT: memset(&wc->masks.nw_proto, 0xff, sizeof wc->masks.nw_proto); + memset(&wc->masks.tp_dst, 0xff, sizeof wc->masks.tp_dst); if (is_ip_any(flow)) { flow->tp_dst = htons(ofpact_get_SET_L4_DST_PORT(a)->port); } @@ -2131,7 +2144,8 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len, break; case OFPACT_STACK_POP: - nxm_execute_stack_pop(ofpact_get_STACK_POP(a), flow, &ctx->stack); + nxm_execute_stack_pop(ofpact_get_STACK_POP(a), flow, wc, + &ctx->stack); break; case OFPACT_PUSH_MPLS: @@ -2156,6 +2170,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len, break; case OFPACT_DEC_TTL: + wc->masks.nw_ttl = 0xff; if (compose_dec_ttl(ctx, ofpact_get_DEC_TTL(a))) { goto out; } diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at index d5e5e3c6c..00affe8a0 100644 --- a/tests/ofproto-dpif.at +++ b/tests/ofproto-dpif.at @@ -2249,6 +2249,9 @@ AT_BANNER([ofproto-dpif -- megaflows]) # Strips out uninteresting parts of megaflow output, as well as parts # that vary from one run to another (e.g., timing and bond actions). +m4_define([STRIP_USED], [[sed ' + s/used:[0-9]*\.[0-9]*/used:0.0/ +' | sort]]) m4_define([STRIP_XOUT], [[sed ' s/used:[0-9]*\.[0-9]*/used:0.0/ s/Datapath actions:.*/Datapath actions: / @@ -2633,6 +2636,32 @@ skb_priority=0,icmp,in_port=1,nw_src=10.0.0.4,nw_dst=10.0.0.3,nw_tos=0,nw_ecn=0, OVS_VSWITCHD_STOP AT_CLEANUP +AT_SETUP([ofproto-dpif megaflow - set dl_dst]) +OVS_VSWITCHD_START +ADD_OF_PORTS([br0], [1], [2]) +AT_DATA([flows.txt], [dnl +table=0 in_port=1 actions=mod_dl_dst(50:54:00:00:00:0a),output(2) +]) +AT_CHECK([ovs-ofctl add-flows br0 flows.txt]) +AT_CHECK([ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.0.0.2,dst=10.0.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)']) +AT_CHECK([ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=50:54:00:00:00:0b,dst=50:54:00:00:00:0c),eth_type(0x0800),ipv4(src=10.0.0.4,dst=10.0.0.3,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)']) +dnl The megaflows do not match the same fields, since the first packet +dnl is essentially a no-op. (The new destination MAC is the same as the +dnl original.) The ofproto-dpif library un-wildcards the destination MAC +dnl so that a packet that doesn't need its MAC address changed doesn't +dnl hide one that does. Since the first entry doesn't need to change, +dnl only the destination MAC address is matched (as decided by +dnl ofproto-dpif). The second entry actually updates the destination +dnl MAC, so both the source and destination MAC addresses are +dnl un-wildcarded, since the ODP commit functions update both the source +dnl and destination MAC addresses. +AT_CHECK([ovs-appctl dpif/dump-megaflows br0 | STRIP_USED], [0], [dnl +skb_priority=0,ip,in_port=1,dl_dst=50:54:00:00:00:0a,nw_frag=no, n_subfacets:1, used:0.0s, Datapath actions: 2 +skb_priority=0,ip,in_port=1,dl_src=50:54:00:00:00:0b,dl_dst=50:54:00:00:00:0c,nw_frag=no, n_subfacets:1, used:0.0s, Datapath actions: set(eth(src=50:54:00:00:00:0b,dst=50:54:00:00:00:0a)),2 +]) +OVS_VSWITCHD_STOP +AT_CLEANUP + AT_SETUP([ofproto-dpif - datapath port number change]) OVS_VSWITCHD_START([set Bridge br0 fail-mode=standalone]) ADD_OF_PORTS([br0], 1)