[ovs-dev] [PATCH 2/2] Set datapath mask bits when setting a flow field.
Jarno Rajahalme
jrajahalme at nicira.com
Mon Oct 14 22:57:35 UTC 2013
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 explicity 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 <jrajahalme at nicira.com>
---
lib/nx-match.c | 35 +++++++++++++++++++++++++++++------
lib/nx-match.h | 3 ++-
ofproto/ofproto-dpif-xlate.c | 18 ++++++++++--------
3 files changed, 41 insertions(+), 15 deletions(-)
diff --git a/lib/nx-match.c b/lib/nx-match.c
index 8444ab7..fe8b199 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_force_prereqs(move->dst.field, &wc->masks);
+ mf_force_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 explicity 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_force_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 9dcc19a..7065225 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 efa5cbe..5524c79 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -2254,6 +2254,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;
@@ -2320,40 +2322,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;
@@ -2379,7 +2381,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:
--
1.7.10.4
More information about the dev
mailing list