[LibOS] Rework signal handling #2090
Conversation
There was a problem hiding this comment.
Reviewable status: 0 of 55 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL)
a discussion (no related file):
There is (at least) one known issue: I don't know how to handle a case, where we are executing a blocking syscall (e.g. read) and a signal comes after we enter LibOS, but before we do actual blocking host syscall. In such case we will just block and handle the signal only when returning to the user app (which can take arbitrarily long, since read can block indefinitely.
I see only one solution: just say that signals can arrive anytime and ignore the problem :) There is one caveat tho: we would need to change the way time_to_die is handled - probably introduce something like old preemption counter and check it every time we reach 0 (i.e. when we give up last lock). Generally speaking - doable.
Pal/src/host/Linux-SGX/db_exception.c, line 283 at r1 (raw file):
} /* TODO: shouldn't this function ignore sync events???
There was a problem hiding this comment.
Reviewable status: 0 of 55 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners
Pal/src/host/Linux/pal_host.h, line 157 at r2 (raw file):
#define HANDLE_TYPE(handle) ((handle)->hdr.type) // TODO: remove these
As a follow up PR, not to generate many conflicts here.
There was a problem hiding this comment.
Reviewable status: 0 of 55 files reviewed, 9 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski and @dimakuv)
LibOS/shim/include/shim_defs.h, line 6 at r2 (raw file):
#include "shim_syscalls.h" /* Names taken from the Linux kernel. */
value is also taken from kernel?
LibOS/shim/include/shim_thread.h, line 224 at r2 (raw file):
static inline void thread_wakeup(struct shim_thread* thread) { DkEventSet(thread->scheduler_event); DkThreadResume(thread->pal_handle);
Why is this needed?
LibOS/shim/src/shim_context-x86_64.c, line 234 at r2 (raw file):
'\0'
0
LibOS/shim/src/bookkeep/shim_signal.c, line 76 at r2 (raw file):
} SIGHANDLER_T; static SIGHANDLER_T default_sighandler[NUM_SIGS] = {
nitpick: const
LibOS/shim/src/bookkeep/shim_signal.c, line 610 at r2 (raw file):
context_is_libos(context)
This needs to be more exact. For example rip is in return_from_syscall, context_is_libos() is true, but it will just return to app. So the signal is queued, but won't be delivered until next app syscall.
The boundary needs to be determined and the code would be trickey.
and vDSO would complicates it.
LibOS/shim/src/bookkeep/shim_signal.c, line 795 at r2 (raw file):
long sysnr = shim_get_tcb()->context.syscall_nr; if (sysnr >= 0) { switch (pal_context_get_retval(context)) {
It's very nice to naturally handle syscall restart.
LibOS/shim/src/sys/shim_clone.c, line 325 at r2 (raw file):
thread->clear_child_tid = child_tidptr; unsigned long tls_base = flags & CLONE_SETTLS ? tls : get_tls_base();
nice catch.
LibOS/shim/src/vdso/vdso.c, line 15 at r1 (raw file):
/* XXX(borysp): What's the point of manual relocations/emulation of GOT here? Why cannot this be * just normal, generic relocation? Wouldn't loading vdso as first library be enough? */
The reason is to support fork emulated by graphene.
graphene fork does host fork and can load libos into new address.
which means vDSO bundled with LibOS can have new address. So
- transfer also vDSO from parent into child. so app sees vdso at the same address.
- but the entry point for libos, syscalldb, can be different address, so fixup the entry point.
With recent manifest redesign, the situation has changed a bit.
Pal/src/host/Linux/db_exception.c, line 44 at r2 (raw file):
SIGINT,
What if SIGINT arrived?
Pal/src/host/Linux-SGX/db_exception.c, line 283 at r1 (raw file):
This function is used only when async signal arrived during ocall. so this isn't called for sync signal.
My feeling is, this function should be deleted eventually.
the current logic should be sorted out somehow.
There was a problem hiding this comment.
Reviewed 51 of 55 files at r1, 4 of 4 files at r2.
Reviewable status: all files reviewed, 9 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski and @dimakuv)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 11 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, and @mkow)
LibOS/shim/src/elf/shim_rtld.c, line 1338 at r2 (raw file):
*/ static void* vdso_addr __attribute_migratable = NULL; static ElfW(Addr)* __vdso_shim_clock_gettime __attribute_migratable = NULL;
Why does syscalldb need to be in the vdso object (and not just LibOS)?
Also, why do we need the vdso object at all? I get that gettimeofday etc. will be fast anyway (no context switch to kernel needed), so no need to include them in vdso, but then is there anything preventing us from removing all of this? I think at least glibc should handle vdso not being vailable.
See also #2086 - I can rebase it once this PR is merged, but the point is that I would like syscalldb not to be exposed for dynamic linking, just passed through a pointer in TCB.
LibOS/shim/src/vdso/vdso.c, line 15 at r1 (raw file):
graphene fork does host fork and can load libos into new address.
Does that actually happen? I asked @mkow and he didn't think so.
Or is it that we still want to keep this possibility open?
LibOS/shim/test/regression/test_libos.py, line 614 at r2 (raw file):
# TODO: DISABLED TEMPORARILY #def test_000_gdb_backtrace(self):
Any problems with this? I would expect we need to store the old IP value on the new stack somewhere, and also use the __morestack trick (same as we do on enclave exit).
There was a problem hiding this comment.
Reviewable status: 53 of 55 files reviewed, 12 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @dimakuv, @mkow, @pwmarcz, and @yamahata)
LibOS/shim/include/shim_defs.h, line 6 at r2 (raw file):
Previously, yamahata wrote…
value is also taken from kernel?
Base value (512) is, but they rest might not be, i.e. I've just assigned them iteratively.
Update: seems to be exactly as in kernel, should I update the comment?
LibOS/shim/include/shim_thread.h, line 224 at r2 (raw file):
Previously, yamahata wrote…
Why is this needed?
Hmm, now I think of it, it might have been a mistake. I thought that thread_wakeup should also interrupt thread, even if it is not sleeping on scheduler_event, but these are probably 2 different things and should be done separately (i.e. sometimes we do not need/want to interrupt it). Let me think about it (I'll probably drop this change).
LibOS/shim/src/shim_context-x86_64.c, line 234 at r2 (raw file):
Previously, yamahata wrote…
'\0'0
Done.
LibOS/shim/src/bookkeep/shim_signal.c, line 76 at r2 (raw file):
Previously, yamahata wrote…
nitpick: const
Done.
LibOS/shim/src/bookkeep/shim_signal.c, line 610 at r2 (raw file):
Previously, yamahata wrote…
context_is_libos(context)This needs to be more exact. For example rip is in return_from_syscall, context_is_libos() is true, but it will just return to app. So the signal is queued, but won't be delivered until next app syscall.
The boundary needs to be determined and the code would be trickey.
and vDSO would complicates it.
This is true, but that should be fine in general case, I suppose? On normal Linux (async) signals are delivered only on transition from kernel to user space, so they can too be delayed for arbitrary time interval. The only difference is that on Linux in normal case scheduler will preempt a process and cause a signal delivery from time to time, but I think that can be delayed arbitrarily in some cases, couldn't it?
LibOS/shim/src/elf/shim_rtld.c, line 1338 at r2 (raw file):
Previously, pwmarcz (Paweł Marczewski) wrote…
Why does
syscalldbneed to be in the vdso object (and not just LibOS)?Also, why do we need the vdso object at all? I get that
gettimeofdayetc. will be fast anyway (no context switch to kernel needed), so no need to include them in vdso, but then is there anything preventing us from removing all of this? I think at least glibc should handle vdso not being vailable.See also #2086 - I can rebase it once this PR is merged, but the point is that I would like
syscalldbnot to be exposed for dynamic linking, just passed through a pointer in TCB.
See the comment in vdso.c.
I only made this code work with other changes, which requires to go through syscalldb when entering LibOS (the old code called shim_do_* directly, which is now prohibited.
Why do we need vdso at all? No idea, but I suppose glibc requires it, or some weird apps somehow called it directly, because the world is evil.
Actually what's the problem? We can add a similar stub (just like syscalldb, calling gs) here too. Possibly just include the header from glibc patches, I don't see any reason for it not to work.
LibOS/shim/src/vdso/vdso.c, line 15 at r1 (raw file):
Previously, pwmarcz (Paweł Marczewski) wrote…
graphene fork does host fork and can load libos into new address.
Does that actually happen? I asked @mkow and he didn't think so.
Or is it that we still want to keep this possibility open?
I'm not sure what's the situation now, but before loader rework, Pal loaded LibOS, which meant it could land at any address.
LibOS/shim/src/vdso/vdso.c, line 14 at r2 (raw file):
#include "vdso_syscall.h" /* XXX(borysp): What's the point of manual relocations/emulation of GOT here? Why cannot this be
TODO: remove this comment (answer below)
LibOS/shim/test/regression/test_libos.py, line 614 at r2 (raw file):
Previously, pwmarcz (Paweł Marczewski) wrote…
Any problems with this? I would expect we need to store the old IP value on the new stack somewhere, and also use the
__morestacktrick (same as we do on enclave exit).
Yes, gdb is stupid and does not understand stack switching unless it happens in function __morestack. I've just yet not invested enough time into investigating how to make it work, will do after this PR is "generally" accepted.
Pal/src/host/Linux/db_exception.c, line 44 at r2 (raw file):
Previously, yamahata wrote…
SIGINT,What if SIGINT arrived?
It's completely ignored, as we discussed it on one of the weekly calls a while ago. The reasoning is: we do not want to inject host signals at all, but there is one situation that seems useful: gracefully exit an application (e.g. web server etc.). For this case, applications usually use SIGTERM, so we allow for a one-time host-injection of this signal (but only if allowed explicitly in manifest).
Pal/src/host/Linux-SGX/db_exception.c, line 283 at r1 (raw file):
Previously, yamahata wrote…
This function is used only when async signal arrived during ocall. so this isn't called for sync signal.
My feeling is, this function should be deleted eventually.
the current logic should be sorted out somehow.
The untrusted host can easily call this function with arbitrary event, hence can call any upcall, both sync and async.
There was a problem hiding this comment.
Reviewable status: 53 of 55 files reviewed, 10 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @dimakuv, @mkow, and @yamahata)
LibOS/shim/src/elf/shim_rtld.c, line 1338 at r2 (raw file):
Oh, so gettimeofday etc. are still available through vdso, it's just that we need to keep this one address (__vdso_syscalldb) up to date. I misunderstood and somehow thought you are removing these functions. Never mind.
Actually what's the problem? We can add a similar stub (just like syscalldb, calling gs) here too. Possibly just include the header from glibc patches, I don't see any reason for it not to work.
You mean so that the stub will be imported by glibc/application code? I thought about this but AFAICT, functions from vdso are not linked for you. You have to parse the vdso page and find them yourself (or call dlopen and dlsym - I dug into glibc code and it looks like it also does that). And anyway, I'd need this pretty early in ld.so startup, before it issues any system call.
LibOS/shim/src/vdso/vdso.c, line 15 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
I'm not sure what's the situation now, but before loader rework, Pal loaded LibOS, which meant it could land at any address.
Right, so if it changed, it was relatively recently. I think it'd be nice to be able to assume that someday (isn't the whole "checkpointing" machinery, with rebasing values, because of LibOS possibly changing location?) but that's probably for later.
There was a problem hiding this comment.
Reviewable status: 53 of 55 files reviewed, 10 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @dimakuv, @mkow, and @yamahata)
LibOS/shim/src/elf/shim_rtld.c, line 1338 at r2 (raw file):
Previously, pwmarcz (Paweł Marczewski) wrote…
Oh, so gettimeofday etc. are still available through vdso, it's just that we need to keep this one address (
__vdso_syscalldb) up to date. I misunderstood and somehow thought you are removing these functions. Never mind.Actually what's the problem? We can add a similar stub (just like syscalldb, calling gs) here too. Possibly just include the header from glibc patches, I don't see any reason for it not to work.
You mean so that the stub will be imported by glibc/application code? I thought about this but AFAICT, functions from vdso are not linked for you. You have to parse the vdso page and find them yourself (or call dlopen and dlsym - I dug into glibc code and it looks like it also does that). And anyway, I'd need this pretty early in
ld.sostartup, before it issues any system call.
I mean if we use jmp *%gs:syscalldb then there is no need for linking (or any other way of chaning syscalldb address) so that works for you, right?
Yeah, vdso is not meant to be used by apps directly, but you know, app sometimes do weird things and we want to be as similar to normal Linux as possible
LibOS/shim/src/vdso/vdso.c, line 15 at r1 (raw file):
Previously, pwmarcz (Paweł Marczewski) wrote…
Right, so if it changed, it was relatively recently. I think it'd be nice to be able to assume that someday (isn't the whole "checkpointing" machinery, with rebasing values, because of LibOS possibly changing location?) but that's probably for later.
Checkpointing needs rebasing in case that the checkpoint blob is allocated at different address, as it has self-referencing pointers inside.
There was a problem hiding this comment.
Reviewable status: 53 of 55 files reviewed, 10 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @dimakuv, @mkow, and @yamahata)
LibOS/shim/src/elf/shim_rtld.c, line 1338 at r2 (raw file):
I mean if we use jmp *%gs:syscalldb then there is no need for linking (or any other way of chaning syscalldb address) so that works for you, right?
That's right, it works for me.
There was a problem hiding this comment.
Reviewed 50 of 55 files at r1, 3 of 4 files at r2, 2 of 2 files at r3.
Reviewable status: all files reviewed, 10 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @dimakuv, @mkow, and @yamahata)
LibOS/shim/src/sys/shim_clone.c, line 325 at r2 (raw file):
Previously, yamahata wrote…
nice catch.
Can you use ... ? tls_to_tls_base(tls) : get_tls_base() ?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 18 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, @mkow, and @yamahata)
LibOS/shim/src/sys/shim_clone.c, line 32 at r3 (raw file):
void* stack; unsigned long tls_base; PAL_CONTEXT* regs;
I wished this alone could be split off.
LibOS/shim/src/sys/shim_exit.c, line 158 at r3 (raw file):
} noreturn long shim_do_exit_group(int error_code) {
It would be minot, but can we split this off?
LibOS/shim/src/sys/shim_exit.c, line 168 at r3 (raw file):
} noreturn long shim_do_exit(int error_code) {
Ditto.
LibOS/shim/src/sys/shim_sigaction.c, line 30 at r3 (raw file):
#include "shim_utils.h" long shim_do_rt_sigaction(int signum, const struct __kernel_sigaction* act,
Also split off the renaming of this call?
LibOS/shim/src/sys/shim_sigaction.c, line 68 at r3 (raw file):
} long shim_do_rt_sigreturn(void) {
Minor, but also here.
LibOS/shim/src/sys/shim_sigaction.c, line 86 at r3 (raw file):
} long shim_do_rt_sigprocmask(int how, const __sigset_t* set, __sigset_t* oldset) {
... and here...
LibOS/shim/src/sys/shim_sigaction.c, line 181 at r3 (raw file):
} long shim_do_rt_sigsuspend(const __sigset_t* mask_ptr, size_t setsize) {
Also this one.
LibOS/shim/src/sys/shim_sigaction.c, line 224 at r3 (raw file):
} long shim_do_rt_sigpending(__sigset_t* set, size_t sigsetsize) {
.. and this one
There was a problem hiding this comment.
Reviewable status: all files reviewed, 19 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, @mkow, and @yamahata)
LibOS/shim/include/shim_table.h, line 40 at r3 (raw file):
struct __kernel_sigaction* oldact, size_t sigsetsize); long shim_do_rt_sigprocmask(int how, const __sigset_t* set, __sigset_t* oldset); long shim_do_rt_sigreturn(void);
Split off the renamings?
There was a problem hiding this comment.
Reviewable status: 47 of 55 files reviewed, 19 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, @mkow, @stefanberger, and @yamahata)
LibOS/shim/include/shim_table.h, line 40 at r3 (raw file):
Previously, stefanberger (Stefan Berger) wrote…
Split off the renamings?
Why? What's the point? You do not change these in you port, do you?
LibOS/shim/src/sys/shim_clone.c, line 325 at r2 (raw file):
Previously, stefanberger (Stefan Berger) wrote…
Can you use
... ? tls_to_tls_base(tls) : get_tls_base()?
I would rather rename get_tls_base to get_tls and set_tls accordingly. This value is arch specific and can be only understood by arch specific code and it might have nothing to do with a base.
There was a problem hiding this comment.
Reviewable status: 47 of 55 files reviewed, 18 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, @mkow, @stefanberger, and @yamahata)
LibOS/shim/src/sys/shim_clone.c, line 325 at r2 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
I would rather rename
get_tls_basetoget_tlsandset_tlsaccordingly. This value is arch specific and can be only understood by arch specific code and it might have nothing to do with abase.
Also good.
There was a problem hiding this comment.
Reviewed 39 of 55 files at r1, 1 of 4 files at r2, 6 of 8 files at r4.
Reviewable status: 53 of 55 files reviewed, 63 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @mkow, @stefanberger, and @yamahata)
a discussion (no related file):
Previously, boryspoplawski (Borys Popławski) wrote…
There is (at least) one known issue: I don't know how to handle a case, where we are executing a blocking syscall (e.g. read) and a signal comes after we enter LibOS, but before we do actual blocking host syscall. In such case we will just block and handle the signal only when returning to the user app (which can take arbitrarily long, since read can block indefinitely.
I see only one solution: just say that signals can arrive anytime and ignore the problem :) There is one caveat tho: we would need to change the way time_to_die is handled - probably introduce something like old preemption counter and check it every time we reach 0 (i.e. when we give up last lock). Generally speaking - doable.
This looks like the case of an async signal from another process in the same Graphene instance? Or is it all signals? I'm trying to understand the scope of this issue.
And IIUC, if another signal comes during the blocking host syscall, Linux will interrupt it anyway and Graphene will unfreeze and handle the first "pending" signal, right? In this case, the sysadmin tool (or the user) can simply repeat the signal after couple seconds, and Graphene will continue working.
Is there a particular scenario when this will lead to Graphene hanging completely? Like a parent waiting for a child's SIGCHLD, and being unfortunate to get SIGCHLD right-before issuing a host-level waitpid()?
LibOS/glibc-patches/glibc-2.23.patch, line 2 at r4 (raw file):
diff --git a/Makeconfig b/Makeconfig index 87a22e88bed375d073663063dd4a93444d72ba25..b69ff41077a5f279bde52aac11f03604fa61ae65 100644
Could you regenerate these patches with --full-index? I forgot why we did this but I specifically changed it in this PR: #1641. You can ask Michal about the history of that (I guess we simply wanted to make it more explicit)?
LibOS/glibc-patches/syscalldb-api.patch, line 82 at r4 (raw file):
+.macro SYSCALLDB +leaq .Lafter_syscalldb\@(%rip), %rcx +jmpq *syscalldb@GOTPCREL(%rip)
Are you using jmp instead of call to not mess with the user-level stack?
LibOS/shim/include/shim_defs.h, line 6 at r2 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Base value (512) is, but they rest might not be, i.e. I've just assigned them iteratively.
Update: seems to be exactly as in kernel, should I update the comment?
Yes, please add ...and values are taken from...
LibOS/shim/include/shim_defs.h, line 7 at r4 (raw file):
/* Names taken from the Linux kernel. */ #define ERESTARTSYS 512 /* Usual case - restart if SA_RESTART is set. */
Maybe we should add an assert that these are greater than LIBOS_SYSCALL_BOUND (or __NR_syscalls which is the same)
LibOS/shim/include/shim_internal.h, line 76 at r4 (raw file):
void syscalldb(void); noreturn void shim_do_syscall(PAL_CONTEXT* context); noreturn void return_from_syscall(PAL_CONTEXT* regs);
Why is the argument called regs and not context like others? Is it intentional?
LibOS/shim/include/shim_lock.h, line 63 at r4 (raw file):
shim_tcb_t* tcb = shim_get_tcb(); disable_preempt(tcb);
For my understanding: we don't need to block signals when we do lock() because signals are anyway are marked as "pending" if they arrive during LibOS/PAL execution? And we append such signals in a data race-free manner to lists of pending signals, and only examine them after the LibOS-syscall is done?
In other words, there is nothing that can "get in the way" of the current lock on the current thread, right? So there is no need for disabling preemption.
LibOS/shim/include/shim_signal.h, line 108 at r4 (raw file):
struct shim_signal { siginfo_t siginfo; };
This is an unnecessary level of indirection now. We could remove shim_signal and just use siginfo now. But maybe this will be useful in the future when we add some fields... Not blocking on this.
LibOS/shim/include/shim_signal.h, line 114 at r4 (raw file):
uint64_t get_stack_for_sighandler(uint64_t sp, bool use_altstack); bool is_on_altstack(uint64_t sp, stack_t* alt_stack);
Can you add some brief comments on what the arguments are and what the return values are? For example, I don't understand why we have sp argument in get_stack_for_sighandler()? And why we have alt_stack in is_on_altstack() -- does it also return the alt stack, in addition to telling true/false?
The function names are confusing, maybe just change them... Or fix function signatures.
LibOS/shim/include/shim_tcb.h, line 27 at r4 (raw file):
void* libos_stack_bottom; struct shim_context context; void* syscall_scratch_pc;
Could you add a brief comment what this is?
LibOS/shim/include/shim_thread.h, line 224 at r2 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Hmm, now I think of it, it might have been a mistake. I thought that
thread_wakeupshould also interrupt thread, even if it is not sleeping onscheduler_event, but these are probably 2 different things and should be done separately (i.e. sometimes we do not need/want to interrupt it). Let me think about it (I'll probably drop this change).
Doesn't seem to be needed...
LibOS/shim/include/shim_thread.h, line 52 at r4 (raw file):
* obtained via `malloc`. * `pending_mask` stores mask of signals present in this queue. * Accesses to this queue should be protected by a lock.
Which particular lock? I'd prefer to put its name here.
LibOS/shim/include/shim_thread.h, line 67 at r4 (raw file):
/* Internal LibOS stack size: 3 pages + one guard page. */ #define SHIM_THREAD_LIBOS_STACK_SIZE (3 * PAGE_SIZE + PAGE_SIZE)
This constant will be hard to find if we need to increase the internal stack size for whatever reason. Could you move it to shim_defs.h?
Also, any reason for 3 pages (12KB)? I guess you just wanted something small and a power of two?
LibOS/shim/include/shim_thread.h, line 282 at r4 (raw file):
* \brief Allocate a new stack for LibOS calls (emulated syscalls). * * \param thread Thread to get the stack.
Maybe 'Thread for which to allocate a new stack'
LibOS/shim/include/shim_thread.h, line 284 at r4 (raw file):
* \param thread Thread to get the stack. * * Allocates a new stack used to handle LibOS calls (emulated sysacalls).
Simply remove this sentence, it is exactly the same as above. Or at least fix the typo sysacalls
LibOS/shim/src/shim_context-x86_64.c, line 28 at r4 (raw file):
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0x80000000, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
Is this correct? You removed 0x80000000 which I thought was necessary here.
LibOS/shim/src/shim_context-x86_64.c, line 151 at r4 (raw file):
"shim_xstate_restore:\n" "push %rdi\n" "call is_xstate_extended\n"
Aren't you supposed to do smth like subq $128, %%rsp before calling this function? On the other hand, this is a manually written asm function, we don't use this 128B scratch space, so we don't need to sub/add it...
LibOS/shim/src/shim_context-x86_64.c, line 176 at r4 (raw file):
int src_is_xstate = is_xstate_extended(src); if (src_is_xstate) { copy_size = src->fpstate.sw_reserved.xstate_size + SHIM_FP_XSTATE_MAGIC2_SIZE;
Why not simply src->fpstate.sw_reserved.extended_size?
LibOS/shim/src/shim_context-x86_64.c, line 198 at r4 (raw file):
context->regs->rax = 0; context->syscall_nr = -1;
Is this unsetting just for sanity? We do not return from this function anyway...
LibOS/shim/src/shim_context-x86_64.c, line 203 at r4 (raw file):
PAL_CONTEXT* regs = context->regs; context->regs = NULL;
Is this zeroing just for sanity? We do not return from this function anyway...
LibOS/shim/src/shim_context-x86_64.c, line 239 at r4 (raw file):
/* Graphene does not change SS and assumes that it is constant so these flags are not strictly * needed, but we do store SS in ucontext so let's just set them. */
Could you add ...SS (stack segment register) and assumes...? It was not immediately clear what SS is.
LibOS/shim/src/shim_context-x86_64.c, line 248 at r4 (raw file):
/* XXX: Currently we assume that `struct shim_xstate`, `PAL_XREGS_STATE` and `struct _fpstate` * (just the header) are the very same sturcture. This mess needs to be fixed. */
typo: structure
LibOS/shim/src/shim_context-x86_64.c, line 254 at r4 (raw file):
"sse state structs differ"); if (shim_xstate_copy(xstate, (struct shim_xstate*)sigframe->uc.uc_mcontext.fpstate)) { sigframe->uc.uc_flags |= UC_FP_XSTATE;
Could you add a comment like /* this is an xsave-made xstate, it has extended state info */
LibOS/shim/src/shim_context-x86_64.c, line 271 at r4 (raw file):
context->rip = handler; context->rsp = stack; /* x64 SysV ABI mandates that DF flag is cleard and states that rest of flags is *not* preserved
typo: cleared
LibOS/shim/src/shim_context-x86_64.c, line 272 at r4 (raw file):
context->rsp = stack; /* x64 SysV ABI mandates that DF flag is cleard and states that rest of flags is *not* preserved * across functions calls, hence we just set flags to a default value (IF). */
typo: function calls
LibOS/shim/src/shim_context-x86_64.c, line 275 at r4 (raw file):
context->efl = 0x202; /* In case handler was defined as variadic/without prototype. */ context->rax = 0;
Could you expand a bit on this RAX = 0? I don't understand the comment. (Apparently to have a sensible return value in the handler that doesn't have a proper function epilogue?)
LibOS/shim/src/shim_context-x86_64.c, line 293 at r4 (raw file):
shim_xstate_copy(syscall_fpregs_buf, (struct shim_xstate*)sigframe->uc.uc_mcontext.fpstate); context->fpregs = (PAL_XREGS_STATE*)syscall_fpregs_buf;
What's the point of this line? The context->fpregs pointer anyhow points to syscall_fpregs_buf, this looks like a noop. Or is it just for sanity?
LibOS/shim/src/shim_syscalls.c, line 20 at r4 (raw file):
arch_syscall_arg_t, arch_syscall_arg_t); /* `context` are expected to be placed at the bottom of Graphene-internal stack. */
are -> is
LibOS/shim/src/shim_syscalls.c, line 47 at r4 (raw file):
/* Some syscalls e.g. `sigreturn` could have changed context and in reality we might be not * returning from a syscall. */ if (!handle_signal(context, NULL) && SHIM_TCB_GET(context.syscall_nr) >= 0) {
I can't understand this IF clause. "If there was no signal we handled, and there was an actual syscall we are returning from, then restart it this syscall is re-startable."
Or maybe handle_signal() returns FALSE if there was at least one handled signal? Then it starts to make sense, but then why does handle_signal have such a counter-intuitive return value?
LibOS/shim/src/syscallas-x86_64.S, line 32 at r4 (raw file):
.global syscalldb .type syscalldb, @function syscalldb:
Could you add a brief comment which registers contain which arguments and important values (like RSP contains user stack)
LibOS/shim/src/syscallas-x86_64.S, line 49 at r4 (raw file):
# Create PAL_CONTEXT struct on the stack. # reserve space for mxcsr + fpcw
...and for is_fpregs_used, right?
LibOS/shim/src/syscallas-x86_64.S, line 56 at r4 (raw file):
# zero rax as we will be storing some 0 on the stack mov $0, %eax
I suggest to remove the comment "zero rax ..." and just move this mov $0 ... close to pushes below (so it's clear that RAX is used here as a scratch register to push zeroes on the stack)
LibOS/shim/src/syscallas-x86_64.S, line 107 at r4 (raw file):
# save FP Control Word & MXCSR into current thread's TCB stmxcsr 0xc0(%rsp) fnstcw 0xc4(%rsp)
Why didn't you want to introduce macros with OFFSETs?
LibOS/shim/src/syscallas-x86_64.S, line 109 at r4 (raw file):
fnstcw 0xc4(%rsp) # fpregs is not populated
Please add: ..., i.e., is_fpregs_used = 0
LibOS/shim/src/syscallas-x86_64.S, line 116 at r4 (raw file):
mov %rsp, %rdi # save pointer to PAL_CONTEXT sub %rax, %rsp # allocate space for xstate and $~(0x40 - 1), %rsp
I assume 0x40 is the proper alignment for xstate? Could you add a comment on this (0x40 is the xstate alignment, same as SHIM_XSTATE_ALIGN)?
LibOS/shim/src/syscallas-x86_64.S, line 120 at r4 (raw file):
and $~0xF, %rsp # Required by System V AMD64 ABI. sub $8, %rsp
Don't you want to put 0 in ($rsp) to make it clear (for e.g. debuggers) that there is no RIP to return to?
LibOS/shim/src/syscallas-x86_64.S, line 130 at r4 (raw file):
.global return_from_syscall .type return_from_syscall, @function return_from_syscall:
Could you add a brief comment which registers contain which arguments? (RDI contains pointer to shim_context)
LibOS/shim/src/syscallas-x86_64.S, line 135 at r4 (raw file):
mov %rdi, %rbx movb 0xc6(%rbx), %al
Please add a comment that this is is_fpregs_used
LibOS/shim/src/syscallas-x86_64.S, line 139 at r4 (raw file):
je .Lrestore_fpu_state mov 0xb8(%rbx), %rdi
Please add a comment that this is pointer to fpregs
LibOS/shim/src/syscallas-x86_64.S, line 182 at r4 (raw file):
pop %rcx pop %rsp jmp *%gs:(SHIM_TCB_OFFSET + SHIM_TCB_SCRATCH_PC)
Why do you need SHIM_TCB_SCRATCH_PC? Why can't you just jmp *%rcx?
LibOS/shim/src/sys/shim_sigaction.c, line 120 at r4 (raw file):
struct shim_thread* cur = get_cur_thread(); lock(&cur->lock);
Doesn't this PR enable us to remove all such lock-current-thread pieces of code in LibOS? Because this was done (I assume) to prevent the situation where a nested signal handler also jumps into LibOS and races on cur->fields...
LibOS/shim/src/sys/shim_sigaction.c, line 207 at r4 (raw file):
/* XXX: This basicaly doubles the work of `shim_do_syscall`. The alternative would be to add * handling of saved signal maks (probably inside `current`) to `shim_do_syscall`, but as it
typo: masks
LibOS/shim/src/sys/shim_sigaction.c, line 208 at r4 (raw file):
/* XXX: This basicaly doubles the work of `shim_do_syscall`. The alternative would be to add * handling of saved signal maks (probably inside `current`) to `shim_do_syscall`, but as it * is specific to sigsuspend I'm leaving this here for now. */
I think it is fine. Maybe only add a comment that if you change shim_do_syscall(), please also change shim_do_rt_sigsuspend() in shim_syscalls.c
Pal/src/host/Linux/db_exception.c, line 104 at r4 (raw file):
/* This function must be reentrant and thread-safe - this includes `upcall` too! */ static void perform_signal_handling(int event, PAL_NUM arg, ucontext_t* uc) {
arg is not really "some" argument now, it is always a RIP at which signal arrived (or is it a boolean actually?).
Pal/src/host/Linux/db_exception.c, line 122 at r4 (raw file):
if (!ADDR_IN_PAL(rip)) { /* exception happened in application or LibOS code, normal benign case */ perform_signal_handling(event, (PAL_NUM)info->si_addr, uc);
In all other cases, you use the second argument as a boolean "was exception during PAL?", but not here. Bug?
Pal/src/host/Linux-SGX/db_exception.c, line 283 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
The untrusted host can easily call this function with arbitrary
event, hence can call anyupcall, both sync and async.
Borys is right, this function should ignore sync events. _DkHandleExternalEvent() can only be called in case of async signal (on a benign host). We didn't sanitize this case...
Pal/src/host/Linux-SGX/db_exception.c, line 262 at r4 (raw file):
case PAL_EVENT_QUIT: case PAL_EVENT_INTERRUPTED: arg = ADDR_IN_PAL(uc->rip);
This is a boolean. Is that what you want?
Pal/src/host/Linux-SGX/db_exception.c, line 284 at r4 (raw file):
/* TODO: shouldn't this function ignore sync events??? * actually what is the point of this function? */
This function is needed to abandon any PAL state, inject -EINTR, also call the async-signal handler if any, and restore the normal context. This is a remnant from the old days... Ideally we should merge it into another signal-handling func _DkExceptionHandler().
Pal/src/host/Linux-SGX/sgx_exception.c, line 178 at r4 (raw file):
/* TODO: we abandon PAL state here (possibly still holding some locks, etc) and return to * enclave; ideally we must unwind/fix the state and only then jump into enclave */ ucontext_set_function_parameters(uc, sgx_entry_return, -EINTR, event);
Wait why did you change it back? There was a reason why we do this: #2020
There was a problem hiding this comment.
Reviewable status: 38 of 55 files reviewed, 61 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, @stefanberger, and @yamahata)
a discussion (no related file):
This looks like the case of an async signal from another process in the same Graphene instance? Or is it all signals? I'm trying to understand the scope of this issue.
What's the difference? We only handle syscalls from inside Graphene (instance), with the small exception of SIGINT (only on demand tho).
Anyway, it's all (async) signals.
And IIUC, if another signal comes during the blocking host syscall, Linux will interrupt it anyway and Graphene will unfreeze and handle the first "pending" signal, right?
Yes, if it reached the host-syscall, it works. The problem is in the window between entering LibOS and issuing host syscall.
In this case, the sysadmin tool (or the user) can simply repeat the signal after couple seconds, and Graphene will continue working.
I do not understand this. I'm not concerned about host injected signals, they are not essential anyway.
Is there a particular scenario when this will lead to Graphene hanging completely? Like a parent waiting for a child's SIGCHLD, and being unfortunate to get SIGCHLD right-before issuing a host-level waitpid()?
No, I can't think of any valid scenario, but it doesn't mean there is none. And Graphene does not use host-level waitpid, waiting for children is emulated purely in LibOS. This also means that waitpid is immune to this issue - it calls Graphene thread sleeping function, which checks pending signals, so we are good here.
a discussion (no related file):
Forgot it before: PAL_CONTEXT should be renamed to something else. This now is general Graphene context (CPU context actually) and is arch-specific, not Pal specific.
Any ideas for the name? I won't change it in this PR, for less conflicts.
a discussion (no related file):
TODO: fix gcc-patches
LibOS/glibc-patches/glibc-2.23.patch, line 2 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Could you regenerate these patches with
--full-index? I forgot why we did this but I specifically changed it in this PR: #1641. You can ask Michal about the history of that (I guess we simply wanted to make it more explicit)?
There are some comments in the orginal PR (linked to the one you've posted).
Done.
LibOS/glibc-patches/syscalldb-api.patch, line 82 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Are you using
jmpinstead ofcallto not mess with the user-level stack?
Yes. This PR has this nice feature, that user app can even have no stack at all.
LibOS/shim/include/shim_defs.h, line 6 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Yes, please add
...and values are taken from...
Done.
LibOS/shim/include/shim_defs.h, line 7 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Maybe we should add an assert that these are greater than
LIBOS_SYSCALL_BOUND(or__NR_syscallswhich is the same)
Why would we add such an assert? These are error values, used internally in Graphene, generally not visible to the app (tho they might appear in the debug logs) and they have nothing to do with syscall numbers.
LibOS/shim/include/shim_internal.h, line 76 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why is the argument called
regsand notcontextlike others? Is it intentional?
No, not intentional. Otoh I've used context above in below, to name 2 different structs... Need some good naming convention here. BTW PAL_CONTEXT should be rename, because it's more like Graphene context now - it's arch specific, not Pal specific.
I should also add comments to all of these functions.
LibOS/shim/include/shim_lock.h, line 63 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
For my understanding: we don't need to block signals when we do
lock()because signals are anyway are marked as "pending" if they arrive during LibOS/PAL execution? And we append such signals in a data race-free manner to lists of pending signals, and only examine them after the LibOS-syscall is done?In other words, there is nothing that can "get in the way" of the current lock on the current thread, right? So there is no need for disabling preemption.
Exactly. But keep in mind LibOS-syscall will not sleep in such case (thread_sleep handles the case of having pending signals), but the host-level syscall still can block (as discussed in top-level comment). Also we sometimes examine them inside LibOS (e.g. to check if there are any pending).
LibOS/shim/include/shim_signal.h, line 108 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This is an unnecessary level of indirection now. We could remove
shim_signaland just usesiginfonow. But maybe this will be useful in the future when we add some fields... Not blocking on this.
Yes there is, but is there any harm in it? I've just liked the idea of hiding the details from as much code as possible (only exposing struct shim_signal).
LibOS/shim/include/shim_signal.h, line 114 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Can you add some brief comments on what the arguments are and what the return values are? For example, I don't understand why we have
spargument inget_stack_for_sighandler()? And why we havealt_stackinis_on_altstack()-- does it also return the alt stack, in addition to telling true/false?The function names are confusing, maybe just change them... Or fix function signatures.
Done.
I made it this way just to have more generic functions, as we have all these parameters (like alt_stack) on all call sites.
LibOS/shim/include/shim_tcb.h, line 27 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Could you add a brief comment what this is?
Done.
LibOS/shim/include/shim_thread.h, line 52 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Which particular lock? I'd prefer to put its name here.
Any lock. This defines the struct, not a particular instance of it.
LibOS/shim/include/shim_thread.h, line 67 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This constant will be hard to find if we need to increase the internal stack size for whatever reason. Could you move it to
shim_defs.h?Also, any reason for 3 pages (12KB)? I guess you just wanted something small and a power of two?
Done.
Yeah, there was no particular reasoning behind picking this value, except it looked nice :)
LibOS/shim/include/shim_thread.h, line 282 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Maybe 'Thread for which to allocate a new stack'
Done.
LibOS/shim/include/shim_thread.h, line 284 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Simply remove this sentence, it is exactly the same as above. Or at least fix the typo
sysacalls
Done.
LibOS/shim/src/shim_context-x86_64.c, line 28 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Is this correct? You removed
0x80000000which I thought was necessary here.
Why would it be? It did not set XSTATE_MAGIC and such, so it was always treated as non-extended context (with fxsrstor).
LibOS/shim/src/shim_context-x86_64.c, line 151 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Aren't you supposed to do smth like
subq $128, %%rspbefore calling this function? On the other hand, this is a manually written asm function, we don't use this 128B scratch space, so we don't need to sub/add it...
I'm completely confused. Why would I want to do it?
LibOS/shim/src/shim_context-x86_64.c, line 176 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why not simply
src->fpstate.sw_reserved.extended_size?
Because I have no idea what extended_size is for and I have no idea why would it be equal to xstate_size + MAGIC2_SIZE. The Linux kernel seems not to be using it (or at least I couldn't find it).
LibOS/shim/src/shim_context-x86_64.c, line 198 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Is this unsetting just for sanity? We do not return from this function anyway...
We do not return from this function in a sense that it does not need an epilogue, but we do jump out of it permanently.
As far as I remember it is only for sanity.
LibOS/shim/src/shim_context-x86_64.c, line 203 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Is this zeroing just for sanity? We do not return from this function anyway...
ditto
LibOS/shim/src/shim_context-x86_64.c, line 239 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Could you add
...SS (stack segment register) and assumes...? It was not immediately clear what SS is.
Done.
LibOS/shim/src/shim_context-x86_64.c, line 248 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
typo:
structure
Done.
LibOS/shim/src/shim_context-x86_64.c, line 254 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Could you add a comment like
/* this is an xsave-made xstate, it has extended state info */
Done.
LibOS/shim/src/shim_context-x86_64.c, line 271 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
typo:
cleared
Done.
LibOS/shim/src/shim_context-x86_64.c, line 272 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
typo:
function calls
Done.
LibOS/shim/src/shim_context-x86_64.c, line 275 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Could you expand a bit on this RAX = 0? I don't understand the comment. (Apparently to have a sensible return value in the handler that doesn't have a proper function epilogue?)
Done.
These are values at the entry to signal handler and eax contains the number of sse register used as arguments if the function is variadic.
LibOS/shim/src/shim_context-x86_64.c, line 293 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
What's the point of this line? The
context->fpregspointer anyhow points tosyscall_fpregs_buf, this looks like a noop. Or is it just for sanity?
ucontext_to_pal_context overwrites this field.
LibOS/shim/src/shim_syscalls.c, line 20 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
are->is
Done.
LibOS/shim/src/shim_syscalls.c, line 47 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I can't understand this IF clause. "If there was no signal we handled, and there was an actual syscall we are returning from, then restart it this syscall is re-startable."
Or maybe
handle_signal()returns FALSE if there was at least one handled signal? Then it starts to make sense, but then why doeshandle_signalhave such a counter-intuitive return value?
It's almost as you described: it's only restarted if the actual syscall implementation asked for it (returned -ERESTART*). If we have handled a syscall it means the context is already set and ready to be restored (the first part of if clause) and if we are not returning from a syscall (can happen for sigreturn syscall, which restores a different context than it had on the entry) then we should not restart (then the rax - on x86 - can have one of these values by coincidence).
LibOS/shim/src/syscallas-x86_64.S, line 32 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Could you add a brief comment which registers contain which arguments and important values (like
RSP contains user stack)
Done.
LibOS/shim/src/syscallas-x86_64.S, line 49 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
...and for
is_fpregs_used, right?
Yes, added this comment before there was is_fpregs_used.
LibOS/shim/src/syscallas-x86_64.S, line 56 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I suggest to remove the comment "zero rax ..." and just move this
mov $0 ...close to pushes below (so it's clear that RAX is used here as a scratch register to push zeroes on the stack)
Done.
LibOS/shim/src/syscallas-x86_64.S, line 107 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why didn't you want to introduce macros with OFFSETs?
Forgot about this and it was easier to test this way.
Done.
LibOS/shim/src/syscallas-x86_64.S, line 109 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please add:
..., i.e., is_fpregs_used = 0
Done.
LibOS/shim/src/syscallas-x86_64.S, line 116 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I assume
0x40is the proper alignment for xstate? Could you add a comment on this (0x40 is the xstate alignment, same as SHIM_XSTATE_ALIGN)?
Done.
LibOS/shim/src/syscallas-x86_64.S, line 120 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Don't you want to put
0in($rsp)to make it clear (for e.g. debuggers) that there is no RIP to return to?
I don't think debuggers can treat 0 as an invalid RIP. Also that what's cfi is used for (I'll fix that later, but in this PR, hopefully).
LibOS/shim/src/syscallas-x86_64.S, line 130 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Could you add a brief comment which registers contain which arguments? (
RDI contains pointer to shim_context)
Done.
LibOS/shim/src/syscallas-x86_64.S, line 135 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please add a comment that this is
is_fpregs_used
Done.
LibOS/shim/src/syscallas-x86_64.S, line 139 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please add a comment that this is
pointer to fpregs
Done.
LibOS/shim/src/syscallas-x86_64.S, line 182 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why do you need
SHIM_TCB_SCRATCH_PC? Why can't you justjmp *%rcx?
Because it might be the case that rip != rcx.
LibOS/shim/src/sys/shim_sigaction.c, line 120 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Doesn't this PR enable us to remove all such lock-current-thread pieces of code in LibOS? Because this was done (I assume) to prevent the situation where a nested signal handler also jumps into LibOS and races on
cur->fields...
Most of the fields can be read concurrently, but it might be doable on some fields, need to investigate.
LibOS/shim/src/sys/shim_sigaction.c, line 207 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
typo:
masks
Done (mask).
LibOS/shim/src/sys/shim_sigaction.c, line 208 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I think it is fine. Maybe only add a comment that
if you change shim_do_syscall(), please also change shim_do_rt_sigsuspend()inshim_syscalls.c
Done.
Pal/src/host/Linux/db_exception.c, line 104 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
argis not really "some" argument now, it is always a RIP at which signal arrived (or is it a boolean actually?).
There is a comment somewhere to document this behavior, which seems I have not done yet. Any good idea where to put a comment? Because it touches both Pal and LibOS (upcalls) I'm not sure where to put it.
Basically it's: bool is_in_pal for async signals and exception address for sync signals. Maybe this requires a rework, not a comment, idk.
Pal/src/host/Linux/db_exception.c, line 122 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
In all other cases, you use the second argument as a boolean "was exception during PAL?", but not here. Bug?
ditto
Pal/src/host/Linux-SGX/db_exception.c, line 262 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This is a boolean. Is that what you want?
ditto
Pal/src/host/Linux-SGX/sgx_exception.c, line 178 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Wait why did you change it back? There was a reason why we do this: #2020
The problem with the code before #2020 was that it used variadic function, passed an int, but read uint64_t. This function has an explicit argument of type uint64_t so it's ok.
There was a problem hiding this comment.
Reviewable status: 38 of 55 files reviewed, 62 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, @stefanberger, and @yamahata)
a discussion (no related file):
TODO: at rebase, squash the first commit ([LibOS] Move DkThreadResumeintothread_wakeup``) into the second one ([LibOS] Rework signal handling and syscall emulation). The first one was reverted as one of the fixup to the second one (unfortunately some code was changed around, so couldn't make reversing changes as a fixup to the first one).
LibOS/shim/include/shim_thread.h, line 224 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Doesn't seem to be needed...
Done - removed it (restored to the original). Also see the top level comment.
There was a problem hiding this comment.
Reviewed 11 of 16 files at r5, 4 of 4 files at r6.
Reviewable status: 52 of 55 files reviewed, 26 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @stefanberger, and @yamahata)
a discussion (no related file):
Previously, boryspoplawski (Borys Popławski) wrote…
This looks like the case of an async signal from another process in the same Graphene instance? Or is it all signals? I'm trying to understand the scope of this issue.
What's the difference? We only handle syscalls from inside Graphene (instance), with the small exception of SIGINT (only on demand tho).
Anyway, it's all (async) signals.And IIUC, if another signal comes during the blocking host syscall, Linux will interrupt it anyway and Graphene will unfreeze and handle the first "pending" signal, right?
Yes, if it reached the host-syscall, it works. The problem is in the window between entering LibOS and issuing host syscall.
In this case, the sysadmin tool (or the user) can simply repeat the signal after couple seconds, and Graphene will continue working.
I do not understand this. I'm not concerned about host injected signals, they are not essential anyway.
Is there a particular scenario when this will lead to Graphene hanging completely? Like a parent waiting for a child's SIGCHLD, and being unfortunate to get SIGCHLD right-before issuing a host-level waitpid()?
No, I can't think of any valid scenario, but it doesn't mean there is none. And Graphene does not use host-level
waitpid, waiting for children is emulated purely in LibOS. This also means thatwaitpidis immune to this issue - it calls Graphene thread sleeping function, which checks pending signals, so we are good here.
OK, understood all points (also during offline discussion).
Looks like it's fine (at least for now) to ignore this case. I would assume real-world applications have timeouts/timers/alarms to detect blocking-for-too-long syscalls.
a discussion (no related file):
Previously, boryspoplawski (Borys Popławski) wrote…
Forgot it before:
PAL_CONTEXTshould be renamed to something else. This now is general Graphene context (CPU context actually) and is arch-specific, not Pal specific.
Any ideas for the name? I won't change it in this PR, for less conflicts.
COMMON_CPU_CONTEXT? Or simply CPU_CONTEXT.
LibOS/glibc-patches/glibc-2.23.patch, line 2 at r6 (raw file):
diff --git a/Makeconfig b/Makeconfig index 87a22e88bed375d073663063dd4a93444d72ba25..3e2cf93fad804b81e67a476398a9c86c93ffa3cd 100644
Why did this hash change? Strange, but not blocking.
LibOS/glibc-patches/glibc-2.27.patch, line 2 at r6 (raw file):
diff --git a/Makeconfig b/Makeconfig index 86a71e580213f6e5de6e619d9f3370f4365b4ae2..515f8be0d13a14fe0588c357cfc5608503aca341 100644
ditto (not blocking)
LibOS/glibc-patches/glibc-2.31.patch, line 2 at r6 (raw file):
diff --git a/Makeconfig b/Makeconfig index f252842979a1d777e0f0c2bdafa7a65aee0805cd..0a044a7b60ae305af56c47982e3bdf1e25769527 100644
ditto (not blocking)
LibOS/glibc-patches/syscalldb-api.patch, line 82 at r4 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Yes. This PR has this nice feature, that user app can even have no stack at all.
Ok. Awesome.
LibOS/shim/include/shim_defs.h, line 7 at r4 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Why would we add such an assert? These are error values, used internally in Graphene, generally not visible to the app (tho they might appear in the debug logs) and they have nothing to do with syscall numbers.
Ok. I confused these UNIX error codes with syscall numbers.
LibOS/shim/include/shim_internal.h, line 76 at r4 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
No, not intentional. Otoh I've used context above in below, to name 2 different structs... Need some good naming convention here. BTW
PAL_CONTEXTshould be rename, because it's more like Graphene context now - it's arch specific, not Pal specific.
I should also add comments to all of these functions.
I see a stranded /**/, I guess still working on this.
LibOS/shim/include/shim_signal.h, line 108 at r4 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Yes there is, but is there any harm in it? I've just liked the idea of hiding the details from as much code as possible (only exposing
struct shim_signal).
Ok, fair enough.
LibOS/shim/src/shim_context-x86_64.c, line 28 at r4 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Why would it be? It did not set
XSTATE_MAGICand such, so it was always treated as non-extended context (withfxsrstor).
OK
LibOS/shim/src/shim_context-x86_64.c, line 151 at r4 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
I'm completely confused. Why would I want to do it?
This is what compilers add to function prologues and epilogues (to use these 128B as a scratch space for random data inside the function). But you don't need it here. This was me thinking out loud and forgetting to remove the comment afterwards.
LibOS/shim/src/shim_context-x86_64.c, line 176 at r4 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Because I have no idea what
extended_sizeis for and I have no idea why would it be equal toxstate_size+MAGIC2_SIZE. The Linux kernel seems not to be using it (or at least I couldn't find it).
Ok.
LibOS/shim/src/shim_context-x86_64.c, line 275 at r4 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Done.
These are values at the entry to signal handler andeaxcontains the number of sse register used as arguments if the function is variadic.
Ok. Cool, didn't know that.
LibOS/shim/src/shim_syscalls.c, line 47 at r4 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
It's almost as you described: it's only restarted if the actual syscall implementation asked for it (returned
-ERESTART*). If we have handled a syscall it means the context is already set and ready to be restored (the first part ofifclause) and if we are not returning from a syscall (can happen forsigreturnsyscall, which restores a different context than it had on the entry) then we should not restart (then therax- on x86 - can have one of these values by coincidence).
OK. I originally misunderstood the return value of handle_signal(). It returns true if it's gonna jump into the signal handler, and false if it's gonna return to normal user context.
LibOS/shim/src/syscallas-x86_64.S, line 182 at r4 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Because it might be the case that
rip != rcx.
Done. That was my blooper. I didn't notice pop $rcx in this code.
LibOS/shim/src/syscallas-x86_64.S, line 34 at r6 (raw file):
syscalldb: # on entry to this function rcx contains the return value (next instruction after syscall) # all other register can have arbitrary values
typo: registers
LibOS/shim/src/sys/shim_sigaction.c, line 120 at r4 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Most of the fields can be read concurrently, but it might be doable on some fields, need to investigate.
OK. Sure. We can postpone it to other PRs.
Pal/src/host/Linux/db_exception.c, line 104 at r4 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
There is a comment somewhere to document this behavior, which seems I have not done yet. Any good idea where to put a comment? Because it touches both Pal and LibOS (upcalls) I'm not sure where to put it.
Basically it's:
bool is_in_palfor async signals andexception addressfor sync signals. Maybe this requires a rework, not a comment, idk.
That's interesting. I guess you should add a comment in the LibOS upcall definition, and then brief comments in places in PAL where you actually set it.
Pal/src/host/Linux/db_exception.c, line 122 at r4 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
ditto
I suggest to add a comment that arg (or second argument) is an exception address for sync signals.
Please do it everywhere, I will resolve all other comments for now.
Pal/src/host/Linux-SGX/sgx_exception.c, line 178 at r4 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
The problem with the code before #2020 was that it used variadic function, passed an
int, but readuint64_t. This function has an explicit argument of typeuint64_tso it's ok.
OK. Sorry, didn't notice that you changed that function completely. The PR is way too big, can't keep it in my head :)
There was a problem hiding this comment.
Reviewed 3 of 55 files at r1, 2 of 4 files at r2, 1 of 8 files at r4, 1 of 16 files at r5.
Reviewable status: 54 of 55 files reviewed, 38 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @stefanberger, and @yamahata)
LibOS/shim/include/shim_internal.h, line 76 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I see a stranded
/**/, I guess still working on this.
I suggest simply: shim_context context and PAL_CONTEXT* regs (or PAL_CONTEXT* cpu_state)
LibOS/shim/include/shim_thread.h, line 90 at r6 (raw file):
struct shim_signal_queue signal_queue; /* For the field below, see the explanation in "LibOS/shim/src/bookkeep/shim_signal.c" near * `process_pending_signals_cnt`. */
This variable now has a prefix g_
LibOS/shim/src/bookkeep/shim_signal.c, line 130 at r6 (raw file):
static bool is_rt_sq_empty(struct shim_rt_signal_queue* queue) { return queue->get_idx == queue->put_idx;
For my own understanding: we don't need atomics here because all callers already take the appropriate (current thread's) lock?
LibOS/shim/src/bookkeep/shim_signal.c, line 149 at r6 (raw file):
} void get_pending_signals(__sigset_t* set) {
This function returns all pending signals, disregarding the current thread's signal mask. The next function applies the current thread's signal mask. Could we change their names so this is reflected? E.g.:
get_pending_signals->get_all_pending_signalshave_pending_signals->have_unblocked_pending_signals
In other words, I'd like to add a dimension of all | blocked | unblocked signals to the naming scheme.
Well, have_unblocked_pending_signals sounds unwieldy. Maybe we should assume "unblocked" by default in the naming scheme, but add all | blocked in all other names.
LibOS/shim/src/bookkeep/shim_signal.c, line 280 at r6 (raw file):
} static void force_signal(siginfo_t* info) {
Why do we have a special thread::forced_signal field? Why couldn't we add this signal via queue_append_signal()?
Is it because we don't have any priorities of signals? So we need to check forced_signal first, and then go to the queue of pending signals.
LibOS/shim/src/bookkeep/shim_signal.c, line 322 at r6 (raw file):
assert(context); if (is_internal_tid(get_cur_tid()) || context_is_libos(context)) {
Not that important, but isn't the first clause excessive? How can an internal thread be running something other than the LibOS code?
LibOS/shim/src/bookkeep/shim_signal.c, line 332 at r6 (raw file):
}; force_signal(&info); handle_signal(context, NULL);
Can you add /*old_mask_ptr=*/NULL? Here and everywhere.
LibOS/shim/src/bookkeep/shim_signal.c, line 351 at r6 (raw file):
} if (is_internal_tid(get_cur_tid()) || context_is_libos(context)) {
ditto (first clause excessive)
LibOS/shim/src/bookkeep/shim_signal.c, line 558 at r6 (raw file):
struct shim_vma_info vma_info = {.file = NULL}; if (is_internal(get_cur_thread()) || context_is_libos(context)
ditto (first clause excessive)
LibOS/shim/src/bookkeep/shim_signal.c, line 582 at r6 (raw file):
} static void quit_upcall(PAL_NUM arg, PAL_CONTEXT* context) {
TODO for myself: continue from here
LibOS/shim/src/sys/shim_sigaction.c, line 231 at r6 (raw file):
return -EFAULT; get_pending_signals(set);
Is this correct? get_pending_signals() returns all pending signals, whether blocked or unblocked. This is fine.
But manpages also say: If a signal is both blocked and has a disposition of "ignored", it is not added to the mask of pending signals. I see that get_pending_signals() returns blocked signals, but what about the second part of this clause ("disposition of ignored")?
LibOS/shim/src/vdso/arch/x86_64/vdso_syscall.h, line 8 at r6 (raw file):
#define _VDSO_SYSCALL_H static inline long arch_syscall(long (*syscalldb)(void), long nr, long arg1, long arg2) {
Can we change this name to something like vdso_arch_syscall()? This function is not generic, it only allows two arguments.
LibOS/shim/test/regression/test_libos.py, line 613 at r6 (raw file):
return match.group(1).strip() # TODO: DISABLED TEMPORARILY
Putting a blocking comment on this.
There was a problem hiding this comment.
Reviewable status: 54 of 55 files reviewed, 40 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @stefanberger, and @yamahata)
LibOS/shim/src/bookkeep/shim_signal.c, line 610 at r2 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
This is true, but that should be fine in general case, I suppose? On normal Linux (async) signals are delivered only on transition from kernel to user space, so they can too be delayed for arbitrary time interval. The only difference is that on Linux in normal case scheduler will preempt a process and cause a signal delivery from time to time, but I think that can be delayed arbitrarily in some cases, couldn't it?
Agreed with Borys. It's not perfect (the signal may be delivered much later), but at least for now it should be enough.
LibOS/shim/src/bookkeep/shim_signal.c, line 668 at r6 (raw file):
} /* XXX: This function assumes that stack is growning down. */
typo: growing
LibOS/shim/src/bookkeep/shim_signal.c, line 678 at r6 (raw file):
} /* XXX: This function assumes that stack is growning down. */
typo: growing
LibOS/shim/src/bookkeep/shim_signal.c, line 707 at r6 (raw file):
} struct shim_signal signal = { 0 };
TODO for myself: continue from here.
There was a problem hiding this comment.
Reviewable status: 54 of 55 files reviewed, 39 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, @stefanberger, and @yamahata)
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
OK, understood all points (also during offline discussion).
Looks like it's fine (at least for now) to ignore this case. I would assume real-world applications have timeouts/timers/alarms to detect blocking-for-too-long syscalls.
They rather use epoll, with a timeout
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
COMMON_CPU_CONTEXT? Or simplyCPU_CONTEXT.
CPU_CONTEXT or maybe struct cpu_context sounds good.
LibOS/glibc-patches/glibc-2.23.patch, line 2 at r6 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why did this hash change? Strange, but not blocking.
TBH, no idea. I've just double checked - I'm getting the new hash.
I generated this patches by applying the old one, changing needed things and generating them.
LibOS/glibc-patches/glibc-2.27.patch, line 2 at r6 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
ditto (not blocking)
ditto
LibOS/glibc-patches/glibc-2.31.patch, line 2 at r6 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
ditto (not blocking)
ditto
LibOS/shim/include/shim_internal.h, line 76 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I see a stranded
/**/, I guess still working on this.
Ah, yes, I've added this just so that if I forget, there is still some change that somebody see and remind me, I guess it worked :)
Done.
LibOS/shim/src/shim_context-x86_64.c, line 151 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This is what compilers add to function prologues and epilogues (to use these 128B as a scratch space for random data inside the function). But you don't need it here. This was me thinking out loud and forgetting to remove the comment afterwards.
Ah, 128 sounded like some red zone, I guess just number coincidence.
LibOS/shim/src/syscallas-x86_64.S, line 34 at r6 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
typo:
registers
Done.
LibOS/shim/src/sys/shim_sigaction.c, line 120 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
OK. Sure. We can postpone it to other PRs.
I'll leave a blocking comment to remind me to add a TODO or GH issue before merging it.
Pal/src/host/Linux/db_exception.c, line 104 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
That's interesting. I guess you should add a comment in the LibOS
upcalldefinition, and then brief comments in places in PAL where you actually set it.
Done.
Pal/src/host/Linux/db_exception.c, line 122 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I suggest to add a comment that
arg(orsecond argument) is an exception address for sync signals.Please do it everywhere, I will resolve all other comments for now.
I've added the comment to the function definition. Should I also add some at the call sites?
Pal/src/host/Linux-SGX/sgx_exception.c, line 178 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
OK. Sorry, didn't notice that you changed that function completely. The PR is way too big, can't keep it in my head :)
Yea, sorry about that, but I don't see a way to split it without doubling like half of the work.
There was a problem hiding this comment.
Reviewed 5 of 6 files at r7.
Reviewable status: 53 of 55 files reviewed, 37 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @stefanberger, and @yamahata)
LibOS/shim/include/shim_internal.h, line 139 at r7 (raw file):
* * \param context original CPU context * \param new_mask (OUT) new signal mask
The typical way is \param[out] new_mask new signal mask
LibOS/shim/include/shim_internal.h, line 141 at r7 (raw file):
* \param new_mask (OUT) new signal mask * * Restores cpu context in a architecure specific way. On entry to this funciton \p context holds
typo: function
LibOS/shim/include/shim_internal.h, line 143 at r7 (raw file):
* Restores cpu context in a architecure specific way. On entry to this funciton \p context holds * initial CPU context and this function extracts signal frame (generated by `prepare_sigframe`) * and restores it into \p context. The new signal mask (to be set) is written into \p new_mask.
What is this new signal mask? Isn't it just the previously saved old_mask from the signal frame generated by prepare_sigframe? If yes, then could you reword along these lines? It would be easier to understand.
LibOS/shim/include/shim_internal.h, line 153 at r7 (raw file):
* If the current instruction pointer in \p context points to a syscall instruction, arrange * \p context so that it is emulated. * Returns `true` if it was a syscall instruction (hence it was emulated).
was emulated? Technically will be emulated (looking at the code of handle_signal)
Ok, you have it explained in the next sentence. But why do you use past tense here then?
LibOS/shim/include/shim_internal.h, line 168 at r7 (raw file):
* If \p old_mask_ptr is not `NULL`, it is stored into the signal frame, otherwise the current * signal mask is used. * Returns `true` if a not-ignored signal was handled (hence \p context wasa changed), `false`
typo: was
Pal/src/host/Linux/db_exception.c, line 122 at r4 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
I've added the comment to the function definition. Should I also add some at the call sites?
That should be enough, resolving.
There was a problem hiding this comment.
Reviewable status: 53 of 55 files reviewed, 37 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, @stefanberger, and @yamahata)
LibOS/shim/include/shim_internal.h, line 139 at r7 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
The typical way is
\param[out] new_mask new signal mask
Done.
LibOS/shim/include/shim_internal.h, line 141 at r7 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
typo:
function
Done.
LibOS/shim/include/shim_internal.h, line 143 at r7 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
What is this
new signal mask? Isn't it just thepreviously saved old_mask from the signal frame generated by prepare_sigframe? If yes, then could you reword along these lines? It would be easier to understand.
How about this?
LibOS/shim/include/shim_internal.h, line 153 at r7 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
was emulated? Technicallywill be emulated(looking at the code ofhandle_signal)Ok, you have it explained in the next sentence. But why do you use past tense here then?
Done.
LibOS/shim/include/shim_internal.h, line 168 at r7 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
typo:
was
Done.
LibOS/shim/include/shim_thread.h, line 90 at r6 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This variable now has a prefix
g_
Done.
LibOS/shim/src/bookkeep/shim_signal.c, line 130 at r6 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
For my own understanding: we don't need atomics here because all callers already take the appropriate (current thread's) lock?
Yes, except there is also a global queue (with a appropriate lock).
LibOS/shim/src/bookkeep/shim_signal.c, line 149 at r6 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This function returns all pending signals, disregarding the current thread's signal mask. The next function applies the current thread's signal mask. Could we change their names so this is reflected? E.g.:
get_pending_signals->get_all_pending_signalshave_pending_signals->have_unblocked_pending_signalsIn other words, I'd like to add a dimension of
all | blocked | unblockedsignals to the naming scheme.Well,
have_unblocked_pending_signalssounds unwieldy. Maybe we should assume "unblocked" by default in the naming scheme, but addall | blockedin all other names.
Done.
I went with get_all_pending_signals and have_pending_signals.
LibOS/shim/src/bookkeep/shim_signal.c, line 280 at r6 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why do we have a special
thread::forced_signalfield? Why couldn't we add this signal viaqueue_append_signal()?Is it because we don't have any priorities of signals? So we need to check
forced_signalfirst, and then go to the queue of pending signals.
Two reasons: 1) priorities (as you pointed), 2) there could be a user generated SIGSEGV (via kill) while a sync SIGSEGV happens and in such case we need to forcibly deliver the latter.
LibOS/shim/src/bookkeep/shim_signal.c, line 322 at r6 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Not that important, but isn't the first clause excessive? How can an internal thread be running something other than the LibOS code?
No idea, old code always checked this, so I've left it. I guess there could be some weird async helper that doesn't run LibOS? Probably not ...
LibOS/shim/src/bookkeep/shim_signal.c, line 332 at r6 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Can you add
/*old_mask_ptr=*/NULL? Here and everywhere.
Done.
LibOS/shim/src/bookkeep/shim_signal.c, line 668 at r6 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
typo:
growing
Done.
LibOS/shim/src/bookkeep/shim_signal.c, line 678 at r6 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
typo:
growing
Done.
LibOS/shim/src/sys/shim_sigaction.c, line 231 at r6 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Is this correct?
get_pending_signals()returns all pending signals, whether blocked or unblocked. This is fine.But manpages also say:
If a signal is both blocked and has a disposition of "ignored", it is not added to the mask of pending signals. I see thatget_pending_signals()returns blocked signals, but what about the second part of this clause ("disposition of ignored")?
Well, I've just changed the existing code a bit and missed that it was incorrect.
Done.
LibOS/shim/src/vdso/arch/x86_64/vdso_syscall.h, line 8 at r6 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Can we change this name to something like
vdso_arch_syscall()? This function is not generic, it only allows two arguments.
Done.
There was a problem hiding this comment.
Reviewed 1 of 16 files at r5, 9 of 9 files at r8.
Reviewable status: all files reviewed, 38 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @stefanberger, and @yamahata)
a discussion (no related file):
One interesting observation is that we only switch to libos stack on syscalls. But we do not switch to the libos stack on signals. I.e., the alt stack created by PAL and provided by the host OS is used for LibOS upcalls (but when LibOS jumps into the app's signal handler, we switch to the app-provided stack, so that's fine).
I wonder if this is benign? It's a bit weird that we don't switch from PAL stack to LibOS stack on signal handling. But maybe it's just a matter of renaming libos_stack to graphene_stack or internal_stack?
a discussion (no related file):
Please add sys.enable_sigterm_injection to the manifest-options docs file.
LibOS/shim/include/shim_internal.h, line 88 at r8 (raw file):
* Emulates the syscall given the entry \p context. */ noreturn void shim_do_syscall(PAL_CONTEXT* context);
Maybe add a warning Never call this function in the LibOS code, the only entry to it is from syscalldb()!.
On the other hand, we must never call shim_do_* functions anyway. So feel free to ignore this comment.
LibOS/shim/include/shim_internal.h, line 172 at r8 (raw file):
* otherwise. */ bool handle_signal(PAL_CONTEXT* context, __sigset_t* old_mask_ptr);
As mentioned in my other comments, there is a subtle flow around calling this function: it must be called after the syscall-return value is set but before the syscall number is invalidated. I would add a comment about this here.
LibOS/shim/include/shim_thread.h, line 93 at r8 (raw file):
uint64_t pending_signals; struct shim_signal forced_signal;
Please add a comment about the need for this field.
LibOS/shim/src/shim_context-x86_64.c, line 165 at r8 (raw file):
"ret\n" );
Please remove one empty line
LibOS/shim/src/bookkeep/shim_signal.c, line 280 at r6 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Two reasons: 1) priorities (as you pointed), 2) there could be a user generated
SIGSEGV(viakill) while a syncSIGSEGVhappens and in such case we need to forcibly deliver the latter.
Could you add this description as a comment to forced_signal field in shim_thread.h?
LibOS/shim/src/bookkeep/shim_signal.c, line 322 at r6 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
No idea, old code always checked this, so I've left it. I guess there could be some weird async helper that doesn't run LibOS? Probably not ...
OK. Resolving this, doesn't matter much.
LibOS/shim/src/bookkeep/shim_signal.c, line 262 at r8 (raw file):
if (queue->get_idx < queue->put_idx) { *signal = queue->queue[queue->get_idx % ARRAY_SIZE(queue->queue)];
For standard signals, you mark the queue slot as empty. Don't you want to do the same here? I think simply queue->queue[queue->get_idx % ARRAY_SIZE(queue->queue)] = NULL; is enough.
LibOS/shim/src/bookkeep/shim_signal.c, line 694 at r8 (raw file):
} bool handle_signal(PAL_CONTEXT* context, __sigset_t* old_mask_ptr) {
Why does this function handle only one signal? Why not all pending signals? E.g., in a situation where my own thread got a SIGILL (say due to "syscall" instruction in SGX) and simultaneously another process sent my SIGCHLD, the current code will only run a SIGILL handler but won't run the SIGCHLD handler until after the start/restart of the next syscall.
LibOS/shim/src/bookkeep/shim_signal.c, line 716 at r8 (raw file):
bool got = false; bool was_process = false; /* First try to handle thread targeted signals, then these proces wide targeted. */
these proces -> this process (two typos)
LibOS/shim/src/bookkeep/shim_signal.c, line 769 at r8 (raw file):
int sig = signal.siginfo.si_signo; if (sig) {
Can we revert it to:
if (!sig)
return false;
... continue with the signal (but now no need for indentation)...
LibOS/shim/src/bookkeep/shim_signal.c, line 770 at r8 (raw file):
int sig = signal.siginfo.si_signo; if (sig) { lock(¤t->signal_dispositions->lock);
I got confused with this lock on signal_dispositions. Why do we need this lock, why can't the logic be protected via current->lock? Is this because a single signal_dispositions object may be shared between different threads?
In other words, would it be possible to change the implementation such that we only need the current->lock? Signals are anyway on the slow path, so there is no performance argument.
LibOS/shim/src/bookkeep/shim_signal.c, line 793 at r8 (raw file):
long sysnr = shim_get_tcb()->context.syscall_nr; if (sysnr >= 0) { switch (pal_context_get_retval(context)) {
The correct return value in the context depends on the fact that this function handle_signal() is called after the syscall return (more specifically, after the syscall-return path in Graphene executed pal_context_set_retval()). Could you add a comment on this, in case we ever change this logic (or add another special case where we call handle_signal() like in shim_do_rt_sigsuspend())?
Also, we end up here only if sysnr >= 0 which depends on the fact that handle_signal() is called before SHIM_TCB_SET(context.syscall_nr, -1). Again, could you add a comment about that?
In other words, we have a very specific assumption on calling handle_signal() in between the two steps after the syscall was executed. We need to document it.
LibOS/shim/src/bookkeep/shim_signal.c, line 829 at r8 (raw file):
/* Imo it should be `sigaction_make_defaults(sa);`, but Linux does this and LTP * explicitly tests for this ... */ sa->k_sa_handler = SIG_DFL;
This is an interesting quirk. Did something break when you used sigaction_make_defaults(sa); here? Why did LTP complain? Did LTP check for non-zero (preserved) sa flags?
LibOS/shim/src/bookkeep/shim_signal.c, line 845 at r8 (raw file):
// TODO: ignore SIGCHLD even if it's masked, when handler is set to SIG_IGN (probably not here) struct shim_signal* signal = malloc(sizeof(*signal));
Could you add a comment here that:
- for standard signals, a copy of this signal is saved in the signal queue
- but for RT signals, the pointer to this malloced signal is saved in the RT-signal queue
and that's why we need a malloc here instead of a stack-allocated signal.
LibOS/shim/src/bookkeep/shim_thread.c, line 129 at r8 (raw file):
need_mem_free = true; if (!DkVirtualMemoryProtect(addr, PAGE_SIZE, PAL_PROT_NONE)) {
Could you add a comment that we create a guard page for this stack?
LibOS/shim/src/bookkeep/shim_thread.c, line 196 at r8 (raw file):
/* TODO: I believe there is some Pal allocated initial stack which could be reused by the first * thread. */
What is the point of this TODO comment?
LibOS/shim/src/bookkeep/shim_thread.c, line 585 at r8 (raw file):
size_t roff = ADD_CP_OFFSET(sizeof(*thread->shim_tcb->context.regs)); new_thread->shim_tcb->context.regs = (void*)(base + roff); pal_context_copy(new_thread->shim_tcb->context.regs, thread->shim_tcb->context.regs);
How did the previous code work? I don't see any copying of context registers in the old code... Was it a bug? Did it work by coincidence?
LibOS/shim/test/regression/test_libos.py, line 613 at r6 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Putting a blocking comment on this.
Any update on this? Why it doesn't work?
Pal/src/host/Linux/db_exception.c, line 104 at r8 (raw file):
/* * This function must be reentrant (but only from Pal) and thread-safe - this includes `upcall`
What does this comment but only from Pal mean?
Pal/src/host/Linux-SGX/db_exception.c, line 283 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Borys is right, this function should ignore sync events.
_DkHandleExternalEvent()can only be called in case of async signal (on a benign host). We didn't sanitize this case...
Borys, could you add this sanitization? Seems trivial.
Pal/src/host/Linux-SGX/db_exception.c, line 297 at r8 (raw file):
/* we only end up in _DkHandleExternalEvent() if interrupted during host syscall; inform LibOS * layer that PAL was interrupted (by setting PAL_ERRNO) */ _DkRaiseFailure(PAL_ERROR_INTERRUPTED);
FYI: SGX PAL will need a cleanup in a follow-up PR. I believe a lot of stuff won't be needed here.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 39 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @stefanberger, and @yamahata)
a discussion (no related file):
Could you fix new-headers' macros like this: _SGX_LOG_H -> SGX_LOG_H_. Michal says the current macro names are reserved.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 36 unresolved discussions, "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, and @stefanberger)
LibOS/shim/src/bookkeep/shim_signal.c, line 610 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Agreed with Borys. It's not perfect (the signal may be delivered much later), but at least for now it should be enough.
Ok. Closing the gap will require careful change/review and this PR is already very big. hard to review.
Let's make it progress.
Can you please add some comment on it.
Pal/src/host/Linux/db_exception.c, line 44 at r2 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
It's completely ignored, as we discussed it on one of the weekly calls a while ago. The reasoning is: we do not want to inject host signals at all, but there is one situation that seems useful: gracefully exit an application (e.g. web server etc.). For this case, applications usually use
SIGTERM, so we allow for a one-time host-injection of this signal (but only if allowed explicitly in manifest).
Okay. Can you please add such description into commit message?
I didn't expect such change based on commit message. (or split off the behaviour change to another PR)
NOTE: I'm not opposing such behavior change itself.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 9 unresolved discussions, "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski and @yamahata)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 7 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski and @mkow)
LibOS/shim/src/bookkeep/shim_signal.c, line 135 at r12 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Can we not name it "a queue" then? It's just an object which is optional. The "queue" name is IMO misleading, an obvious connotation to a "queue" is that it holds a bunch of ordered elements, which are waiting in that order to be dealt with. Also, this name doesn't work well with standard signals, which are not queued, but in this implementation we insert them into this "queue".
But this is not just a object pointer, this a pointer to a "slot" (slot in a queue, to be fair), it always exists, we just check whether it's used.
And I would argue this is a queue and the fact that we limit it's size to 1 is an implementation detail which reader of the signature doesn't need to care about.
But to speed things up, I renamed it to something else.
LibOS/shim/src/bookkeep/shim_thread.c, line 196 at r8 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
TODO: write down all leftovers from this PR in an issue.
Done.
LibOS/shim/src/sys/shim_sigaction.c, line 120 at r4 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
I'll leave a blocking comment to remind me to add a TODO or GH issue before merging it.
Done.
Pal/src/host/Linux-SGX/db_exception.c, line 283 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Yeah, I'll open a issue with all leftovers/TODOs from this PR. Just waiting for this PR to be ready.
Done.
Pal/src/host/Linux-SGX/db_exception.c, line 317 at r14 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Then please add any text to it. Right now it looks like some garbage you forgot to delete before submitting the PR. No one except you known what you mean by this line ;)
Done.
There was a problem hiding this comment.
Reviewed 5 of 5 files at r17.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski)
LibOS/shim/src/bookkeep/shim_signal.c, line 135 at r12 (raw file):
And I would argue this is a queue and the fact that we limit it's size to 1 is an implementation detail which reader of the signature doesn't need to care about.
But using the name "queue" for just one slot is IMO misleading and so the signature with such an argument.
Anyways, the new name is much better :)
There was a problem hiding this comment.
Reviewed 12 of 16 files at r15, 1 of 2 files at r16, 5 of 5 files at r17.
Reviewable status: all files reviewed, 4 unresolved discussions, "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski)
The old version did not consider e.g. PLT as code.
Change log (most important only):
- unify CPU context structures - now we have only one version -
`PAL_CONTEXT` - which is shared between LibOS and PALs and it should
depend only on the host architecture (not OS),
- syscalls emulation changed:
- dedicated LibOS stack is now used for syscalls emulation,
- removed one indirection level in syscalls table - now it stores
`shim_do_*` functions directly,
- signal handling - completely rewritten:
- all signal queues use proper locking schemes now,
- signals are handled *only* when returning to the user app from LibOS
or PAL,
- nested signals are now possible,
- the app is allowed to jump out of signal handler with the same
sematics as on normal Linux,
- signal altstack is now fully supported,
- syscall restarting is now supported,
- doing a backtrace from the signal handler works properly,
- disallow injecting host-level signals, with one exception, see
`sys.enable_sigterm_injection` manifest option for more details.
boryspoplawski
force-pushed
the
borys/stacking
branch
from
d647f77 to
c24bddd
Compare
There was a problem hiding this comment.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @mkow)
a discussion (no related file):
Previously, mkow (Michał Kowalczyk) wrote…
yeah, I meant only in the message body, not
[Pal](we use "Pal" because that's the name of the corresponding top-level directory, but it's quite a questionable choice, I'm following it only for consistency)
Done.
LibOS/shim/src/shim_arch_prctl-x86_64.c, line 25 at r12 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
TODO: at rebase
Done.
LibOS/shim/src/shim_syscalls.c, line 13 at r12 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
TODO: at rebase
Done.
LibOS/shim/src/bookkeep/shim_thread.c, line 142 at r12 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
TODO: at rebase
Done.
There was a problem hiding this comment.
Reviewed 17 of 17 files at r18.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from different teams (1 more required, approved so far: ITL)
|
Jenkins, retest this please (just making sure, 2/5) |
|
Jenkins, retest Jenkins-SGX-apps please ( |
There was a problem hiding this comment.
Reviewable status:
complete! all files reviewed, all discussions resolved
|
Jenkins, retest this please (just making sure, 3/5) |
There was a problem hiding this comment.
Reviewed 2 of 8 files at r4, 5 of 16 files at r5, 1 of 9 files at r8, 2 of 7 files at r9, 2 of 14 files at r12, 14 of 27 files at r13, 2 of 2 files at r14, 9 of 16 files at r15, 1 of 2 files at r16, 2 of 5 files at r17, 17 of 17 files at r18.
Reviewable status:
|
Jenkins, retest this please (just making sure, 4/5) |
|
3/5 had one fail: Jenkins-18.04 - LibOS/regression |
|
Jenkins, retest this please (just making sure, 5/5) |
There was a problem hiding this comment.
looks like this 'storm' is arriving now ;-)
Reviewable status:
There was a problem hiding this comment.
Just merged :)
Feel free to pm me or send me an email (address in commits) if I can be of any help with rebasing
Reviewable status:
@boryspoplawski Thanks for the offer. The PPC64 port is back on track after the pitstop. It has become really beautiful with the PAL_CONTEXT used also now on the shim layer! The rebasing was a challenge in some parts but I am amazed in how things fit together. Great work, guys! |
Description of the changes
This PR completely reworks signal handling and (as a side effect) syscall emulation in LibOS.
More info in commit message and appropriate places in code.
Please point me to places in the code that need more comments/explanation.
Fixes #348, closes #1218, fixes #1404, fixes #1481
Need to test #396, #453, #1919 (after all issues in this PR are fixed)
(This is next version of #2071)
This change is