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. Thanks a lot! _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
