On 3/1/22 10:07, Adrian Moreno wrote: > > > On 2/25/22 18:14, Dumitru Ceara wrote: >> 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 >> >> Signed-off-by: Dumitru Ceara <[email protected]> >> --- >> v4: Rebase. >> v3: Split out from old patch 07/11. >> --- >> include/openvswitch/util.h | 3 +++ >> lib/meta-flow.c | 6 ++++++ >> lib/nx-match.c | 7 +++++++ >> ofproto/ofproto-dpif-xlate.c | 13 +++++++++---- >> 4 files changed, 25 insertions(+), 4 deletions(-) >> >> diff --git a/include/openvswitch/util.h b/include/openvswitch/util.h >> index 228b185c3a5f..f45dc505164c 100644 >> --- a/include/openvswitch/util.h >> +++ b/include/openvswitch/util.h >> @@ -285,6 +285,9 @@ is_pow2(uintmax_t x) >> * segfault, so it is important to be aware of correct alignment. */ >> #define ALIGNED_CAST(TYPE, ATTR) ((TYPE) (void *) (ATTR)) >> +#define IS_PTR_ALIGNED(OBJ) \ >> + (!(OBJ) || (uintptr_t) (OBJ) % __alignof__(OVS_TYPEOF(OBJ)) == 0) >> + >> #ifdef __cplusplus >> } >> #endif >> diff --git a/lib/meta-flow.c b/lib/meta-flow.c >> index c576ae6202a4..2e64fed5f8f2 100644 >> --- a/lib/meta-flow.c >> +++ b/lib/meta-flow.c >> @@ -3172,6 +3172,8 @@ mf_format(const struct mf_field *mf, >> const struct ofputil_port_map *port_map, >> struct ds *s) >> { >> + union mf_value aligned_mask; >> + >> if (mask) { >> if (is_all_zeros(mask, mf->n_bytes)) { >> ds_put_cstr(s, "ANY"); >> @@ -3207,6 +3209,10 @@ mf_format(const struct mf_field *mf, >> break; >> case MFS_ETHERNET: >> + if (!IS_PTR_ALIGNED(mask)) { >> + memcpy(&aligned_mask.mac, mask, sizeof aligned_mask.mac); >> + mask = &aligned_mask; >> + } >> eth_format_masked(value->mac, mask ? &mask->mac : NULL, s); >> break; >> diff --git a/lib/nx-match.c b/lib/nx-match.c >> index 440f5f7630c9..6e87deffc0d0 100644 >> --- a/lib/nx-match.c >> +++ b/lib/nx-match.c >> @@ -31,6 +31,7 @@ >> #include "openvswitch/ofp-match.h" >> #include "openvswitch/ofp-port.h" >> #include "openvswitch/ofpbuf.h" >> +#include "openvswitch/util.h" >> #include "openvswitch/vlog.h" >> #include "packets.h" >> #include "openvswitch/shash.h" >> @@ -1491,9 +1492,15 @@ nx_put_entry(struct ofpbuf *b, const struct >> mf_field *mff, >> enum ofp_version version, const union mf_value *value, >> const union mf_value *mask) >> { >> + union mf_value aligned_mask; >> bool masked; >> int len, offset; >> + if (!IS_PTR_ALIGNED(mask)) { >> + memcpy(&aligned_mask, mask, mff->n_bytes); >> + mask = &aligned_mask; >> + } >> + >> len = mf_field_len(mff, value, mask, &masked); >> offset = mff->n_bytes - len; >> diff --git a/ofproto/ofproto-dpif-xlate.c >> b/ofproto/ofproto-dpif-xlate.c >> index 5806eacd2e74..c02dcfdc2bf8 100644 >> --- a/ofproto/ofproto-dpif-xlate.c >> +++ b/ofproto/ofproto-dpif-xlate.c >> @@ -7124,10 +7124,15 @@ do_xlate_actions(const struct ofpact *ofpacts, >> size_t ofpacts_len, >> /* Set the field only if the packet actually has it. */ >> if (mf_are_prereqs_ok(mf, flow, wc)) { >> - mf_mask_field_masked(mf, >> ofpact_set_field_mask(set_field), wc); >> - mf_set_flow_value_masked(mf, set_field->value, >> - >> ofpact_set_field_mask(set_field), >> - flow); >> + union mf_value *mask = ofpact_set_field_mask(set_field); >> + union mf_value aligned_mask; >> + >> + if (!IS_PTR_ALIGNED(mask)) { >> + memcpy(&aligned_mask, mask, mf->n_bytes); >> + mask = &aligned_mask; >> + } >> + mf_mask_field_masked(mf, mask, wc); >> + mf_set_flow_value_masked(mf, set_field->value, mask, >> flow); >> } else { >> xlate_report(ctx, OFT_WARN, >> "unmet prerequisites for %s, set_field >> ignored", >> > > Hi Dumitru, >
Hi Adrian, > I'm still trying to understand this patch properly, so don't consider > this a proper review, but I have an initial question. > > Does this problem only appear on "eth"? From what I can tell, it can only appear on "eth", at least in the current code. > Looking at: > > /* 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) > > It seems that the mask could be misaligned for all fields whose n_bytes > is not 4-byte aligned. If so, shouldn't we deal with this issue in > ofpact_set_field_mask (probably making it a function)? > It might be safer, but do you have a suggestion about how to do this nicely? I see a few options but I'm not really sure what's best: a. ofpact_set_field_mask() could always allocate an aligned 'union mf_value *mask' and return it. This seems inefficient. b. ofpact_set_field_mask() could use some per thread static storage and use that to store an aligned copy of the mask it would normally return and then return a pointer to that storage. This seems error-prone, if the caller retains pointers to the 'mask'. c. all callers of ofpact_set_field_mask() could pass some extra, aligned, storage that can be used to store a copy of the mask, if needed. This puts the burden on the callers and is also not necessarily nice and simple to follow. > Thanks Thanks, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
