| Message ID | 20240502132824.1917458-1-amusil@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v2] hash, jhash: Fix unaligned access to the hash remainder. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | fail | test: fail |
On 5/2/24 15:28, Ales Musil wrote: > The pointer was passed to memcpy as uin32_t *, however the hash bytes > might be unaligned at that point. Case it to uint8_t * instead 'Case' ? > which has only single byte alignment requirement. This seems to be > a false positive reported by clang [0]. After thinking some more, it's not actually a false positive per se. According to the C spec we're not actually allowed to have misaligned pointers even if we're not reading/writing through them. So, technically, the initial cast to uint32_t pointer is no correct. I don't think we can fully avoid such casts without loosing type checking, but I think we need to revert changes to hash functions made in commit db5a101931c5 ("clang: Fix the alignment warning."). i.e. we should go back to using uint8_t pointer and cast it on the get_unaligned_u32() call with ALIGNED_CAST. We will still have a misaligned pointer, but it will be immediately cast back, so should cause less issues. Note: all arithmetic should be done on the uint8_t pointer, not a misaligned uin32_t one to avoid potential other UB conditions. Best regards, Ilya Maximets. > > lib/hash.c:46:22: runtime error: load of misaligned address > 0x507000000065 for type 'const uint32_t *' (aka 'const unsigned int *'), > which requires 4 byte alignment > 0x507000000065: note: pointer points here > 73 62 2e 73 6f 63 6b 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > ^ > 00 00 00 00 00 00 00 00 00 > 0 0x6191cb in hash_bytes ovs/lib/hash.c:46:9 > 1 0x69d064 in hash_string ovs/lib/hash.h:404:12 > 2 0x69d064 in hash_name ovs/lib/shash.c:29:12 > 3 0x69d064 in shash_find ovs/lib/shash.c:237:49 > 4 0x69dada in shash_find_data ovs/lib/shash.c:251:31 > 5 0x507987 in add_remote ovs/ovsdb/ovsdb-server.c:1382:15 > 6 0x507987 in parse_options ovs/ovsdb/ovsdb-server.c:2659:13 > 7 0x507987 in main ovs/ovsdb/ovsdb-server.c:751:5 > > SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior lib/hash.c:46:22 > > [0] https://github.com/llvm/llvm-project/issues/90848 > Signed-off-by: Ales Musil <amusil@redhat.com> > --- > lib/hash.c | 2 +- > lib/jhash.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/lib/hash.c b/lib/hash.c > index c722f3c3c..986fa6643 100644 > --- a/lib/hash.c > +++ b/lib/hash.c > @@ -43,7 +43,7 @@ hash_bytes(const void *p_, size_t n, uint32_t basis) > if (n) { > uint32_t tmp = 0; > > - memcpy(&tmp, p, n); > + memcpy(&tmp, (const uint8_t *) p, n); > hash = hash_add(hash, tmp); > } > > diff --git a/lib/jhash.c b/lib/jhash.c > index c59b51b61..0a0628589 100644 > --- a/lib/jhash.c > +++ b/lib/jhash.c > @@ -114,7 +114,7 @@ jhash_bytes(const void *p_, size_t n, uint32_t basis) > uint32_t tmp[3]; > > tmp[0] = tmp[1] = tmp[2] = 0; > - memcpy(tmp, p, n); > + memcpy(tmp, (const uint8_t *) p, n); > a += tmp[0]; > b += tmp[1]; > c += tmp[2];
On Thu, May 2, 2024 at 8:03 PM Ilya Maximets <i.maximets@ovn.org> wrote: > On 5/2/24 15:28, Ales Musil wrote: > > The pointer was passed to memcpy as uin32_t *, however the hash bytes > > might be unaligned at that point. Case it to uint8_t * instead > > 'Case' ? > > > which has only single byte alignment requirement. This seems to be > > a false positive reported by clang [0]. > > After thinking some more, it's not actually a false positive per se. > According to the C spec we're not actually allowed to have misaligned > pointers even if we're not reading/writing through them. > > So, technically, the initial cast to uint32_t pointer is no correct. > I don't think we can fully avoid such casts without loosing type checking, > but I think we need to revert changes to hash functions made in > commit db5a101931c5 ("clang: Fix the alignment warning."). > i.e. we should go back to using uint8_t pointer and cast it on the > get_unaligned_u32() call with ALIGNED_CAST. We will still have a > misaligned pointer, but it will be immediately cast back, so should > cause less issues. > > Note: all arithmetic should be done on the uint8_t pointer, not a > misaligned uin32_t one to avoid potential other UB conditions. > > Best regards, Ilya Maximets. > Makes sense, done in v3. > > > > > lib/hash.c:46:22: runtime error: load of misaligned address > > 0x507000000065 for type 'const uint32_t *' (aka 'const unsigned int *'), > > which requires 4 byte alignment > > 0x507000000065: note: pointer points here > > 73 62 2e 73 6f 63 6b 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > > ^ > > 00 00 00 00 00 00 00 00 00 > > 0 0x6191cb in hash_bytes ovs/lib/hash.c:46:9 > > 1 0x69d064 in hash_string ovs/lib/hash.h:404:12 > > 2 0x69d064 in hash_name ovs/lib/shash.c:29:12 > > 3 0x69d064 in shash_find ovs/lib/shash.c:237:49 > > 4 0x69dada in shash_find_data ovs/lib/shash.c:251:31 > > 5 0x507987 in add_remote ovs/ovsdb/ovsdb-server.c:1382:15 > > 6 0x507987 in parse_options ovs/ovsdb/ovsdb-server.c:2659:13 > > 7 0x507987 in main ovs/ovsdb/ovsdb-server.c:751:5 > > > > SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior lib/hash.c:46:22 > > > > [0] https://github.com/llvm/llvm-project/issues/90848 > > Signed-off-by: Ales Musil <amusil@redhat.com> > > --- > > lib/hash.c | 2 +- > > lib/jhash.c | 2 +- > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/lib/hash.c b/lib/hash.c > > index c722f3c3c..986fa6643 100644 > > --- a/lib/hash.c > > +++ b/lib/hash.c > > @@ -43,7 +43,7 @@ hash_bytes(const void *p_, size_t n, uint32_t basis) > > if (n) { > > uint32_t tmp = 0; > > > > - memcpy(&tmp, p, n); > > + memcpy(&tmp, (const uint8_t *) p, n); > > hash = hash_add(hash, tmp); > > } > > > > diff --git a/lib/jhash.c b/lib/jhash.c > > index c59b51b61..0a0628589 100644 > > --- a/lib/jhash.c > > +++ b/lib/jhash.c > > @@ -114,7 +114,7 @@ jhash_bytes(const void *p_, size_t n, uint32_t basis) > > uint32_t tmp[3]; > > > > tmp[0] = tmp[1] = tmp[2] = 0; > > - memcpy(tmp, p, n); > > + memcpy(tmp, (const uint8_t *) p, n); > > a += tmp[0]; > > b += tmp[1]; > > c += tmp[2]; > > Thanks, Ales
diff --git a/lib/hash.c b/lib/hash.c index c722f3c3c..986fa6643 100644 --- a/lib/hash.c +++ b/lib/hash.c @@ -43,7 +43,7 @@ hash_bytes(const void *p_, size_t n, uint32_t basis) if (n) { uint32_t tmp = 0; - memcpy(&tmp, p, n); + memcpy(&tmp, (const uint8_t *) p, n); hash = hash_add(hash, tmp); } diff --git a/lib/jhash.c b/lib/jhash.c index c59b51b61..0a0628589 100644 --- a/lib/jhash.c +++ b/lib/jhash.c @@ -114,7 +114,7 @@ jhash_bytes(const void *p_, size_t n, uint32_t basis) uint32_t tmp[3]; tmp[0] = tmp[1] = tmp[2] = 0; - memcpy(tmp, p, n); + memcpy(tmp, (const uint8_t *) p, n); a += tmp[0]; b += tmp[1]; c += tmp[2];
The pointer was passed to memcpy as uin32_t *, however the hash bytes might be unaligned at that point. Case it to uint8_t * instead which has only single byte alignment requirement. This seems to be a false positive reported by clang [0]. lib/hash.c:46:22: runtime error: load of misaligned address 0x507000000065 for type 'const uint32_t *' (aka 'const unsigned int *'), which requires 4 byte alignment 0x507000000065: note: pointer points here 73 62 2e 73 6f 63 6b 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ^ 00 00 00 00 00 00 00 00 00 0 0x6191cb in hash_bytes ovs/lib/hash.c:46:9 1 0x69d064 in hash_string ovs/lib/hash.h:404:12 2 0x69d064 in hash_name ovs/lib/shash.c:29:12 3 0x69d064 in shash_find ovs/lib/shash.c:237:49 4 0x69dada in shash_find_data ovs/lib/shash.c:251:31 5 0x507987 in add_remote ovs/ovsdb/ovsdb-server.c:1382:15 6 0x507987 in parse_options ovs/ovsdb/ovsdb-server.c:2659:13 7 0x507987 in main ovs/ovsdb/ovsdb-server.c:751:5 SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior lib/hash.c:46:22 [0] https://github.com/llvm/llvm-project/issues/90848 Signed-off-by: Ales Musil <amusil@redhat.com> --- lib/hash.c | 2 +- lib/jhash.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)