On Tue, Mar 1, 2022 at 7:54 AM Dumitru Ceara <[email protected]> wrote: > > On 3/1/22 13:44, Adrian Moreno wrote: > > > > > > On 3/1/22 12:59, Dumitru Ceara wrote: > >> On 3/1/22 12:54, Adrian Moreno wrote: > >>> > >>> > >>> On 3/1/22 10:35, Dumitru Ceara wrote: > >>>> 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. > >>>> > >>> > >>> I can quickly reproduce it with: > >>> > >>> ./utilities/ovs-ofctl -O OpenFlow15 parse-flow "tcp > >>> actions=set_field:0x00ab/0x00ff->tcp_src" > >>> > >>> lib/meta-flow.c:1370:37: runtime error: member access within misaligned > >>> address 0x000002a247d2 for type 'const union mf_value', which requires 8 > >>> byte alignment > >>> 0x000002a247d2: note: pointer points here > >>> 00 00 00 ab 00 ff 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > >>> 00 00 > >>> 00 00 00 31 00 00 00 00 00 > >>> ^ > >>> > >>> lib/nx-match.c:1501:60: runtime error: member access within misaligned > >>> address 0x000002a247d2 for type 'const union mf_value', which requires 8 > >>> byte alignment > >>> 0x000002a247d2: note: pointer points here > >>> 00 00 00 ab 00 ff 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > >>> 00 00 > >>> 00 00 00 31 00 00 00 00 00 > >>> ^ > >>> > >> > >> Oops you're right, we should add a test case. > >> > >>> > >>>>> 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. > >>>> > >>> > >>> I agree neither are perfect. I need a bit more thinking. > >>> > >>> I'm not familiar with this part of the code but, would it be possible to > >>> insert padding between the value and the mask in "ofpact_put_set_field"? > >>> I'm not sure if it's possible to add some extra metadata to the > >>> "ofpact_set_field" structure so that it can be retrieved by: > >>> > >> > >> I think the problem with this is that it breaks compatibility with > >> already existing controllers. I'm afraid we just need to figure out > >> when it's safe (aligned) to access the mask or otherwise copy it. > >> > >>>>> #define ofpact_set_field_mask(SF) \ > >>>>> ALIGNED_CAST(union mf_value *, \ > >>>>> (uint8_t *)(SF)->value + (SF)->padding + \ > >>> (SF)->field->n_bytes) > >>> > >>> or maybe just always round up to the nearest 8byte address. Is this > >>> possible? > >>> > >> > >> I'm not sure I understand this part. Could you please share some more > >> details? > > > > Maybe it just doesn't make sense :-) > > > > I don't really know if the mask is misaligned in the OpenFlow packet > > but, it can definitely be so in the struct ofpact_set_field which, > > AFAIK, is the generic > > representation of an action (independent of OpenFlow version). > > > > So, even if the controller sends the same openflow packet, > > "ofpact_put_set_field" could then pad the mask to the closest 8 byte > > address when adding it to "ofpact_set_field". (Currently it just > > concatenates the value and the mask) > > > > If we do that, the problem becomes: how do we know the new address of > > the mask? One option would be to add a metadata field in > > ofpact_put_set_field (e.g: uint8_t padding). This field would of course > > not be encoded back to the openflow format. If padding has been added, > > that field would indicate how much and the way to calculate the mask > > would be the macro example above. > > > > But the value of that field is a bit redundant since you could always do > > something like this: > > #define ofpact_set_field_mask(SF) \ > > ALIGNED_CAST(union mf_value *, \ > > (uint8_t *)(SF)->value + ROUND_UP((SF)->field->n_bytes, 8)) > > > > I think that would ensure the mask is always aligned without touching > > the callers. But again, it might not make sense at all. > > I think this would work. I'll try it out and let you know how it goes.
The union to ensure alignment is probably a good idea, if it is technically feasible. Maybe the next release of OpenVPN should perform a major version bump so it can break an ABI and force plugins to recompile. OpenSSL had similar warnings several years ago. OpenSSL produced about 34k warnings due to casting to/from uint64_t in its lhast_st structure. See https://mta.openssl.org/pipermail/openssl-dev/2016-March/006391.html . The union fixed things for OpenSSL. See further into the thread at https://mta.openssl.org/pipermail/openssl-dev/2016-March/006403.html . Jeff _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
