[for_v23,2/3] x86/sgx: Do not add in-use EPC page to the free page list
diff mbox series

Message ID 20191022224922.28144-3-sean.j.christopherson@intel.com
State New
Headers show
Series
  • x86/sgx: More cleanup for v23
Related show

Commit Message

Sean Christopherson Oct. 22, 2019, 10:49 p.m. UTC
Don't add an EPC page to the free page list of EREMOVE fails, as doing
so will cause any future attempt to use the EPC page to fail, and likely
WARN as well.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kernel/cpu/sgx/main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Jarkko Sakkinen Oct. 23, 2019, 12:43 p.m. UTC | #1
On Tue, Oct 22, 2019 at 03:49:21PM -0700, Sean Christopherson wrote:
> Don't add an EPC page to the free page list of EREMOVE fails, as doing
> so will cause any future attempt to use the EPC page to fail, and likely
> WARN as well.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  arch/x86/kernel/cpu/sgx/main.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> index d45bf6fca0c8..8e7557d3ff03 100644
> --- a/arch/x86/kernel/cpu/sgx/main.c
> +++ b/arch/x86/kernel/cpu/sgx/main.c
> @@ -138,7 +138,8 @@ int sgx_free_page(struct sgx_epc_page *page)
>  	spin_unlock(&sgx_active_page_list_lock);
>  
>  	ret = __eremove(sgx_epc_addr(page));
> -	WARN_ONCE(ret, "EREMOVE returned %d (0x%x)", ret, ret);
> +	if (WARN_ONCE(ret, "EREMOVE returned %d (0x%x)", ret, ret))
> +		return -EIO;

How did you end up choosing -EIO? Don't immediately have any better
suggestion but neither sure if that is the best choice. That is why I'm
asking.

/Jarkko
Sean Christopherson Oct. 23, 2019, 3:23 p.m. UTC | #2
On Wed, Oct 23, 2019 at 03:43:40PM +0300, Jarkko Sakkinen wrote:
> On Tue, Oct 22, 2019 at 03:49:21PM -0700, Sean Christopherson wrote:
> > Don't add an EPC page to the free page list of EREMOVE fails, as doing
> > so will cause any future attempt to use the EPC page to fail, and likely
> > WARN as well.
> > 
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > ---
> >  arch/x86/kernel/cpu/sgx/main.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> > index d45bf6fca0c8..8e7557d3ff03 100644
> > --- a/arch/x86/kernel/cpu/sgx/main.c
> > +++ b/arch/x86/kernel/cpu/sgx/main.c
> > @@ -138,7 +138,8 @@ int sgx_free_page(struct sgx_epc_page *page)
> >  	spin_unlock(&sgx_active_page_list_lock);
> >  
> >  	ret = __eremove(sgx_epc_addr(page));
> > -	WARN_ONCE(ret, "EREMOVE returned %d (0x%x)", ret, ret);
> > +	if (WARN_ONCE(ret, "EREMOVE returned %d (0x%x)", ret, ret))
> > +		return -EIO;
> 
> How did you end up choosing -EIO? Don't immediately have any better
> suggestion but neither sure if that is the best choice. That is why I'm
> asking.

sgx_edbgrd() and sgx_edbgwr() return -EIO on failure, and they're the only
other case I can think of where an ENCLS instruction failure is reported
to userspace *and* may or may not be due to a fault.

I honestly didn't spend much time thinking about it as the code is dropped
in the next path.

Patch
diff mbox series

diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
index d45bf6fca0c8..8e7557d3ff03 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -138,7 +138,8 @@  int sgx_free_page(struct sgx_epc_page *page)
 	spin_unlock(&sgx_active_page_list_lock);
 
 	ret = __eremove(sgx_epc_addr(page));
-	WARN_ONCE(ret, "EREMOVE returned %d (0x%x)", ret, ret);
+	if (WARN_ONCE(ret, "EREMOVE returned %d (0x%x)", ret, ret))
+		return -EIO;
 
 	spin_lock(&section->lock);
 	list_add_tail(&page->list, &section->page_list);