| Message ID | 20220225171417.16476.92374.stgit@dceara.remote.csb |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Fix UndefinedBehaviorSanitizer reported issues and enable it in CI. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/intel-ovs-compilation | success | test: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
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 <dceara@redhat.com> > --- > 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, 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"? 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)? Thanks
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 <dceara@redhat.com> >> --- >> 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
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 <dceara@redhat.com> >>> --- >>> 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 ^ >> 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: >> #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? >> Thanks > > Thanks, > Dumitru > Thanks
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 <dceara@redhat.com> >>>> --- >>>> 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? Thanks!
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 <dceara@redhat.com> >>>>> --- >>>>> 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.
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 <dceara@redhat.com> >>>>>> --- >>>>>> 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!
On Tue, Mar 1, 2022 at 7:54 AM Dumitru Ceara <dceara@redhat.com> 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 <dceara@redhat.com> > >>>>>> --- > >>>>>> 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
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",
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 <dceara@redhat.com> --- 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(-)