From: Jarno Rajahalme Date: Tue, 15 Oct 2013 19:40:38 +0000 (-0700) Subject: Set datapath mask bits when setting a flow field. X-Git-Tag: sliver-openvswitch-2.0.90-1~7^2~71 X-Git-Url: http://git.onelab.eu/?a=commitdiff_plain;h=f47ea0215f0fc2898815c903276d9ec8afd0306e;p=sliver-openvswitch.git Set datapath mask bits when setting a flow field. Since at the datapath interface we do not have set actions for individual fields, but larger sets of fields for a given protocol layer, the set action will in practice only ever apply to exactly matched flows for the given protocol layer. For example, if the reg_load changes the IP TTL, the corresponding datapath action will rewrite also the IP addresses and TOS byte. Since these other field values may not be explicitly set, they depend on the incoming flow field values, and are hence all of them are set in the wildcards masks, when the action is committed to the datapath. For the rare case, where the reg_load action does not actually change the value, and no other flow field values are set (or loaded), the datapath action is skipped, and no mask bits are set. Such a datapath flow should, however, be dependent on the specific field value, so the corresponding wildcard mask bits must be set, lest the datapath flow be applied to packets containing some other value in the field and the field value remain unchanged regardless of the incoming value. Signed-off-by: Jarno Rajahalme Signed-off-by: Ben Pfaff --- diff --git a/lib/nx-match.c b/lib/nx-match.c index 8444ab759..15143f17c 100644 --- a/lib/nx-match.c +++ b/lib/nx-match.c @@ -1308,13 +1308,11 @@ void nxm_execute_reg_move(const struct ofpact_reg_move *move, struct flow *flow, struct flow_wildcards *wc) { - union mf_subvalue mask_value; union mf_value src_value; 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_mask_field_and_prereqs(move->dst.field, &wc->masks); + mf_mask_field_and_prereqs(move->src.field, &wc->masks); mf_get_value(move->dst.field, flow, &dst_value); mf_get_value(move->src.field, flow, &src_value); @@ -1325,8 +1323,33 @@ nxm_execute_reg_move(const struct ofpact_reg_move *move, } void -nxm_execute_reg_load(const struct ofpact_reg_load *load, struct flow *flow) -{ +nxm_execute_reg_load(const struct ofpact_reg_load *load, struct flow *flow, + struct flow_wildcards *wc) +{ + /* Since at the datapath interface we do not have set actions for + * individual fields, but larger sets of fields for a given protocol + * layer, the set action will in practice only ever apply to exactly + * matched flows for the given protocol layer. For example, if the + * reg_load changes the IP TTL, the corresponding datapath action will + * rewrite also the IP addresses and TOS byte. Since these other field + * values may not be explicitly set, they depend on the incoming flow field + * values, and are hence all of them are set in the wildcards masks, when + * the action is committed to the datapath. For the rare case, where the + * reg_load action does not actually change the value, and no other flow + * field values are set (or loaded), the datapath action is skipped, and + * no mask bits are set. Such a datapath flow should, however, be + * dependent on the specific field value, so the corresponding wildcard + * mask bits must be set, lest the datapath flow be applied to packets + * containing some other value in the field and the field value remain + * unchanged regardless of the incoming value. + * + * We set the masks here for the whole fields, and their prerequisities. + * Even if only the lower byte of a TCP destination port is set, + * we set the mask for the whole field, and also the ip_proto in the IP + * header, so that the kernel flow would not be applied on, e.g., a UDP + * packet, or any other IP protocol in addition to TCP packets. + */ + mf_mask_field_and_prereqs(load->dst.field, &wc->masks); mf_write_subfield_flow(&load->dst, &load->subvalue, flow); } diff --git a/lib/nx-match.h b/lib/nx-match.h index 9dcc19a1d..70652256f 100644 --- a/lib/nx-match.h +++ b/lib/nx-match.h @@ -85,7 +85,8 @@ void nxm_reg_load_to_nxast(const struct ofpact_reg_load *, 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_execute_reg_load(const struct ofpact_reg_load *, struct flow *, + struct flow_wildcards *); void nxm_reg_load(const struct mf_subfield *, uint64_t src_data, struct flow *, struct flow_wildcards *); diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 9313a7fb7..737175033 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -2283,6 +2283,8 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len, struct flow *flow = &ctx->xin->flow; const struct ofpact *a; + /* dl_type already in the mask, not set below. */ + OFPACT_FOR_EACH (a, ofpacts, ofpacts_len) { struct ofpact_controller *controller; const struct ofpact_metadata *metadata; @@ -2349,40 +2351,40 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_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)) { + memset(&wc->masks.nw_src, 0xff, sizeof wc->masks.nw_src); 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)) { + memset(&wc->masks.nw_dst, 0xff, sizeof wc->masks.nw_dst); 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)) { + wc->masks.nw_tos |= IP_DSCP_MASK; flow->nw_tos &= ~IP_DSCP_MASK; flow->nw_tos |= ofpact_get_SET_IPV4_DSCP(a)->dscp; } break; 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)) { + memset(&wc->masks.nw_proto, 0xff, sizeof wc->masks.nw_proto); + memset(&wc->masks.tp_src, 0xff, sizeof wc->masks.tp_src); flow->tp_src = htons(ofpact_get_SET_L4_SRC_PORT(a)->port); } break; 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)) { + memset(&wc->masks.nw_proto, 0xff, sizeof wc->masks.nw_proto); + memset(&wc->masks.tp_dst, 0xff, sizeof wc->masks.tp_dst); flow->tp_dst = htons(ofpact_get_SET_L4_DST_PORT(a)->port); } break; @@ -2408,7 +2410,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len, break; case OFPACT_REG_LOAD: - nxm_execute_reg_load(ofpact_get_REG_LOAD(a), flow); + nxm_execute_reg_load(ofpact_get_REG_LOAD(a), flow, wc); break; case OFPACT_STACK_PUSH: