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@xxxxxxxxxxxx>
> ---
>  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.