[PATCH] x86/sgx: Return VM_FAULT_SIGBUS for EPC exhaustion

[PATCH] x86/sgx: Return VM_FAULT_SIGBUS for EPC exhaustion

[Haitao Huang]

In the EAUG on page fault path, VM_FAULT_OOM is returned when the
Enclave Page Cache (EPC) runs out. This may trigger unneeded OOM kill
that will not free any EPCs. Return VM_FAULT_SIGBUS instead.

Fixes: 5a90d2c3f5ef ("x86/sgx: Support adding of pages to an initialized enclave")
Cc: stable@vger.kernel.org # v6.0+
Signed-off-by: Haitao Huang <haitao.huang@linux.intel.com>
---
 arch/x86/kernel/cpu/sgx/encl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
index 279148e72459..d13b7e4ad0f5 100644
--- a/arch/x86/kernel/cpu/sgx/encl.c
+++ b/arch/x86/kernel/cpu/sgx/encl.c
@@ -322,7 +322,7 @@ struct sgx_encl_page *sgx_encl_load_page(struct sgx_encl *encl,
  * ENCLS[EAUG] instruction.
  *
  * Returns: Appropriate vm_fault_t: VM_FAULT_NOPAGE when PTE was installed
- * successfully, VM_FAULT_SIGBUS or VM_FAULT_OOM as error otherwise.
+ * successfully, VM_FAULT_SIGBUS as error otherwise.
  */
 static vm_fault_t sgx_encl_eaug_page(struct vm_area_struct *vma,
 				     struct sgx_encl *encl, unsigned long addr)
@@ -348,7 +348,7 @@ static vm_fault_t sgx_encl_eaug_page(struct vm_area_struct *vma,
 	secinfo_flags = SGX_SECINFO_R | SGX_SECINFO_W | SGX_SECINFO_X;
 	encl_page = sgx_encl_page_alloc(encl, addr - encl->base, secinfo_flags);
 	if (IS_ERR(encl_page))
-		return VM_FAULT_OOM;
+		return VM_FAULT_SIGBUS;
 
 	mutex_lock(&encl->lock);
 
-- 
2.25.1

Re: [PATCH] x86/sgx: Return VM_FAULT_SIGBUS for EPC exhaustion

[Jarkko Sakkinen]

On Fri Oct 20, 2023 at 5:53 AM EEST, Haitao Huang wrote:
> In the EAUG on page fault path, VM_FAULT_OOM is returned when the
> Enclave Page Cache (EPC) runs out. This may trigger unneeded OOM kill
> that will not free any EPCs. Return VM_FAULT_SIGBUS instead.
>
> Fixes: 5a90d2c3f5ef ("x86/sgx: Support adding of pages to an initialized enclave")
> Cc: stable@vger.kernel.org # v6.0+
> Signed-off-by: Haitao Huang <haitao.huang@linux.intel.com>
> ---
>  arch/x86/kernel/cpu/sgx/encl.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
> index 279148e72459..d13b7e4ad0f5 100644
> --- a/arch/x86/kernel/cpu/sgx/encl.c
> +++ b/arch/x86/kernel/cpu/sgx/encl.c
> @@ -322,7 +322,7 @@ struct sgx_encl_page *sgx_encl_load_page(struct sgx_encl *encl,
>   * ENCLS[EAUG] instruction.
>   *
>   * Returns: Appropriate vm_fault_t: VM_FAULT_NOPAGE when PTE was installed
> - * successfully, VM_FAULT_SIGBUS or VM_FAULT_OOM as error otherwise.
> + * successfully, VM_FAULT_SIGBUS as error otherwise.
>   */
>  static vm_fault_t sgx_encl_eaug_page(struct vm_area_struct *vma,
>  				     struct sgx_encl *encl, unsigned long addr)
> @@ -348,7 +348,7 @@ static vm_fault_t sgx_encl_eaug_page(struct vm_area_struct *vma,
>  	secinfo_flags = SGX_SECINFO_R | SGX_SECINFO_W | SGX_SECINFO_X;
>  	encl_page = sgx_encl_page_alloc(encl, addr - encl->base, secinfo_flags);
>  	if (IS_ERR(encl_page))
> -		return VM_FAULT_OOM;
> +		return VM_FAULT_SIGBUS;
>  
>  	mutex_lock(&encl->lock);
>  

Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

BR, Jarkko

Re: [PATCH] x86/sgx: Return VM_FAULT_SIGBUS for EPC exhaustion

[Dave Hansen]

On 10/19/23 19:53, Haitao Huang wrote:
> In the EAUG on page fault path, VM_FAULT_OOM is returned when the
> Enclave Page Cache (EPC) runs out. This may trigger unneeded OOM kill
> that will not free any EPCs. Return VM_FAULT_SIGBUS instead.

So, when picking an error code and we look the documentation for the
bits, we see:

>  * @VM_FAULT_OOM:               Out Of Memory
>  * @VM_FAULT_SIGBUS:            Bad access

So if anything we'll need a bit more changelog where you explain how
running out of enclave memory is more "Bad access" than "Out Of Memory".
 Because on the surface this patch looks wrong.

But that's just a naming thing.  What *behavior* is bad here?  With the
old code, what happens?  With the new code, what happens?  Why is the
old better than the new?

Re: [PATCH] x86/sgx: Return VM_FAULT_SIGBUS for EPC exhaustion

[Huang, Kai]

On Wed, 2023-10-25 at 07:31 -0700, Hansen, Dave wrote:
> On 10/19/23 19:53, Haitao Huang wrote:
> > In the EAUG on page fault path, VM_FAULT_OOM is returned when the
> > Enclave Page Cache (EPC) runs out. This may trigger unneeded OOM kill
> > that will not free any EPCs. Return VM_FAULT_SIGBUS instead.
> 
> So, when picking an error code and we look the documentation for the
> bits, we see:
> 
> >  * @VM_FAULT_OOM:               Out Of Memory
> >  * @VM_FAULT_SIGBUS:            Bad access
> 
> So if anything we'll need a bit more changelog where you explain how
> running out of enclave memory is more "Bad access" than "Out Of Memory".
>  Because on the surface this patch looks wrong.
> 
> But that's just a naming thing.  What *behavior* is bad here?  With the
> old code, what happens?  With the new code, what happens?  Why is the
> old better than the new?

I think Haitao meant if we return OOM, the core-MM fault handler will believe
the fault couldn't be handled because of running out of memory, and then it
could invoke the OOM killer which might select an unrelated victim who might
have no EPC at all.

If we return SIGBUS, then the faulting process/enclave will get the signal and,
e.g., get killed.

static inline
void do_user_addr_fault(struct pt_regs *regs,
                        unsigned long error_code,
                        unsigned long address)
{
	...

	fault = handle_mm_fault(vma, address, ...);
	
	...

done:
	...

        if (fault & VM_FAULT_OOM) {
                /* Kernel mode? Handle exceptions or die: */
                if (!user_mode(regs)) {
                        kernelmode_fixup_or_oops(regs, error_code, address,
                                                 SIGSEGV, SEGV_MAPERR,
                                                 ARCH_DEFAULT_PKEY);
                        return;
                }

                /*
                 * We ran out of memory, call the OOM killer, and return the
                 * userspace (which will retry the fault, or kill us if we got
                 * oom-killed):
                 */
                pagefault_out_of_memory();
        } else {
                if (fault & (VM_FAULT_SIGBUS|VM_FAULT_HWPOISON|
                             VM_FAULT_HWPOISON_LARGE))
                        do_sigbus(regs, error_code, address, fault);
                else if (fault & VM_FAULT_SIGSEGV)
                        bad_area_nosemaphore(regs, error_code, address);
                else
                        BUG();
        }
}

Btw, Ingo has already queued this patch to tip/urgent:

https://lore.kernel.org/all/169778941056.3135.14169781154210769341.tip-bot2@tip-bot2/T/

(Also, currently the non-EAUG code path (ELDU) in sgx_vma_fault() also returns
SIGBUS if it fails to allocate EPC, so making EAUG code path return SIGBUS also
matches the ELDU path.)

Re: [PATCH] x86/sgx: Return VM_FAULT_SIGBUS for EPC exhaustion

[Reinette Chatre]

On 10/25/2023 4:58 PM, Huang, Kai wrote:
> On Wed, 2023-10-25 at 07:31 -0700, Hansen, Dave wrote:
>> On 10/19/23 19:53, Haitao Huang wrote:
>>> In the EAUG on page fault path, VM_FAULT_OOM is returned when the
>>> Enclave Page Cache (EPC) runs out. This may trigger unneeded OOM kill
>>> that will not free any EPCs. Return VM_FAULT_SIGBUS instead.

This commit message does not seem accurate to me. From what I can tell
VM_FAULT_SIGBUS is indeed returned when EPC runs out. What is addressed
with this patch is the error returned when kernel (not EPC) memory runs
out.

>> So, when picking an error code and we look the documentation for the
>> bits, we see:
>>
>>>  * @VM_FAULT_OOM:               Out Of Memory
>>>  * @VM_FAULT_SIGBUS:            Bad access
>>
>> So if anything we'll need a bit more changelog where you explain how
>> running out of enclave memory is more "Bad access" than "Out Of Memory".
>>  Because on the surface this patch looks wrong.
>>
>> But that's just a naming thing.  What *behavior* is bad here?  With the
>> old code, what happens?  With the new code, what happens?  Why is the
>> old better than the new?
> 
> I think Haitao meant if we return OOM, the core-MM fault handler will believe
> the fault couldn't be handled because of running out of memory, and then it
> could invoke the OOM killer which might select an unrelated victim who might
> have no EPC at all.

Since the issue is that system is out of kernel memory the resolution may need to
look further than owners with EPC memory.

...

> 
> (Also, currently the non-EAUG code path (ELDU) in sgx_vma_fault() also returns
> SIGBUS if it fails to allocate EPC, so making EAUG code path return SIGBUS also
> matches the ELDU path.)
> 

These errors all seem related to EPC memory to me, not kernel memory.

Reinette

Re: [PATCH] x86/sgx: Return VM_FAULT_SIGBUS for EPC exhaustion

[Haitao Huang]

On Thu, 26 Oct 2023 11:01:57 -0500, Reinette Chatre  
<reinette.chatre@intel.com> wrote:

>
>
> On 10/25/2023 4:58 PM, Huang, Kai wrote:
>> On Wed, 2023-10-25 at 07:31 -0700, Hansen, Dave wrote:
>>> On 10/19/23 19:53, Haitao Huang wrote:
>>>> In the EAUG on page fault path, VM_FAULT_OOM is returned when the
>>>> Enclave Page Cache (EPC) runs out. This may trigger unneeded OOM kill
>>>> that will not free any EPCs. Return VM_FAULT_SIGBUS instead.
>
> This commit message does not seem accurate to me. From what I can tell
> VM_FAULT_SIGBUS is indeed returned when EPC runs out. What is addressed
> with this patch is the error returned when kernel (not EPC) memory runs
> out.
>


Sorry I got it mixed up between sgx_alloc_epc_page and sgx_encl_page_alloc  
returns.
You are right. Please drop this patch.

Thanks
Haitao

Re: [PATCH] x86/sgx: Return VM_FAULT_SIGBUS for EPC exhaustion

[Huang, Kai]

On Thu, 2023-10-26 at 09:01 -0700, Chatre, Reinette wrote:
> 
> On 10/25/2023 4:58 PM, Huang, Kai wrote:
> > On Wed, 2023-10-25 at 07:31 -0700, Hansen, Dave wrote:
> > > On 10/19/23 19:53, Haitao Huang wrote:
> > > > In the EAUG on page fault path, VM_FAULT_OOM is returned when the
> > > > Enclave Page Cache (EPC) runs out. This may trigger unneeded OOM kill
> > > > that will not free any EPCs. Return VM_FAULT_SIGBUS instead.
> 
> This commit message does not seem accurate to me. From what I can tell
> VM_FAULT_SIGBUS is indeed returned when EPC runs out. What is addressed
> with this patch is the error returned when kernel (not EPC) memory runs
> out.
> 
> > > So, when picking an error code and we look the documentation for the
> > > bits, we see:
> > > 
> > > >  * @VM_FAULT_OOM:               Out Of Memory
> > > >  * @VM_FAULT_SIGBUS:            Bad access
> > > 
> > > So if anything we'll need a bit more changelog where you explain how
> > > running out of enclave memory is more "Bad access" than "Out Of Memory".
> > >  Because on the surface this patch looks wrong.
> > > 
> > > But that's just a naming thing.  What *behavior* is bad here?  With the
> > > old code, what happens?  With the new code, what happens?  Why is the
> > > old better than the new?
> > 
> > I think Haitao meant if we return OOM, the core-MM fault handler will believe
> > the fault couldn't be handled because of running out of memory, and then it
> > could invoke the OOM killer which might select an unrelated victim who might
> > have no EPC at all.
> 
> Since the issue is that system is out of kernel memory the resolution may need to
> look further than owners with EPC memory.

Oh right, I didn't look into the sgx_encl_page_alloc():

        encl_page = kzalloc(sizeof(*encl_page), GFP_KERNEL);
        if (!encl_page)
                return ERR_PTR(-ENOMEM);

> 
> ...
> 
> > 
> > (Also, currently the non-EAUG code path (ELDU) in sgx_vma_fault() also returns
> > SIGBUS if it fails to allocate EPC, so making EAUG code path return SIGBUS also
> > matches the ELDU path.)
> > 
> 
> These errors all seem related to EPC memory to me, not kernel memory.

Right.

Re: [PATCH] x86/sgx: Return VM_FAULT_SIGBUS for EPC exhaustion

[Huang, Kai]

On Thu, 2023-10-26 at 11:34 -0500, Haitao Huang wrote:
> On Thu, 26 Oct 2023 11:01:57 -0500, Reinette Chatre  
> <reinette.chatre@intel.com> wrote:
> 
> > 
> > 
> > On 10/25/2023 4:58 PM, Huang, Kai wrote:
> > > On Wed, 2023-10-25 at 07:31 -0700, Hansen, Dave wrote:
> > > > On 10/19/23 19:53, Haitao Huang wrote:
> > > > > In the EAUG on page fault path, VM_FAULT_OOM is returned when the
> > > > > Enclave Page Cache (EPC) runs out. This may trigger unneeded OOM kill
> > > > > that will not free any EPCs. Return VM_FAULT_SIGBUS instead.
> > 
> > This commit message does not seem accurate to me. From what I can tell
> > VM_FAULT_SIGBUS is indeed returned when EPC runs out. What is addressed
> > with this patch is the error returned when kernel (not EPC) memory runs
> > out.
> > 
> 
> 
> Sorry I got it mixed up between sgx_alloc_epc_page and sgx_encl_page_alloc  
> returns.
> You are right. Please drop this patch.
> 

It's already in tip/x86/urgent.  Please send a patch to revert?

Re: [PATCH] x86/sgx: Return VM_FAULT_SIGBUS for EPC exhaustion

[Ingo Molnar]

* Huang, Kai <kai.huang@intel.com> wrote:

> On Thu, 2023-10-26 at 11:34 -0500, Haitao Huang wrote:
> > On Thu, 26 Oct 2023 11:01:57 -0500, Reinette Chatre  
> > <reinette.chatre@intel.com> wrote:
> > 
> > > 
> > > 
> > > On 10/25/2023 4:58 PM, Huang, Kai wrote:
> > > > On Wed, 2023-10-25 at 07:31 -0700, Hansen, Dave wrote:
> > > > > On 10/19/23 19:53, Haitao Huang wrote:
> > > > > > In the EAUG on page fault path, VM_FAULT_OOM is returned when the
> > > > > > Enclave Page Cache (EPC) runs out. This may trigger unneeded OOM kill
> > > > > > that will not free any EPCs. Return VM_FAULT_SIGBUS instead.
> > > 
> > > This commit message does not seem accurate to me. From what I can tell
> > > VM_FAULT_SIGBUS is indeed returned when EPC runs out. What is addressed
> > > with this patch is the error returned when kernel (not EPC) memory runs
> > > out.
> > > 
> > 
> > 
> > Sorry I got it mixed up between sgx_alloc_epc_page and sgx_encl_page_alloc  
> > returns.
> > You are right. Please drop this patch.
> > 
> 
> It's already in tip/x86/urgent.  Please send a patch to revert?

No, let's just zap it.

Thanks,

	Ingo