| Message ID | 20121027.002724.1098296946918586594.davem@davemloft.net |
|---|---|
| State | New |
| Headers | show |
On Fri, Oct 26, 2012 at 9:27 PM, David Miller <davem@davemloft.net> wrote: > > I'm getting a SIGBUS on every backtrace libbacktrace generates > on 32-bit sparc builds. The crashes usually happen in > add_function_range(), where 'p' is not 8-byte aligned. > > It seems that the vector code doesn't take care to align the pointers > it returns. I cribbed the size alignment done in mmap.c's > implementation of backtrace_alloc() to fix this. Sorry about the problem, but I don't see how this can be the right fix. A single vector will always be an array of the same struct, so I don't see how any individual struct can be misaligned. It seems like increasing the requested size is just going to consistently misalign any struct that does not require 8 byte alignment, so later references into the vector using an index will fail. Also backtrace_vector_grow is based on top of backtrace_alloc, so again aligning the size shouldn't matter. The struct used by add_function_range is struct function_addrs { uint64_t low; uint64_t high; struct function *function; }; So on a 32-bit system, this should have a size of 20 if uint64_t requires 4-byte alignment, but it should have a size of 24 if uint64_t requires 8-byte alignment. It sounds like uint64_t requires 8-byte alignment, so the size of this struct should be 24, so your patch shouldn't change matters. Since your patch presumably works, it sounds like sizeof (struct function_addrs) is returning 20, but that does not make sense. It would mean that allocating an array of struct function_addrs wouldn't work correctly. So I don't know what is going on. Ian
From: Ian Lance Taylor <iant@google.com> Date: Sat, 27 Oct 2012 21:06:59 -0700 > The struct used by add_function_range is > > struct function_addrs > { > uint64_t low; > uint64_t high; > struct function *function; > }; > > So on a 32-bit system, this should have a size of 20 if uint64_t > requires 4-byte alignment, but it should have a size of 24 if uint64_t > requires 8-byte alignment. It sounds like uint64_t requires 8-byte > alignment, so the size of this struct should be 24, so your patch > shouldn't change matters. Since your patch presumably works, it > sounds like sizeof (struct function_addrs) is returning 20, but that > does not make sense. It would mean that allocating an array of struct > function_addrs wouldn't work correctly. > > So I don't know what is going on. The size is 24, and my patch definitely makes the crashes go away. It seems like a vector is being used for a mixed set of objects. I'll try to figure out how that is happening.
From: David Miller <davem@davemloft.net> Date: Sun, 28 Oct 2012 00:31:27 -0400 (EDT) > The size is 24, and my patch definitely makes the crashes go away. > > It seems like a vector is being used for a mixed set of objects. > I'll try to figure out how that is happening. Ok, the problem seems to have to do with releases. The releases place vector memory chunks into a global pool. So a memory chunk from a vector used for one type of object, can be sucked into and used by another vector. But the alignment requirements are different, so we can obtain a chunk from the freelist that was being used for a vector of 4-byte aligned objects. The crash sequences are always of the form: vec_release(0xffb37ac8) base+size(0xf0199008) amount(312) ... vec_grow(0xffb37ac8:24) from 0x975168, ret=0xf01754cc [size(24):alc(360)] That size alignment done by backtrace_alloc() has no influence upon this issue. Since chunks are released from wherever the vector's allocation point was at the time of the release. In fact I bet that alignment in backtrace_alloc() never triggers when it is invoked from backtrace_vector_grow().
On Sat, Oct 27, 2012 at 10:12 PM, David Miller <davem@davemloft.net> wrote: > From: David Miller <davem@davemloft.net> > Date: Sun, 28 Oct 2012 00:31:27 -0400 (EDT) > >> The size is 24, and my patch definitely makes the crashes go away. >> >> It seems like a vector is being used for a mixed set of objects. >> I'll try to figure out how that is happening. > > Ok, the problem seems to have to do with releases. > > The releases place vector memory chunks into a global pool. > > So a memory chunk from a vector used for one type of object, > can be sucked into and used by another vector. > > But the alignment requirements are different, so we can > obtain a chunk from the freelist that was being used for > a vector of 4-byte aligned objects. > > The crash sequences are always of the form: > > vec_release(0xffb37ac8) base+size(0xf0199008) amount(312) > ... > vec_grow(0xffb37ac8:24) from 0x975168, ret=0xf01754cc [size(24):alc(360)] > > That size alignment done by backtrace_alloc() has no influence upon > this issue. Since chunks are released from wherever the vector's > allocation point was at the time of the release. > > In fact I bet that alignment in backtrace_alloc() never triggers when > it is invoked from backtrace_vector_grow(). Thanks for tracking it down. This patch should fix it. Bootstrapped and ran libbacktrace testsuite on x86_64-unknown-linux-gnu. Committed to mainline. Ian 2012-10-29 Ian Lance Taylor <iant@google.com> * mmap.c (backtrace_vector_release): Make sure freed block is aligned on 8-byte boundary.
From: Ian Lance Taylor <iant@google.com> Date: Mon, 29 Oct 2012 08:43:42 -0700 > This patch should fix it. Bootstrapped and ran libbacktrace testsuite > on x86_64-unknown-linux-gnu. Committed to mainline. I can tell just by looking at your patch that it doesn't fix the problem. It's "vec->base + vec->size" that's not aligned, rather than the length. All your change does it align the length term. You have to align the vec->size term in the pointer computation. Actually you need to align both. First you have to align vec->size to an 8 byte boundary. Then you have to align the remaining length that results to an 8 byte boundary as well. Can you post a test patch for me this time, instead of just committing a change which hasn't been tested to fix the problem? I'd very much appreciate that. Thanks.
On Mon, Oct 29, 2012 at 11:03 AM, David Miller <davem@davemloft.net> wrote: > From: Ian Lance Taylor <iant@google.com> > Date: Mon, 29 Oct 2012 08:43:42 -0700 > >> This patch should fix it. Bootstrapped and ran libbacktrace testsuite >> on x86_64-unknown-linux-gnu. Committed to mainline. > > I can tell just by looking at your patch that it doesn't fix the > problem. > > It's "vec->base + vec->size" that's not aligned, rather than the > length. All your change does it align the length term. > > You have to align the vec->size term in the pointer computation. I did. I changed it from vec->base + vec->size. Now it is vec->base + size, where size is the aligned version of vec->size. I know that vec->base is aligned. Now that size is aligned, I know that vec->base + size is aligned. > First you have to align vec->size to an 8 byte boundary. Then > you have to align the remaining length that results to an 8 > byte boundary as well. Yes, the patch does both of those things. > Can you post a test patch for me this time, instead of just committing > a change which hasn't been tested to fix the problem? I'd very much > appreciate that. Sure, if you can confirm that this patch fails, I will take another look. Ian
From: Ian Lance Taylor <iant@google.com> Date: Mon, 29 Oct 2012 11:13:19 -0700 > Sure, if you can confirm that this patch fails, I will take another look. It fails. Same problem, SIGBUS.
From: Ian Lance Taylor <iant@google.com> Date: Mon, 29 Oct 2012 11:13:19 -0700 > I changed it from vec->base + vec->size. Now it is vec->base > + size, where size is the aligned version of vec->size. It is not. You don't modify 'size' at all.
diff --git a/libbacktrace/alloc.c b/libbacktrace/alloc.c index 501f386..59072ed 100644 --- a/libbacktrace/alloc.c +++ b/libbacktrace/alloc.c @@ -78,6 +78,10 @@ backtrace_vector_grow (struct backtrace_state *state ATTRIBUTE_UNUSED, { void *ret; + /* Round for alignment; we assume that no type we care about + is more than 8 bytes. */ + size = (size + 7) & ~ (size_t) 7; + if (size > vec->alc) { size_t alc; diff --git a/libbacktrace/mmap.c b/libbacktrace/mmap.c index e07810d..6e51a0d 100644 --- a/libbacktrace/mmap.c +++ b/libbacktrace/mmap.c @@ -175,6 +175,10 @@ backtrace_vector_grow (struct backtrace_state *state,size_t size, { void *ret; + /* Round for alignment; we assume that no type we care about + is more than 8 bytes. */ + size = (size + 7) & ~ (size_t) 7; + if (size > vec->alc) { size_t pagesize;