Re: [RFC PATCH v2 1/1] x86/sgx: Explicitly give up the CPU in EDMM's ioctl() to avoid softlockup
Hi Dave,
Appreciate for your review!
> On Apr 27, 2024, at 01:06, Dave Hansen wrote:
>
> On 4/26/24 07:18, Bojun Zhu wrote:
>> for (c = 0 ; c < modp->length; c += PAGE_SIZE) {
>> +if (sgx_check_signal_and_resched()) {
>> +if (!c)
>> +ret = -ERESTARTSYS;
>> +
>> +goto out;
>> +}
>
> This construct is rather fugly. Let's not perpetuate it, please. Why
> not do:
>
> int ret = -ERESTARTSYS;
>
> ...
> for (c = 0 ; c < modp->length; c += PAGE_SIZE) {
> if (sgx_check_signal_and_resched())
> goto out;
>
> Then, voila, when c==0 on the first run through the loop, you'll get a
> ret=-ERESTARTSYS.
>
Okay, I will refine it later.
> But honestly, it seems kinda silly to annotate all these loops with
> explicit cond_resched()s. I'd much rather do this once and, for
> instance, just wrap the enclave locks:
>
> - mutex_lock(>lock);
> + sgx_lock_enclave(encl);
>
> and then have the lock function do the rescheds. I assume that
> mutex_lock() isn't doing this generically for performance reasons. But
> we don't care in SGX land and can just resched to our heart's content.
`mutex_lock(>lock)` appears in everywhere in SGX in-tree driver.
But it seems that we only need to additionally invoke `cond_resched()` for
the sgx_enclave_{restrict_permissions | modify_types | remove_pages }
and sgx_ioc_add_pages()’s ioctl()s.
Shall we replace all the `mutex_lock(>lock) with `sgx_lock_enclave(encl)`
in SGX in-tree driver and then wrap reschedule operation in
`sgx_lock_enclave()` ?
Regards,
Bojun
Re: [RFC PATCH v2 1/1] x86/sgx: Explicitly give up the CPU in EDMM's ioctl() to avoid softlockup
On Fri Apr 26, 2024 at 5:18 PM EEST, Bojun Zhu wrote:
> EDMM's ioctl()s support batch operations, which may be
> time-consuming. Try to explicitly give up the CPU as the prefix
> operation at the every begin of "for loop" in
> sgx_enclave_{ modify_types | restrict_permissions | remove_pages}
> to give other tasks a chance to run, and avoid softlockup warning.
>
> Additionally perform pending signals check as the prefix operation,
> and introduce sgx_check_signal_and_resched(),
> which wraps all the checks.
>
> The following has been observed on Linux v6.9-rc5 with kernel
> preemptions disabled(by configuring "PREEMPT_NONE=y"), when kernel
> is requested to restrict page permissions of a large number of EPC pages.
>
> [ cut here ]
> watchdog: BUG: soft lockup - CPU#45 stuck for 22s!
> ...
> RIP: 0010:sgx_enclave_restrict_permissions+0xba/0x1f0
> ...
> Call Trace:
> sgx_ioctl
> __x64_sys_ioctl
> x64_sys_call
> do_syscall_64
> entry_SYSCALL_64_after_hwframe
> [ end trace ]
>
> Signed-off-by: Bojun Zhu
> ---
> arch/x86/kernel/cpu/sgx/ioctl.c | 40 +
> 1 file changed, 36 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
> index b65ab214bdf5..e0645920bcb5 100644
> --- a/arch/x86/kernel/cpu/sgx/ioctl.c
> +++ b/arch/x86/kernel/cpu/sgx/ioctl.c
> @@ -365,6 +365,20 @@ static int sgx_validate_offset_length(struct sgx_encl
> *encl,
> return 0;
> }
>
> +/*
> + * Check signals and invoke scheduler. Return true for a pending signal.
> + */
> +static bool sgx_check_signal_and_resched(void)
> +{
> + if (signal_pending(current))
> + return true;
> +
> + if (need_resched())
> + cond_resched();
> +
> + return false;
> +}
> +
> /**
> * sgx_ioc_enclave_add_pages() - The handler for %SGX_IOC_ENCLAVE_ADD_PAGES
> * @encl: an enclave pointer
> @@ -432,16 +446,13 @@ static long sgx_ioc_enclave_add_pages(struct sgx_encl
> *encl, void __user *arg)
> return -EINVAL;
>
> for (c = 0 ; c < add_arg.length; c += PAGE_SIZE) {
> - if (signal_pending(current)) {
> + if (sgx_check_signal_and_resched()) {
> if (!c)
> ret = -ERESTARTSYS;
>
> break;
> }
>
> - if (need_resched())
> - cond_resched();
> -
> ret = sgx_encl_add_page(encl, add_arg.src + c, add_arg.offset +
> c,
> , add_arg.flags);
> if (ret)
> @@ -746,6 +757,13 @@ sgx_enclave_restrict_permissions(struct sgx_encl *encl,
> secinfo.flags = modp->permissions & SGX_SECINFO_PERMISSION_MASK;
>
> for (c = 0 ; c < modp->length; c += PAGE_SIZE) {
> + if (sgx_check_signal_and_resched()) {
> + if (!c)
> + ret = -ERESTARTSYS;
> +
> + goto out;
> + }
> +
> addr = encl->base + modp->offset + c;
>
> sgx_reclaim_direct();
> @@ -913,6 +931,13 @@ static long sgx_enclave_modify_types(struct sgx_encl
> *encl,
> secinfo.flags = page_type << 8;
>
> for (c = 0 ; c < modt->length; c += PAGE_SIZE) {
> + if (sgx_check_signal_and_resched()) {
> + if (!c)
> + ret = -ERESTARTSYS;
> +
> + goto out;
> + }
> +
> addr = encl->base + modt->offset + c;
>
> sgx_reclaim_direct();
> @@ -1101,6 +1126,13 @@ static long sgx_encl_remove_pages(struct sgx_encl
> *encl,
> secinfo.flags = SGX_SECINFO_R | SGX_SECINFO_W | SGX_SECINFO_X;
>
> for (c = 0 ; c < params->length; c += PAGE_SIZE) {
> + if (sgx_check_signal_and_resched()) {
> + if (!c)
> + ret = -ERESTARTSYS;
> +
> + goto out;
> + }
> +
> addr = encl->base + params->offset + c;
>
> sgx_reclaim_direct();
I think Dave's suggestions make sense, so unfortunately needs yet
another spin.
BR, Jarkko
Re: [RFC PATCH v2 1/1] x86/sgx: Explicitly give up the CPU in EDMM's ioctl() to avoid softlockup
On 4/26/24 07:18, Bojun Zhu wrote:
> for (c = 0 ; c < modp->length; c += PAGE_SIZE) {
> + if (sgx_check_signal_and_resched()) {
> + if (!c)
> + ret = -ERESTARTSYS;
> +
> + goto out;
> + }
This construct is rather fugly. Let's not perpetuate it, please. Why
not do:
int ret = -ERESTARTSYS;
...
for (c = 0 ; c < modp->length; c += PAGE_SIZE) {
if (sgx_check_signal_and_resched())
goto out;
Then, voila, when c==0 on the first run through the loop, you'll get a
ret=-ERESTARTSYS.
But honestly, it seems kinda silly to annotate all these loops with
explicit cond_resched()s. I'd much rather do this once and, for
instance, just wrap the enclave locks:
- mutex_lock(>lock);
+ sgx_lock_enclave(encl);
and then have the lock function do the rescheds. I assume that
mutex_lock() isn't doing this generically for performance reasons. But
we don't care in SGX land and can just resched to our heart's content.
[RFC PATCH v2 1/1] x86/sgx: Explicitly give up the CPU in EDMM's ioctl() to avoid softlockup
EDMM's ioctl()s support batch operations, which may be
time-consuming. Try to explicitly give up the CPU as the prefix
operation at the every begin of "for loop" in
sgx_enclave_{ modify_types | restrict_permissions | remove_pages}
to give other tasks a chance to run, and avoid softlockup warning.
Additionally perform pending signals check as the prefix operation,
and introduce sgx_check_signal_and_resched(),
which wraps all the checks.
The following has been observed on Linux v6.9-rc5 with kernel
preemptions disabled(by configuring "PREEMPT_NONE=y"), when kernel
is requested to restrict page permissions of a large number of EPC pages.
[ cut here ]
watchdog: BUG: soft lockup - CPU#45 stuck for 22s!
...
RIP: 0010:sgx_enclave_restrict_permissions+0xba/0x1f0
...
Call Trace:
sgx_ioctl
__x64_sys_ioctl
x64_sys_call
do_syscall_64
entry_SYSCALL_64_after_hwframe
[ end trace ]
Signed-off-by: Bojun Zhu
---
arch/x86/kernel/cpu/sgx/ioctl.c | 40 +
1 file changed, 36 insertions(+), 4 deletions(-)
diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
index b65ab214bdf5..e0645920bcb5 100644
--- a/arch/x86/kernel/cpu/sgx/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/ioctl.c
@@ -365,6 +365,20 @@ static int sgx_validate_offset_length(struct sgx_encl
*encl,
return 0;
}
+/*
+ * Check signals and invoke scheduler. Return true for a pending signal.
+ */
+static bool sgx_check_signal_and_resched(void)
+{
+ if (signal_pending(current))
+ return true;
+
+ if (need_resched())
+ cond_resched();
+
+ return false;
+}
+
/**
* sgx_ioc_enclave_add_pages() - The handler for %SGX_IOC_ENCLAVE_ADD_PAGES
* @encl: an enclave pointer
@@ -432,16 +446,13 @@ static long sgx_ioc_enclave_add_pages(struct sgx_encl
*encl, void __user *arg)
return -EINVAL;
for (c = 0 ; c < add_arg.length; c += PAGE_SIZE) {
- if (signal_pending(current)) {
+ if (sgx_check_signal_and_resched()) {
if (!c)
ret = -ERESTARTSYS;
break;
}
- if (need_resched())
- cond_resched();
-
ret = sgx_encl_add_page(encl, add_arg.src + c, add_arg.offset +
c,
, add_arg.flags);
if (ret)
@@ -746,6 +757,13 @@ sgx_enclave_restrict_permissions(struct sgx_encl *encl,
secinfo.flags = modp->permissions & SGX_SECINFO_PERMISSION_MASK;
for (c = 0 ; c < modp->length; c += PAGE_SIZE) {
+ if (sgx_check_signal_and_resched()) {
+ if (!c)
+ ret = -ERESTARTSYS;
+
+ goto out;
+ }
+
addr = encl->base + modp->offset + c;
sgx_reclaim_direct();
@@ -913,6 +931,13 @@ static long sgx_enclave_modify_types(struct sgx_encl *encl,
secinfo.flags = page_type << 8;
for (c = 0 ; c < modt->length; c += PAGE_SIZE) {
+ if (sgx_check_signal_and_resched()) {
+ if (!c)
+ ret = -ERESTARTSYS;
+
+ goto out;
+ }
+
addr = encl->base + modt->offset + c;
sgx_reclaim_direct();
@@ -1101,6 +1126,13 @@ static long sgx_encl_remove_pages(struct sgx_encl *encl,
secinfo.flags = SGX_SECINFO_R | SGX_SECINFO_W | SGX_SECINFO_X;
for (c = 0 ; c < params->length; c += PAGE_SIZE) {
+ if (sgx_check_signal_and_resched()) {
+ if (!c)
+ ret = -ERESTARTSYS;
+
+ goto out;
+ }
+
addr = encl->base + params->offset + c;
sgx_reclaim_direct();
--
2.25.1
