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

[Jarkko Sakkinen <jarkko@kernel.org>]

On Wed, May 11, 2022 at 11:02:47AM -0700, Reinette Chatre wrote:
> 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);
> 	}
> 
> }

Right.

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

I fully agree with your conclusion.

> Reinette	

Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

Also

Tested-by: Jarkko Sakkinen <jarkko@kernel.org>

BR, Jarkko