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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <F7EBCAA4-6412-42AE-9A56-36914E22B4A1@antgroup.com>
Date: Mon, 29 Apr 2024 10:23:12 +0800
From: "Bojun Zhu" <zhubojun.zbj@...group.com>
To: Dave Hansen <dave.hansen@...el.com>
Cc:  <linux-kernel@...r.kernel.org>,
   <linux-sgx@...r.kernel.org>,
  "Jarkko Sakkinen" <jarkko@...nel.org>,
   <dave.hansen@...ux.intel.com>,
  "Chatre, Reinette" <reinette.chatre@...el.com>,
  "刘双(轩屹)" <ls123674@...group.com>
Subject: 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 <dave.hansen@...el.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