@@ -549,7 +549,8 @@ 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);
@@ -557,9 +558,10 @@ 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
*
@@ -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. */
@@ -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])
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 <aconole@redhat.com> Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- v6: Added Aaron's ack. v5: Implemented Adrian's suggestion to always align the 'mask' within 'struct ofpact_set_field' to 8 bytes. v4: Rebase. v3: Split out from old patch 07/11. --- include/openvswitch/ofp-actions.h | 10 ++++++---- lib/ofp-actions.c | 19 +++++++++++++------ tests/ovs-ofctl.at | 2 +- 3 files changed, 20 insertions(+), 11 deletions(-)