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.
Signed-off-by: Dumitru Ceara <[email protected]>
---
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(-)
diff --git a/include/openvswitch/ofp-actions.h
b/include/openvswitch/ofp-actions.h
index b7231c7bb334..711e7c773d6c 100644
--- a/include/openvswitch/ofp-actions.h
+++ b/include/openvswitch/ofp-actions.h
@@ -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
*
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index 4ada0f4a3c49..783af84439e2 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -3376,21 +3376,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) {
@@ -3399,7 +3406,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. */
diff --git a/tests/ovs-ofctl.at b/tests/ovs-ofctl.at
index 267711bfa482..858dcda1f347 100644
--- a/tests/ovs-ofctl.at
+++ b/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])
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
