[ovs-dev] [PATCH] ofp-actions: Only set defined bits when encoding "load" actions.

[Ben Pfaff]

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 <pshelar at nicira.com>
Signed-off-by: Ben Pfaff <blp at nicira.com>
---
 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