Re: [PATCH] BUG/CRITICAL: SIGBUS crash on aarch64
On Thu, Nov 15, 2018 at 02:26:59PM +0100, Willy Tarreau wrote: > On Thu, Nov 15, 2018 at 11:58:36AM +0100, Olivier Houchard wrote: > > Willy, can you push the attached patch ? > > Applied, thanks. I've just slightly edited it to put parenthesis around > "i" below : > > > +#define round_ptr_size(i) ((i + (sizeof(void *) - 1)) &~ (sizeof(void *) - > > 1)) > > so that if someone decides to do round_ptr_size(x & mask) he doesn't end up > doing (x & (mask + sizeof(void*)-1)) but ((x & mask) + sizeof(...)) > > :-) > I give up, macros are way too complicated. Olivier
Re: [PATCH] BUG/CRITICAL: SIGBUS crash on aarch64
On Thu, Nov 15, 2018 at 11:58:36AM +0100, Olivier Houchard wrote: > Willy, can you push the attached patch ? Applied, thanks. I've just slightly edited it to put parenthesis around "i" below : > +#define round_ptr_size(i) ((i + (sizeof(void *) - 1)) &~ (sizeof(void *) - > 1)) so that if someone decides to do round_ptr_size(x & mask) he doesn't end up doing (x & (mask + sizeof(void*)-1)) but ((x & mask) + sizeof(...)) :-) Willy
Re: [PATCH] BUG/CRITICAL: SIGBUS crash on aarch64
On Thu, Nov 15, 2018 at 09:48:20AM +, Paul Martin wrote:
> On Wed, Nov 14, 2018 at 06:05:00PM +0100, Olivier Houchard wrote:
>
> > Oops, you're right indeed.
> > I'm not sure I'm a big fan of special-casing STD_T_UINT. For example,
> > STD_T_FRQP is probably 12bytes too, so it'd be a problem.
> > Can you test the (untested, but hopefully right) patch attached ?
>
> Yes, your patch works on aarch64 too.
>
Thanks for testing !
Willy, can you push the attached patch ?
Olivier
>From 521b6527c71921a96ba4134048f6d9ea1d2689d3 Mon Sep 17 00:00:00 2001
From: Olivier Houchard
Date: Wed, 14 Nov 2018 17:54:36 +0100
Subject: [PATCH] BUG/MEDIUM: Make sure stksess is properly aligned.
When we allocate struct stksess, we also allocate memory to store the
associated data before the struct itself.
As the data can be of different types, they can have different size. However,
we need the struct stksess to be properly aligned, as it can do 64bits
load/store (including atomic load/stores) on 64bits platforms, and some of
them doesn't support unaligned access.
So, when allocating the struct stksess, round the size up to the next
multiple of sizeof(void *), and make sure the struct stksess itself is
properly aligned.
Many thanks to Paul Martin for investigating and reporting that bug.
This should be backported to earlier releases.
---
src/stick_table.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/src/stick_table.c b/src/stick_table.c
index 7f141631..4ec7861e 100644
--- a/src/stick_table.c
+++ b/src/stick_table.c
@@ -45,6 +45,7 @@
/* structure used to return a table key built from a sample */
static THREAD_LOCAL struct stktable_key static_table_key;
+#define round_ptr_size(i) ((i + (sizeof(void *) - 1)) &~ (sizeof(void *) - 1))
/*
* Free an allocated sticky session , and decrease sticky sessions counter
* in table .
@@ -52,7 +53,7 @@ static THREAD_LOCAL struct stktable_key static_table_key;
void __stksess_free(struct stktable *t, struct stksess *ts)
{
t->current--;
- pool_free(t->pool, (void *)ts - t->data_size);
+ pool_free(t->pool, (void *)ts - round_ptr_size(t->data_size));
}
/*
@@ -230,7 +231,7 @@ struct stksess *__stksess_new(struct stktable *t, struct
stktable_key *key)
ts = pool_alloc(t->pool);
if (ts) {
t->current++;
- ts = (void *)ts + t->data_size;
+ ts = (void *)ts + round_ptr_size(t->data_size);
__stksess_init(t, ts);
if (key)
stksess_setkey(t, ts, key);
@@ -598,7 +599,7 @@ int stktable_init(struct stktable *t)
t->updates = EB_ROOT_UNIQUE;
HA_SPIN_INIT(>lock);
- t->pool = create_pool("sticktables", sizeof(struct stksess) +
t->data_size + t->key_size, MEM_F_SHARED);
+ t->pool = create_pool("sticktables", sizeof(struct stksess) +
round_ptr_size(t->data_size) + t->key_size, MEM_F_SHARED);
t->exp_next = TICK_ETERNITY;
if ( t->expire ) {
--
2.17.1
Re: [PATCH] BUG/CRITICAL: SIGBUS crash on aarch64
On Wed, Nov 14, 2018 at 06:05:00PM +0100, Olivier Houchard wrote: > Oops, you're right indeed. > I'm not sure I'm a big fan of special-casing STD_T_UINT. For example, > STD_T_FRQP is probably 12bytes too, so it'd be a problem. > Can you test the (untested, but hopefully right) patch attached ? Yes, your patch works on aarch64 too. -- Paul Martin http://www.codethink.co.uk/ Senior Software Developer, Codethink Ltd.
[PATCH] BUG/CRITICAL: SIGBUS crash on aarch64
Atomic operations on aarch64 (arm64) have to be aligned to 8 byte
boundaries (same size as a pointer type), otherwise a SIGBUS is raised.
Because the variable ts here isn't guaranteed to be aligned due to the
various data_size adjustments, make sure that data_size is always
incremented by a minimum of sizeof(int *) rather than sizeof(int).
Program received signal SIGBUS, Bus error.
0xaab1176c in process_store_rules (s=s@entry=0xaae01060,
rep=0xaae010d0, rep=0xaae010d0, an_bit=8388608)
at src/stream.c:1609
1609HA_RWLOCK_WRLOCK(STK_SESS_LOCK, >lock);
(gdb) bt
%0 0xaab1176c in process_store_rules (s=s@entry=0xaae01060,
rep=0xaae010d0, rep=0xaae010d0, an_bit=8388608)
at src/stream.c:1609
%1 0xaab18898 in process_stream (t=,
context=0xaae01060, state=) at src/stream.c:2054
%2 0xaabb0220 in process_runnable_tasks () at src/task.c:421
%3 0xaab51b40 in run_poll_loop () at src/haproxy.c:2609
%4 run_thread_poll_loop (data=) at src/haproxy.c:2674
%5 0xaaac715c in main (argc=, argv=0xf290)
at src/haproxy.c:3286
---
include/proto/stick_table.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/proto/stick_table.h b/include/proto/stick_table.h
index 40bb8ca6..6e39ad47 100644
--- a/include/proto/stick_table.h
+++ b/include/proto/stick_table.h
@@ -64,7 +64,7 @@ static inline int stktable_type_size(int type)
switch(type) {
case STD_T_SINT:
case STD_T_UINT:
- return sizeof(int);
+ return sizeof(int *);
case STD_T_ULL:
return sizeof(unsigned long long);
case STD_T_FRQP:
--
2.19.1
