| Message ID | 1492009821-22428-4-git-send-email-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Apr 12, 2017 at 08:10:20AM -0700, Sean Christopherson wrote: > Combine sgx_encl_page's epc_page and va_page into a union. On-demand > VA slot/page allocations guarantees only one of epc_page or va_page > will be valid at any given time. > > Add a new flag, SGX_ENCL_PAGE_EPC_VALID, to track if an encl page has > been loaded into the EPC. If SGX_ENCL_PAGE_EPC_VALID is set, then the > epc_page member of the epc_page/va_page union is valid, otherwise > va_page *may* be valid, e.g. neither is valid if EADD has not been > completed. > > Move the functionality of sgx_evict_page into the success path of > EWB, as the EPC page must be freed before updating the encl_page's > va_page. > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> This is that I proposed in the Fall when we discussed about what needs to be done and it has been sitting in my backlog. Thanks for fixing this issue! /Jarkko > --- > drivers/platform/x86/intel_sgx/sgx.h | 9 ++++++--- > drivers/platform/x86/intel_sgx/sgx_ioctl.c | 2 ++ > drivers/platform/x86/intel_sgx/sgx_page_cache.c | 11 ++++++----- > drivers/platform/x86/intel_sgx/sgx_util.c | 11 +++++------ > 4 files changed, 19 insertions(+), 14 deletions(-) > > diff --git a/drivers/platform/x86/intel_sgx/sgx.h b/drivers/platform/x86/intel_sgx/sgx.h > index 633526f..a1f59dd 100644 > --- a/drivers/platform/x86/intel_sgx/sgx.h > +++ b/drivers/platform/x86/intel_sgx/sgx.h > @@ -91,15 +91,18 @@ static inline void sgx_free_va_slot(struct sgx_va_page *page, > enum sgx_encl_page_flags { > SGX_ENCL_PAGE_TCS = BIT(0), > SGX_ENCL_PAGE_RESERVED = BIT(1), > + SGX_ENCL_PAGE_EPC_VALID = BIT(2), > }; > > struct sgx_encl_page { > unsigned long addr; > unsigned int flags; > - struct sgx_epc_page *epc_page; > - struct list_head load_list; > - struct sgx_va_page *va_page; > unsigned int va_offset; > + union { > + struct sgx_epc_page *epc_page; > + struct sgx_va_page *va_page; > + }; > + struct list_head load_list; > }; > > struct sgx_tgid_ctx { > diff --git a/drivers/platform/x86/intel_sgx/sgx_ioctl.c b/drivers/platform/x86/intel_sgx/sgx_ioctl.c > index af80571..6719e56 100644 > --- a/drivers/platform/x86/intel_sgx/sgx_ioctl.c > +++ b/drivers/platform/x86/intel_sgx/sgx_ioctl.c > @@ -267,6 +267,7 @@ static bool sgx_process_add_page_req(struct sgx_add_page_req *req) > goto out; > } > > + encl_page->flags |= SGX_ENCL_PAGE_EPC_VALID; > encl_page->epc_page = epc_page; > sgx_test_and_clear_young(encl_page, encl); > list_add_tail(&encl_page->load_list, &encl->load_list); > @@ -558,6 +559,7 @@ static long sgx_ioc_enclave_create(struct file *filep, unsigned int cmd, > goto out; > } > > + encl->secs_page.flags |= SGX_ENCL_PAGE_EPC_VALID; > encl->secs_page.epc_page = secs_epc; > createp->src = (unsigned long)encl->base; > > diff --git a/drivers/platform/x86/intel_sgx/sgx_page_cache.c b/drivers/platform/x86/intel_sgx/sgx_page_cache.c > index a2e39a1..346b82c 100644 > --- a/drivers/platform/x86/intel_sgx/sgx_page_cache.c > +++ b/drivers/platform/x86/intel_sgx/sgx_page_cache.c > @@ -341,10 +341,14 @@ static int __sgx_ewb(struct sgx_encl *encl, > sgx_put_page(va); > sgx_put_page(epc); > > - if (ret == SGX_SUCCESS) > + if (ret == SGX_SUCCESS) { > + encl_page->flags &= ~(SGX_ENCL_PAGE_RESERVED | SGX_ENCL_PAGE_EPC_VALID); > + sgx_free_page(encl_page->epc_page, encl); > encl_page->va_page = va_page; > - else > + } > + else { > sgx_free_va_slot(va_page, encl_page->va_offset); > + } > > out_pcmd: > sgx_put_backing(pcmd, true); > @@ -380,9 +384,6 @@ static void sgx_evict_page(struct sgx_encl_page *entry, > struct sgx_encl *encl) > { > sgx_ewb(encl, entry); > - sgx_free_page(entry->epc_page, encl); > - entry->epc_page = NULL; > - entry->flags &= ~SGX_ENCL_PAGE_RESERVED; > } > > static void sgx_write_pages(struct sgx_encl *encl, struct list_head *src) > diff --git a/drivers/platform/x86/intel_sgx/sgx_util.c b/drivers/platform/x86/intel_sgx/sgx_util.c > index f4b2f1a..f3c8b39 100644 > --- a/drivers/platform/x86/intel_sgx/sgx_util.c > +++ b/drivers/platform/x86/intel_sgx/sgx_util.c > @@ -220,6 +220,7 @@ static int sgx_eldu(struct sgx_encl *encl, > ret = -EFAULT; > } else { > sgx_free_va_slot(encl_page->va_page, encl_page->va_offset); > + encl_page->flags |= SGX_ENCL_PAGE_EPC_VALID; > encl_page->epc_page = epc_page; > } > > @@ -281,7 +282,7 @@ static struct sgx_encl_page *sgx_do_fault(struct vm_area_struct *vma, > } > > /* Legal race condition, page is already faulted. */ > - if (entry->epc_page) { > + if (entry->flags & SGX_ENCL_PAGE_EPC_VALID) { > if (reserve) > entry->flags |= SGX_ENCL_PAGE_RESERVED; > goto out; > @@ -295,7 +296,7 @@ static struct sgx_encl_page *sgx_do_fault(struct vm_area_struct *vma, > } > > /* If SECS is evicted then reload it first */ > - if (!encl->secs_page.epc_page) { > + if (!(encl->secs_page.flags & SGX_ENCL_PAGE_EPC_VALID)) { > secs_epc_page = sgx_alloc_page(SGX_ALLOC_ATOMIC); > if (IS_ERR(secs_epc_page)) { > rc = PTR_ERR(secs_epc_page); > @@ -374,7 +375,7 @@ void sgx_encl_release(struct kref *ref) > > radix_tree_for_each_slot(slot, &encl->page_tree, &iter, 0) { > entry = *slot; > - if (entry->epc_page) { > + if (entry->flags & SGX_ENCL_PAGE_EPC_VALID) { > list_del(&entry->load_list); > sgx_free_page(entry->epc_page, encl); > } > @@ -390,11 +391,9 @@ void sgx_encl_release(struct kref *ref) > kfree(va_page); > } > > - if (encl->secs_page.epc_page) > + if (encl->secs_page.flags & SGX_ENCL_PAGE_EPC_VALID) > sgx_free_page(encl->secs_page.epc_page, encl); > > - encl->secs_page.epc_page = NULL; > - > if (encl->tgid_ctx) > kref_put(&encl->tgid_ctx->refcount, sgx_tgid_ctx_release); > > -- > 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
diff --git a/drivers/platform/x86/intel_sgx/sgx.h b/drivers/platform/x86/intel_sgx/sgx.h index 633526f..a1f59dd 100644 --- a/drivers/platform/x86/intel_sgx/sgx.h +++ b/drivers/platform/x86/intel_sgx/sgx.h @@ -91,15 +91,18 @@ static inline void sgx_free_va_slot(struct sgx_va_page *page, enum sgx_encl_page_flags { SGX_ENCL_PAGE_TCS = BIT(0), SGX_ENCL_PAGE_RESERVED = BIT(1), + SGX_ENCL_PAGE_EPC_VALID = BIT(2), }; struct sgx_encl_page { unsigned long addr; unsigned int flags; - struct sgx_epc_page *epc_page; - struct list_head load_list; - struct sgx_va_page *va_page; unsigned int va_offset; + union { + struct sgx_epc_page *epc_page; + struct sgx_va_page *va_page; + }; + struct list_head load_list; }; struct sgx_tgid_ctx { diff --git a/drivers/platform/x86/intel_sgx/sgx_ioctl.c b/drivers/platform/x86/intel_sgx/sgx_ioctl.c index af80571..6719e56 100644 --- a/drivers/platform/x86/intel_sgx/sgx_ioctl.c +++ b/drivers/platform/x86/intel_sgx/sgx_ioctl.c @@ -267,6 +267,7 @@ static bool sgx_process_add_page_req(struct sgx_add_page_req *req) goto out; } + encl_page->flags |= SGX_ENCL_PAGE_EPC_VALID; encl_page->epc_page = epc_page; sgx_test_and_clear_young(encl_page, encl); list_add_tail(&encl_page->load_list, &encl->load_list); @@ -558,6 +559,7 @@ static long sgx_ioc_enclave_create(struct file *filep, unsigned int cmd, goto out; } + encl->secs_page.flags |= SGX_ENCL_PAGE_EPC_VALID; encl->secs_page.epc_page = secs_epc; createp->src = (unsigned long)encl->base; diff --git a/drivers/platform/x86/intel_sgx/sgx_page_cache.c b/drivers/platform/x86/intel_sgx/sgx_page_cache.c index a2e39a1..346b82c 100644 --- a/drivers/platform/x86/intel_sgx/sgx_page_cache.c +++ b/drivers/platform/x86/intel_sgx/sgx_page_cache.c @@ -341,10 +341,14 @@ static int __sgx_ewb(struct sgx_encl *encl, sgx_put_page(va); sgx_put_page(epc); - if (ret == SGX_SUCCESS) + if (ret == SGX_SUCCESS) { + encl_page->flags &= ~(SGX_ENCL_PAGE_RESERVED | SGX_ENCL_PAGE_EPC_VALID); + sgx_free_page(encl_page->epc_page, encl); encl_page->va_page = va_page; - else + } + else { sgx_free_va_slot(va_page, encl_page->va_offset); + } out_pcmd: sgx_put_backing(pcmd, true); @@ -380,9 +384,6 @@ static void sgx_evict_page(struct sgx_encl_page *entry, struct sgx_encl *encl) { sgx_ewb(encl, entry); - sgx_free_page(entry->epc_page, encl); - entry->epc_page = NULL; - entry->flags &= ~SGX_ENCL_PAGE_RESERVED; } static void sgx_write_pages(struct sgx_encl *encl, struct list_head *src) diff --git a/drivers/platform/x86/intel_sgx/sgx_util.c b/drivers/platform/x86/intel_sgx/sgx_util.c index f4b2f1a..f3c8b39 100644 --- a/drivers/platform/x86/intel_sgx/sgx_util.c +++ b/drivers/platform/x86/intel_sgx/sgx_util.c @@ -220,6 +220,7 @@ static int sgx_eldu(struct sgx_encl *encl, ret = -EFAULT; } else { sgx_free_va_slot(encl_page->va_page, encl_page->va_offset); + encl_page->flags |= SGX_ENCL_PAGE_EPC_VALID; encl_page->epc_page = epc_page; } @@ -281,7 +282,7 @@ static struct sgx_encl_page *sgx_do_fault(struct vm_area_struct *vma, } /* Legal race condition, page is already faulted. */ - if (entry->epc_page) { + if (entry->flags & SGX_ENCL_PAGE_EPC_VALID) { if (reserve) entry->flags |= SGX_ENCL_PAGE_RESERVED; goto out; @@ -295,7 +296,7 @@ static struct sgx_encl_page *sgx_do_fault(struct vm_area_struct *vma, } /* If SECS is evicted then reload it first */ - if (!encl->secs_page.epc_page) { + if (!(encl->secs_page.flags & SGX_ENCL_PAGE_EPC_VALID)) { secs_epc_page = sgx_alloc_page(SGX_ALLOC_ATOMIC); if (IS_ERR(secs_epc_page)) { rc = PTR_ERR(secs_epc_page); @@ -374,7 +375,7 @@ void sgx_encl_release(struct kref *ref) radix_tree_for_each_slot(slot, &encl->page_tree, &iter, 0) { entry = *slot; - if (entry->epc_page) { + if (entry->flags & SGX_ENCL_PAGE_EPC_VALID) { list_del(&entry->load_list); sgx_free_page(entry->epc_page, encl); } @@ -390,11 +391,9 @@ void sgx_encl_release(struct kref *ref) kfree(va_page); } - if (encl->secs_page.epc_page) + if (encl->secs_page.flags & SGX_ENCL_PAGE_EPC_VALID) sgx_free_page(encl->secs_page.epc_page, encl); - encl->secs_page.epc_page = NULL; - if (encl->tgid_ctx) kref_put(&encl->tgid_ctx->refcount, sgx_tgid_ctx_release);
Combine sgx_encl_page's epc_page and va_page into a union. On-demand VA slot/page allocations guarantees only one of epc_page or va_page will be valid at any given time. Add a new flag, SGX_ENCL_PAGE_EPC_VALID, to track if an encl page has been loaded into the EPC. If SGX_ENCL_PAGE_EPC_VALID is set, then the epc_page member of the epc_page/va_page union is valid, otherwise va_page *may* be valid, e.g. neither is valid if EADD has not been completed. Move the functionality of sgx_evict_page into the success path of EWB, as the EPC page must be freed before updating the encl_page's va_page. Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- drivers/platform/x86/intel_sgx/sgx.h | 9 ++++++--- drivers/platform/x86/intel_sgx/sgx_ioctl.c | 2 ++ drivers/platform/x86/intel_sgx/sgx_page_cache.c | 11 ++++++----- drivers/platform/x86/intel_sgx/sgx_util.c | 11 +++++------ 4 files changed, 19 insertions(+), 14 deletions(-)