linux-kernel - Re: [PATCH v5 09/18] x86/sgx: Store struct sgx

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]

Date:   Wed, 27 Sep 2023 10:35:24 -0500
From:   "Haitao Huang" <haitao.huang@...ux.intel.com>
To:     "hpa@...or.com" <hpa@...or.com>,
        "linux-sgx@...r.kernel.org" <linux-sgx@...r.kernel.org>,
        "x86@...nel.org" <x86@...nel.org>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
        "cgroups@...r.kernel.org" <cgroups@...r.kernel.org>,
        "bp@...en8.de" <bp@...en8.de>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "jarkko@...nel.org" <jarkko@...nel.org>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "Mehta, Sohil" <sohil.mehta@...el.com>,
        "tj@...nel.org" <tj@...nel.org>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "Huang, Kai" <kai.huang@...el.com>
Cc:     "kristen@...ux.intel.com" <kristen@...ux.intel.com>,
        "yangjie@...rosoft.com" <yangjie@...rosoft.com>,
        "Li, Zhiquan1" <zhiquan1.li@...el.com>,
        "Christopherson,, Sean" <seanjc@...gle.com>,
        "mikko.ylinen@...ux.intel.com" <mikko.ylinen@...ux.intel.com>,
        "Zhang, Bo" <zhanb@...rosoft.com>,
        "anakrish@...rosoft.com" <anakrish@...rosoft.com>
Subject: Re: [PATCH v5 09/18] x86/sgx: Store struct sgx_encl when allocating
 new VA pages

Hi Kai,

On Wed, 27 Sep 2023 06:14:20 -0500, Huang, Kai <kai.huang@...el.com> wrote:

> On Fri, 2023-09-22 at 20:06 -0700, Haitao Huang wrote:
>> From: Sean Christopherson <sean.j.christopherson@...el.com>
>>
>> In a later patch, when a cgroup has exceeded the max capacity for EPC
>> pages, it may need to identify and OOM kill a less active enclave to
>> make room for other enclaves within the same group. Such a victim
>> enclave would have no active pages other than the unreclaimable Version
>> Array (VA) and SECS pages.  Therefore, the cgroup needs examine its
>> unreclaimable page list, and finding an enclave given a SECS page or a
>> VA page. This will require a backpointer from a page to an enclave,
>> which is not available for VA pages.
>>
>> Because struct sgx_epc_page instances of VA pages are not owned by an
>> sgx_encl_page instance, mark their owner as sgx_encl: pass the struct
>> sgx_encl of the enclave allocating the VA page to sgx_alloc_epc_page(),
>> which will store this value in the owner field of the struct
>> sgx_epc_page.  In a later patch, VA pages will be placed in an
>> unreclaimable queue that can be examined by the cgroup to select the OOM
>> killed enclave.
>>
>> Signed-off-by: Sean Christopherson <sean.j.christopherson@...el.com>
>> Co-developed-by: Kristen Carlson Accardi <kristen@...ux.intel.com>
>> Signed-off-by: Kristen Carlson Accardi <kristen@...ux.intel.com>
>> Co-developed-by: Haitao Huang <haitao.huang@...ux.intel.com>
>> Signed-off-by: Haitao Huang <haitao.huang@...ux.intel.com>
>> Cc: Sean Christopherson <seanjc@...gle.com>
>>
>
> [...]
>
>> @@ -562,7 +562,7 @@ struct sgx_epc_page *sgx_alloc_epc_page(void  
>> *owner, bool reclaim)
>>  	for ( ; ; ) {
>>  		page = __sgx_alloc_epc_page();
>>  		if (!IS_ERR(page)) {
>> -			page->owner = owner;
>> +			page->encl_page = owner;
>
> Looks using 'encl_page' is arbitrary.
>
> Also actually for virtual EPC page the owner is set to the 'sgx_vepc'  
> instance.
>
>>  			break;
>>  		}
>>
>> @@ -607,7 +607,7 @@ void sgx_free_epc_page(struct sgx_epc_page *page)
>>
>>  	spin_lock(&node->lock);
>>
>> -	page->owner = NULL;
>> +	page->encl_page = NULL;
>
> Ditto.
>
>>  	if (page->poison)
>>  		list_add(&page->list, &node->sgx_poison_page_list);
>>  	else
>> @@ -642,7 +642,7 @@ static bool __init sgx_setup_epc_section(u64  
>> phys_addr, u64 size,
>>  	for (i = 0; i < nr_pages; i++) {
>>  		section->pages[i].section = index;
>>  		section->pages[i].flags = 0;
>> -		section->pages[i].owner = NULL;
>> +		section->pages[i].encl_page = NULL;
>>  		section->pages[i].poison = 0;
>>  		list_add_tail(&section->pages[i].list, &sgx_dirty_page_list);
>>  	}
>> diff --git a/arch/x86/kernel/cpu/sgx/sgx.h  
>> b/arch/x86/kernel/cpu/sgx/sgx.h
>> index 764cec23f4e5..5110dd433b80 100644
>> --- a/arch/x86/kernel/cpu/sgx/sgx.h
>> +++ b/arch/x86/kernel/cpu/sgx/sgx.h
>> @@ -68,7 +68,12 @@ struct sgx_epc_page {
>>  	unsigned int section;
>>  	u16 flags;
>>  	u16 poison;
>> -	struct sgx_encl_page *owner;
>> +
>> +	/* Possible owner types */
>> +	union {
>> +		struct sgx_encl_page *encl_page;
>> +		struct sgx_encl *encl;
>> +	};
>
> Sadly for virtual EPC page the owner is set to the 'sgx_vepc' instance it
> belongs to.
>
> Given how sgx_{alloc|free}_epc_page() arbitrarily uses encl_page,  
> perhaps we
> should do below?
>
> 	union {
> 		struct sgx_encl_page *encl_page;
> 		struct sgx_encl *encl;
> 		struct sgx_vepc *vepc;
> 		void *owner;
> 	};
>
> And in sgx_{alloc|free}_epc_page() we can use 'owner' instead.
>

As I mentioned in cover letter and change log in 11/18, this series does  
not track virtual EPC.
We can add vepc field into the union in future if such tracking is needed.  
Don't think "void *owner" is needed though.

Thanks
Haitao