| Message ID | 20240426141823.112366-2-zhubojun.zbj@antgroup.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/sgx: Explicitly give up the CPU in EDMM's ioctl() to avoid softlockup | expand |
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(&encl->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.
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 <zhubojun.zbj@antgroup.com> > --- > 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, > &secinfo, 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
Hi Dave, Appreciate for your review! > On Apr 27, 2024, at 01:06, Dave Hansen <dave.hansen@intel.com> 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(&encl->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(&encl->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(&encl->lock) with `sgx_lock_enclave(encl)` in SGX in-tree driver and then wrap reschedule operation in `sgx_lock_enclave()` ? Regards, Bojun
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, &secinfo, 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();
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 <zhubojun.zbj@antgroup.com> --- arch/x86/kernel/cpu/sgx/ioctl.c | 40 +++++++++++++++++++++++++++++---- 1 file changed, 36 insertions(+), 4 deletions(-)