Re: [PATCH V2 3/5] x86/sgx: Obtain backing storage page with enclave mutex held

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Jarkko,

On 5/11/2022 4:13 AM, Jarkko Sakkinen wrote:
> On Mon, May 09, 2022 at 02:48:01PM -0700, Reinette Chatre wrote:

...

>> diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
>> index fad3d6c4756e..a60f8b2780fb 100644
>> --- a/arch/x86/kernel/cpu/sgx/main.c
>> +++ b/arch/x86/kernel/cpu/sgx/main.c
>> @@ -310,6 +310,7 @@ static void sgx_reclaimer_write(struct sgx_epc_page *epc_page,
>>  	sgx_encl_ewb(epc_page, backing);
>>  	encl_page->epc_page = NULL;
>>  	encl->secs_child_cnt--;
>> +	sgx_encl_put_backing(backing);
>>  
>>  	if (!encl->secs_child_cnt && test_bit(SGX_ENCL_INITIALIZED, &encl->flags)) {
>>  		ret = sgx_encl_get_backing(encl, PFN_DOWN(encl->size),
>> @@ -381,11 +382,14 @@ static void sgx_reclaim_pages(void)
>>  			goto skip;
>>  
>>  		page_index = PFN_DOWN(encl_page->desc - encl_page->encl->base);
>> +
>> +		mutex_lock(&encl_page->encl->lock);
>>  		ret = sgx_encl_get_backing(encl_page->encl, page_index, &backing[i]);
>> -		if (ret)
>> +		if (ret) {
>> +			mutex_unlock(&encl_page->encl->lock);
>>  			goto skip;
>> +		}
>>  
>> -		mutex_lock(&encl_page->encl->lock);
>>  		encl_page->desc |= SGX_ENCL_PAGE_BEING_RECLAIMED;
>>  		mutex_unlock(&encl_page->encl->lock);
>>  		continue;
>> @@ -413,7 +417,6 @@ static void sgx_reclaim_pages(void)
>>  
>>  		encl_page = epc_page->owner;
>>  		sgx_reclaimer_write(epc_page, &backing[i]);
>> -		sgx_encl_put_backing(&backing[i]);
>>  
>>  		kref_put(&encl_page->encl->refcount, sgx_encl_release);
>>  		epc_page->flags &= ~SGX_EPC_PAGE_RECLAIMER_TRACKED;
>> -- 
>> 2.25.1
>>
> 
> I get the locking part but why is the move of sgx_encl_put_backing
> relevant?

Moving sgx_encl_put_backing() accomplishes the locking goal.

Before the patch:

sgx_reclaim_pages() {
	...
	sgx_reclaimer_write() {
		mutex_lock(&encl->lock);
		...
		mutex_unlock(&encl->lock);
	}
	sgx_encl_put_backing(); /* Not protected by enclave mutex */
}

After the patch:

sgx_reclaim_pages() {
	...
	sgx_reclaimer_write() {
		mutex_lock(&encl->lock);
		...
			sgx_encl_put_backing(); /* Protected by enclave mutex */
		...
		mutex_unlock(&encl->lock);
	}

}

Even so, because of patch 1/1 the first scenario described in the
changelog is no longer valid since the page is marked as dirty
with the enclave mutex held. It may thus not be required
to call sgx_encl_put_backing() with enclave mutex held but it
remains important for sgx_encl_get_backing() to be called with
enclave mutex held since it ensures that SGX_ENCL_PAGE_BEING_RECLAIMED
can be used (in patch 4/5) to reliably reflect references to the
backing storage.
Considering that I would like to continue to consistently protect
sgx_encl_get_backing()/sgx_encl_put_backing() with the enclave mutex.

Reinette