| Message ID | 1483544024-6154-4-git-send-email-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jan 04, 2017 at 07:33:43AM -0800, Sean Christopherson wrote: > Swap the order of the calls to vm_insert_pfn and do_eldu to make ELDU > the last action in the fault handling sequence, which eleminates the > need to do EREMOVE of the page if vm_insert_pfn fails. EREMOVE fails > if there are active threads in the enclave, i.e. the previous code > could result in kernel panics due to EREMOVE failure. > > Inserting the page before ELDU does not create a race condition > as accesses to the page will still #PF due to failing the EPCM > checks, i.e. user-visible behavior is identical whether an access > faults due to an invalid PTE or an invalid EPCM entry. > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> Thanks this much better now. Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> Commit log really is the only valid documentation for most of the time... /Jarkko > --- > drivers/platform/x86/intel_sgx_vma.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/drivers/platform/x86/intel_sgx_vma.c b/drivers/platform/x86/intel_sgx_vma.c > index e670405..f356eed 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,18 +242,18 @@ static struct sgx_encl_page *sgx_vma_do_fault(struct vm_area_struct *vma, > goto out; > } > > - rc = do_eldu(encl, entry, epc_page, backing, false /* is_secs */); > + rc = vm_insert_pfn(vma, entry->addr, PFN_DOWN(epc_page->pa)); > if (rc) { > sgx_put_backing(backing, 0); > entry = ERR_PTR(rc); > goto out; > } > > - rc = vm_insert_pfn(vma, entry->addr, PFN_DOWN(epc_page->pa)); > + 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); > entry = ERR_PTR(rc); > goto out; > } > @@ -274,7 +273,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; > -- > 2.7.4 > > _______________________________________________ > intel-sgx-kernel-dev mailing list > intel-sgx-kernel-dev@lists.01.org > https://lists.01.org/mailman/listinfo/intel-sgx-kernel-dev
On Tue, 2017-01-10 at 14:10 +0200, Jarkko Sakkinen wrote: > On Wed, Jan 04, 2017 at 07:33:43AM -0800, Sean Christopherson wrote: > > > > Swap the order of the calls to vm_insert_pfn and do_eldu to make ELDU > > the last action in the fault handling sequence, which eleminates the > > need to do EREMOVE of the page if vm_insert_pfn fails. EREMOVE fails > > if there are active threads in the enclave, i.e. the previous code > > could result in kernel panics due to EREMOVE failure. > > > > Inserting the page before ELDU does not create a race condition > > as accesses to the page will still #PF due to failing the EPCM > > checks, i.e. user-visible behavior is identical whether an access > > faults due to an invalid PTE or an invalid EPCM entry. > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > Thanks this much better now. > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > Commit log really is the only valid documentation for most of the > time... > > /Jarkko > Egad! Don't merge this commit, performing do_eldu before vm_insert_pfn does break the driver. The processor will signal a #GP(0) instead of the expected #PF if the page is accessed after inserting the PFN but before ELDU. > > > > --- > > drivers/platform/x86/intel_sgx_vma.c | 9 ++++----- > > 1 file changed, 4 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/platform/x86/intel_sgx_vma.c > > b/drivers/platform/x86/intel_sgx_vma.c > > index e670405..f356eed 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,18 +242,18 @@ static struct sgx_encl_page *sgx_vma_do_fault(struct > > vm_area_struct *vma, > > goto out; > > } > > > > - rc = do_eldu(encl, entry, epc_page, backing, false /* is_secs */); > > + rc = vm_insert_pfn(vma, entry->addr, PFN_DOWN(epc_page->pa)); > > if (rc) { > > sgx_put_backing(backing, 0); > > entry = ERR_PTR(rc); > > goto out; > > } > > > > - rc = vm_insert_pfn(vma, entry->addr, PFN_DOWN(epc_page->pa)); > > + 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); > > entry = ERR_PTR(rc); > > goto out; > > } > > @@ -274,7 +273,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;
On Tue, Jan 10, 2017 at 10:40:17AM -0800, Sean Christopherson wrote: > On Tue, 2017-01-10 at 14:10 +0200, Jarkko Sakkinen wrote: > > On Wed, Jan 04, 2017 at 07:33:43AM -0800, Sean Christopherson wrote: > > > > > > Swap the order of the calls to vm_insert_pfn and do_eldu to make ELDU > > > the last action in the fault handling sequence, which eleminates the > > > need to do EREMOVE of the page if vm_insert_pfn fails. EREMOVE fails > > > if there are active threads in the enclave, i.e. the previous code > > > could result in kernel panics due to EREMOVE failure. > > > > > > Inserting the page before ELDU does not create a race condition > > > as accesses to the page will still #PF due to failing the EPCM > > > checks, i.e. user-visible behavior is identical whether an access > > > faults due to an invalid PTE or an invalid EPCM entry. > > > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > > Thanks this much better now. > > > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > > > Commit log really is the only valid documentation for most of the > > time... > > > > /Jarkko > > > > Egad! Don't merge this commit, performing do_eldu before vm_insert_pfn does > break the driver. The processor will signal a #GP(0) instead of the expected > #PF if the page is accessed after inserting the PFN but before ELDU. Hey, it's in my master but I'll drop it and leave rest of the three patches. Not much harm done as we are not in the mainline :-) I'll do it within couple of days as this is not really critical at this point. /Jarkko
diff --git a/drivers/platform/x86/intel_sgx_vma.c b/drivers/platform/x86/intel_sgx_vma.c index e670405..f356eed 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,18 +242,18 @@ static struct sgx_encl_page *sgx_vma_do_fault(struct vm_area_struct *vma, goto out; } - rc = do_eldu(encl, entry, epc_page, backing, false /* is_secs */); + rc = vm_insert_pfn(vma, entry->addr, PFN_DOWN(epc_page->pa)); if (rc) { sgx_put_backing(backing, 0); entry = ERR_PTR(rc); goto out; } - rc = vm_insert_pfn(vma, entry->addr, PFN_DOWN(epc_page->pa)); + 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); entry = ERR_PTR(rc); goto out; } @@ -274,7 +273,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;
Swap the order of the calls to vm_insert_pfn and do_eldu to make ELDU the last action in the fault handling sequence, which eleminates the need to do EREMOVE of the page if vm_insert_pfn fails. EREMOVE fails if there are active threads in the enclave, i.e. the previous code could result in kernel panics due to EREMOVE failure. Inserting the page before ELDU does not create a race condition as accesses to the page will still #PF due to failing the EPCM checks, i.e. user-visible behavior is identical whether an access faults due to an invalid PTE or an invalid EPCM entry. Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- drivers/platform/x86/intel_sgx_vma.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)