Thread suspension: Implement for Linux

Thread suspension: Implement for Linux

Linux doesn't have an equivalent of Windows's SuspendThread() call for
suspending individual threads (except for ptrace(), which we can't
readily use inside Chromium's outer sandbox), so we use an
asynchronous signal to interrupt and suspend a thread.

For speed and simplicity, and to avoid deadlocks in the signal
handler, we use Linux futexes for the synchronisation.

Since NaClAppThreadSetSuspendState() now has multiple implementations,
it moves into the OS-specific files.  Add a "generic" directory for
the non-OS-specific no-op implementation.

Add an implementation of NaClTlsGetIdx() for x86-32 Linux that mirrors
the x86-64 equivalent so that we can get the interrupted thread's
identity inside the signal handler.

This is intended for use by the debug stub.  However, it does not save
registers yet.  This just implements for Linux what we already have
for Windows.

BUG=http://code.google.com/p/nativeclient/issues/detail?id=2758
TEST=run_thread_suspension_test

Committed: https://src.chromium.org/viewvc/native_client?view=rev&revision=8670

[halyavin, 2012-05-11 11:47:28]

Imagine this scenario.
Thread 1 in transitioning from untrusted code to trusted code.
natp->suspend_state is NACL_APP_THREAD_UNTRUSTED.

Thread 2 tries to suspend it. It sets natp->suspend_state, sends USR1 signal and
starts waiting on the conditional variable.

This means that natp->mu became unlocked. So in the thread 1 the USR1 signal
handler can interrupt NaClXMutexLock in NaClAppThreadSetSuspendState (executed
at start of NaClSyscallCSegHook) in arbitrary moment. This is not a good thing.

Even with synchronous pthread_kill, the USR1 signal handler can interrupt this
NaClXMutexLock call on its way to blocking state.

[Mark Seaborn, 2012-05-15 00:39:31]

On 11 May 2012 04:47, <halyavin@google.com> wrote:

> Imagine this scenario.
> Thread 1 in transitioning from untrusted code to trusted code.
> natp->suspend_state is NACL_APP_THREAD_UNTRUSTED.
>
> Thread 2 tries to suspend it. It sets natp->suspend_state, sends USR1
> signal and
> starts waiting on the conditional variable.
>
> This means that natp->mu became unlocked. So in the thread 1 the USR1
> signal
> handler can interrupt NaClXMutexLock in NaClAppThreadSetSuspendState
> (executed
> at start of NaClSyscallCSegHook) in arbitrary moment. This is not a good
> thing.
>

Yes, that argument makes sense.  A possible fix to patch set 1 would be to
use a separate variable and mutex (and probably condvar) for signalling, in
the signal hander, when the thread has suspended.  (By the way, I had not
posted patch set 1 for review and I hadn't thought too hard about its
correctness.)

Anyway, I have replaced the mutex-based implementation with a futex-based
implementation.  It is more lightweight than using pthreads.  Hopefully it
is easier to check for correctness.  It should be more efficient because it
will require just one atomic compare-and-swap per context switch.

Even with synchronous pthread_kill, the USR1 signal handler can interrupt
> this
> NaClXMutexLock call on its way to blocking state.
>

Hmm, I am not sure whether that is a problem though.  If pthread_kill()
took effect immediately while its caller still holds natp->mu, things
should be OK.  Interrupting pthread_mutex_lock() should be OK:  it will be
implemented in terms of atomic ops and futexes, both of which should be
fine to interrupt.

Mark

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To post to this group, send email to native-client-reviews@googlegroups.com.
To unsubscribe from this group, send email to
native-client-reviews+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.

[Mark Seaborn, 2012-05-16 21:02:19]

I haven't finished checking this with my model checker (the one in
https://github.com/mseaborn/nacl-model-check) and I might need to tweak it, but
it's pretty much ready for review.

[bradn, 2012-05-18 19:56:35]

http://codereview.chromium.org/10392005/diff/2012/SConstruct
File SConstruct (right):

http://codereview.chromium.org/10392005/diff/2012/SConstruct#newcode767
SConstruct:767: 'run_thread_suspension_test',
? doesn't work on qemu?

http://codereview.chromium.org/10392005/diff/2012/src/trusted/service_runtime...
File src/trusted/service_runtime/arch/x86_32/nacl_tls_32.c (right):

http://codereview.chromium.org/10392005/diff/2012/src/trusted/service_runtime...
src/trusted/service_runtime/arch/x86_32/nacl_tls_32.c:87: * index on the x86-32.
 We use segmentation (%gs) to provide access
This comment and the one above are starting to feel misleading.
Maybe collapse into a single comment describing the 3 cases of use:
linux32 - just for signal handler
x86-64 - generally
x86-32 (non-linux) - not

Given how fickle using gs is (and all the random asm that also uses it), I can't
help but wonder if we should move to using this one in more places. Just
thinking, no real change here.

http://codereview.chromium.org/10392005/diff/2012/src/trusted/service_runtime...
File src/trusted/service_runtime/generic/thread_suspension.c (right):

http://codereview.chromium.org/10392005/diff/2012/src/trusted/service_runtime...
src/trusted/service_runtime/generic/thread_suspension.c:12: enum
NaClSuspendState old_state,
Did you want to consider just adding this as an osx one, as that's where you'll
end up next?

http://codereview.chromium.org/10392005/diff/2012/src/trusted/service_runtime...
File src/trusted/service_runtime/linux/thread_suspension.c (right):

http://codereview.chromium.org/10392005/diff/2012/src/trusted/service_runtime...
src/trusted/service_runtime/linux/thread_suspension.c:22: if (errno != EINTR &&
errno != EAGAIN) {
Not seeing in the docs where EAGAIN is a valid return for FUTEX_WAIT?

http://codereview.chromium.org/10392005/diff/2012/src/trusted/service_runtime...
src/trusted/service_runtime/linux/thread_suspension.c:22: if (errno != EINTR &&
errno != EAGAIN) {
Might be nice to have a comment like:
If *addr still contains value, wait to be woken up by FutexWake, otherwise
return immediately.

http://codereview.chromium.org/10392005/diff/2012/src/trusted/service_runtime...
src/trusted/service_runtime/linux/thread_suspension.c:30: NaClLog(LOG_FATAL,
"FutexWake: futex() failed with error %d\n", errno);
Comment:
Wake up to 'waiters' threads waiting on addr using FutexWait.

http://codereview.chromium.org/10392005/diff/2012/src/trusted/service_runtime...
src/trusted/service_runtime/linux/thread_suspension.c:39: if ((state &
NACL_APP_THREAD_SUSPENDING) != 0) {
I find the !=0 confusing (and a deviation from the style guide)
if (state & NACL_APP_THREAD_SUSPENDING)
reads to me as if in the suspending state.

http://codereview.chromium.org/10392005/diff/2012/src/trusted/service_runtime...
src/trusted/service_runtime/linux/thread_suspension.c:72: if ((state &
NACL_APP_THREAD_SUSPENDED) != 0) {
drop the !=0

http://codereview.chromium.org/10392005/diff/2012/src/trusted/service_runtime...
src/trusted/service_runtime/linux/thread_suspension.c:82: Atomic32
suspending_state) {
I get the motivation for keeping all the state bits in a single atomic, but it
certainly makes it tricky to follow.
If I am tracing things thru correctly, I believe suspending_state can only ever
be NACL_APP_THREAD_SUSPENDING | NACL_APP_THREAD_UNTRUSTED. Maybe drop the
parameter and make this WaitForUntrustedThreadToSuspend?

http://codereview.chromium.org/10392005/diff/2012/src/trusted/service_runtime...
File src/trusted/service_runtime/win/thread_suspension.c (right):

http://codereview.chromium.org/10392005/diff/2012/src/trusted/service_runtime...
src/trusted/service_runtime/win/thread_suspension.c:19: while
((natp->suspend_state & NACL_APP_THREAD_SUSPENDING) != 0) {
nix !=0

[Mark Seaborn, 2012-05-21 15:03:56]

Please take a look.

http://codereview.chromium.org/10392005/diff/2012/SConstruct
File SConstruct (right):

http://codereview.chromium.org/10392005/diff/2012/SConstruct#newcode767
SConstruct:767: 'run_thread_suspension_test',
On 2012/05/18 19:56:35, bradn wrote:
> ? doesn't work on qemu?

That's correct, it doesn't work under qemu-arm.  I wouldn't necessarily expect
pthread_kill() to work in this case: it's hard to do.

http://codereview.chromium.org/10392005/diff/2012/src/trusted/service_runtime...
File src/trusted/service_runtime/arch/x86_32/nacl_tls_32.c (right):

http://codereview.chromium.org/10392005/diff/2012/src/trusted/service_runtime...
src/trusted/service_runtime/arch/x86_32/nacl_tls_32.c:87: * index on the x86-32.
 We use segmentation (%gs) to provide access
On 2012/05/18 19:56:35, bradn wrote:
> This comment and the one above are starting to feel misleading.

We discussed this and you were OK with the current comments.  The comments are
applicable to the #ifs they are inside.

http://codereview.chromium.org/10392005/diff/2012/src/trusted/service_runtime...
File src/trusted/service_runtime/generic/thread_suspension.c (right):

http://codereview.chromium.org/10392005/diff/2012/src/trusted/service_runtime...
src/trusted/service_runtime/generic/thread_suspension.c:12: enum
NaClSuspendState old_state,
On 2012/05/18 19:56:35, bradn wrote:
> Did you want to consider just adding this as an osx one, as that's
> where you'll end up next?

I think it would be a little misleading to put non-Mac-specific code in an "osx"
directory.  Also, when I implement this for Mac I will probably use the same
NaClAppThreadSetSuspendState() as on Windows, so that will move here.

http://codereview.chromium.org/10392005/diff/2012/src/trusted/service_runtime...
File src/trusted/service_runtime/linux/thread_suspension.c (right):

http://codereview.chromium.org/10392005/diff/2012/src/trusted/service_runtime...
src/trusted/service_runtime/linux/thread_suspension.c:22: if (errno != EINTR &&
errno != EAGAIN) {
On 2012/05/18 19:56:35, bradn wrote:
> Not seeing in the docs where EAGAIN is a valid return for FUTEX_WAIT?

You get EAGAIN if *addr != value.  I've added a comment.

http://codereview.chromium.org/10392005/diff/2012/src/trusted/service_runtime...
src/trusted/service_runtime/linux/thread_suspension.c:22: if (errno != EINTR &&
errno != EAGAIN) {
On 2012/05/18 19:56:35, bradn wrote:
> Might be nice to have a comment like:
> If *addr still contains value, wait to be woken up by FutexWake, otherwise
> return immediately.

Done.

http://codereview.chromium.org/10392005/diff/2012/src/trusted/service_runtime...
src/trusted/service_runtime/linux/thread_suspension.c:30: NaClLog(LOG_FATAL,
"FutexWake: futex() failed with error %d\n", errno);
On 2012/05/18 19:56:35, bradn wrote:
> Comment:
> Wake up to 'waiters' threads waiting on addr using FutexWait.

Done.

http://codereview.chromium.org/10392005/diff/2012/src/trusted/service_runtime...
src/trusted/service_runtime/linux/thread_suspension.c:39: if ((state &
NACL_APP_THREAD_SUSPENDING) != 0) {
On 2012/05/18 19:56:35, bradn wrote:
> I find the !=0 confusing (and a deviation from the style guide)
> if (state & NACL_APP_THREAD_SUSPENDING)
> reads to me as if in the suspending state.

It's what we do in the existing win/thread_suspension.c.  It's generally the
NaCl style not to rely on implicit casts to boolean.

http://codereview.chromium.org/10392005/diff/2012/src/trusted/service_runtime...
src/trusted/service_runtime/linux/thread_suspension.c:72: if ((state &
NACL_APP_THREAD_SUSPENDED) != 0) {
On 2012/05/18 19:56:35, bradn wrote:
> drop the !=0

See other reply.

http://codereview.chromium.org/10392005/diff/2012/src/trusted/service_runtime...
src/trusted/service_runtime/linux/thread_suspension.c:82: Atomic32
suspending_state) {
On 2012/05/18 19:56:35, bradn wrote:
> I get the motivation for keeping all the state bits in a single atomic, but it
> certainly makes it tricky to follow.
> If I am tracing things thru correctly, I believe suspending_state can only
ever
> be NACL_APP_THREAD_SUSPENDING | NACL_APP_THREAD_UNTRUSTED. Maybe drop the
> parameter and make this WaitForUntrustedThreadToSuspend?

OK, renamed.

http://codereview.chromium.org/10392005/diff/2012/src/trusted/service_runtime...
File src/trusted/service_runtime/win/thread_suspension.c (right):

http://codereview.chromium.org/10392005/diff/2012/src/trusted/service_runtime...
src/trusted/service_runtime/win/thread_suspension.c:19: while
((natp->suspend_state & NACL_APP_THREAD_SUSPENDING) != 0) {
On 2012/05/18 19:56:35, bradn wrote:
> nix !=0

See other reply.

[bradn, 2012-05-21 17:42:57]

lgtm

http://codereview.chromium.org/10392005/diff/17006/src/trusted/service_runtim...
File src/trusted/service_runtime/linux/thread_suspension.c (right):

http://codereview.chromium.org/10392005/diff/17006/src/trusted/service_runtim...
src/trusted/service_runtime/linux/thread_suspension.c:30: * We use the *_PRIVATE
variant to use process-local futexes which are
Mention that these are well know but undocumented (as the docs are stale).

[Mark Seaborn, 2012-05-22 04:28:10]

On 21 May 2012 10:42, <bradnelson@google.com> wrote:

>
>
http://codereview.chromium.org/10392005/diff/17006/src/trusted/service_runtim...
> File src/trusted/service_runtime/**linux/thread_suspension.c (right):
>
> http://codereview.chromium.**org/10392005/diff/17006/src/**
>
trusted/service_runtime/linux/**thread_suspension.c#newcode30<http://codereview.chromium.org/10392005/diff/17006/src/trusted/service_runtime/linux/thread_suspension.c#newcode30>
> src/trusted/service_runtime/**linux/thread_suspension.c:30: * We use the
> *_PRIVATE variant to use process-local futexes which are
> Mention that these are well know but undocumented (as the docs are
> stale).
>

Done, and committed.  Thanks!

Mark

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To post to this group, send email to native-client-reviews@googlegroups.com.
To unsubscribe from this group, send email to
native-client-reviews+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.