Re: [RFC PATCH v3 04/27] x86/sgx: Wipe out EREMOVE from sgx_free_epc_page()

[Kai Huang <kai.huang@intel.com>]

On Tue, 26 Jan 2021 08:04:35 -0800 Dave Hansen wrote:
> On 1/26/21 1:30 AM, Kai Huang wrote:
> > From: Jarkko Sakkinen <jarkko@kernel.org>
> > 
> > Encapsulate the snippet in sgx_free_epc_page() concerning EREMOVE to
> > sgx_reset_epc_page(), which is a static helper function for
> > sgx_encl_release().  It's the only function existing, which deals with
> > initialized pages.
> 
> Yikes.  I have no idea what that is saying.  Here's a rewrite:
> 
> EREMOVE takes a pages and removes any association between that page and
> an enclave.  It must be run on a page before it can be added into
> another enclave.  Currently, EREMOVE is run as part of pages being freed
> into the SGX page allocator.  It is not expected to fail.
> 
> KVM does not track how guest pages are used, which means that SGX
> virtualization use of EREMOVE might fail.
> 
> Break out the EREMOVE call from the SGX page allocator.  This will allow
> the SGX virtualization code to use the allocator directly.  (SGX/KVM
> will also introduce a more permissive EREMOVE helper).

Thanks.

Hi Jarkko,

Do you want me to update your patch directly, or do you want to take the
change, and send me the patch again?

> 
> > diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
> > index ee50a5010277..a78b71447771 100644
> > --- a/arch/x86/kernel/cpu/sgx/encl.c
> > +++ b/arch/x86/kernel/cpu/sgx/encl.c
> > @@ -389,6 +389,16 @@ const struct vm_operations_struct sgx_vm_ops = {
> >  	.access = sgx_vma_access,
> >  };
> >  
> > +
> > +static void sgx_reset_epc_page(struct sgx_epc_page *epc_page)
> > +{
> > +	int ret;
> > +
> > +	ret = __eremove(sgx_get_epc_virt_addr(epc_page));
> > +	if (WARN_ONCE(ret, "EREMOVE returned %d (0x%x)", ret, ret))
> > +		return;
> > +}
> > +
> >  /**
> >   * sgx_encl_release - Destroy an enclave instance
> >   * @kref:	address of a kref inside &sgx_encl
> > @@ -412,6 +422,7 @@ void sgx_encl_release(struct kref *ref)
> >  			if (sgx_unmark_page_reclaimable(entry->epc_page))
> >  				continue;
> >  
> > +			sgx_reset_epc_page(entry->epc_page);
> >  			sgx_free_epc_page(entry->epc_page);
> >  			encl->secs_child_cnt--;
> >  			entry->epc_page = NULL;
> > @@ -423,6 +434,7 @@ void sgx_encl_release(struct kref *ref)
> >  	xa_destroy(&encl->page_array);
> >  
> >  	if (!encl->secs_child_cnt && encl->secs.epc_page) {
> > +		sgx_reset_epc_page(encl->secs.epc_page);
> >  		sgx_free_epc_page(encl->secs.epc_page);
> >  		encl->secs.epc_page = NULL;
> >  	}
> > @@ -431,6 +443,7 @@ void sgx_encl_release(struct kref *ref)
> >  		va_page = list_first_entry(&encl->va_pages, struct sgx_va_page,
> >  					   list);
> >  		list_del(&va_page->list);
> > +		sgx_reset_epc_page(va_page->epc_page);
> >  		sgx_free_epc_page(va_page->epc_page);
> >  		kfree(va_page);
> >  	}
> > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> > index f330abdb5bb1..21c2ffa13870 100644
> > --- a/arch/x86/kernel/cpu/sgx/main.c
> > +++ b/arch/x86/kernel/cpu/sgx/main.c
> > @@ -598,16 +598,14 @@ struct sgx_epc_page *sgx_alloc_epc_page(void *owner, bool reclaim)
> >   * sgx_free_epc_page() - Free an EPC page
> >   * @page:	an EPC page
> >   *
> > - * Call EREMOVE for an EPC page and insert it back to the list of free pages.
> > + * Put the EPC page back to the list of free pages. It's the callers
> 
> "caller's"
> 
> > + * responsibility to make sure that the page is in uninitialized state In other
> 
> Period after "state", please.
> 
> > + * words, do EREMOVE, EWB or whatever operation is necessary before calling
> > + * this function.
> >   */
> 
> OK, so if you're going to say "the caller must put the page in
> uninitialized state", let's also add a comment to the place that *DO*
> that, like the shiny new sgx_reset_epc_page().

Hi Dave,

Sorry I am a little bit confused here. Do you mean we should add a comment in
sgx_reset_epc_page() to say, for instance: sgx_free_epc_page() requires the EPC
page already been EREMOVE'd?