mem-pool: Fix unaligned access causing fatal SIGBUS on sparc #2669
Conversation
|
Can one of the admins verify this patch? |
2 similar comments
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
CLANG-FORMAT FAILURE: index c51741f5b..5257cddcb 100644
--- a/libglusterfs/src/glusterfs/mem-pool.h
+++ b/libglusterfs/src/glusterfs/mem-pool.h
@@ -24,7 +24,7 @@
#include <sys/param.h> /* roundup */
#ifndef roundup
/* not a standard, so some platforms may not have it. Define a generic one */
-# define roundup(x, y) ((((x) + ((y) - 1)) / (y)) * (y))
+#define roundup(x, y) ((((x) + ((y)-1)) / (y)) * (y))
#endif
/*
@@ -38,7 +38,7 @@
#include <cmocka.h>
#endif
-typedef uint32_t gf_mem_magic_t;
+typedef uint32_t gf_mem_magic_t;
#define GF_MEM_TRAILER_SIZE sizeof(gf_mem_magic_t);
#define GF_MEM_HEADER_MAGIC 0xCAFEBABE
#define GF_MEM_TRAILER_MAGIC 0xBAADF00D
diff --git a/libglusterfs/src/mem-pool.c b/libglusterfs/src/mem-pool.c
index a8e448387..1c3fcba65 100644
--- a/libglusterfs/src/mem-pool.c
+++ b/libglusterfs/src/mem-pool.c
@@ -48,9 +48,8 @@ __gf_total_alloc_size(size_t req_size)
* Unaligned access can be fatal on SPARC with gcc compiled code. On
* others it can still be slow.
*/
- return roundup((req_size + GF_MEM_HEADER_SIZE),
- alignof(gf_mem_magic_t))
- + GF_MEM_TRAILER_SIZE;
+ return roundup((req_size + GF_MEM_HEADER_SIZE), alignof(gf_mem_magic_t)) +
+ GF_MEM_TRAILER_SIZE;
}
static void *
@@ -351,9 +350,9 @@ __gf_free(void *free_ptr)
// This points to a memory overrun
{
- void *end = ((char *)ptr) + __gf_total_alloc_size(header->size);
- gf_mem_magic_t *trailer = end - GF_MEM_TRAILER_SIZE;
- GF_ASSERT(GF_MEM_TRAILER_MAGIC == *trailer);
+ void *end = ((char *)ptr) + __gf_total_alloc_size(header->size);
+ gf_mem_magic_t *trailer = end - GF_MEM_TRAILER_SIZE;
+ GF_ASSERT(GF_MEM_TRAILER_MAGIC == *trailer);
}
LOCK(&mem_acct->rec[header->type].lock);
diff --git a/libglusterfs/src/unittest/mem_pool_unittest.c b/libglusterfs/src/unittest/mem_pool_unittest.c
index 67fbc72a1..b75ef0dcd 100644
--- a/libglusterfs/src/unittest/mem_pool_unittest.c
+++ b/libglusterfs/src/unittest/mem_pool_unittest.c
@@ -103,10 +103,9 @@ helper_check_memory_headers(char *mem, xlator_t *xl, size_t size, uint32_t type)
assert_int_equal(p->size, size);
assert_true(p->xl == xl);
assert_int_equal(p->header_magic, GF_MEM_HEADER_MAGIC);
- assert_true(*(gf_mem_magic_t *)(mem
- + roundup(sizeof(mem_header_t) + size,
- alignof(gf_mem_magic_t)))
- == GF_MEM_TRAILER_MAGIC);
+ assert_true(*(gf_mem_magic_t *)(mem + roundup(sizeof(mem_header_t) + size,
+ alignof(gf_mem_magic_t))) ==
+ GF_MEM_TRAILER_MAGIC);
}
/* |
| #include <sys/param.h> /* roundup */ | ||
| #ifndef roundup | ||
| /* not a standard, so some platforms may not have it. Define a generic one */ | ||
| # define roundup(x, y) ((((x) + ((y) - 1)) / (y)) * (y)) |
There was a problem hiding this comment.
Our allocation speed isn't the greatest (using tcmalloc and disabling memory pools seem to provide better performance) - are we sure we want to introduce division and multiplication to the allocation path?
There was a problem hiding this comment.
I'd be happy to update this patch to just remove the trailer magic and complexity associated. Faster in the general case, but does not preclude using other tools (runtime or compile) for debugging malloc.
There was a problem hiding this comment.
I think this is a question for a broader forum of the developer community than in a PR - can you send such a question to the devel mailing list?
There was a problem hiding this comment.
Worth noting the glibc roundup uses a GCC builtin, which IIRC is a single instruction on x86. Other platforms might have similar.
I've added this one for platform compatibility.
There was a problem hiding this comment.
Given that y should always be a power of 2, why not this:
#define roundup(_x, _y) (((_x) + (_y) - 1) & ~((_y) - 1))
|
CLANG-FORMAT FAILURE: index 67fbc72a1..b75ef0dcd 100644
--- a/libglusterfs/src/unittest/mem_pool_unittest.c
+++ b/libglusterfs/src/unittest/mem_pool_unittest.c
@@ -103,10 +103,9 @@ helper_check_memory_headers(char *mem, xlator_t *xl, size_t size, uint32_t type)
assert_int_equal(p->size, size);
assert_true(p->xl == xl);
assert_int_equal(p->header_magic, GF_MEM_HEADER_MAGIC);
- assert_true(*(gf_mem_magic_t *)(mem
- + roundup(sizeof(mem_header_t) + size,
- alignof(gf_mem_magic_t)))
- == GF_MEM_TRAILER_MAGIC);
+ assert_true(*(gf_mem_magic_t *)(mem + roundup(sizeof(mem_header_t) + size,
+ alignof(gf_mem_magic_t))) ==
+ GF_MEM_TRAILER_MAGIC);
}
/* |
|
Anyone know what that "strfmt errors in a 32 build" check is about? I can't find the actionable error in the console output? |
Perhaps clang formatting error? Try to run 'clang-format -i ' on your changes first. |
Ah, I'll tweak that once this CI run finishes :). They're not dependent loads, so either the compiler or CPU is free to order them better. So... maybe.. it doesn't matter. But I'll tidy it up. |
|
I've also tested removing the trailer that works too. I've asked on the list about which way would be better. One reply so far to retain the trailer functionality and fix it: https://lists.gluster.org/pipermail/gluster-devel/2021-July/057286.html So shall we go with the fix approach? Without this pull, glusterfs crashes in one of the first memory allocations. |
|
/run regression |
|
1 test(s) failed 1 test(s) generated core 3 test(s) needed retry 2 flaky test(s) marked as success even though they failed |
| @@ -32,7 +38,8 @@ | |||
| #include <cmocka.h> | |||
| #endif | |||
|
|
|||
| #define GF_MEM_TRAILER_SIZE 8 | |||
| typedef uint32_t gf_mem_magic_t; | |||
| #define GF_MEM_TRAILER_SIZE sizeof(gf_mem_magic_t); | |||
There was a problem hiding this comment.
Extra ; at the end of #define is redundant.
There was a problem hiding this comment.
Doh. New push in a sec.
|
/run regression |
|
1 test(s) failed 1 test(s) generated core 1 test(s) needed retry |
|
When I run that test locally, and it says: "Logs preserved in tarball inodelk-iteration-2.tar" - where has it left that? Doesn't seem to be inside my glusterfs/ code directory? |
|
|
/run regression |
1 similar comment
|
/run regression |
|
1 test(s) failed 1 test(s) generated core 2 test(s) needed retry |
From the backtrace, looks like it may be somehow related to this patch: Unsure. Not sure I fully understand the comment @ glusterfs/libglusterfs/src/mem-pool.c Line 111 in f73b84f |
|
It's annoying. I get this failure too on my SPARC64 box, but I can't get any useful information from the core (SPARC64 seems to optimise away a lot of the information needed to unpick the stack). Additionally, the SPARC64 is very slow at compiling, so the compile+test cycle takes /ages/. On a much faster, much more debug-friendly x86-64 this test either doesn't fail, or fails but without a crash/core. I'll keep picking away at this, in between other work. Valgrind would be useful. Is there a quick way to get the tests to use valgrind? |
Sure, reconfigure with |
Are you checking the configured core file location on your machine? |
Interesting. Not sure the comment is correct, both list_move and the underlying _list_del it calls write into the old list_head it appears: That's UB on realloc'ed memory that's been moved. |
The mem-pool code writes a trailer marker at the end of memory allocations to sanity check for memory write over-runs. The code does not align the marker on its required boundary though. Causing unaligned accesses. Unaligned access may be just slower on some platforms, but on others it can cause faults and abnormal exits. E.g., this was causing a SIGBUS on Linux/SPARC64. Possible approaches for a fix: 1. Round up the header+data size to the needed alignment for the trailer. 2. Use byte-by-byte memory accesses to write and read the trailer. Option 1 may sometimes leave a small gap between the end of the allocation and the trailer, a gap where memory write overruns of the allocations would not be detected by the trailer check. Additionally, Option 1 was found to be slower than Option 2. Discussion: gluster#2669 So fix the trailer via option 2.
Byte-level memory access will be just 1 memory read to cache, and I guess Intel is v good at fixing up unaligned access within a cache-line. I guess sometimes a trailer will straddle cachelines, but it probably had to fetch the memory from the allocation recently anyway. And if DEBUG is on it will (for the case of verifying the trailer on dealloc), in order to poison the memory in __gf_mem_invalidate. So then the removal of the roundup-calculations is the main difference? Was your performance with or without DEBUG? Likely much harder to see a difference with DEBUG? |
I've rebased and squashed the commits into just 3, with all of the alignment fix in one commit, and pushed that! |
I simply created a small program to test reading and writing to randomly aligned addresses in different ways, independent from Gluster. |
|
/run regression |
xlators/cluster/dht/src/dht-rename.c
Outdated
|
|
||
| if (subvol_cnt >= 0) | ||
| local->ret_cache[subvol_cnt] = op_ret; |
There was a problem hiding this comment.
This is from another patch it shouldn't be here.
|
0 test(s) failed 1 test(s) generated core 4 test(s) needed retry 2 flaky test(s) marked as success even though they failed |
|
/run regression |
|
1 test(s) failed 0 test(s) generated core 3 test(s) needed retry 1 flaky test(s) marked as success even though they failed |
Can put a ZLA at the end of the mem header to automatically get generally the optimal padding at the end of the struct for the following data.
|
I don't see how this unit test gets run. My own make check locally doesn't seem to compile / run it. I don't see the file name referenced anywhere? |
|
/run regression |
|
/run full regression |
The mem-pool code writes a trailer marker at the end of memory allocations to sanity check for memory write over-runs. The code does not align the marker on its required boundary though. Causing unaligned accesses. Unaligned access may be just slower on some platforms, but on others it can cause faults and abnormal exits. E.g., this was causing a SIGBUS on Linux/SPARC64. Possible approaches for a fix: 1. Round up the header+data size to the needed alignment for the trailer. 2. Use byte-by-byte memory accesses to write and read the trailer. Option 1 may sometimes leave a small gap between the end of the allocation and the trailer, a gap where memory write overruns of the allocations would not be detected by the trailer check. Additionally, Option 1 was found to be slower than Option 2. Discussion: gluster#2669 So fix the trailer via option 2.
|
CLANG-FORMAT FAILURE: index 3db727758..84242ca36 100644
--- a/libglusterfs/src/unittest/mem_pool_unittest.c
+++ b/libglusterfs/src/unittest/mem_pool_unittest.c
@@ -44,15 +44,16 @@ typedef struct {
int
gf_mem_set_acct_info(xlator_t *xl, char **alloc_ptr, size_t size, uint32_t type,
const char *typestr);
-size_t __gf_total_alloc_size(size_t req_size);
-void __gf_mem_trailer_write(uint8_t *trailer)
-gf_mem_magic_t __gf_mem_trailer_read(uint8_t *trailer)
-
-/*
- * Helper functions
- */
-static xlator_t *
-helper_xlator_init(uint32_t num_types)
+size_t
+__gf_total_alloc_size(size_t req_size);
+void
+__gf_mem_trailer_write(uint8_t *trailer) gf_mem_magic_t
+ __gf_mem_trailer_read(uint8_t *trailer)
+
+ /*
+ * Helper functions
+ */
+ static xlator_t *helper_xlator_init(uint32_t num_types)
{
xlator_t *xl;
int i, ret;
@@ -105,8 +106,8 @@ helper_check_memory_headers(char *mem, xlator_t *xl, size_t size, uint32_t type)
assert_int_equal(p->size, size);
assert_true(p->xl == xl);
assert_int_equal(p->header_magic, GF_MEM_HEADER_MAGIC);
- assert_true(__gf_mem_trailer_read((uint8_t *)mem + sizeof(mem_header_t) + size) ==
- GF_MEM_TRAILER_MAGIC);
+ assert_true(__gf_mem_trailer_read((uint8_t *)mem + sizeof(mem_header_t) +
+ size) == GF_MEM_TRAILER_MAGIC);
}
/* |
The mem-pool code writes a trailer marker at the end of memory allocations to sanity check for memory write over-runs. The code does not align the marker on its required boundary though. Causing unaligned accesses. Unaligned access may be just slower on some platforms, but on others it can cause faults and abnormal exits. E.g., this was causing a SIGBUS on Linux/SPARC64. Possible approaches for a fix: 1. Round up the header+data size to the needed alignment for the trailer. 2. Use byte-by-byte memory accesses to write and read the trailer. Option 1 may sometimes leave a small gap between the end of the allocation and the trailer, a gap where memory write overruns of the allocations would not be detected by the trailer check. Additionally, Option 1 was found to be slower than Option 2. Discussion: gluster#2669 So fix the trailer via option 2.
As per mykaul's suggestion, try re-arrange the struct to minimise padding. Move the obviously larger types to earlier in the struct.
|
5 test(s) needed retry 2 flaky test(s) marked as success even though they failed |
|
Can we merge this now? I addressed that comment of @xhernandez's by rebasing, but github doesn't allow me to to resolve it because that commit/file obviously isn't there anymore. |
|
/run full regression |
|
/run regression |
|
I checked @xhernandez's request for changes, and seems like thats now addressed. And hence merging this. |
The mem-pool code writes a trailer marker at the end of memory allocations
to sanity check for memory write over-runs. The code does not align the
marker on its required boundary though. This causes unaligned accesses.
Unaligned access may be just slower on some platforms, but on others it can
cause faults and abnormal exits. E.g., this was causing a SIGBUS on
Linux/SPARC64.
Fix by rounding up the header+data size to the needed alignment for the
trailer.
Another solution might be just to remove the trailer code, and rely on other
malloc debugging tools. E.g., LD_PRELOADable malloc debug libraries, GCC
ASAN and so on, valgrind, etc.