Commit 7eb4b1f1d70345f ("ofp-actions: Support OF1.5 (draft) masked
Set-Field, merge with reg_load.") introduced a bug in that a set_field
action that set an entire field would be translated incorrectly to
reg_load, if the field being set only occupied a portion of the bytes that
it contains. For example, an MPLS label is 20 bits but has a 4-byte field,
which meant that a set_field would get translated into a reg_load that
wrote all 32 bits; in turn, the receiver of that reg_load would reject it
because it was attempting to set invalid bits (the top 12 bits).
This commit fixes the problem by omitting invalid bits when encoding a
reg_load action.
Reported-by: Pravin Shelar <[email protected]>
Signed-off-by: Ben Pfaff <[email protected]>
---
lib/ofp-actions.c | 15 ++++++++-------
tests/ofp-actions.at | 15 +++++++++++++++
2 files changed, 23 insertions(+), 7 deletions(-)
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index 33b419d..23ca55b 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -2183,16 +2183,17 @@ static bool
next_load_segment(const struct ofpact_set_field *sf,
struct mf_subfield *dst, uint64_t *value)
{
- int w = sf->field->n_bytes;
+ int n_bits = sf->field->n_bits;
+ int n_bytes = sf->field->n_bytes;
int start = dst->ofs + dst->n_bits;
- if (start < 8 * w) {
+ if (start < n_bits) {
dst->field = sf->field;
- dst->ofs = bitwise_scan(&sf->mask, w, 1, start, 8 * w);
- if (dst->ofs < 8 * w) {
- dst->n_bits = bitwise_scan(&sf->mask, w, 0, dst->ofs + 1,
- MIN(dst->ofs + 64, 8 * w)) - dst->ofs;
- *value = bitwise_get(&sf->value, w, dst->ofs, dst->n_bits);
+ dst->ofs = bitwise_scan(&sf->mask, n_bytes, 1, start, n_bits);
+ if (dst->ofs < n_bits) {
+ dst->n_bits = bitwise_scan(&sf->mask, n_bytes, 0, dst->ofs + 1,
+ MIN(dst->ofs + 64, n_bits)) - dst->ofs;
+ *value = bitwise_get(&sf->value, n_bytes, dst->ofs, dst->n_bits);
return true;
}
}
diff --git a/tests/ofp-actions.at b/tests/ofp-actions.at
index af5dd19..876be67 100644
--- a/tests/ofp-actions.at
+++ b/tests/ofp-actions.at
@@ -581,3 +581,18 @@ ovs-ofctl: none of the usable flow formats
(OpenFlow10,NXM) is among the allowed
])
OVS_VSWITCHD_STOP
AT_CLEANUP
+
+AT_SETUP([reg_load <-> set_field translation corner case])
+AT_KEYWORDS([ofp-actions])
+OVS_VSWITCHD_START
+dnl In OpenFlow 1.3, set_field always sets all the bits in the field,
+dnl but when we translate to NXAST_LOAD we need to only set the bits that
+dnl actually exist (e.g. mpls_label only has 20 bits) otherwise OVS rejects
+dnl the "load" action as invalid. Check that we do this correctly.
+AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br0
mpls,actions=set_field:10-\>mpls_label])
+AT_CHECK([ovs-ofctl -O OpenFlow10 dump-flows br0 | ofctl_strip], [0], [dnl
+NXST_FLOW reply:
+ mpls actions=load:0xa->OXM_OF_MPLS_LABEL[[]]
+])
+OVS_VSWITCHD_STOP
+AT_CLEANUP
--
1.7.10.4
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
