| Message ID | 20191010214301.25669-2-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:53PM -0700, Sean Christopherson wrote: > Add a WARN to detect EPC page leaks when releasing an enclave. The > release flow uses the common sgx_encl_destroy() helper, which is allowed > to be called while the reclaimer holds references to the enclave's EPC > pages and so can't WARN in the scenario where the SECS is leaked because > it has active child pages. > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > --- > arch/x86/kernel/cpu/sgx/encl.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c > index c13c3ba3430a..b4d7b2f9609f 100644 > --- a/arch/x86/kernel/cpu/sgx/encl.c > +++ b/arch/x86/kernel/cpu/sgx/encl.c > @@ -511,6 +511,7 @@ void sgx_encl_release(struct kref *ref) > fput(encl->backing); > > WARN_ONCE(!list_empty(&encl->mm_list), "mm_list non-empty"); > + WARN_ON_ONCE(encl->secs_child_cnt || encl->secs.epc_page); I'd prefer to have two WARN_ON_ONCE()'s. I think disjunction's should not be used with WARN*() (conjunction's obviously should when they are required). I changed this to: /* Detect EPC page leak's. */ WARN_ON_ONCE(encl->secs_child_cnt); WARN_ON_ONCE(encl->secs.epc_page); The patch has been merged. /Jarkko
On Mon, Oct 14, 2019 at 11:31:56PM +0300, Jarkko Sakkinen wrote: > On Thu, Oct 10, 2019 at 02:42:53PM -0700, Sean Christopherson wrote: > > Add a WARN to detect EPC page leaks when releasing an enclave. The > > release flow uses the common sgx_encl_destroy() helper, which is allowed > > to be called while the reclaimer holds references to the enclave's EPC > > pages and so can't WARN in the scenario where the SECS is leaked because > > it has active child pages. > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > > --- > > arch/x86/kernel/cpu/sgx/encl.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c > > index c13c3ba3430a..b4d7b2f9609f 100644 > > --- a/arch/x86/kernel/cpu/sgx/encl.c > > +++ b/arch/x86/kernel/cpu/sgx/encl.c > > @@ -511,6 +511,7 @@ void sgx_encl_release(struct kref *ref) > > fput(encl->backing); > > > > WARN_ONCE(!list_empty(&encl->mm_list), "mm_list non-empty"); > > + WARN_ON_ONCE(encl->secs_child_cnt || encl->secs.epc_page); > > I'd prefer to have two WARN_ON_ONCE()'s. I think disjunction's should > not be used with WARN*() (conjunction's obviously should when they are > required). > > I changed this to: > > /* Detect EPC page leak's. */ > WARN_ON_ONCE(encl->secs_child_cnt); > WARN_ON_ONCE(encl->secs.epc_page); > > The patch has been merged. Should the mm_list check also use the same macro? The associated message is somewhat useless, isn't it? /Jarkko
On Mon, Oct 14, 2019 at 11:33:43PM +0300, Jarkko Sakkinen wrote: > On Mon, Oct 14, 2019 at 11:31:56PM +0300, Jarkko Sakkinen wrote: > > On Thu, Oct 10, 2019 at 02:42:53PM -0700, Sean Christopherson wrote: > > > Add a WARN to detect EPC page leaks when releasing an enclave. The > > > release flow uses the common sgx_encl_destroy() helper, which is allowed > > > to be called while the reclaimer holds references to the enclave's EPC > > > pages and so can't WARN in the scenario where the SECS is leaked because > > > it has active child pages. > > > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > > > --- > > > arch/x86/kernel/cpu/sgx/encl.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c > > > index c13c3ba3430a..b4d7b2f9609f 100644 > > > --- a/arch/x86/kernel/cpu/sgx/encl.c > > > +++ b/arch/x86/kernel/cpu/sgx/encl.c > > > @@ -511,6 +511,7 @@ void sgx_encl_release(struct kref *ref) > > > fput(encl->backing); > > > > > > WARN_ONCE(!list_empty(&encl->mm_list), "mm_list non-empty"); > > > + WARN_ON_ONCE(encl->secs_child_cnt || encl->secs.epc_page); > > > > I'd prefer to have two WARN_ON_ONCE()'s. I think disjunction's should > > not be used with WARN*() (conjunction's obviously should when they are > > required). > > > > I changed this to: > > > > /* Detect EPC page leak's. */ > > WARN_ON_ONCE(encl->secs_child_cnt); > > WARN_ON_ONCE(encl->secs.epc_page); > > > > The patch has been merged. > > Should the mm_list check also use the same macro? The associated message > is somewhat useless, isn't it? Yep.
diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c index c13c3ba3430a..b4d7b2f9609f 100644 --- a/arch/x86/kernel/cpu/sgx/encl.c +++ b/arch/x86/kernel/cpu/sgx/encl.c @@ -511,6 +511,7 @@ void sgx_encl_release(struct kref *ref) fput(encl->backing); WARN_ONCE(!list_empty(&encl->mm_list), "mm_list non-empty"); + WARN_ON_ONCE(encl->secs_child_cnt || encl->secs.epc_page); kfree(encl); }
Add a WARN to detect EPC page leaks when releasing an enclave. The release flow uses the common sgx_encl_destroy() helper, which is allowed to be called while the reclaimer holds references to the enclave's EPC pages and so can't WARN in the scenario where the SECS is leaked because it has active child pages. Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- arch/x86/kernel/cpu/sgx/encl.c | 1 + 1 file changed, 1 insertion(+)