[intel-sgx-kernel-dev,1/6] intel_sgx: Abort sgx_vma_do_fault if do_eldu fails
diff mbox

Message ID 1482437735-4722-2-git-send-email-sean.j.christopherson@intel.com
State New
Headers show

Commit Message

Sean Christopherson Dec. 22, 2016, 8:15 p.m. UTC
Abort sgx_vma_do_fault if do_eldu fails when loading the faulting
page.  Swap the order of the calls to vm_insert_pfn and do_eldu;
ELDU is more expensive than modifying a PTE, and do_eldu doesn't
fail under normal circumstances, i.e. we should only have to undo
vm_insert_pfn if there is a driver or hardware bug.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 drivers/platform/x86/intel_sgx_vma.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

Comments

Jarkko Sakkinen Dec. 29, 2016, 12:52 p.m. UTC | #1
On Thu, Dec 22, 2016 at 12:15:30PM -0800, Sean Christopherson wrote:
> Abort sgx_vma_do_fault if do_eldu fails when loading the faulting
> page.  Swap the order of the calls to vm_insert_pfn and do_eldu;
> ELDU is more expensive than modifying a PTE, and do_eldu doesn't
> fail under normal circumstances, i.e. we should only have to undo
> vm_insert_pfn if there is a driver or hardware bug.

Is there a risk that another harware thread could hit a page where PTE
is inserted but ELDU is not yet done, or is there not? The commit
message does not clear this concern. It should.

/Jarkko
Jarkko Sakkinen Dec. 29, 2016, 2:25 p.m. UTC | #2
On Thu, Dec 29, 2016 at 02:52:58PM +0200, Jarkko Sakkinen wrote:
> On Thu, Dec 22, 2016 at 12:15:30PM -0800, Sean Christopherson wrote:
> > Abort sgx_vma_do_fault if do_eldu fails when loading the faulting
> > page.  Swap the order of the calls to vm_insert_pfn and do_eldu;
> > ELDU is more expensive than modifying a PTE, and do_eldu doesn't
> > fail under normal circumstances, i.e. we should only have to undo
> > vm_insert_pfn if there is a driver or hardware bug.
> 
> Is there a risk that another harware thread could hit a page where PTE
> is inserted but ELDU is not yet done, or is there not? The commit
> message does not clear this concern. It should.

I would split this into two patches. There's a bug fix and change
of call order. They are separae changes.

/Jarkko

Patch
diff mbox

diff --git a/drivers/platform/x86/intel_sgx_vma.c b/drivers/platform/x86/intel_sgx_vma.c
index c7b406b..bb4d5d0 100644
--- a/drivers/platform/x86/intel_sgx_vma.c
+++ b/drivers/platform/x86/intel_sgx_vma.c
@@ -160,7 +160,6 @@  static struct sgx_encl_page *sgx_vma_do_fault(struct vm_area_struct *vma,
 	struct sgx_epc_page *epc_page = NULL;
 	struct sgx_epc_page *secs_epc_page = NULL;
 	struct page *backing;
-	unsigned int free_flags = SGX_FREE_SKIP_EREMOVE;
 	int rc;
 
 	/* If process was forked, VMA is still there but vm_private_data is set
@@ -243,12 +242,16 @@  static struct sgx_encl_page *sgx_vma_do_fault(struct vm_area_struct *vma,
 		goto out;
 	}
 
-	do_eldu(encl, entry, epc_page, backing, false /* is_secs */);
 	rc = vm_insert_pfn(vma, entry->addr, PFN_DOWN(epc_page->pa));
-	sgx_put_backing(backing, 0);
+	if (rc) {
+		sgx_put_backing(backing, 0);
+		goto out;
+	}
 
+	rc = do_eldu(encl, entry, epc_page, backing, false /* is_secs */);
+	sgx_put_backing(backing, 0);
 	if (rc) {
-		free_flags = 0;
+		zap_vma_ptes(vma, entry->addr, PAGE_SIZE);
 		goto out;
 	}
 
@@ -267,7 +270,7 @@  static struct sgx_encl_page *sgx_vma_do_fault(struct vm_area_struct *vma,
 out:
 	mutex_unlock(&encl->lock);
 	if (epc_page)
-		sgx_free_page(epc_page, encl, free_flags);
+		sgx_free_page(epc_page, encl, SGX_FREE_SKIP_EREMOVE);
 	if (secs_epc_page)
 		sgx_free_page(secs_epc_page, encl, SGX_FREE_SKIP_EREMOVE);
 	return entry;