| Message ID | 20191010214301.25669-4-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/sgx: misc page related fixes | expand |
On Thu, Oct 10, 2019 at 02:42:55PM -0700, Sean Christopherson wrote: > Delete an enclave page's entry in the radix tree regardless of whether > or not it has an associated EPC page. > > Note, the reclaimer doesn't need the tree to reclaim EPC pages, i.e. > it's safe to delete the tree entry even if sgx_free_page() fails. > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > --- > arch/x86/kernel/cpu/sgx/encl.c | 15 +++++++-------- > 1 file changed, 7 insertions(+), 8 deletions(-) > > diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c > index ea21d3737a32..68ec8944692a 100644 > --- a/arch/x86/kernel/cpu/sgx/encl.c > +++ b/arch/x86/kernel/cpu/sgx/encl.c > @@ -468,15 +468,14 @@ void sgx_encl_destroy(struct sgx_encl *encl) > > radix_tree_for_each_slot(slot, &encl->page_tree, &iter, 0) { > entry = *slot; > - if (entry->epc_page) { > - if (!sgx_free_page(entry->epc_page)) { > - encl->secs_child_cnt--; > - entry->epc_page = NULL; > - } > - > - radix_tree_delete(&entry->encl->page_tree, > - PFN_DOWN(entry->desc)); > + if (entry->epc_page && > + !sgx_free_page(entry->epc_page)) { > + encl->secs_child_cnt--; > + entry->epc_page = NULL; > } > + > + radix_tree_delete(&entry->encl->page_tree, > + PFN_DOWN(entry->desc)); Uh, just realized we're not freeing the entry itself. That's obviously not good. I'll spin a v2 to fix this proper. > } > > if (!encl->secs_child_cnt && encl->secs.epc_page) { > -- > 2.22.0 >
On Thu, Oct 10, 2019 at 03:49:48PM -0700, Sean Christopherson wrote: > On Thu, Oct 10, 2019 at 02:42:55PM -0700, Sean Christopherson wrote: > > Delete an enclave page's entry in the radix tree regardless of whether > > or not it has an associated EPC page. > > > > Note, the reclaimer doesn't need the tree to reclaim EPC pages, i.e. > > it's safe to delete the tree entry even if sgx_free_page() fails. > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > > --- > > arch/x86/kernel/cpu/sgx/encl.c | 15 +++++++-------- > > 1 file changed, 7 insertions(+), 8 deletions(-) > > > > diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c > > index ea21d3737a32..68ec8944692a 100644 > > --- a/arch/x86/kernel/cpu/sgx/encl.c > > +++ b/arch/x86/kernel/cpu/sgx/encl.c > > @@ -468,15 +468,14 @@ void sgx_encl_destroy(struct sgx_encl *encl) > > > > radix_tree_for_each_slot(slot, &encl->page_tree, &iter, 0) { > > entry = *slot; > > - if (entry->epc_page) { > > - if (!sgx_free_page(entry->epc_page)) { > > - encl->secs_child_cnt--; > > - entry->epc_page = NULL; > > - } > > - > > - radix_tree_delete(&entry->encl->page_tree, > > - PFN_DOWN(entry->desc)); > > + if (entry->epc_page && > > + !sgx_free_page(entry->epc_page)) { > > + encl->secs_child_cnt--; > > + entry->epc_page = NULL; > > } > > + > > + radix_tree_delete(&entry->encl->page_tree, > > + PFN_DOWN(entry->desc)); > > Uh, just realized we're not freeing the entry itself. That's obviously > not good. I'll spin a v2 to fix this proper. OK. So I applied 1/9 and 2/9 from this one. I guess they were good (just a sanity check)? /Jarkko
On Tue, Oct 15, 2019 at 12:00:27AM +0300, Jarkko Sakkinen wrote: > On Thu, Oct 10, 2019 at 03:49:48PM -0700, Sean Christopherson wrote: > > On Thu, Oct 10, 2019 at 02:42:55PM -0700, Sean Christopherson wrote: > > > Delete an enclave page's entry in the radix tree regardless of whether > > > or not it has an associated EPC page. > > > > > > Note, the reclaimer doesn't need the tree to reclaim EPC pages, i.e. > > > it's safe to delete the tree entry even if sgx_free_page() fails. > > > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > > > --- > > > arch/x86/kernel/cpu/sgx/encl.c | 15 +++++++-------- > > > 1 file changed, 7 insertions(+), 8 deletions(-) > > > > > > diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c > > > index ea21d3737a32..68ec8944692a 100644 > > > --- a/arch/x86/kernel/cpu/sgx/encl.c > > > +++ b/arch/x86/kernel/cpu/sgx/encl.c > > > @@ -468,15 +468,14 @@ void sgx_encl_destroy(struct sgx_encl *encl) > > > > > > radix_tree_for_each_slot(slot, &encl->page_tree, &iter, 0) { > > > entry = *slot; > > > - if (entry->epc_page) { > > > - if (!sgx_free_page(entry->epc_page)) { > > > - encl->secs_child_cnt--; > > > - entry->epc_page = NULL; > > > - } > > > - > > > - radix_tree_delete(&entry->encl->page_tree, > > > - PFN_DOWN(entry->desc)); > > > + if (entry->epc_page && > > > + !sgx_free_page(entry->epc_page)) { > > > + encl->secs_child_cnt--; > > > + entry->epc_page = NULL; > > > } > > > + > > > + radix_tree_delete(&entry->encl->page_tree, > > > + PFN_DOWN(entry->desc)); > > > > Uh, just realized we're not freeing the entry itself. That's obviously > > not good. I'll spin a v2 to fix this proper. > > OK. > > So I applied 1/9 and 2/9 from this one. I guess they were good > (just a sanity check)? Yep.
diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c index ea21d3737a32..68ec8944692a 100644 --- a/arch/x86/kernel/cpu/sgx/encl.c +++ b/arch/x86/kernel/cpu/sgx/encl.c @@ -468,15 +468,14 @@ void sgx_encl_destroy(struct sgx_encl *encl) radix_tree_for_each_slot(slot, &encl->page_tree, &iter, 0) { entry = *slot; - if (entry->epc_page) { - if (!sgx_free_page(entry->epc_page)) { - encl->secs_child_cnt--; - entry->epc_page = NULL; - } - - radix_tree_delete(&entry->encl->page_tree, - PFN_DOWN(entry->desc)); + if (entry->epc_page && + !sgx_free_page(entry->epc_page)) { + encl->secs_child_cnt--; + entry->epc_page = NULL; } + + radix_tree_delete(&entry->encl->page_tree, + PFN_DOWN(entry->desc)); } if (!encl->secs_child_cnt && encl->secs.epc_page) {
Delete an enclave page's entry in the radix tree regardless of whether or not it has an associated EPC page. Note, the reclaimer doesn't need the tree to reclaim EPC pages, i.e. it's safe to delete the tree entry even if sgx_free_page() fails. Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- arch/x86/kernel/cpu/sgx/encl.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)