ccan.lists.ozlabs.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/4] altstack: Small bugfixes
@ 2016-02-16  6:09 David Gibson
  2016-02-16  6:09 ` [PATCH 1/4] altstack: Increase signal stack size David Gibson
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: David Gibson @ 2016-02-16  6:09 UTC (permalink / raw)
  To: dan; +Cc: ccan

So, by building myself a docker container with Ubuntu 12.04 (the
default distro used by Travis builds), I thought I'd figured out why
altstack has been breaking Travis builds for ages.

Unfortunately, I was wrong - 1/4 certainly fixes *a* bug on Ubuntu
12.04, but there's at least one more - I'm getting a SIGBUS in dn()
for reasons I haven't figured out.  I think it must be a different
behaviour in whatever kernel the Travis builders are using (so it
doesn't affect my local container build), but I don't know for sure.

Still might as well fix the bug I did track down.  While I was at it I
found a couple more bugs which I haven't seen bite in practice, but
I'm pretty sure could cause problems at least theoretically, hence
patches 2..3/4.

Finally 4/4 isn't a bugfix, but reworks some of the test code to make
failures a bit easier to diagnose.

David Gibson (4):
  altstack: Increase signal stack size
  altstack: Include config.h in run.c
  altstack: Declare memory clobbers
  altstack: Clarify checking macros

 ccan/altstack/altstack.c |  7 ++++---
 ccan/altstack/test/run.c | 30 +++++++++++++++++++++++++-----
 2 files changed, 29 insertions(+), 8 deletions(-)

-- 
2.5.0

_______________________________________________
ccan mailing list
ccan@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/ccan

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/4] altstack: Increase signal stack size
  2016-02-16  6:09 [PATCH 0/4] altstack: Small bugfixes David Gibson
@ 2016-02-16  6:09 ` David Gibson
  2016-02-16  6:09 ` [PATCH 2/4] altstack: Include config.h in run.c David Gibson
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 9+ messages in thread
From: David Gibson @ 2016-02-16  6:09 UTC (permalink / raw)
  To: dan; +Cc: ccan

At present the altstack module uses a stack of size MINSIGSTKSZ for its
SIGSEGV handler.  Although MINSIGSTKSZ is defined to be large enough to
execute a signal handler, it doesn't guarantee that you can do anything
very much within it.

With certain libc versions, MINSIGSTKSZ is not enough to execute the
longjmp() used in altstack.  Specfically, with Ubuntu 12.04 (the default
install for Travis containers), the first time longjmp() is executed the
symbol must be resolved by the dynamic linker in a process which overruns
the MINSIGSTKSZ sized stack.  That then corrupts local variables in
altstack() itself causing a number of subsequent failures.

This patch addresses the problem by changing from MINSIGSTKSZ to SIGSTKSZ
which is supposed to cover "the usual requirements for an alternate signal
stack".

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 ccan/altstack/altstack.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ccan/altstack/altstack.c b/ccan/altstack/altstack.c
index 67f457b..640344d 100644
--- a/ccan/altstack/altstack.c
+++ b/ccan/altstack/altstack.c
@@ -92,7 +92,7 @@ int altstack(rlim_t max, void *(*fn)(void *), void *arg, void **out)
 	undo++;
 
 	if (setjmp(jmp) == 0) {
-		unsigned char sigstk[MINSIGSTKSZ];
+		unsigned char sigstk[SIGSTKSZ];
 		stack_t ss = { .ss_sp = sigstk, .ss_size = sizeof(sigstk) };
 		struct sigaction sa = { .sa_handler = segvjmp, .sa_flags = SA_NODEFER|SA_RESETHAND|SA_ONSTACK };
 
-- 
2.5.0

_______________________________________________
ccan mailing list
ccan@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/ccan

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/4] altstack: Include config.h in run.c
  2016-02-16  6:09 [PATCH 0/4] altstack: Small bugfixes David Gibson
  2016-02-16  6:09 ` [PATCH 1/4] altstack: Increase signal stack size David Gibson
@ 2016-02-16  6:09 ` David Gibson
  2016-02-16  6:09 ` [PATCH 3/4] altstack: Declare memory clobbers David Gibson
  2016-02-16  6:09 ` [PATCH 4/4] altstack: Clarify checking macros David Gibson
  3 siblings, 0 replies; 9+ messages in thread
From: David Gibson @ 2016-02-16  6:09 UTC (permalink / raw)
  To: dan; +Cc: ccan

ccan programs should always include config.h before anything else to make
sure everything is set up correctly.  Doing so in altstack's run.c means
it no longer needs an explicit _XOPEN_SOURCE 700, since _GNU_SOURCE is set
in config.h (for GNU libc, anyway).

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 ccan/altstack/test/run.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ccan/altstack/test/run.c b/ccan/altstack/test/run.c
index de94887..389ecb9 100644
--- a/ccan/altstack/test/run.c
+++ b/ccan/altstack/test/run.c
@@ -1,3 +1,4 @@
+#include "config.h"
 #include <assert.h>
 #include <err.h>
 #include <errno.h>
@@ -8,7 +9,6 @@
 #include <sys/mman.h>
 #include <ccan/tap/tap.h>
 #include <ccan/altstack/altstack.h>
-#define _XOPEN_SOURCE 700
 #include <stdio.h>
 
 enum {
-- 
2.5.0

_______________________________________________
ccan mailing list
ccan@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/ccan

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 3/4] altstack: Declare memory clobbers
  2016-02-16  6:09 [PATCH 0/4] altstack: Small bugfixes David Gibson
  2016-02-16  6:09 ` [PATCH 1/4] altstack: Increase signal stack size David Gibson
  2016-02-16  6:09 ` [PATCH 2/4] altstack: Include config.h in run.c David Gibson
@ 2016-02-16  6:09 ` David Gibson
  2016-02-16 17:29   ` Dan Good
  2016-02-16  6:09 ` [PATCH 4/4] altstack: Clarify checking macros David Gibson
  3 siblings, 1 reply; 9+ messages in thread
From: David Gibson @ 2016-02-16  6:09 UTC (permalink / raw)
  To: dan; +Cc: ccan

altstack includes a couple of inline asm blocks with x86 push and pop
instructions.  These instructions will access memory (the stack), but
that's not declared in inline asm statement.  We seem to be getting away
with it, but in theory that could allow the compiler to re-order accesses
to local variables across the asm block.  Since those blocks change the
location of the stack, that could be very bad.

Adding a "memory" clobber should prevent this (effectively making the asm
blocks a compiler memory barrier).

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 ccan/altstack/altstack.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/ccan/altstack/altstack.c b/ccan/altstack/altstack.c
index 640344d..6351293 100644
--- a/ccan/altstack/altstack.c
+++ b/ccan/altstack/altstack.c
@@ -108,9 +108,10 @@ int altstack(rlim_t max, void *(*fn)(void *), void *arg, void **out)
 			"mov %1, %%rsp\n\t"
 			"sub $8, %%rsp\n\t"
 			"push %%r10"
-			: "=r" (rsp_save_[0]) : "0" (m + max) : "r10");
+			: "=r" (rsp_save_[0]) : "0" (m + max) : "r10", "memory");
 		out_ = fn_(arg_);
-		asm volatile ("pop %rsp");
+		asm volatile ("pop %%rsp"
+			      : : : "memory");
 		ret = 0;
 		if (out) *out = out_;
 	}
-- 
2.5.0

_______________________________________________
ccan mailing list
ccan@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/ccan

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 4/4] altstack: Clarify checking macros
  2016-02-16  6:09 [PATCH 0/4] altstack: Small bugfixes David Gibson
                   ` (2 preceding siblings ...)
  2016-02-16  6:09 ` [PATCH 3/4] altstack: Declare memory clobbers David Gibson
@ 2016-02-16  6:09 ` David Gibson
  3 siblings, 0 replies; 9+ messages in thread
From: David Gibson @ 2016-02-16  6:09 UTC (permalink / raw)
  To: dan; +Cc: ccan

The chkfail() and chkok() macros in altstack's test program are pretty
difficult to read.  More importantly, though, they do all their tests with
one big ok1().  That means if the test fails, you get no indication which
of the checks was actually wrong, making debugging harder.

This reworks the macros into a more verbose form that's easier to read,
and splits them into multiple ok1() tests to make failures more explicit.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 ccan/altstack/test/run.c | 28 ++++++++++++++++++++++++----
 1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/ccan/altstack/test/run.c b/ccan/altstack/test/run.c
index 389ecb9..12cc460 100644
--- a/ccan/altstack/test/run.c
+++ b/ccan/altstack/test/run.c
@@ -57,14 +57,34 @@ static void *wrap(void *i)
 	return wrap;
 }
 
+#define chkfail(x, y, z, c1, c2)					\
+	do {								\
+		call1 = 0;						\
+		call2 = 0;						\
+		errno = 0;						\
+		ok1((fail = x) && (y));					\
+		ok1(errno == (z));					\
+		ok1(call1 == (c1));					\
+		ok1(call2 == (c2));					\
+	} while (0);
+
+#define chkok(y, z, c1, c2)						\
+	do {								\
+		call1 = 0;						\
+		call2 = 0;						\
+		errno = 0;						\
+		fail = 0;						\
+		ok1((y));						\
+		ok1(errno == (z));					\
+		ok1(call1 == (c1));					\
+		ok1(call2 == (c2));					\
+	} while (0)
+
 int main(void)
 {
 	long pgsz = sysconf(_SC_PAGESIZE);
 
-	plan_tests(17);
-
-#define chkfail(x, y, z, c1, c2) (call1 = 0, call2 = 0, errno = 0, ok1((fail = x) && (y) && errno == (z) && call1 == (c1) && call2 == (c2)));
-#define   chkok(   y, z, c1, c2) (call1 = 0, call2 = 0, errno = 0, fail = 0,     ok1((y) && errno == (z) && call1 == (c1) && call2 == (c2)));
+	plan_tests(50);
 
 	chkfail(getrlimit_,	altstack(8*MiB, wrap, 0, 0) == -1, e(getrlimit_),
 		0,
-- 
2.5.0

_______________________________________________
ccan mailing list
ccan@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/ccan

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/4] altstack: Declare memory clobbers
  2016-02-16  6:09 ` [PATCH 3/4] altstack: Declare memory clobbers David Gibson
@ 2016-02-16 17:29   ` Dan Good
  2016-02-17  0:09     ` David Gibson
  0 siblings, 1 reply; 9+ messages in thread
From: Dan Good @ 2016-02-16 17:29 UTC (permalink / raw)
  To: David Gibson; +Cc: ccan


[-- Attachment #1.1: Type: text/plain, Size: 1824 bytes --]

Thank you for the fixes and improvements.  The clobber change is the only
that gives me pause.  I think the volatile keyword on both is sufficient to
prevent re-ordering.  Are you sure we need the memory clobber?  Thanks.
 -Dan

On Tue, Feb 16, 2016 at 5:04 AM David Gibson <david@gibson.dropbear.id.au>
wrote:

> altstack includes a couple of inline asm blocks with x86 push and pop
> instructions.  These instructions will access memory (the stack), but
> that's not declared in inline asm statement.  We seem to be getting away
> with it, but in theory that could allow the compiler to re-order accesses
> to local variables across the asm block.  Since those blocks change the
> location of the stack, that could be very bad.
>
> Adding a "memory" clobber should prevent this (effectively making the asm
> blocks a compiler memory barrier).
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  ccan/altstack/altstack.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/ccan/altstack/altstack.c b/ccan/altstack/altstack.c
> index 640344d..6351293 100644
> --- a/ccan/altstack/altstack.c
> +++ b/ccan/altstack/altstack.c
> @@ -108,9 +108,10 @@ int altstack(rlim_t max, void *(*fn)(void *), void
> *arg, void **out)
>                         "mov %1, %%rsp\n\t"
>                         "sub $8, %%rsp\n\t"
>                         "push %%r10"
> -                       : "=r" (rsp_save_[0]) : "0" (m + max) : "r10");
> +                       : "=r" (rsp_save_[0]) : "0" (m + max) : "r10",
> "memory");
>                 out_ = fn_(arg_);
> -               asm volatile ("pop %rsp");
> +               asm volatile ("pop %%rsp"
> +                             : : : "memory");
>                 ret = 0;
>                 if (out) *out = out_;
>         }
> --
> 2.5.0
>
>

[-- Attachment #1.2: Type: text/html, Size: 2524 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
ccan mailing list
ccan@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/ccan

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/4] altstack: Declare memory clobbers
  2016-02-16 17:29   ` Dan Good
@ 2016-02-17  0:09     ` David Gibson
       [not found]       ` <CACNkOJPwRNWyUw+mv=RR+9hShPfUaCu3EGbNTeOhGRmmc6zA-w@mail.gmail.com>
  0 siblings, 1 reply; 9+ messages in thread
From: David Gibson @ 2016-02-17  0:09 UTC (permalink / raw)
  To: Dan Good; +Cc: ccan


[-- Attachment #1.1: Type: text/plain, Size: 3336 bytes --]

On Tue, Feb 16, 2016 at 05:29:39PM +0000, Dan Good wrote:
> Thank you for the fixes and improvements.  The clobber change is the only
> that gives me pause.  I think the volatile keyword on both is sufficient to
> prevent re-ordering.  Are you sure we need the memory clobber?
> Thanks.

I'm not 100% sure, but I'm fairly confident.  AFAICT the volatile
keyword stops the asm section being elided if the output arguments
aren't used, and it prevents it being moved outside a loop if the
compiler things the input arguments don't change.  It doesn't appear
to prevent other code, including memory accesses from being moved
around the asm.  In fact, from the gcc manual:

|  Note that the compiler can move even volatile 'asm' instructions
| relative to other code, including across jump instructions.  For
| example, on many targets there is a system register that controls the
| rounding mode of floating-point operations.  Setting it with a volatile
| 'asm', as in the following PowerPC example, does not work reliably.
| 
|      asm volatile("mtfsf 255, %0" : : "f" (fpenv));
|      sum = x + y;
| 
|  The compiler may move the addition back before the volatile 'asm'.  To
| make it work as expected, add an artificial dependency to the 'asm' by
| referencing a variable in the subsequent code, for example:
| 
|      asm volatile ("mtfsf 255,%1" : "=X" (sum) : "f" (fpenv));
|      sum = x + y;

>  -Dan
> 
> On Tue, Feb 16, 2016 at 5:04 AM David Gibson <david@gibson.dropbear.id.au>
> wrote:
> 
> > altstack includes a couple of inline asm blocks with x86 push and pop
> > instructions.  These instructions will access memory (the stack), but
> > that's not declared in inline asm statement.  We seem to be getting away
> > with it, but in theory that could allow the compiler to re-order accesses
> > to local variables across the asm block.  Since those blocks change the
> > location of the stack, that could be very bad.
> >
> > Adding a "memory" clobber should prevent this (effectively making the asm
> > blocks a compiler memory barrier).
> >
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  ccan/altstack/altstack.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/ccan/altstack/altstack.c b/ccan/altstack/altstack.c
> > index 640344d..6351293 100644
> > --- a/ccan/altstack/altstack.c
> > +++ b/ccan/altstack/altstack.c
> > @@ -108,9 +108,10 @@ int altstack(rlim_t max, void *(*fn)(void *), void
> > *arg, void **out)
> >                         "mov %1, %%rsp\n\t"
> >                         "sub $8, %%rsp\n\t"
> >                         "push %%r10"
> > -                       : "=r" (rsp_save_[0]) : "0" (m + max) : "r10");
> > +                       : "=r" (rsp_save_[0]) : "0" (m + max) : "r10",
> > "memory");
> >                 out_ = fn_(arg_);
> > -               asm volatile ("pop %rsp");
> > +               asm volatile ("pop %%rsp"
> > +                             : : : "memory");
> >                 ret = 0;
> >                 if (out) *out = out_;
> >         }
> >
> >

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
ccan mailing list
ccan@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/ccan

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/4] altstack: Declare memory clobbers
       [not found]       ` <CACNkOJPwRNWyUw+mv=RR+9hShPfUaCu3EGbNTeOhGRmmc6zA-w@mail.gmail.com>
@ 2016-02-18  3:09         ` David Gibson
       [not found]           ` <CACNkOJMsLzcKVuhWwNYKw-LQJhjJT4kYCFLfbYUsYW911WVu1w@mail.gmail.com>
  0 siblings, 1 reply; 9+ messages in thread
From: David Gibson @ 2016-02-18  3:09 UTC (permalink / raw)
  To: Dan Good; +Cc: ccan


[-- Attachment #1.1: Type: text/plain, Size: 4114 bytes --]

On Wed, Feb 17, 2016 at 07:06:09AM +0000, Dan Good wrote:
> Looking at the objdump before and after, the clobber changes seem to add a
> single mov instruction (9 additional bytes plus subsequent address
> offsets).  That seems a very low price to pay for the additional
> assurances.  -Dan

I would tend to agree.

Do you want to apply these patches, or will I?

> 
> On Tue, Feb 16, 2016 at 7:08 PM David Gibson <david@gibson.dropbear.id.au>
> wrote:
> 
> > On Tue, Feb 16, 2016 at 05:29:39PM +0000, Dan Good wrote:
> > > Thank you for the fixes and improvements.  The clobber change is the only
> > > that gives me pause.  I think the volatile keyword on both is sufficient
> > to
> > > prevent re-ordering.  Are you sure we need the memory clobber?
> > > Thanks.
> >
> > I'm not 100% sure, but I'm fairly confident.  AFAICT the volatile
> > keyword stops the asm section being elided if the output arguments
> > aren't used, and it prevents it being moved outside a loop if the
> > compiler things the input arguments don't change.  It doesn't appear
> > to prevent other code, including memory accesses from being moved
> > around the asm.  In fact, from the gcc manual:
> >
> > |  Note that the compiler can move even volatile 'asm' instructions
> > | relative to other code, including across jump instructions.  For
> > | example, on many targets there is a system register that controls the
> > | rounding mode of floating-point operations.  Setting it with a volatile
> > | 'asm', as in the following PowerPC example, does not work reliably.
> > |
> > |      asm volatile("mtfsf 255, %0" : : "f" (fpenv));
> > |      sum = x + y;
> > |
> > |  The compiler may move the addition back before the volatile 'asm'.  To
> > | make it work as expected, add an artificial dependency to the 'asm' by
> > | referencing a variable in the subsequent code, for example:
> > |
> > |      asm volatile ("mtfsf 255,%1" : "=X" (sum) : "f" (fpenv));
> > |      sum = x + y;
> >
> > >  -Dan
> > >
> > > On Tue, Feb 16, 2016 at 5:04 AM David Gibson <
> > david@gibson.dropbear.id.au>
> > > wrote:
> > >
> > > > altstack includes a couple of inline asm blocks with x86 push and pop
> > > > instructions.  These instructions will access memory (the stack), but
> > > > that's not declared in inline asm statement.  We seem to be getting
> > away
> > > > with it, but in theory that could allow the compiler to re-order
> > accesses
> > > > to local variables across the asm block.  Since those blocks change the
> > > > location of the stack, that could be very bad.
> > > >
> > > > Adding a "memory" clobber should prevent this (effectively making the
> > asm
> > > > blocks a compiler memory barrier).
> > > >
> > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > > ---
> > > >  ccan/altstack/altstack.c | 5 +++--
> > > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/ccan/altstack/altstack.c b/ccan/altstack/altstack.c
> > > > index 640344d..6351293 100644
> > > > --- a/ccan/altstack/altstack.c
> > > > +++ b/ccan/altstack/altstack.c
> > > > @@ -108,9 +108,10 @@ int altstack(rlim_t max, void *(*fn)(void *), void
> > > > *arg, void **out)
> > > >                         "mov %1, %%rsp\n\t"
> > > >                         "sub $8, %%rsp\n\t"
> > > >                         "push %%r10"
> > > > -                       : "=r" (rsp_save_[0]) : "0" (m + max) : "r10");
> > > > +                       : "=r" (rsp_save_[0]) : "0" (m + max) : "r10",
> > > > "memory");
> > > >                 out_ = fn_(arg_);
> > > > -               asm volatile ("pop %rsp");
> > > > +               asm volatile ("pop %%rsp"
> > > > +                             : : : "memory");
> > > >                 ret = 0;
> > > >                 if (out) *out = out_;
> > > >         }
> > > >
> > > >
> >
> >

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
ccan mailing list
ccan@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/ccan

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/4] altstack: Declare memory clobbers
       [not found]           ` <CACNkOJMsLzcKVuhWwNYKw-LQJhjJT4kYCFLfbYUsYW911WVu1w@mail.gmail.com>
@ 2016-02-18  5:49             ` David Gibson
  0 siblings, 0 replies; 9+ messages in thread
From: David Gibson @ 2016-02-18  5:49 UTC (permalink / raw)
  To: Dan Good; +Cc: ccan


[-- Attachment #1.1: Type: text/plain, Size: 4754 bytes --]

On Thu, Feb 18, 2016 at 04:45:36AM +0000, Dan Good wrote:
> Would you, please?  Thank you.

Done.

> 
> On Wed, Feb 17, 2016 at 10:11 PM David Gibson <david@gibson.dropbear.id.au>
> wrote:
> 
> > On Wed, Feb 17, 2016 at 07:06:09AM +0000, Dan Good wrote:
> > > Looking at the objdump before and after, the clobber changes seem to add
> > a
> > > single mov instruction (9 additional bytes plus subsequent address
> > > offsets).  That seems a very low price to pay for the additional
> > > assurances.  -Dan
> >
> > I would tend to agree.
> >
> > Do you want to apply these patches, or will I?
> >
> > >
> > > On Tue, Feb 16, 2016 at 7:08 PM David Gibson <
> > david@gibson.dropbear.id.au>
> > > wrote:
> > >
> > > > On Tue, Feb 16, 2016 at 05:29:39PM +0000, Dan Good wrote:
> > > > > Thank you for the fixes and improvements.  The clobber change is the
> > only
> > > > > that gives me pause.  I think the volatile keyword on both is
> > sufficient
> > > > to
> > > > > prevent re-ordering.  Are you sure we need the memory clobber?
> > > > > Thanks.
> > > >
> > > > I'm not 100% sure, but I'm fairly confident.  AFAICT the volatile
> > > > keyword stops the asm section being elided if the output arguments
> > > > aren't used, and it prevents it being moved outside a loop if the
> > > > compiler things the input arguments don't change.  It doesn't appear
> > > > to prevent other code, including memory accesses from being moved
> > > > around the asm.  In fact, from the gcc manual:
> > > >
> > > > |  Note that the compiler can move even volatile 'asm' instructions
> > > > | relative to other code, including across jump instructions.  For
> > > > | example, on many targets there is a system register that controls the
> > > > | rounding mode of floating-point operations.  Setting it with a
> > volatile
> > > > | 'asm', as in the following PowerPC example, does not work reliably.
> > > > |
> > > > |      asm volatile("mtfsf 255, %0" : : "f" (fpenv));
> > > > |      sum = x + y;
> > > > |
> > > > |  The compiler may move the addition back before the volatile 'asm'.
> > To
> > > > | make it work as expected, add an artificial dependency to the 'asm'
> > by
> > > > | referencing a variable in the subsequent code, for example:
> > > > |
> > > > |      asm volatile ("mtfsf 255,%1" : "=X" (sum) : "f" (fpenv));
> > > > |      sum = x + y;
> > > >
> > > > >  -Dan
> > > > >
> > > > > On Tue, Feb 16, 2016 at 5:04 AM David Gibson <
> > > > david@gibson.dropbear.id.au>
> > > > > wrote:
> > > > >
> > > > > > altstack includes a couple of inline asm blocks with x86 push and
> > pop
> > > > > > instructions.  These instructions will access memory (the stack),
> > but
> > > > > > that's not declared in inline asm statement.  We seem to be getting
> > > > away
> > > > > > with it, but in theory that could allow the compiler to re-order
> > > > accesses
> > > > > > to local variables across the asm block.  Since those blocks
> > change the
> > > > > > location of the stack, that could be very bad.
> > > > > >
> > > > > > Adding a "memory" clobber should prevent this (effectively making
> > the
> > > > asm
> > > > > > blocks a compiler memory barrier).
> > > > > >
> > > > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > > > > ---
> > > > > >  ccan/altstack/altstack.c | 5 +++--
> > > > > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/ccan/altstack/altstack.c b/ccan/altstack/altstack.c
> > > > > > index 640344d..6351293 100644
> > > > > > --- a/ccan/altstack/altstack.c
> > > > > > +++ b/ccan/altstack/altstack.c
> > > > > > @@ -108,9 +108,10 @@ int altstack(rlim_t max, void *(*fn)(void *),
> > void
> > > > > > *arg, void **out)
> > > > > >                         "mov %1, %%rsp\n\t"
> > > > > >                         "sub $8, %%rsp\n\t"
> > > > > >                         "push %%r10"
> > > > > > -                       : "=r" (rsp_save_[0]) : "0" (m + max) :
> > "r10");
> > > > > > +                       : "=r" (rsp_save_[0]) : "0" (m + max) :
> > "r10",
> > > > > > "memory");
> > > > > >                 out_ = fn_(arg_);
> > > > > > -               asm volatile ("pop %rsp");
> > > > > > +               asm volatile ("pop %%rsp"
> > > > > > +                             : : : "memory");
> > > > > >                 ret = 0;
> > > > > >                 if (out) *out = out_;
> > > > > >         }
> > > > > >
> > > > > >
> > > >
> > > >
> >
> >

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
ccan mailing list
ccan@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/ccan

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2016-02-18  5:56 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-02-16  6:09 [PATCH 0/4] altstack: Small bugfixes David Gibson
2016-02-16  6:09 ` [PATCH 1/4] altstack: Increase signal stack size David Gibson
2016-02-16  6:09 ` [PATCH 2/4] altstack: Include config.h in run.c David Gibson
2016-02-16  6:09 ` [PATCH 3/4] altstack: Declare memory clobbers David Gibson
2016-02-16 17:29   ` Dan Good
2016-02-17  0:09     ` David Gibson
     [not found]       ` <CACNkOJPwRNWyUw+mv=RR+9hShPfUaCu3EGbNTeOhGRmmc6zA-w@mail.gmail.com>
2016-02-18  3:09         ` David Gibson
     [not found]           ` <CACNkOJMsLzcKVuhWwNYKw-LQJhjJT4kYCFLfbYUsYW911WVu1w@mail.gmail.com>
2016-02-18  5:49             ` David Gibson
2016-02-16  6:09 ` [PATCH 4/4] altstack: Clarify checking macros David Gibson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).