ofp-actions: Ensure aligned accesses to masked fields.

For example is parsing the OVN "eth.dst[40] = 1;" action, which
triggered the following warning from UndefinedBehaviorSanitizer:

  lib/meta-flow.c:3210:9:
  runtime error: member access within misaligned address 0x000000de4e36
  for type 'const union mf_value', which requires 8 byte alignment
  0x000000de4e36: note: pointer points here
   00 00 00 00 01 00  00 00 00 00 00 00 00 00 70 4e de 00 00 00 00 00
               ^
   10 51 de 00 00 00 00 00  c0 4f

      0 0x5818bc in mf_format lib/meta-flow.c:3210
      1 0x5b6047 in format_SET_FIELD lib/ofp-actions.c:3342
      2 0x5d68ab in ofpact_format lib/ofp-actions.c:9213
      3 0x5d6ee0 in ofpacts_format lib/ofp-actions.c:9237
      4 0x410922 in test_parse_actions tests/test-ovn.c:1360

To avoid this we now change the internal representation of the set_field
actions, struct ofpact_set_field, such that the mask is always stored
at a correctly aligned address, multiple of OFPACT_ALIGNTO.

We also need to adapt the "ovs-ofctl show-flows - Oversized flow" test
because now the ofpact representation of the set_field action uses more
bytes in memory (for the extra alignment).  Change the test to use
dec_ttl instead.

Acked-by: Aaron Conole <[email protected]>
Signed-off-by: Dumitru Ceara <[email protected]>
Signed-off-by: Ilya Maximets <[email protected]>

include/openvswitch/ofp-actions.h

@@ -549,17 +549,19 @@ struct ofpact_set_field {
         const struct mf_field *field;
     );
     union mf_value value[];  /* Significant value bytes followed by
-                              * significant mask bytes. */
+                              * significant mask bytes aligned at
+                              * OFPACT_ALIGNTO bytes. */
 };
 BUILD_ASSERT_DECL(offsetof(struct ofpact_set_field, value)
                   % OFPACT_ALIGNTO == 0);
 BUILD_ASSERT_DECL(offsetof(struct ofpact_set_field, value)
                   == sizeof(struct ofpact_set_field));
 
 /* Use macro to not have to deal with constness. */
-#define ofpact_set_field_mask(SF)                               \
-    ALIGNED_CAST(union mf_value *,                              \
-                 (uint8_t *)(SF)->value + (SF)->field->n_bytes)
+#define ofpact_set_field_mask(SF)                                           \
+    ALIGNED_CAST(union mf_value *,                                          \
+                 (uint8_t *)(SF)->value +                                   \
+                            ROUND_UP((SF)->field->n_bytes, OFPACT_ALIGNTO))
 
 /* OFPACT_PUSH_VLAN/MPLS/PBB
  *

lib/ofp-actions.c

@@ -3381,21 +3381,28 @@ check_SET_FIELD(struct ofpact_set_field *a,
     return 0;
 }
 
-/* Appends an OFPACT_SET_FIELD ofpact with enough space for the value and mask
- * for the 'field' to 'ofpacts' and returns it.  Fills in the value from
- * 'value', if non-NULL, and mask from 'mask' if non-NULL.  If 'value' is
- * non-NULL and 'mask' is NULL, an all-ones mask will be filled in. */
+/* Appends an OFPACT_SET_FIELD ofpact with enough space for the value and a
+ * properly aligned mask for the 'field' to 'ofpacts' and returns it.  Fills
+ * in the value from 'value', if non-NULL, and mask from 'mask' if non-NULL.
+ * If 'value' is non-NULL and 'mask' is NULL, an all-ones mask will be
+ * filled in. */
 struct ofpact_set_field *
 ofpact_put_set_field(struct ofpbuf *ofpacts, const struct mf_field *field,
                      const void *value, const void *mask)
 {
+    /* Ensure there's enough space for:
+     * - value (8-byte aligned)
+     * - mask  (8-byte aligned)
+     * - padding (to make the whole ofpact_set_field 8-byte aligned)
+     */
+    size_t total_size = 2 * ROUND_UP(field->n_bytes, OFPACT_ALIGNTO);
     struct ofpact_set_field *sf = ofpact_put_SET_FIELD(ofpacts);
     sf->field = field;
 
     /* Fill in the value and mask if given, otherwise put zeroes so that the
      * caller may fill in the value and mask itself. */
     if (value) {
-        ofpbuf_put_uninit(ofpacts, 2 * field->n_bytes);
+        ofpbuf_put_uninit(ofpacts, total_size);
         sf = ofpacts->header;
         memcpy(sf->value, value, field->n_bytes);
         if (mask) {
@@ -3404,7 +3411,7 @@ ofpact_put_set_field(struct ofpbuf *ofpacts, const struct mf_field *field,
             memset(ofpact_set_field_mask(sf), 0xff, field->n_bytes);
         }
     } else {
-        ofpbuf_put_zeros(ofpacts, 2 * field->n_bytes);
+        ofpbuf_put_zeros(ofpacts, total_size);
         sf = ofpacts->header;
     }
     /* Update length. */

tests/ovs-ofctl.at

@@ -3258,7 +3258,7 @@ AT_SETUP([ovs-ofctl show-flows - Oversized flow])
 OVS_VSWITCHD_START
 
 printf " priority=90,icmp,reg15=0x8005,metadata=0x1,nw_dst=11.0.0.1,icmp_type=8,icmp_code=0 actions=" > flow.txt
-for i in `seq 1 1022`; do printf "set_field:0x399->reg13,set_field:0x$i->reg15,resubmit(,39),"; done >> flow.txt
+for i in `seq 1 2045`; do printf "dec_ttl,resubmit(,39),"; done >> flow.txt
 printf "resubmit(,39)\n" >> flow.txt
 
 AT_CHECK([ovs-ofctl -O OpenFlow15 add-flows br0 flow.txt])