[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 1/4] coroutine: support SafeStack in ucontext backend
From: |
Daniele Buono |
Subject: |
Re: [PATCH 1/4] coroutine: support SafeStack in ucontext backend |
Date: |
Fri, 22 May 2020 11:18:20 -0400 |
User-agent: |
Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.8.0 |
Hi Stefan,
thank you so much for reviewing this.
See my answers below:
On 5/21/2020 5:44 AM, Stefan Hajnoczi wrote:
On Wed, Apr 29, 2020 at 03:44:17PM -0400, Daniele Buono wrote:
diff --git a/include/qemu/coroutine_int.h b/include/qemu/coroutine_int.h
index bd6b0468e1..2ffd75ddbe 100644
--- a/include/qemu/coroutine_int.h
+++ b/include/qemu/coroutine_int.h
@@ -28,6 +28,12 @@
#include "qemu/queue.h"
#include "qemu/coroutine.h"
+#if defined(__has_feature) && __has_feature(safe_stack)
+#define CONFIG_SAFESTACK 1
Please perform this feature check in ./configure. That way
CONFIG_SAFESTACK will be defined alongside all the other CONFIG_* values
and be available to C and Makefiles via config-host.h and
config-host.mak.
Sure, will do this in v2.
@@ -160,6 +169,19 @@ Coroutine *qemu_coroutine_new(void)
/* swapcontext() in, siglongjmp() back out */
if (!sigsetjmp(old_env, 0)) {
start_switch_fiber(&fake_stack_save, co->stack, co->stack_size);
+#ifdef CONFIG_SAFESTACK
+ /*
+ * Before we swap the context, set the new unsafe stack
+ * The unsafe stack grows just like the normal stack, so start from
+ * the last usable location of the memory area.
+ * NOTE: we don't have to re-set it afterwards because sigsetjmp was
+ * called with the original usp. Since we are not coming back with a
+ * swapcontext, but with a siglongjmp, when we are back here we
+ * already have usp restored to the valid one for this function
I don't understand this comment. __safestack_unsafe_stack_ptr is a
thread-local variable, not a CPU register. How will siglongjmp()
automatically restore it?
Correct, setjmp/longjmp have no visibility of the unsafe stack. What I
meant is that it is not automatically restored by the longjmp itself,
but by code the compiler adds around the sigsetjmp.
Specifically, every sigsetjmp/sigjmp is intercepted by the compiler, the
current value of __safestack_unsafe_stack_ptr is saved on the normal
(safe) stack.
Right after the sigsetjmp call it is then restored.
I will change the comment to make it clearer.
In practice, this is what happens:
Original clang implementation in qemu_coroutine_new:
----
40130c: callq 4008d0 <__sigsetjmp@plt>
401311: cmp $0x0,%eax
401314: jne 40132d <qemu_coroutine_new+0x12d>
----
Clang Implementation with safestack:
----
4027a7: mov %rdx,-0x38(%rbp) <- Save unsafe ptr onto the safe stack
[...]
40289c: callq 401410 <__sigsetjmp@plt>
4028a1: mov 0x201738(%rip),%rdi # 603fe0
<__safestack_unsafe_stack_ptr@@Base+0x603fe0>
4028a8: mov -0x38(%rbp),%rbx
4028ac: mov %rbx,%fs:(%rdi) <- Restore the unsafe ptr
4028b0: cmp $0x0,%eax
4028b3: jne 4028d9 <qemu_coroutine_new+0x179>
+ */
+ void *usp = co->unsafe_stack + co->unsafe_stack_size;
+ __safestack_unsafe_stack_ptr = usp;
+#endif
swapcontext(&old_uc, &uc);
}