Hi forks,
This is the second version of the patch to fix the softlockup in EDMM iotcl()[1].
If we run an enclave equipped with large EPC(30G or greater on my platfrom)
on the Linux with kernel preemptions disabled(by configuring
"CONFIG_PREEMPT_NONE=y"), we will get the following softlockup warning
messages being reported in "dmesg" log:
The EDMM's ioctl()s (sgx_ioc_enclave_{ modify_types | restrict_permissions | remove_pages})
interface provided by kernel support batch changing attributes of enclave's EPC.
If userspace App requests kernel to handle too many EPC pages, kernel
may stuck for a long time(with preemption disabled).
The log is as follows:
------------[ cut here ]------------
[ 901.101294] watchdog: BUG: soft lockup - CPU#92 stuck for 23s! [occlum-run:4289]
[ 901.109617] Modules linked in: veth xt_conntrack xt_MASQUERADE nf_conntrack_netlink nfnetlink xfrm_user xfrm_algo iptable_nat nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 libcrc32c xt_addrtype iptable_filter br_netfilter bridge stp llc overlay nls_iso8859_1 intel_rapl_msr intel_rapl_common intel_uncore_frequency intel_uncore_frequency_common i10nm_edac nfit binfmt_misc ipmi_ssif x86_pkg_temp_thermal intel_powerclamp coretemp kvm_intel kvm crct10dif_pclmul polyval_clmulni polyval_generic ghash_clmulni_intel sha512_ssse3 sha256_ssse3 pmt_telemetry sha1_ssse3 pmt_class joydev intel_sdsi input_leds aesni_intel crypto_simd cryptd dax_hmem cxl_acpi cmdlinepart rapl cxl_core ast spi_nor intel_cstate drm_shmem_helper einj mtd drm_kms_helper mei_me idxd isst_if_mmio isst_if_mbox_pci isst_if_common intel_vsec idxd_bus mei acpi_ipmi ipmi_si ipmi_devintf ipmi_msghandler acpi_pad acpi_power_meter mac_hid sch_fq_codel msr parport_pc ppdev lp parport ramoops reed_solomon pstore_blk pstore_zone efi_pstore drm ip_tables x_tables
[ 901.109670] autofs4 mlx5_ib ib_uverbs ib_core hid_generic usbhid hid ses enclosure scsi_transport_sas mlx5_core pci_hyperv_intf mlxfw igb ahci psample i2c_algo_bit i2c_i801 spi_intel_pci xhci_pci tls megaraid_sas dca spi_intel crc32_pclmul i2c_smbus i2c_ismt libahci xhci_pci_renesas wmi pinctrl_emmitsburg
[ 901.109691] CPU: 92 PID: 4289 Comm: occlum-run Not tainted 6.9.0-rc5 #3
[ 901.109693] Hardware name: Inspur NF5468-M7-A0-R0-00/NF5468-M7-A0-R0-00, BIOS 05.02.01 05/08/2023
[ 901.109695] RIP: 0010:sgx_enclave_restrict_permissions+0xba/0x1f0
[ 901.109701] Code: 48 c1 e6 05 48 89 d1 48 8d 5c 24 40 b8 0e 00 00 00 48 2b 8e 70 8e 15 8b 48 c1 e9 05 48 c1 e1 0c 48 03 8e 68 8e 15 8b 0f 01 cf <a9> 00 00 00 40 0f 85 b2 00 00 00 85 c0 0f 85 db 00 00 00 4c 89 ef
[ 901.109702] RSP: 0018:ffffad0ae5d0f8c0 EFLAGS: 00000202
[ 901.109704] RAX: 0000000000000000 RBX: ffffad0ae5d0f900 RCX: ffffad11dfc0e000
[ 901.109705] RDX: ffffad2adcff81c0 RSI: 0000000000000000 RDI: ffff9a12f5f4f000
[ 901.109706] RBP: ffffad0ae5d0f9b0 R08: 0000000000000002 R09: ffff9a1289f57520
[ 901.109707] R10: 000000000000005d R11: 0000000000000002 R12: 00000006d8ff2000
[ 901.109708] R13: ffff9a12f5f4f000 R14: ffffad0ae5d0fa18 R15: ffff9a12f5f4f020
[ 901.109709] FS: 00007fb20ad1d740(0000) GS:ffff9a317fe00000(0000) knlGS:0000000000000000
[ 901.109710] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 901.109711] CR2: 00007f8041811000 CR3: 0000000118530006 CR4: 0000000000770ef0
[ 901.109712] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 901.109713] DR3: 0000000000000000 DR6: 00000000fffe07f0 DR7: 0000000000000400
[ 901.109714] PKRU: 55555554
[ 901.109714] Call Trace:
[ 901.109716] <IRQ>
[ 901.109718] ? show_regs+0x67/0x70
[ 901.109722] ? watchdog_timer_fn+0x1f3/0x280
[ 901.109725] ? __pfx_watchdog_timer_fn+0x10/0x10
[ 901.109727] ? __hrtimer_run_queues+0xc8/0x220
[ 901.109731] ? hrtimer_interrupt+0x10c/0x250
[ 901.109733] ? __sysvec_apic_timer_interrupt+0x53/0x130
[ 901.109736] ? sysvec_apic_timer_interrupt+0x7b/0x90
[ 901.109739] </IRQ>
[ 901.109740] <TASK>
[ 901.109740] ? asm_sysvec_apic_timer_interrupt+0x1b/0x20
[ 901.109745] ? sgx_enclave_restrict_permissions+0xba/0x1f0
[ 901.109747] ? aa_file_perm+0x145/0x550
[ 901.109750] sgx_ioctl+0x1ab/0x900
[ 901.109751] ? xas_find+0x84/0x200
[ 901.109754] ? sgx_enclave_etrack+0xbb/0x140
[ 901.109756] ? sgx_encl_may_map+0x19a/0x240
[ 901.109758] ? common_file_perm+0x8a/0x1b0
[ 901.109760] ? obj_cgroup_charge_pages+0xa2/0x100
[ 901.109763] ? tlb_flush_mmu+0x31/0x1c0
[ 901.109766] ? tlb_finish_mmu+0x42/0x80
[ 901.109767] ? do_mprotect_pkey+0x150/0x530
[ 901.109769] ? __fget_light+0xc0/0x100
[ 901.109772] __x64_sys_ioctl+0x95/0xd0
[ 901.109775] x64_sys_call+0x1209/0x20c0
[ 901.109777] do_syscall_64+0x6d/0x110
[ 901.109779] ? syscall_exit_to_user_mode+0x86/0x1c0
[ 901.109782] ? do_syscall_64+0x79/0x110
[ 901.109783] ? syscall_exit_to_user_mode+0x86/0x1c0
[ 901.109784] ? do_syscall_64+0x79/0x110
[ 901.109785] ? free_unref_page+0x10e/0x180
[ 901.109788] ? __do_fault+0x36/0x130
[ 901.109791] ? do_pte_missing+0x2e8/0xcc0
[ 901.109792] ? __pte_offset_map+0x1c/0x190
[ 901.109795] ? __handle_mm_fault+0x7b9/0xe60
[ 901.109796] ? __count_memcg_events+0x70/0x100
[ 901.109798] ? handle_mm_fault+0x256/0x360
[ 901.109799] ? do_user_addr_fault+0x3c1/0x860
[ 901.109801] ? irqentry_exit_to_user_mode+0x67/0x190
[ 901.109803] ? irqentry_exit+0x3b/0x50
[ 901.109804] ? exc_page_fault+0x89/0x180
[ 901.109806] entry_SYSCALL_64_after_hwframe+0x76/0x7e
[ 901.109807] RIP: 0033:0x7fb20b4315cb
[ 901.109810] Code: 0f 1e fa 48 8b 05 c5 78 0d 00 64 c7 00 26 00 00 00 48 c7 c0 ff ff ff ff c3 66 0f 1f 44 00 00 f3 0f 1e fa b8 10 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 95 78 0d 00 f7 d8 64 89 01 48
[ 901.109811] RSP: 002b:00007ffc0e7af718 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
[ 901.109812] RAX: ffffffffffffffda RBX: 0000000780000000 RCX: 00007fb20b4315cb
[ 901.109813] RDX: 00007ffc0e7af720 RSI: 00000000c028a405 RDI: 0000000000000005
[ 901.109814] RBP: 0000000000000005 R08: 0000000000000000 R09: 00007ffc0e7af794
[ 901.109815] R10: 00007ffc0e7af7c8 R11: 0000000000000246 R12: 00000000c028a405
[ 901.109815] R13: 00007ffc0e7af720 R14: 0000000780000000 R15: 00007fb20b2ea980
[ 901.109817] </TASK>
------------[ end trace ]------------
We suggest to give up CPU in the ioctl() handler explicitly. I have attached a
patch which can fix such issue.
Appreciate for your review and thanks for your time!
Regards,
Bojun Zhu
---
v1 -> v2:
- Make the cond_resched() as a prefix op instead of a postfix op (Jarkko)
- Additionly check the pending signal at the begin of every for loop (Jarkko)
- Introduce sgx_check_signal_and_resched() to wrap the pending check
and reschedule operations (Jarkko)
- Refine the trace log in commit() message (Kai)
[1] v1: https://lore.kernel.org/linux-sgx/[email protected]/
Bojun Zhu (1):
x86/sgx: Explicitly give up the CPU in EDMM's ioctl() to avoid
softlockup
arch/x86/kernel/cpu/sgx/ioctl.c | 40 +++++++++++++++++++++++++++++----
1 file changed, 36 insertions(+), 4 deletions(-)
base-commit: ed30a4a51bb196781c8058073ea720133a65596f
--
2.25.1
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 <[email protected]>
---
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();
--
2.25.1
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 <[email protected]>
> ---
> 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 <[email protected]> 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