Re: [PATCH V3 15/30] x86/sgx: Support adding of pages to an initialized enclave

[Reinette Chatre]

Hi Jarkko,

On 4/6/2022 12:37 AM, Jarkko Sakkinen wrote:

...

> For what is worth I also get a full pass with our test suite,
> where the runtime is using EAUG together with EACCEPTCOPY:

Thank you very much for all the testing.

Haitao is also busy with significant testing and uncovered a bug via encountering
the WARN at arch/x86/kernel/cpu/sgx/ioctl.c:40. The issue is clear
and I prepared the fix below that can be applied on top of this series.
I plan to split this patch in two (1st the main change, 2nd the changes
to sgx_encl_eaug_page() that will be rolled into "x86/sgx: Support adding
of pages to an initialized enclave") and roll it into the next version
of this series:

-----8<-----

Subject: [PATCH] x86/sgx: Support VA page allocation without reclaiming

VA page allocation is done during enclave creation and adding of
pages to an enclave. In both usages VA pages are allocated
to always attempt a direct reclaim if no EPC pages are available and
because of this the VA pages are allocated without the enclave's mutex
held. Both usages are protected from concurrent attempts with an
atomic operation on SGX_ENCL_IOCTL making it possible to allocate
the VA pages without the enclave's mutex held.

Dynamically adding pages via the page fault handler does not
have the protection of SGX_ENCL_IOCTL but does not require
VA pages to be allocated with default direct reclaim.

Make VA page allocation with direct reclaim optional to make
it possible to perform allocation with enclave's mutex held
and thus protect against concurrent updates to encl->page_cnt.

Reported-by: Haitao Huang <haitao.huang@xxxxxxxxx>
Tested-by: Haitao Huang <haitao.huang@xxxxxxxxx>
Signed-off-by: Reinette Chatre <reinette.chatre@xxxxxxxxx>
---
 arch/x86/kernel/cpu/sgx/encl.c  | 27 +++++++++++----------------
 arch/x86/kernel/cpu/sgx/encl.h  |  4 ++--
 arch/x86/kernel/cpu/sgx/ioctl.c |  8 ++++----
 3 files changed, 17 insertions(+), 22 deletions(-)

diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
index 7909570736a0..11f97fdcac1e 100644
--- a/arch/x86/kernel/cpu/sgx/encl.c
+++ b/arch/x86/kernel/cpu/sgx/encl.c
@@ -239,20 +239,14 @@ static vm_fault_t sgx_encl_eaug_page(struct vm_area_struct *vma,
 		return VM_FAULT_SIGBUS;
 	}
 
-	va_page = sgx_encl_grow(encl);
+	mutex_lock(&encl->lock);
+
+	va_page = sgx_encl_grow(encl, false);
 	if (IS_ERR(va_page)) {
 		ret = PTR_ERR(va_page);
-		goto err_out_free;
+		goto err_out_unlock;
 	}
 
-	mutex_lock(&encl->lock);
-
-	/*
-	 * Copy comment from sgx_encl_add_page() to maintain guidance in
-	 * this similar flow:
-	 * Adding to encl->va_pages must be done under encl->lock.  Ditto for
-	 * deleting (via sgx_encl_shrink()) in the error path.
-	 */
 	if (va_page)
 		list_add(&va_page->list, &encl->va_pages);
 
@@ -263,7 +257,7 @@ static vm_fault_t sgx_encl_eaug_page(struct vm_area_struct *vma,
 	 * running without encl->lock
 	 */
 	if (ret)
-		goto err_out_unlock;
+		goto err_out_shrink;
 
 	pginfo.secs = (unsigned long)sgx_get_epc_virt_addr(encl->secs.epc_page);
 	pginfo.addr = encl_page->desc & PAGE_MASK;
@@ -296,11 +290,10 @@ static vm_fault_t sgx_encl_eaug_page(struct vm_area_struct *vma,
 err_out:
 	xa_erase(&encl->page_array, PFN_DOWN(encl_page->desc));
 
-err_out_unlock:
+err_out_shrink:
 	sgx_encl_shrink(encl, va_page);
+err_out_unlock:
 	mutex_unlock(&encl->lock);
-
-err_out_free:
 	sgx_encl_free_epc_page(epc_page);
 	kfree(encl_page);
 
@@ -998,6 +991,8 @@ void sgx_zap_enclave_ptes(struct sgx_encl *encl, unsigned long addr)
 
 /**
  * sgx_alloc_va_page() - Allocate a Version Array (VA) page
+ * @reclaim: Reclaim EPC pages directly if none available. Enclave
+ *           mutex should not be held if this is set.
  *
  * Allocate a free EPC page and convert it to a Version Array (VA) page.
  *
@@ -1005,12 +1000,12 @@ void sgx_zap_enclave_ptes(struct sgx_encl *encl, unsigned long addr)
  *   a VA page,
  *   -errno otherwise
  */
-struct sgx_epc_page *sgx_alloc_va_page(void)
+struct sgx_epc_page *sgx_alloc_va_page(bool reclaim)
 {
 	struct sgx_epc_page *epc_page;
 	int ret;
 
-	epc_page = sgx_alloc_epc_page(NULL, true);
+	epc_page = sgx_alloc_epc_page(NULL, reclaim);
 	if (IS_ERR(epc_page))
 		return ERR_CAST(epc_page);
 
diff --git a/arch/x86/kernel/cpu/sgx/encl.h b/arch/x86/kernel/cpu/sgx/encl.h
index 253ebdd1c5be..66adb8faec45 100644
--- a/arch/x86/kernel/cpu/sgx/encl.h
+++ b/arch/x86/kernel/cpu/sgx/encl.h
@@ -116,14 +116,14 @@ struct sgx_encl_page *sgx_encl_page_alloc(struct sgx_encl *encl,
 					  unsigned long offset,
 					  u64 secinfo_flags);
 void sgx_zap_enclave_ptes(struct sgx_encl *encl, unsigned long addr);
-struct sgx_epc_page *sgx_alloc_va_page(void);
+struct sgx_epc_page *sgx_alloc_va_page(bool reclaim);
 unsigned int sgx_alloc_va_slot(struct sgx_va_page *va_page);
 void sgx_free_va_slot(struct sgx_va_page *va_page, unsigned int offset);
 bool sgx_va_page_full(struct sgx_va_page *va_page);
 void sgx_encl_free_epc_page(struct sgx_epc_page *page);
 struct sgx_encl_page *sgx_encl_load_page(struct sgx_encl *encl,
 					 unsigned long addr);
-struct sgx_va_page *sgx_encl_grow(struct sgx_encl *encl);
+struct sgx_va_page *sgx_encl_grow(struct sgx_encl *encl, bool reclaim);
 void sgx_encl_shrink(struct sgx_encl *encl, struct sgx_va_page *va_page);
 
 #endif /* _X86_ENCL_H */
diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
index f88bc1236276..b163afff239f 100644
--- a/arch/x86/kernel/cpu/sgx/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/ioctl.c
@@ -17,7 +17,7 @@
 #include "encl.h"
 #include "encls.h"
 
-struct sgx_va_page *sgx_encl_grow(struct sgx_encl *encl)
+struct sgx_va_page *sgx_encl_grow(struct sgx_encl *encl, bool reclaim)
 {
 	struct sgx_va_page *va_page = NULL;
 	void *err;
@@ -30,7 +30,7 @@ struct sgx_va_page *sgx_encl_grow(struct sgx_encl *encl)
 		if (!va_page)
 			return ERR_PTR(-ENOMEM);
 
-		va_page->epc_page = sgx_alloc_va_page();
+		va_page->epc_page = sgx_alloc_va_page(reclaim);
 		if (IS_ERR(va_page->epc_page)) {
 			err = ERR_CAST(va_page->epc_page);
 			kfree(va_page);
@@ -64,7 +64,7 @@ static int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs)
 	struct file *backing;
 	long ret;
 
-	va_page = sgx_encl_grow(encl);
+	va_page = sgx_encl_grow(encl, true);
 	if (IS_ERR(va_page))
 		return PTR_ERR(va_page);
 	else if (va_page)
@@ -275,7 +275,7 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long src,
 		return PTR_ERR(epc_page);
 	}
 
-	va_page = sgx_encl_grow(encl);
+	va_page = sgx_encl_grow(encl, true);
 	if (IS_ERR(va_page)) {
 		ret = PTR_ERR(va_page);
 		goto err_out_free;