| Message ID | 1482437735-4722-2-git-send-email-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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;
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(-)