[RFC PATCH 0/1] x86/sgx: Explicitly give up the CPU in EDMM's ioctl() to avoid softlockup

[RFC PATCH 0/1] x86/sgx: Explicitly give up the CPU in EDMM's ioctl() to avoid softlockup

[朱伯君(杰铭)]

Hi folks,

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:

Is it an known issue? I can get the warning messages on Linux v6.9-rc5.

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. I'm looking forward to receiving suggestions
from community regarding this patch.

Thanks for your time!

Regards,
Bojun Zhu

Bojun Zhu (1):
  x86/sgx: Explicitly give up the CPU in EDMM's ioctl() to avoid
    softlockup

 arch/x86/kernel/cpu/sgx/ioctl.c | 9 +++++++++
 1 file changed, 9 insertions(+)

-- 
2.25.1

[RFC PATCH 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 at
the every end of "for loop" in
sgx_enclave_{ modify_types | restrict_permissions | remove_pages}
to give other tasks a chance to run, and avoid softlockup warning.

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! [occlum-run:3905]
    ...
    CPU: 45 PID: 3905 Comm: occlum-run Not tainted 6.9.0-rc5 #7
    ...
    RIP: 0010:sgx_enclave_restrict_permissions+0xba/0x1f0
    Code: 48 c1 e6 05 48 89 d1 48 8d 5c 24 40 b8 0e 00 00 00 48 2b 8e 70 8e f5 93 48 c1 e9 05 48 c1 e1 0c 48 03 8e 68 8e f5 93 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
    RSP: 0018:ffffb55a6591fa80 EFLAGS: 00000202
    RAX: 0000000000000000 RBX: ffffb55a6591fac0 RCX: ffffb581e7384000
    RDX: ffffb59a9e4e8080 RSI: 0000000000000020 RDI: ffff91d69e8cc000
    RBP: ffffb55a6591fb70 R08: 0000000000000002 R09: ffff91d646e12be0
    R10: 000000000000006e R11: 0000000000000002 R12: 000000072052d000
    R13: ffff91d69e8cc000 R14: ffffb55a6591fbd8 R15: ffff91d69e8cc020
    FS:  00007fe10dbda740(0000) GS:ffff92163e480000(0000) knlGS:0000000000000000
    CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
    CR2: 00007fc041811000 CR3: 00000040d95c8005 CR4: 0000000000770ef0
    DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
    DR3: 0000000000000000 DR6: 00000000fffe07f0 DR7: 0000000000000400
    PKRU: 55555554
    Call Trace:
     <IRQ>
     ? show_regs+0x67/0x70
     ? watchdog_timer_fn+0x1f3/0x280
     ? __pfx_watchdog_timer_fn+0x10/0x10
     ? __hrtimer_run_queues+0xc8/0x220
     ? hrtimer_interrupt+0x10c/0x250
     ? __sysvec_apic_timer_interrupt+0x53/0x130
     ? sysvec_apic_timer_interrupt+0x7b/0x90
     </IRQ>
     <TASK>
     ? asm_sysvec_apic_timer_interrupt+0x1b/0x20
     ? sgx_enclave_restrict_permissions+0xba/0x1f0
     ? __pte_offset_map_lock+0x94/0x110
     ? sgx_encl_test_and_clear_young_cb+0x40/0x60
     sgx_ioctl+0x1ab/0x900
     ? do_syscall_64+0x79/0x110
     ? apply_to_page_range+0x14/0x20
     ? sgx_encl_test_and_clear_young+0x6c/0x80
     ? sgx_vma_fault+0x132/0x4f0
     __x64_sys_ioctl+0x95/0xd0
     x64_sys_call+0x1209/0x20c0
     do_syscall_64+0x6d/0x110
     ? do_syscall_64+0x79/0x110
     ? do_pte_missing+0x2e8/0xcc0
     ? __pte_offset_map+0x1c/0x190
     ? __handle_mm_fault+0x7b9/0xe60
     ? __count_memcg_events+0x70/0x100
     ? handle_mm_fault+0x256/0x360
     ? do_user_addr_fault+0x3c1/0x860
     ? irqentry_exit_to_user_mode+0x67/0x190
     ? irqentry_exit+0x3b/0x50
     ? exc_page_fault+0x89/0x180
     entry_SYSCALL_64_after_hwframe+0x76/0x7e
    RIP: 0033:0x7fe10e2ee5cb
    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
    RSP: 002b:00007fffb2c75518 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
    RAX: ffffffffffffffda RBX: 0000000780000000 RCX: 00007fe10e2ee5cb
    RDX: 00007fffb2c75520 RSI: 00000000c028a405 RDI: 0000000000000005
    RBP: 0000000000000005 R08: 0000000000000000 R09: 00007fffb2c75594
    R10: 00007fffb2c755c8 R11: 0000000000000246 R12: 00000000c028a405
    R13: 00007fffb2c75520 R14: 0000000780000000 R15: 00007fe10e1a7980
     </TASK>
     ------------[ end trace ]------------

Signed-off-by: Bojun Zhu <zhubojun.zbj@antgroup.com>
---
 arch/x86/kernel/cpu/sgx/ioctl.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
index b65ab214bdf5..2340a82fa796 100644
--- a/arch/x86/kernel/cpu/sgx/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/ioctl.c
@@ -806,6 +806,9 @@ sgx_enclave_restrict_permissions(struct sgx_encl *encl,
 		}
 
 		mutex_unlock(&encl->lock);
+
+		if (need_resched())
+			cond_resched();
 	}
 
 	ret = 0;
@@ -1010,6 +1013,9 @@ static long sgx_enclave_modify_types(struct sgx_encl *encl,
 		entry->type = page_type;
 
 		mutex_unlock(&encl->lock);
+
+		if (need_resched())
+			cond_resched();
 	}
 
 	ret = 0;
@@ -1156,6 +1162,9 @@ static long sgx_encl_remove_pages(struct sgx_encl *encl,
 		kfree(entry);
 
 		mutex_unlock(&encl->lock);
+
+		if (need_resched())
+			cond_resched();
 	}
 
 	ret = 0;
-- 
2.25.1

Re: [RFC PATCH 1/1] x86/sgx: Explicitly give up the CPU in EDMM's ioctl() to avoid softlockup

[Huang, Kai]

On Tue, 2024-04-23 at 17:25 +0800, 朱伯君(杰铭) wrote:
> EDMM's ioctl()s support batch operations, which may be
> time-consuming. Try to explicitly give up the CPU at
> the every end of "for loop" in
> sgx_enclave_{ modify_types | restrict_permissions | remove_pages}
> to give other tasks a chance to run, and avoid softlockup warning.
> 
> 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! [occlum-run:3905]
>     ...
>     CPU: 45 PID: 3905 Comm: occlum-run Not tainted 6.9.0-rc5 #7
>     ...
>     RIP: 0010:sgx_enclave_restrict_permissions+0xba/0x1f0
>     Code: 48 c1 e6 05 48 89 d1 48 8d 5c 24 40 b8 0e 00 00 00 48 2b 8e 70 8e f5 93 48 c1 e9 05 48 c1 e1 0c 48 03 8e 68 8e f5 93 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
>     RSP: 0018:ffffb55a6591fa80 EFLAGS: 00000202
>     RAX: 0000000000000000 RBX: ffffb55a6591fac0 RCX: ffffb581e7384000
>     RDX: ffffb59a9e4e8080 RSI: 0000000000000020 RDI: ffff91d69e8cc000
>     RBP: ffffb55a6591fb70 R08: 0000000000000002 R09: ffff91d646e12be0
>     R10: 000000000000006e R11: 0000000000000002 R12: 000000072052d000
>     R13: ffff91d69e8cc000 R14: ffffb55a6591fbd8 R15: ffff91d69e8cc020
>     FS:  00007fe10dbda740(0000) GS:ffff92163e480000(0000) knlGS:0000000000000000
>     CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>     CR2: 00007fc041811000 CR3: 00000040d95c8005 CR4: 0000000000770ef0
>     DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>     DR3: 0000000000000000 DR6: 00000000fffe07f0 DR7: 0000000000000400
>     PKRU: 55555554
>     Call Trace:
>      <IRQ>
>      ? show_regs+0x67/0x70
>      ? watchdog_timer_fn+0x1f3/0x280
>      ? __pfx_watchdog_timer_fn+0x10/0x10
>      ? __hrtimer_run_queues+0xc8/0x220
>      ? hrtimer_interrupt+0x10c/0x250
>      ? __sysvec_apic_timer_interrupt+0x53/0x130
>      ? sysvec_apic_timer_interrupt+0x7b/0x90
>      </IRQ>
>      <TASK>
>      ? asm_sysvec_apic_timer_interrupt+0x1b/0x20
>      ? sgx_enclave_restrict_permissions+0xba/0x1f0
>      ? __pte_offset_map_lock+0x94/0x110
>      ? sgx_encl_test_and_clear_young_cb+0x40/0x60
>      sgx_ioctl+0x1ab/0x900
>      ? do_syscall_64+0x79/0x110
>      ? apply_to_page_range+0x14/0x20
>      ? sgx_encl_test_and_clear_young+0x6c/0x80
>      ? sgx_vma_fault+0x132/0x4f0
>      __x64_sys_ioctl+0x95/0xd0
>      x64_sys_call+0x1209/0x20c0
>      do_syscall_64+0x6d/0x110
>      ? do_syscall_64+0x79/0x110
>      ? do_pte_missing+0x2e8/0xcc0
>      ? __pte_offset_map+0x1c/0x190
>      ? __handle_mm_fault+0x7b9/0xe60
>      ? __count_memcg_events+0x70/0x100
>      ? handle_mm_fault+0x256/0x360
>      ? do_user_addr_fault+0x3c1/0x860
>      ? irqentry_exit_to_user_mode+0x67/0x190
>      ? irqentry_exit+0x3b/0x50
>      ? exc_page_fault+0x89/0x180
>      entry_SYSCALL_64_after_hwframe+0x76/0x7e
>     RIP: 0033:0x7fe10e2ee5cb
>     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
>     RSP: 002b:00007fffb2c75518 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
>     RAX: ffffffffffffffda RBX: 0000000780000000 RCX: 00007fe10e2ee5cb
>     RDX: 00007fffb2c75520 RSI: 00000000c028a405 RDI: 0000000000000005
>     RBP: 0000000000000005 R08: 0000000000000000 R09: 00007fffb2c75594
>     R10: 00007fffb2c755c8 R11: 0000000000000246 R12: 00000000c028a405
>     R13: 00007fffb2c75520 R14: 0000000780000000 R15: 00007fe10e1a7980
>      </TASK>
>      ------------[ end trace ]------------

Could you trim down the trace to only include the relevant part?

E.g., please at least remove the two register dumps at the beginning and
end of the trace.

Please refer to "Backtraces in commit messages" section in
Documentation/process/submitting-patches.rst.

> 
> Signed-off-by: Bojun Zhu <zhubojun.zbj@antgroup.com>
> ---
>  arch/x86/kernel/cpu/sgx/ioctl.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
> index b65ab214bdf5..2340a82fa796 100644
> --- a/arch/x86/kernel/cpu/sgx/ioctl.c
> +++ b/arch/x86/kernel/cpu/sgx/ioctl.c
> @@ -806,6 +806,9 @@ sgx_enclave_restrict_permissions(struct sgx_encl *encl,
>  		}
>  
>  		mutex_unlock(&encl->lock);
> +
> +		if (need_resched())
> +			cond_resched();
>  	}
>  
>  	ret = 0;
> @@ -1010,6 +1013,9 @@ static long sgx_enclave_modify_types(struct sgx_encl *encl,
>  		entry->type = page_type;
>  
>  		mutex_unlock(&encl->lock);
> +
> +		if (need_resched())
> +			cond_resched();
>  	}
>  
>  	ret = 0;
> @@ -1156,6 +1162,9 @@ static long sgx_encl_remove_pages(struct sgx_encl *encl,
>  		kfree(entry);
>  
>  		mutex_unlock(&encl->lock);
> +
> +		if (need_resched())
> +			cond_resched();
>  	}
> 

You can remove the need_reshced() in all 3 places above but just call
cond_resched() directly.

Re: [RFC PATCH 1/1] x86/sgx: Explicitly give up the CPU in EDMM's ioctl() to avoid softlockup

[Reinette Chatre]

Hi Kai,

On 4/23/2024 4:50 AM, Huang, Kai wrote:
>> diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
>> index b65ab214bdf5..2340a82fa796 100644
>> --- a/arch/x86/kernel/cpu/sgx/ioctl.c
>> +++ b/arch/x86/kernel/cpu/sgx/ioctl.c
>> @@ -806,6 +806,9 @@ sgx_enclave_restrict_permissions(struct sgx_encl *encl,
>>  		}
>>  
>>  		mutex_unlock(&encl->lock);
>> +
>> +		if (need_resched())
>> +			cond_resched();
>>  	}
>>  
>>  	ret = 0;
>> @@ -1010,6 +1013,9 @@ static long sgx_enclave_modify_types(struct sgx_encl *encl,
>>  		entry->type = page_type;
>>  
>>  		mutex_unlock(&encl->lock);
>> +
>> +		if (need_resched())
>> +			cond_resched();
>>  	}
>>  
>>  	ret = 0;
>> @@ -1156,6 +1162,9 @@ static long sgx_encl_remove_pages(struct sgx_encl *encl,
>>  		kfree(entry);
>>  
>>  		mutex_unlock(&encl->lock);
>> +
>> +		if (need_resched())
>> +			cond_resched();
>>  	}
>>
> 
> You can remove the need_reshced() in all 3 places above but just call
> cond_resched() directly.
> 

This change will call cond_resched() after dealing with each page in a
potentially large page range (cover mentions 30GB but we have also had to
make optimizations for enclaves larger than this). Adding a cond_resched()
here will surely placate the soft lockup detector, but we need to take care
how changes like this impact the performance of the system and having actions
on these page ranges take much longer than necessary.
For reference, please see 7b72c823ddf8 ("x86/sgx: Reduce delay and interference
of enclave release") that turned frequent cond_resched() into batches
to address performance issues.

It looks to me like the need_resched() may be a quick check that can be used
to improve performance? I am not familiar with all use cases that need to be
considered to determine if a batching solution may be needed.

Reinette

Re: [RFC PATCH 1/1] x86/sgx: Explicitly give up the CPU in EDMM's ioctl() to avoid softlockup

[Jarkko Sakkinen]

On Tue Apr 23, 2024 at 12:25 PM EEST, =?UTF-8?B?5pyx5Lyv5ZCbKOadsOmTrSk=?= wrote:
> EDMM's ioctl()s support batch operations, which may be
> time-consuming. Try to explicitly give up the CPU at
> the every end of "for loop" in
> sgx_enclave_{ modify_types | restrict_permissions | remove_pages}
> to give other tasks a chance to run, and avoid softlockup warning.
>
> 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! [occlum-run:3905]
>     ...
>     CPU: 45 PID: 3905 Comm: occlum-run Not tainted 6.9.0-rc5 #7
>     ...
>     RIP: 0010:sgx_enclave_restrict_permissions+0xba/0x1f0
>     Code: 48 c1 e6 05 48 89 d1 48 8d 5c 24 40 b8 0e 00 00 00 48 2b 8e 70 8e f5 93 48 c1 e9 05 48 c1 e1 0c 48 03 8e 68 8e f5 93 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
>     RSP: 0018:ffffb55a6591fa80 EFLAGS: 00000202
>     RAX: 0000000000000000 RBX: ffffb55a6591fac0 RCX: ffffb581e7384000
>     RDX: ffffb59a9e4e8080 RSI: 0000000000000020 RDI: ffff91d69e8cc000
>     RBP: ffffb55a6591fb70 R08: 0000000000000002 R09: ffff91d646e12be0
>     R10: 000000000000006e R11: 0000000000000002 R12: 000000072052d000
>     R13: ffff91d69e8cc000 R14: ffffb55a6591fbd8 R15: ffff91d69e8cc020
>     FS:  00007fe10dbda740(0000) GS:ffff92163e480000(0000) knlGS:0000000000000000
>     CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>     CR2: 00007fc041811000 CR3: 00000040d95c8005 CR4: 0000000000770ef0
>     DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>     DR3: 0000000000000000 DR6: 00000000fffe07f0 DR7: 0000000000000400
>     PKRU: 55555554
>     Call Trace:
>      <IRQ>
>      ? show_regs+0x67/0x70
>      ? watchdog_timer_fn+0x1f3/0x280
>      ? __pfx_watchdog_timer_fn+0x10/0x10
>      ? __hrtimer_run_queues+0xc8/0x220
>      ? hrtimer_interrupt+0x10c/0x250
>      ? __sysvec_apic_timer_interrupt+0x53/0x130
>      ? sysvec_apic_timer_interrupt+0x7b/0x90
>      </IRQ>
>      <TASK>
>      ? asm_sysvec_apic_timer_interrupt+0x1b/0x20
>      ? sgx_enclave_restrict_permissions+0xba/0x1f0
>      ? __pte_offset_map_lock+0x94/0x110
>      ? sgx_encl_test_and_clear_young_cb+0x40/0x60
>      sgx_ioctl+0x1ab/0x900
>      ? do_syscall_64+0x79/0x110
>      ? apply_to_page_range+0x14/0x20
>      ? sgx_encl_test_and_clear_young+0x6c/0x80
>      ? sgx_vma_fault+0x132/0x4f0
>      __x64_sys_ioctl+0x95/0xd0
>      x64_sys_call+0x1209/0x20c0
>      do_syscall_64+0x6d/0x110
>      ? do_syscall_64+0x79/0x110
>      ? do_pte_missing+0x2e8/0xcc0
>      ? __pte_offset_map+0x1c/0x190
>      ? __handle_mm_fault+0x7b9/0xe60
>      ? __count_memcg_events+0x70/0x100
>      ? handle_mm_fault+0x256/0x360
>      ? do_user_addr_fault+0x3c1/0x860
>      ? irqentry_exit_to_user_mode+0x67/0x190
>      ? irqentry_exit+0x3b/0x50
>      ? exc_page_fault+0x89/0x180
>      entry_SYSCALL_64_after_hwframe+0x76/0x7e
>     RIP: 0033:0x7fe10e2ee5cb
>     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
>     RSP: 002b:00007fffb2c75518 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
>     RAX: ffffffffffffffda RBX: 0000000780000000 RCX: 00007fe10e2ee5cb
>     RDX: 00007fffb2c75520 RSI: 00000000c028a405 RDI: 0000000000000005
>     RBP: 0000000000000005 R08: 0000000000000000 R09: 00007fffb2c75594
>     R10: 00007fffb2c755c8 R11: 0000000000000246 R12: 00000000c028a405
>     R13: 00007fffb2c75520 R14: 0000000780000000 R15: 00007fe10e1a7980
>      </TASK>
>      ------------[ end trace ]------------
>
> Signed-off-by: Bojun Zhu <zhubojun.zbj@antgroup.com>

Can you also fixup this as your "firstname lastname" in your emails
from field? This matters so that author field in git log matches your
sob.

> ---
>  arch/x86/kernel/cpu/sgx/ioctl.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
> index b65ab214bdf5..2340a82fa796 100644
> --- a/arch/x86/kernel/cpu/sgx/ioctl.c
> +++ b/arch/x86/kernel/cpu/sgx/ioctl.c
> @@ -806,6 +806,9 @@ sgx_enclave_restrict_permissions(struct sgx_encl *encl,
>  		}
>  
>  		mutex_unlock(&encl->lock);
> +
> +		if (need_resched())
> +			cond_resched();
>  	}
>  
>  	ret = 0;
> @@ -1010,6 +1013,9 @@ static long sgx_enclave_modify_types(struct sgx_encl *encl,
>  		entry->type = page_type;
>  
>  		mutex_unlock(&encl->lock);
> +
> +		if (need_resched())
> +			cond_resched();
>  	}
>  
>  	ret = 0;
> @@ -1156,6 +1162,9 @@ static long sgx_encl_remove_pages(struct sgx_encl *encl,
>  		kfree(entry);
>  
>  		mutex_unlock(&encl->lock);
> +
> +		if (need_resched())
> +			cond_resched();
>  	}
>  
>  	ret = 0;

Makes sense to me but maybe this should be a prefix op instead of
postfix op given how things are laid out in sgx_ioc_enclave_add_pages()

https://elixir.bootlin.com/linux/latest/source/arch/x86/kernel/cpu/sgx/ioctl.c#L443

BR, Jarkko

Re: [RFC PATCH 1/1] x86/sgx: Explicitly give up the CPU in EDMM's ioctl() to avoid softlockup

[Jarkko Sakkinen]

On Wed Apr 24, 2024 at 12:10 AM EEST, Jarkko Sakkinen wrote:
> On Tue Apr 23, 2024 at 12:25 PM EEST, =?UTF-8?B?5pyx5Lyv5ZCbKOadsOmTrSk=?= wrote:
> > EDMM's ioctl()s support batch operations, which may be
> > time-consuming. Try to explicitly give up the CPU at
> > the every end of "for loop" in
> > sgx_enclave_{ modify_types | restrict_permissions | remove_pages}
> > to give other tasks a chance to run, and avoid softlockup warning.
> >
> > 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! [occlum-run:3905]
> >     ...
> >     CPU: 45 PID: 3905 Comm: occlum-run Not tainted 6.9.0-rc5 #7
> >     ...
> >     RIP: 0010:sgx_enclave_restrict_permissions+0xba/0x1f0
> >     Code: 48 c1 e6 05 48 89 d1 48 8d 5c 24 40 b8 0e 00 00 00 48 2b 8e 70 8e f5 93 48 c1 e9 05 48 c1 e1 0c 48 03 8e 68 8e f5 93 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
> >     RSP: 0018:ffffb55a6591fa80 EFLAGS: 00000202
> >     RAX: 0000000000000000 RBX: ffffb55a6591fac0 RCX: ffffb581e7384000
> >     RDX: ffffb59a9e4e8080 RSI: 0000000000000020 RDI: ffff91d69e8cc000
> >     RBP: ffffb55a6591fb70 R08: 0000000000000002 R09: ffff91d646e12be0
> >     R10: 000000000000006e R11: 0000000000000002 R12: 000000072052d000
> >     R13: ffff91d69e8cc000 R14: ffffb55a6591fbd8 R15: ffff91d69e8cc020
> >     FS:  00007fe10dbda740(0000) GS:ffff92163e480000(0000) knlGS:0000000000000000
> >     CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> >     CR2: 00007fc041811000 CR3: 00000040d95c8005 CR4: 0000000000770ef0
> >     DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> >     DR3: 0000000000000000 DR6: 00000000fffe07f0 DR7: 0000000000000400
> >     PKRU: 55555554
> >     Call Trace:
> >      <IRQ>
> >      ? show_regs+0x67/0x70
> >      ? watchdog_timer_fn+0x1f3/0x280
> >      ? __pfx_watchdog_timer_fn+0x10/0x10
> >      ? __hrtimer_run_queues+0xc8/0x220
> >      ? hrtimer_interrupt+0x10c/0x250
> >      ? __sysvec_apic_timer_interrupt+0x53/0x130
> >      ? sysvec_apic_timer_interrupt+0x7b/0x90
> >      </IRQ>
> >      <TASK>
> >      ? asm_sysvec_apic_timer_interrupt+0x1b/0x20
> >      ? sgx_enclave_restrict_permissions+0xba/0x1f0
> >      ? __pte_offset_map_lock+0x94/0x110
> >      ? sgx_encl_test_and_clear_young_cb+0x40/0x60
> >      sgx_ioctl+0x1ab/0x900
> >      ? do_syscall_64+0x79/0x110
> >      ? apply_to_page_range+0x14/0x20
> >      ? sgx_encl_test_and_clear_young+0x6c/0x80
> >      ? sgx_vma_fault+0x132/0x4f0
> >      __x64_sys_ioctl+0x95/0xd0
> >      x64_sys_call+0x1209/0x20c0
> >      do_syscall_64+0x6d/0x110
> >      ? do_syscall_64+0x79/0x110
> >      ? do_pte_missing+0x2e8/0xcc0
> >      ? __pte_offset_map+0x1c/0x190
> >      ? __handle_mm_fault+0x7b9/0xe60
> >      ? __count_memcg_events+0x70/0x100
> >      ? handle_mm_fault+0x256/0x360
> >      ? do_user_addr_fault+0x3c1/0x860
> >      ? irqentry_exit_to_user_mode+0x67/0x190
> >      ? irqentry_exit+0x3b/0x50
> >      ? exc_page_fault+0x89/0x180
> >      entry_SYSCALL_64_after_hwframe+0x76/0x7e
> >     RIP: 0033:0x7fe10e2ee5cb
> >     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
> >     RSP: 002b:00007fffb2c75518 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
> >     RAX: ffffffffffffffda RBX: 0000000780000000 RCX: 00007fe10e2ee5cb
> >     RDX: 00007fffb2c75520 RSI: 00000000c028a405 RDI: 0000000000000005
> >     RBP: 0000000000000005 R08: 0000000000000000 R09: 00007fffb2c75594
> >     R10: 00007fffb2c755c8 R11: 0000000000000246 R12: 00000000c028a405
> >     R13: 00007fffb2c75520 R14: 0000000780000000 R15: 00007fe10e1a7980
> >      </TASK>
> >      ------------[ end trace ]------------
> >
> > Signed-off-by: Bojun Zhu <zhubojun.zbj@antgroup.com>
>
> Can you also fixup this as your "firstname lastname" in your emails
> from field? This matters so that author field in git log matches your
> sob.
>
> > ---
> >  arch/x86/kernel/cpu/sgx/ioctl.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
> > index b65ab214bdf5..2340a82fa796 100644
> > --- a/arch/x86/kernel/cpu/sgx/ioctl.c
> > +++ b/arch/x86/kernel/cpu/sgx/ioctl.c
> > @@ -806,6 +806,9 @@ sgx_enclave_restrict_permissions(struct sgx_encl *encl,
> >  		}
> >  
> >  		mutex_unlock(&encl->lock);
> > +
> > +		if (need_resched())
> > +			cond_resched();
> >  	}
> >  
> >  	ret = 0;
> > @@ -1010,6 +1013,9 @@ static long sgx_enclave_modify_types(struct sgx_encl *encl,
> >  		entry->type = page_type;
> >  
> >  		mutex_unlock(&encl->lock);
> > +
> > +		if (need_resched())
> > +			cond_resched();
> >  	}
> >  
> >  	ret = 0;
> > @@ -1156,6 +1162,9 @@ static long sgx_encl_remove_pages(struct sgx_encl *encl,
> >  		kfree(entry);
> >  
> >  		mutex_unlock(&encl->lock);
> > +
> > +		if (need_resched())
> > +			cond_resched();
> >  	}
> >  
> >  	ret = 0;
>
> Makes sense to me but maybe this should be a prefix op instead of
> postfix op given how things are laid out in sgx_ioc_enclave_add_pages()
>
> https://elixir.bootlin.com/linux/latest/source/arch/x86/kernel/cpu/sgx/ioctl.c#L443

It would make sense also check pending signals, and since these
add up perhaps a helper would make sense to encapsulate all the
shenanigans:

/*
 * Check signals and invoke scheduler. Return true for a pending signal.
 */
static bool sgx_resched(void)
{
	if (signal_pending(current))
		return true;

	if (need_resched())
		cond_resched();

	return false;
}

BR, Jarkko

Re: [RFC PATCH 1/1] x86/sgx: Explicitly give up the CPU in EDMM's ioctl() to avoid softlockup

[Jarkko Sakkinen]

On Tue Apr 23, 2024 at 2:50 PM EEST, Huang, Kai wrote:
> On Tue, 2024-04-23 at 17:25 +0800, 朱伯君(杰铭) wrote:
> > EDMM's ioctl()s support batch operations, which may be
> > time-consuming. Try to explicitly give up the CPU at
> > the every end of "for loop" in
> > sgx_enclave_{ modify_types | restrict_permissions | remove_pages}
> > to give other tasks a chance to run, and avoid softlockup warning.
> > 
> > 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! [occlum-run:3905]
> >     ...
> >     CPU: 45 PID: 3905 Comm: occlum-run Not tainted 6.9.0-rc5 #7
> >     ...
> >     RIP: 0010:sgx_enclave_restrict_permissions+0xba/0x1f0
> >     Code: 48 c1 e6 05 48 89 d1 48 8d 5c 24 40 b8 0e 00 00 00 48 2b 8e 70 8e f5 93 48 c1 e9 05 48 c1 e1 0c 48 03 8e 68 8e f5 93 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
> >     RSP: 0018:ffffb55a6591fa80 EFLAGS: 00000202
> >     RAX: 0000000000000000 RBX: ffffb55a6591fac0 RCX: ffffb581e7384000
> >     RDX: ffffb59a9e4e8080 RSI: 0000000000000020 RDI: ffff91d69e8cc000
> >     RBP: ffffb55a6591fb70 R08: 0000000000000002 R09: ffff91d646e12be0
> >     R10: 000000000000006e R11: 0000000000000002 R12: 000000072052d000
> >     R13: ffff91d69e8cc000 R14: ffffb55a6591fbd8 R15: ffff91d69e8cc020
> >     FS:  00007fe10dbda740(0000) GS:ffff92163e480000(0000) knlGS:0000000000000000
> >     CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> >     CR2: 00007fc041811000 CR3: 00000040d95c8005 CR4: 0000000000770ef0
> >     DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> >     DR3: 0000000000000000 DR6: 00000000fffe07f0 DR7: 0000000000000400
> >     PKRU: 55555554
> >     Call Trace:
> >      <IRQ>
> >      ? show_regs+0x67/0x70
> >      ? watchdog_timer_fn+0x1f3/0x280
> >      ? __pfx_watchdog_timer_fn+0x10/0x10
> >      ? __hrtimer_run_queues+0xc8/0x220
> >      ? hrtimer_interrupt+0x10c/0x250
> >      ? __sysvec_apic_timer_interrupt+0x53/0x130
> >      ? sysvec_apic_timer_interrupt+0x7b/0x90
> >      </IRQ>
> >      <TASK>
> >      ? asm_sysvec_apic_timer_interrupt+0x1b/0x20
> >      ? sgx_enclave_restrict_permissions+0xba/0x1f0
> >      ? __pte_offset_map_lock+0x94/0x110
> >      ? sgx_encl_test_and_clear_young_cb+0x40/0x60
> >      sgx_ioctl+0x1ab/0x900
> >      ? do_syscall_64+0x79/0x110
> >      ? apply_to_page_range+0x14/0x20
> >      ? sgx_encl_test_and_clear_young+0x6c/0x80
> >      ? sgx_vma_fault+0x132/0x4f0
> >      __x64_sys_ioctl+0x95/0xd0
> >      x64_sys_call+0x1209/0x20c0
> >      do_syscall_64+0x6d/0x110
> >      ? do_syscall_64+0x79/0x110
> >      ? do_pte_missing+0x2e8/0xcc0
> >      ? __pte_offset_map+0x1c/0x190
> >      ? __handle_mm_fault+0x7b9/0xe60
> >      ? __count_memcg_events+0x70/0x100
> >      ? handle_mm_fault+0x256/0x360
> >      ? do_user_addr_fault+0x3c1/0x860
> >      ? irqentry_exit_to_user_mode+0x67/0x190
> >      ? irqentry_exit+0x3b/0x50
> >      ? exc_page_fault+0x89/0x180
> >      entry_SYSCALL_64_after_hwframe+0x76/0x7e
> >     RIP: 0033:0x7fe10e2ee5cb
> >     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
> >     RSP: 002b:00007fffb2c75518 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
> >     RAX: ffffffffffffffda RBX: 0000000780000000 RCX: 00007fe10e2ee5cb
> >     RDX: 00007fffb2c75520 RSI: 00000000c028a405 RDI: 0000000000000005
> >     RBP: 0000000000000005 R08: 0000000000000000 R09: 00007fffb2c75594
> >     R10: 00007fffb2c755c8 R11: 0000000000000246 R12: 00000000c028a405
> >     R13: 00007fffb2c75520 R14: 0000000780000000 R15: 00007fe10e1a7980
> >      </TASK>
> >      ------------[ end trace ]------------
>
> Could you trim down the trace to only include the relevant part?
>
> E.g., please at least remove the two register dumps at the beginning and
> end of the trace.
>
> Please refer to "Backtraces in commit messages" section in
> Documentation/process/submitting-patches.rst.
>
> > 
> > Signed-off-by: Bojun Zhu <zhubojun.zbj@antgroup.com>
> > ---
> >  arch/x86/kernel/cpu/sgx/ioctl.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
> > index b65ab214bdf5..2340a82fa796 100644
> > --- a/arch/x86/kernel/cpu/sgx/ioctl.c
> > +++ b/arch/x86/kernel/cpu/sgx/ioctl.c
> > @@ -806,6 +806,9 @@ sgx_enclave_restrict_permissions(struct sgx_encl *encl,
> >  		}
> >  
> >  		mutex_unlock(&encl->lock);
> > +
> > +		if (need_resched())
> > +			cond_resched();
> >  	}
> >  
> >  	ret = 0;
> > @@ -1010,6 +1013,9 @@ static long sgx_enclave_modify_types(struct sgx_encl *encl,
> >  		entry->type = page_type;
> >  
> >  		mutex_unlock(&encl->lock);
> > +
> > +		if (need_resched())
> > +			cond_resched();
> >  	}
> >  
> >  	ret = 0;
> > @@ -1156,6 +1162,9 @@ static long sgx_encl_remove_pages(struct sgx_encl *encl,
> >  		kfree(entry);
> >  
> >  		mutex_unlock(&encl->lock);
> > +
> > +		if (need_resched())
> > +			cond_resched();
> >  	}
> > 
>
> You can remove the need_reshced() in all 3 places above but just call
> cond_resched() directly.

If this is the case, then it should be removed also from
sgx_ioc_enclave_add_pages() for the sake of symmetry and related
reasoning of semantic equivalence should be documented to the
commit message (given that something is taken away from code).

BR, Jarkko

Re: [RFC PATCH 1/1] x86/sgx: Explicitly give up the CPU in EDMM's ioctl() to avoid softlockup

[Jarkko Sakkinen]

On Tue Apr 23, 2024 at 8:08 PM EEST, Reinette Chatre wrote:
> Hi Kai,
>
> On 4/23/2024 4:50 AM, Huang, Kai wrote:
> >> diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
> >> index b65ab214bdf5..2340a82fa796 100644
> >> --- a/arch/x86/kernel/cpu/sgx/ioctl.c
> >> +++ b/arch/x86/kernel/cpu/sgx/ioctl.c
> >> @@ -806,6 +806,9 @@ sgx_enclave_restrict_permissions(struct sgx_encl *encl,
> >>  		}
> >>  
> >>  		mutex_unlock(&encl->lock);
> >> +
> >> +		if (need_resched())
> >> +			cond_resched();
> >>  	}
> >>  
> >>  	ret = 0;
> >> @@ -1010,6 +1013,9 @@ static long sgx_enclave_modify_types(struct sgx_encl *encl,
> >>  		entry->type = page_type;
> >>  
> >>  		mutex_unlock(&encl->lock);
> >> +
> >> +		if (need_resched())
> >> +			cond_resched();
> >>  	}
> >>  
> >>  	ret = 0;
> >> @@ -1156,6 +1162,9 @@ static long sgx_encl_remove_pages(struct sgx_encl *encl,
> >>  		kfree(entry);
> >>  
> >>  		mutex_unlock(&encl->lock);
> >> +
> >> +		if (need_resched())
> >> +			cond_resched();
> >>  	}
> >>
> > 
> > You can remove the need_reshced() in all 3 places above but just call
> > cond_resched() directly.
> > 
>
> This change will call cond_resched() after dealing with each page in a
> potentially large page range (cover mentions 30GB but we have also had to
> make optimizations for enclaves larger than this). Adding a cond_resched()
> here will surely placate the soft lockup detector, but we need to take care
> how changes like this impact the performance of the system and having actions
> on these page ranges take much longer than necessary.
> For reference, please see 7b72c823ddf8 ("x86/sgx: Reduce delay and interference
> of enclave release") that turned frequent cond_resched() into batches
> to address performance issues.
>
> It looks to me like the need_resched() may be a quick check that can be used
> to improve performance? I am not familiar with all use cases that need to be
> considered to determine if a batching solution may be needed.

Ya, well no matter it is the reasoning will need to be documented
because this should have symmetry with sgx_ioc_enclave_add_pages()
(see my response to Kai).

I because this makes dealing with need_resched() a change in code
even if it is left out as a side-effect, I'd support of not removing
which means adding need_resched() as a side-effect.

From this follows that *if* need_resched() needs to be removed then
that is not really part of the bug fix, so in all cases the bug fix
itself must include need_resched() :-)

phew, hope you got my logic here, i think it reasonable...

BR, Jarkko

Re: [RFC PATCH 1/1] x86/sgx: Explicitly give up the CPU in EDMM's ioctl() to avoid softlockup

[Huang, Kai]

On Wed, 2024-04-24 at 00:27 +0300, Jarkko Sakkinen wrote:
> On Tue Apr 23, 2024 at 8:08 PM EEST, Reinette Chatre wrote:
> > Hi Kai,
> > 
> > On 4/23/2024 4:50 AM, Huang, Kai wrote:
> > > > diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
> > > > index b65ab214bdf5..2340a82fa796 100644
> > > > --- a/arch/x86/kernel/cpu/sgx/ioctl.c
> > > > +++ b/arch/x86/kernel/cpu/sgx/ioctl.c
> > > > @@ -806,6 +806,9 @@ sgx_enclave_restrict_permissions(struct sgx_encl *encl,
> > > >  		}
> > > >  
> > > >  		mutex_unlock(&encl->lock);
> > > > +
> > > > +		if (need_resched())
> > > > +			cond_resched();
> > > >  	}
> > > >  
> > > >  	ret = 0;
> > > > @@ -1010,6 +1013,9 @@ static long sgx_enclave_modify_types(struct sgx_encl *encl,
> > > >  		entry->type = page_type;
> > > >  
> > > >  		mutex_unlock(&encl->lock);
> > > > +
> > > > +		if (need_resched())
> > > > +			cond_resched();
> > > >  	}
> > > >  
> > > >  	ret = 0;
> > > > @@ -1156,6 +1162,9 @@ static long sgx_encl_remove_pages(struct sgx_encl *encl,
> > > >  		kfree(entry);
> > > >  
> > > >  		mutex_unlock(&encl->lock);
> > > > +
> > > > +		if (need_resched())
> > > > +			cond_resched();
> > > >  	}
> > > > 
> > > 
> > > You can remove the need_reshced() in all 3 places above but just call
> > > cond_resched() directly.
> > > 
> > 
> > This change will call cond_resched() after dealing with each page in a
> > potentially large page range (cover mentions 30GB but we have also had to
> > make optimizations for enclaves larger than this). Adding a cond_resched()
> > here will surely placate the soft lockup detector, but we need to take care
> > how changes like this impact the performance of the system and having actions
> > on these page ranges take much longer than necessary.
> > For reference, please see 7b72c823ddf8 ("x86/sgx: Reduce delay and interference
> > of enclave release") that turned frequent cond_resched() into batches
> > to address performance issues.

Ah I didn't know this.  Thanks for the info.

> > 
> > It looks to me like the need_resched() may be a quick check that can be used
> > to improve performance? 
> > 

Perhaps?  My assumption is eventually cond_resched() will do similar check
of need_resched() but I am not entirely sure about of that.

Reading the code, it seems cond_resched() eventually does
should_resched().  The generic version indeed does similar check of
need_resched() but it seems the x86 version has a different
implementation.

> > I am not familiar with all use cases that need to be
> > considered to determine if a batching solution may be needed.

Looks at least the REMOVE_PAGES ioctls() could have the same impact to the
performance downgrade problem mentioned in commit 7b72c823ddf8 ("x86/sgx:
Reduce delay and interference of enclave release"), but I guess it's
acceptable to fix softlockup first and then improve performance if there's
someone hit any real issue. 

> 
> Ya, well no matter it is the reasoning will need to be documented
> because this should have symmetry with sgx_ioc_enclave_add_pages()
> (see my response to Kai).

Yeah I was actually going to mention this, but somehow I didn't choose to.

> 
> I because this makes dealing with need_resched() a change in code
> even if it is left out as a side-effect, I'd support of not removing
> which means adding need_resched() as a side-effect.

I am fine with keeping the need_resched().

Re: [RFC PATCH 1/1] x86/sgx: Explicitly give up the CPU in EDMM's ioctl() to avoid softlockup

[Bojun Zhu]

Hi Kai,

On 2024/4/23 19:50, Huang, Kai wrote:
> On Tue, 2024-04-23 at 17:25 +0800, 朱伯君(杰铭) wrote:
>> EDMM's ioctl()s support batch operations, which may be
>> time-consuming. Try to explicitly give up the CPU at
>> the every end of "for loop" in
>> sgx_enclave_{ modify_types | restrict_permissions | remove_pages}
>> to give other tasks a chance to run, and avoid softlockup warning.
>>
>> 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! [occlum-run:3905]
>>      ...
>>      CPU: 45 PID: 3905 Comm: occlum-run Not tainted 6.9.0-rc5 #7
>>      ...
>>      RIP: 0010:sgx_enclave_restrict_permissions+0xba/0x1f0
>>      Code: 48 c1 e6 05 48 89 d1 48 8d 5c 24 40 b8 0e 00 00 00 48 2b 8e 70 8e f5 93 48 c1 e9 05 48 c1 e1 0c 48 03 8e 68 8e f5 93 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
>>      RSP: 0018:ffffb55a6591fa80 EFLAGS: 00000202
>>      RAX: 0000000000000000 RBX: ffffb55a6591fac0 RCX: ffffb581e7384000
>>      RDX: ffffb59a9e4e8080 RSI: 0000000000000020 RDI: ffff91d69e8cc000
>>      RBP: ffffb55a6591fb70 R08: 0000000000000002 R09: ffff91d646e12be0
>>      R10: 000000000000006e R11: 0000000000000002 R12: 000000072052d000
>>      R13: ffff91d69e8cc000 R14: ffffb55a6591fbd8 R15: ffff91d69e8cc020
>>      FS:  00007fe10dbda740(0000) GS:ffff92163e480000(0000) knlGS:0000000000000000
>>      CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>      CR2: 00007fc041811000 CR3: 00000040d95c8005 CR4: 0000000000770ef0
>>      DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>>      DR3: 0000000000000000 DR6: 00000000fffe07f0 DR7: 0000000000000400
>>      PKRU: 55555554
>>      Call Trace:
>>       <IRQ>
>>       ? show_regs+0x67/0x70
>>       ? watchdog_timer_fn+0x1f3/0x280
>>       ? __pfx_watchdog_timer_fn+0x10/0x10
>>       ? __hrtimer_run_queues+0xc8/0x220
>>       ? hrtimer_interrupt+0x10c/0x250
>>       ? __sysvec_apic_timer_interrupt+0x53/0x130
>>       ? sysvec_apic_timer_interrupt+0x7b/0x90
>>       </IRQ>
>>       <TASK>
>>       ? asm_sysvec_apic_timer_interrupt+0x1b/0x20
>>       ? sgx_enclave_restrict_permissions+0xba/0x1f0
>>       ? __pte_offset_map_lock+0x94/0x110
>>       ? sgx_encl_test_and_clear_young_cb+0x40/0x60
>>       sgx_ioctl+0x1ab/0x900
>>       ? do_syscall_64+0x79/0x110
>>       ? apply_to_page_range+0x14/0x20
>>       ? sgx_encl_test_and_clear_young+0x6c/0x80
>>       ? sgx_vma_fault+0x132/0x4f0
>>       __x64_sys_ioctl+0x95/0xd0
>>       x64_sys_call+0x1209/0x20c0
>>       do_syscall_64+0x6d/0x110
>>       ? do_syscall_64+0x79/0x110
>>       ? do_pte_missing+0x2e8/0xcc0
>>       ? __pte_offset_map+0x1c/0x190
>>       ? __handle_mm_fault+0x7b9/0xe60
>>       ? __count_memcg_events+0x70/0x100
>>       ? handle_mm_fault+0x256/0x360
>>       ? do_user_addr_fault+0x3c1/0x860
>>       ? irqentry_exit_to_user_mode+0x67/0x190
>>       ? irqentry_exit+0x3b/0x50
>>       ? exc_page_fault+0x89/0x180
>>       entry_SYSCALL_64_after_hwframe+0x76/0x7e
>>      RIP: 0033:0x7fe10e2ee5cb
>>      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
>>      RSP: 002b:00007fffb2c75518 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
>>      RAX: ffffffffffffffda RBX: 0000000780000000 RCX: 00007fe10e2ee5cb
>>      RDX: 00007fffb2c75520 RSI: 00000000c028a405 RDI: 0000000000000005
>>      RBP: 0000000000000005 R08: 0000000000000000 R09: 00007fffb2c75594
>>      R10: 00007fffb2c755c8 R11: 0000000000000246 R12: 00000000c028a405
>>      R13: 00007fffb2c75520 R14: 0000000780000000 R15: 00007fe10e1a7980
>>       </TASK>
>>       ------------[ end trace ]------------
> Could you trim down the trace to only include the relevant part?
>
> E.g., please at least remove the two register dumps at the beginning and
> end of the trace.
>
> Please refer to "Backtraces in commit messages" section in
> Documentation/process/submitting-patches.rst.


Thanks for your advice! I will refine the trace log according to the 
suggestions
in Documentation/process/submitting-patches.rst. and highlight the 
related part.


>> Signed-off-by: Bojun Zhu <zhubojun.zbj@antgroup.com>
>> ---
>>   arch/x86/kernel/cpu/sgx/ioctl.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
>> index b65ab214bdf5..2340a82fa796 100644
>> --- a/arch/x86/kernel/cpu/sgx/ioctl.c
>> +++ b/arch/x86/kernel/cpu/sgx/ioctl.c
>> @@ -806,6 +806,9 @@ sgx_enclave_restrict_permissions(struct sgx_encl *encl,
>>   		}
>>   
>>   		mutex_unlock(&encl->lock);
>> +
>> +		if (need_resched())
>> +			cond_resched();
>>   	}
>>   
>>   	ret = 0;
>> @@ -1010,6 +1013,9 @@ static long sgx_enclave_modify_types(struct sgx_encl *encl,
>>   		entry->type = page_type;
>>   
>>   		mutex_unlock(&encl->lock);
>> +
>> +		if (need_resched())
>> +			cond_resched();
>>   	}
>>   
>>   	ret = 0;
>> @@ -1156,6 +1162,9 @@ static long sgx_encl_remove_pages(struct sgx_encl *encl,
>>   		kfree(entry);
>>   
>>   		mutex_unlock(&encl->lock);
>> +
>> +		if (need_resched())
>> +			cond_resched();
>>   	}
>>
> You can remove the need_reshced() in all 3 places above but just call
> cond_resched() directly.
>

Based on the the discussion among you, Jarkko and Reinette,
I will keep the need_resched() and wrap the logic in using sgx_resched(),
as suggested by Jarkko.

Regards,

Bojun

Re: [RFC PATCH 1/1] x86/sgx: Explicitly give up the CPU in EDMM's ioctl() to avoid softlockup

[Jarkko Sakkinen]

On Wed Apr 24, 2024 at 9:46 AM EEST, Bojun Zhu wrote:
> Based on the the discussion among you, Jarkko and Reinette,
> I will keep the need_resched() and wrap the logic in using sgx_resched(),
> as suggested by Jarkko.

Sounds like a plan :-)

> Regards,
>
> Bojun

BR, Jarkko

Re: [RFC PATCH 1/1] x86/sgx: Explicitly give up the CPU in EDMM's ioctl() to avoid softlockup

[Jarkko Sakkinen]

On Wed Apr 24, 2024 at 10:02 AM EEST, Jarkko Sakkinen wrote:
> On Wed Apr 24, 2024 at 9:46 AM EEST, Bojun Zhu wrote:
> > Based on the the discussion among you, Jarkko and Reinette,
> > I will keep the need_resched() and wrap the logic in using sgx_resched(),
> > as suggested by Jarkko.
>
> Sounds like a plan :-)

In sgx_ioc_enclave_add_pages() "if (!c)" check might cause possibly
some  confusion.

Reason for it is that in "transaction sense" the operation can
be only meaningfully restarted when no pages have not been added
as MRENCLAVE checksum cannot be reset.

BR, Jarkko

Re: [RFC PATCH 1/1] x86/sgx: Explicitly give up the CPU in EDMM's ioctl() to avoid softlockup

[Bojun Zhu]

Hi Jarkko,

> On Apr 24, 2024, at 18:42, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> 
> On Wed Apr 24, 2024 at 10:02 AM EEST, Jarkko Sakkinen wrote:
>> On Wed Apr 24, 2024 at 9:46 AM EEST, Bojun Zhu wrote:
>>> Based on the the discussion among you, Jarkko and Reinette,
>>> I will keep the need_resched() and wrap the logic in using sgx_resched(),
>>> as suggested by Jarkko.
>> 
>> Sounds like a plan :-)
> 
> In sgx_ioc_enclave_add_pages() "if (!c)" check might cause possibly
> some  confusion.
> 
> Reason for it is that in "transaction sense" the operation can
> be only meaningfully restarted when no pages have not been added
> as MRENCLAVE checksum cannot be reset.
> 
> BR, Jarkko
> 


Thanks for your reminder.

According to the SGX hardware specification, similar to "ADD PAGES”,
the operations in  "MODT PAGEs” and  "REMOVE PAGEs(at runtime)”  
can be only meaningfully restarted when no page has been handled.

For the following code in sgx_ioc_enclave_add_pages():

```c
if (signal_pending(current)) {
	if (!c)
		ret = -ERESTARTSYS;

	break;
}
```
I still have some questions:

It seems that the variable "ret" is set to 0 if there is **some** EPC pages have been 
added when interrupted by signal(Supposed that sgx_encl_add_page() 
always returns successfully).

Shall we set the return value as "-EINTR" if the context is interrupted by 
signal when some EPC pages have been added?
(BTW, return values contain -EINTR as shown in
https://elixir.bootlin.com/linux/latest/source/arch/x86/kernel/cpu/sgx/ioctl.c#L402)

Please correct me if misunderstood it.

Regards,
Bojun

Re: [RFC PATCH 1/1] x86/sgx: Explicitly give up the CPU in EDMM's ioctl() to avoid softlockup

[Jarkko Sakkinen]

On Wed Apr 24, 2024 at 2:50 PM EEST, Bojun Zhu wrote:
> I still have some questions:
>
> It seems that the variable "ret" is set to 0 if there is **some** EPC pages have been 
> added when interrupted by signal(Supposed that sgx_encl_add_page() 
> always returns successfully).

Ah, ok.

Returning zero is right thing to do because it also returns count of
pages successfully added. I.e. the function does not guarantee that
all pages are processsed but it does guarantee that the system is in
predictable state.

It could be that e.g. sgx_alloc_epc_page() calls fails.

So, it is a bit like how read system call works.

BR, Jarkko

Re: [RFC PATCH 1/1] x86/sgx: Explicitly give up the CPU in EDMM's ioctl() to avoid softlockup

[Jarkko Sakkinen]

On Wed Apr 24, 2024 at 8:44 PM EEST, Jarkko Sakkinen wrote:
> On Wed Apr 24, 2024 at 2:50 PM EEST, Bojun Zhu wrote:
> > I still have some questions:
> >
> > It seems that the variable "ret" is set to 0 if there is **some** EPC pages have been 
> > added when interrupted by signal(Supposed that sgx_encl_add_page() 
> > always returns successfully).
>
> Ah, ok.
>
> Returning zero is right thing to do because it also returns count of
> pages successfully added. I.e. the function does not guarantee that
> all pages are processsed but it does guarantee that the system is in
> predictable state.
>
> It could be that e.g. sgx_alloc_epc_page() calls fails.
>
> So, it is a bit like how read system call works.

Good that you asked that had rethink for a while.

What I would suggest that you just put out 2nd verson out and then we
see where it is going.

For sgx_ioc_encl_add_pages() it is important to maintain exact semantics
as we need to maintain backwards compatibility.

BR, Jarkko