| Message ID | 20231020025353.29691-1-haitao.huang@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/sgx: Return VM_FAULT_SIGBUS for EPC exhaustion | expand |
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
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?
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.)
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
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
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.
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?
* 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
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);
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(-)