linux-kernel - Re: [PATCH v8 04/15] x86/sgx: Implement basic EPC misc cgroup functionality

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <op.2id2hcvkwjvjmi@hhuan26-mobl.amr.corp.intel.com>
Date: Tue, 30 Jan 2024 19:59:26 -0600
From: "Haitao Huang" <haitao.huang@...ux.intel.com>
To: "jarkko@...nel.org" <jarkko@...nel.org>, "dave.hansen@...ux.intel.com"
 <dave.hansen@...ux.intel.com>, "tj@...nel.org" <tj@...nel.org>,
 "mkoutny@...e.com" <mkoutny@...e.com>, "linux-kernel@...r.kernel.org"
 <linux-kernel@...r.kernel.org>, "linux-sgx@...r.kernel.org"
 <linux-sgx@...r.kernel.org>, "x86@...nel.org" <x86@...nel.org>,
 "cgroups@...r.kernel.org" <cgroups@...r.kernel.org>, "tglx@...utronix.de"
 <tglx@...utronix.de>, "mingo@...hat.com" <mingo@...hat.com>, "bp@...en8.de"
 <bp@...en8.de>, "hpa@...or.com" <hpa@...or.com>, "Mehta, Sohil"
 <sohil.mehta@...el.com>, "Huang, Kai" <kai.huang@...el.com>
Cc: "Li, Zhiquan1" <zhiquan1.li@...el.com>, "kristen@...ux.intel.com"
 <kristen@...ux.intel.com>, "seanjc@...gle.com" <seanjc@...gle.com>, "Zhang,
 Bo" <zhanb@...rosoft.com>, "anakrish@...rosoft.com" <anakrish@...rosoft.com>,
 "mikko.ylinen@...ux.intel.com" <mikko.ylinen@...ux.intel.com>,
 "yangjie@...rosoft.com" <yangjie@...rosoft.com>, "chrisyan@...rosoft.com"
 <chrisyan@...rosoft.com>
Subject: Re: [PATCH v8 04/15] x86/sgx: Implement basic EPC misc cgroup
 functionality

On Tue, 30 Jan 2024 09:22:14 -0600, Huang, Kai <kai.huang@...el.com> wrote:

>>  struct sgx_epc_page *sgx_alloc_epc_page(void *owner, bool reclaim)  {
>> +	struct sgx_epc_cgroup *epc_cg;
>>  	struct sgx_epc_page *page;
>> +	int ret;
>> +
>> +	epc_cg = sgx_get_current_epc_cg();
>> +	ret = sgx_epc_cgroup_try_charge(epc_cg);
>> +	if (ret) {
>> +		sgx_put_epc_cg(epc_cg);
>> +		return ERR_PTR(ret);
>> +	}
>>
>>  	for ( ; ; ) {
>>  		page = __sgx_alloc_epc_page();
>> @@ -567,8 +578,10 @@ struct sgx_epc_page *sgx_alloc_epc_page(void
>> *owner, bool reclaim)
>>  			break;
>>  		}
>>
>> -		if (list_empty(&sgx_active_page_list))
>> -			return ERR_PTR(-ENOMEM);
>> +		if (list_empty(&sgx_active_page_list)) {
>> +			page = ERR_PTR(-ENOMEM);
>> +			break;
>> +		}
>
> (Sorry for replying from Outlook because I am in travel for Chinese New  
> Year.)
>
> Perhaps I am missing something but I don't understand this change.
>
> An empty sgx_active_page_list means you cannot reclaim any page from it,  
> so why need to break?
>

This is to avoid any escape for sgx_put_epc_cg(), which is added in this  
version.

>>
>>  		if (!reclaim) {
>>  			page = ERR_PTR(-EBUSY);
>> @@ -580,10 +593,25 @@ struct sgx_epc_page *sgx_alloc_epc_page(void
>> *owner, bool reclaim)
>>  			break;
>>  		}
>>
>> +		/*
>> +		 * Need to do a global reclamation if cgroup was not full but
>> free
>> +		 * physical pages run out, causing __sgx_alloc_epc_page() to
>> fail.
>> +		 */
>>  		sgx_reclaim_pages();
>>  		cond_resched();
>>  	}
>
> And why adding this comment, especially in this patch?
>
> I don't see it brings additional clarity because there's only global  
> reclaim now, no matter whether cgroup is enabled or not.
>

True there is only global reclamation at the moment. The comment intended  
to help clarify why we can get here when try_charge() passes. It at least  
took me some thinking to realize that fact so I put it here to help remind  
this can happen even for cases that try_charge() passes. (In some earlier  
debugging for some concurrent issues, I actually tried mistakenly to  
remove this path hoping to narrow down some causes but made situation  
worse.)

I can remove if no one thinks this is needed.

Thanks
Haitao