diff mbox series

[RFC,v4,03/26] x86/sgx: Wipe out EREMOVE from sgx_free_epc_page()

Message ID 237b82e13e52191409577acddf9b4b28b16bf1bc.1612777752.git.kai.huang@intel.com (mailing list archive)
State New, archived
Headers show
Series KVM SGX virtualization support | expand

Commit Message

Huang, Kai Feb. 8, 2021, 10:54 a.m. UTC 
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.

Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
Signed-off-by: Kai Huang <kai.huang@intel.com>
---
v3->v4:

 - Moved WARN() on SGX_EPC_PAGE_RECLAIMER_TRACKED flag to sgx_reset_epc_page(),
   since the patch to remove the WARN() in v3 was removed. Dave and Sean were
   not convinced, and Sean "tripped more than once in the past during one of
   the many rebases of the virtual EPC and EPC cgroup branches".
 - Added a comment in sgx_reset_epc_page() to explain sgx_free_epc_page() now
   won't do EREMOVE and is expecting EPC page already in clean slate, per Dave.
 
---
 arch/x86/kernel/cpu/sgx/encl.c | 20 ++++++++++++++++++++
 arch/x86/kernel/cpu/sgx/main.c | 12 ++++--------
 2 files changed, 24 insertions(+), 8 deletions(-)

Comments

Dave Hansen Feb. 9, 2021, 4:18 p.m. UTC | #1 
On 2/8/21 2:54 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.

I didn't really like the changelog the last time around, so I wrote you
a new one:

> https://lore.kernel.org/kvm/8250aedb-a623-646d-071a-75ece2c41c09@intel.com/

The "v4" changelog is pretty hard for me to read.  It doesn't tell me
why we can "wipe out EREMOVE" or how it is going to get used going forward.


> +
> +/*
> + * Place the page in uninitialized state.  Called by in sgx_encl_release()
> + * before sgx_free_epc_page(), which requires EPC page is already in clean
> + * slate.
> + */

I really don't like comments like that that refer to callers.  They're
basically guaranteed to become obsolete.

/*
 * Place the page in uninitialized state.  Only usable by callers that
 * know the page is in a clean state in which EREMOVE will succeed.
 */

> +static void sgx_reset_epc_page(struct sgx_epc_page *epc_page)
> +{
> +	int ret;
> +
> +	WARN_ON_ONCE(epc_page->flags & SGX_EPC_PAGE_RECLAIMER_TRACKED);
> +
> +	ret = __eremove(sgx_get_epc_virt_addr(epc_page));
> +	if (WARN_ONCE(ret, "EREMOVE returned %d (0x%x)", ret, ret))
> +		return;
> +}

The uncommented warnings aren't very nice, but I guess they're in the
existing code.
Huang, Kai Feb. 9, 2021, 6:26 p.m. UTC | #2 
On Tue, 2021-02-09 at 08:18 -0800, Dave Hansen wrote:
> On 2/8/21 2:54 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.
> 
> I didn't really like the changelog the last time around, so I wrote you
> a new one:
> 
> > https://lore.kernel.org/kvm/8250aedb-a623-646d-071a-75ece2c41c09@intel.com/
> 
> The "v4" changelog is pretty hard for me to read.  It doesn't tell me
> why we can "wipe out EREMOVE" or how it is going to get used going forward.

Oh sorry I missed this. I'll use yours in next version. Thanks.

> 
> 
> > +
> > +/*
> > + * Place the page in uninitialized state.  Called by in sgx_encl_release()
> > + * before sgx_free_epc_page(), which requires EPC page is already in clean
> > + * slate.
> > + */
> 
> I really don't like comments like that that refer to callers.  They're
> basically guaranteed to become obsolete.
> 
> /*
>  * Place the page in uninitialized state.  Only usable by callers that
>  * know the page is in a clean state in which EREMOVE will succeed.
>  */

Thanks. Agreed it's not good to mention specific caller name.

> 
> > +static void sgx_reset_epc_page(struct sgx_epc_page *epc_page)
> > +{
> > +	int ret;
> > +
> > +	WARN_ON_ONCE(epc_page->flags & SGX_EPC_PAGE_RECLAIMER_TRACKED);
> > +
> > +	ret = __eremove(sgx_get_epc_virt_addr(epc_page));
> > +	if (WARN_ONCE(ret, "EREMOVE returned %d (0x%x)", ret, ret))
> > +		return;
> > +}
> 
> The uncommented warnings aren't very nice, but I guess they're in the
> existing code.

Yes. Adding comment should be another patch logically, and I don't want to introduce
it in this patch.
diff mbox series

Patch

diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
index 20a2dd5ba2b4..a758c7870f06 100644
--- a/arch/x86/kernel/cpu/sgx/encl.c
+++ b/arch/x86/kernel/cpu/sgx/encl.c
@@ -381,6 +381,23 @@  const struct vm_operations_struct sgx_vm_ops = {
 	.access = sgx_vma_access,
 };
 
+
+/*
+ * Place the page in uninitialized state.  Called by in sgx_encl_release()
+ * before sgx_free_epc_page(), which requires EPC page is already in clean
+ * slate.
+ */
+static void sgx_reset_epc_page(struct sgx_epc_page *epc_page)
+{
+	int ret;
+
+	WARN_ON_ONCE(epc_page->flags & SGX_EPC_PAGE_RECLAIMER_TRACKED);
+
+	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
@@ -404,6 +421,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;
@@ -415,6 +433,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;
 	}
@@ -423,6 +442,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 8df81a3ed945..21c2ffa13870 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -598,18 +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
+ * responsibility to make sure that the page is in uninitialized state In other
+ * words, do EREMOVE, EWB or whatever operation is necessary before calling
+ * this function.
  */
 void sgx_free_epc_page(struct sgx_epc_page *page)
 {
 	struct sgx_epc_section *section = &sgx_epc_sections[page->section];
-	int ret;
-
-	WARN_ON_ONCE(page->flags & SGX_EPC_PAGE_RECLAIMER_TRACKED);
-
-	ret = __eremove(sgx_get_epc_virt_addr(page));
-	if (WARN_ONCE(ret, "EREMOVE returned %d (0x%x)", ret, ret))
-		return;
 
 	spin_lock(&section->lock);
 	list_add_tail(&page->list, &section->page_list);