| Message ID | 20240503054413.2098114-1-amusil@redhat.com |
|---|---|
| State | Accepted |
| Commit | f0e0e48ec51f06ff67ed6ebd824674a7794e4f7e |
| Delegated to: | Ilya Maximets |
| Headers | show |
| Series | [ovs-dev,v3] 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 | success | test: success |
On 3 May 2024, at 7:44, Ales Musil wrote: > Partially revert db5a101931c5, this was to avoid warning, however we > shouldn't use pointer to "uint32_t" when the data are potentially > unaligned [0]. Use pointer to "uint8_t" right from the start, this > requires us to use ALIGNED_CAST for the get_unaligned_u32, which is > fine in that case, because the function uses > " __attribute__((__packed__))" struct to access the underlying "uint32_t". > > 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 > Fixes: db5a101931c5 ("clang: Fix the alignment warning.") > Signed-off-by: Ales Musil <amusil@redhat.com> > --- > v3: Do partial revert of db5a101931c5 instead of simple cast. Looks good to me. Acked-by: Eelco Chaudron <echaudro@redhat.com>
On Fri, May 03, 2024 at 07:44:13AM +0200, Ales Musil wrote: > Partially revert db5a101931c5, this was to avoid warning, however we > shouldn't use pointer to "uint32_t" when the data are potentially > unaligned [0]. Use pointer to "uint8_t" right from the start, this > requires us to use ALIGNED_CAST for the get_unaligned_u32, which is > fine in that case, because the function uses > " __attribute__((__packed__))" struct to access the underlying "uint32_t". > > 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 > Fixes: db5a101931c5 ("clang: Fix the alignment warning.") > Signed-off-by: Ales Musil <amusil@redhat.com> > --- > v3: Do partial revert of db5a101931c5 instead of simple cast. Acked-by: Simon Horman <horms@ovn.org>
On 5/3/24 13:44, Simon Horman wrote: > On Fri, May 03, 2024 at 07:44:13AM +0200, Ales Musil wrote: >> Partially revert db5a101931c5, this was to avoid warning, however we >> shouldn't use pointer to "uint32_t" when the data are potentially >> unaligned [0]. Use pointer to "uint8_t" right from the start, this >> requires us to use ALIGNED_CAST for the get_unaligned_u32, which is >> fine in that case, because the function uses >> " __attribute__((__packed__))" struct to access the underlying "uint32_t". >> >> 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 >> Fixes: db5a101931c5 ("clang: Fix the alignment warning.") >> Signed-off-by: Ales Musil <amusil@redhat.com> >> --- >> v3: Do partial revert of db5a101931c5 instead of simple cast. > > Acked-by: Simon Horman <horms@ovn.org> > Thanks, Ales, Eelco and Simon! Applied and backported down to 2.17. Best regards, Ilya Maximets.
diff --git a/lib/hash.c b/lib/hash.c index c722f3c3c..3d574de9b 100644 --- a/lib/hash.c +++ b/lib/hash.c @@ -29,15 +29,16 @@ hash_3words(uint32_t a, uint32_t b, uint32_t c) uint32_t hash_bytes(const void *p_, size_t n, uint32_t basis) { - const uint32_t *p = p_; + const uint8_t *p = p_; size_t orig_n = n; uint32_t hash; hash = basis; while (n >= 4) { - hash = hash_add(hash, get_unaligned_u32(p)); + hash = hash_add(hash, + get_unaligned_u32(ALIGNED_CAST(const uint32_t *, p))); n -= 4; - p += 1; + p += 4; } if (n) { diff --git a/lib/jhash.c b/lib/jhash.c index c59b51b61..a8e3f457b 100644 --- a/lib/jhash.c +++ b/lib/jhash.c @@ -96,18 +96,18 @@ jhash_words(const uint32_t *p, size_t n, uint32_t basis) uint32_t jhash_bytes(const void *p_, size_t n, uint32_t basis) { - const uint32_t *p = p_; + const uint8_t *p = p_; uint32_t a, b, c; a = b = c = 0xdeadbeef + n + basis; while (n >= 12) { - a += get_unaligned_u32(p); - b += get_unaligned_u32(p + 1); - c += get_unaligned_u32(p + 2); + a += get_unaligned_u32(ALIGNED_CAST(const uint32_t *, p)); + b += get_unaligned_u32(ALIGNED_CAST(const uint32_t *, p + 4)); + c += get_unaligned_u32(ALIGNED_CAST(const uint32_t *, p + 8)); jhash_mix(&a, &b, &c); n -= 12; - p += 3; + p += 12; } if (n) {
Partially revert db5a101931c5, this was to avoid warning, however we shouldn't use pointer to "uint32_t" when the data are potentially unaligned [0]. Use pointer to "uint8_t" right from the start, this requires us to use ALIGNED_CAST for the get_unaligned_u32, which is fine in that case, because the function uses " __attribute__((__packed__))" struct to access the underlying "uint32_t". 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 Fixes: db5a101931c5 ("clang: Fix the alignment warning.") Signed-off-by: Ales Musil <amusil@redhat.com> --- v3: Do partial revert of db5a101931c5 instead of simple cast. --- lib/hash.c | 7 ++++--- lib/jhash.c | 10 +++++----- 2 files changed, 9 insertions(+), 8 deletions(-)