Re: [RFC PATCH 0/4] SGX shmem backing store issue

[Jarkko Sakkinen <jarkko@kernel.org>]

On Thu, Apr 28, 2022 at 01:11:23PM -0700, Reinette Chatre wrote:
> Hi Everybody,
> 
> Haitao reported encountering the following WARN while stress testing SGX
> with the SGX2 series [1] applied:
> 
> ELDU returned 1073741837 (0x4000000d)
> WARNING: CPU: 72 PID: 24407 at arch/x86/kernel/cpu/sgx/encl.c:81 sgx_encl_eldu+0x3cf/0x400
> ...
> Call Trace:
> <TASK>
> ? xa_load+0x6e/0xa0
> __sgx_encl_load_page+0x3d/0x80
> sgx_encl_load_page_in_vma+0x4a/0x60
> sgx_vma_fault+0x7f/0x3b0
> ? sysvec_call_function_single+0x66/0xd0
> ? asm_sysvec_call_function_single+0x12/0x20
> __do_fault+0x39/0x110
> __handle_mm_fault+0x1222/0x15a0
> handle_mm_fault+0xdb/0x2c0
> do_user_addr_fault+0x1d1/0x650
> ? exit_to_user_mode_prepare+0x45/0x170
> exc_page_fault+0x76/0x170
> ? asm_exc_page_fault+0x8/0x30
> asm_exc_page_fault+0x1e/0x30
> ...
> </TASK>
> 
> ENCLS[ELDU] is returning a #GP when attempting to load an enclave
> page from the backing store into the enclave. This is likely because
> of a MAC verification failure.
> 
> Haitao's stress testing involves running two concurrent loops of the SGX
> selftests on a system with 4GB EPC memory. One of the tests is modified
> to reduce the oversubscription heap size:
> 
> diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
> index d480c2dd2858..12008789325b 100644
> --- a/tools/testing/selftests/sgx/main.c
> +++ b/tools/testing/selftests/sgx/main.c
> @@ -398,7 +398,7 @@ TEST_F_TIMEOUT(enclave, unclobbered_vdso_oversubscribed_remove, 900)
>  	 * Create enclave with additional heap that is as big as all
>  	 * available physical SGX memory.
>  	 */
> -	total_mem = get_total_epc_mem();
> +	total_mem = get_total_epc_mem()/16;
>  	ASSERT_NE(total_mem, 0);
>  	TH_LOG("Creating an enclave with %lu bytes heap may take a while ...",
>  	       total_mem);
> 
> If the the test compiled with above snippet is renamed as "test_sgx_small"
> and the original renamed as "test_sgx_large" the two concurrent loops are
> run as follows:
> 
> (for i in $(seq 1 999); do echo "Iteration $i"; sudo ./test_sgx_large; done ) > log.large 2>&1
> (for i in $(seq 1 9999); do echo "Iteration $i"; sudo ./test_sgx_small; done ) > log.small 2>&1
> 
> If the SGX driver is modified to always WARN when ENCLS[ELDU] encounters a #GP
> (see below) then the WARN appears after a few iterations of "test_sgx_large"
> and shows up throughout the testing.
> 
> diff --git a/arch/x86/kernel/cpu/sgx/encls.h b/arch/x86/kernel/cpu/sgx/encls.h
> index 99004b02e2ed..68c1dbc84ed3 100644
> --- a/arch/x86/kernel/cpu/sgx/encls.h
> +++ b/arch/x86/kernel/cpu/sgx/encls.h
> @@ -18,7 +18,7 @@
>  #define ENCLS_WARN(r, name) {						  \
>  	do {								  \
>  		int _r = (r);						  \
> -		WARN_ONCE(_r, "%s returned %d (0x%x)\n", (name), _r, _r); \
> +		WARN(_r, "%s returned %d (0x%x)\n", (name), _r, _r); \
>  	} while (0);							  \
>  }
> 
> I focused on investigating this issue the past two weeks and was able to
> narrow down the cause but not yet fix the issue. Now I am out of ideas and
> would really appreciate if you could provide guidance on how to proceed.
> 
> Here is what I have learned so far:
> * Reverting commit 08999b2489b4 ("x86/sgx: Free backing memory after
>   faulting the enclave page") resolves the issue. With that commit
>   reverted the concurrent selftest loops can run to completion without
>   encountering any WARNs.
> 
> * The issue is also resolved if only the calls (introduced by commit
>   08999b2489b4 ("x86/sgx: Free backing memory after faulting the enclave
>   page") ) to sgx_encl_truncate_backing_page() within __sgx_encl_eldu()
>   are disabled.
> 
> * Two issues with commit 08999b2489b4 ("x86/sgx: Free backing memory
>   after faulting the enclave page") are fixed with patch 1 and 2 in
>   this series. These fixes do not resolve the WARN.
> 
> * There is a potential race between the reclaimer and the page fault
>   handler while interacting with the backing store. This is addressed
>   in patch 3, but it does not resolve the WARN.
> 
> * Lockdep did reveal a locking issue in SGX2 - the EAUG flow should not
>   do direct reclaim since the page fault handler already has mmap_lock
>   that the reclaimer will attempt to obtain. This will be fixed in the
>   next version of SGX2 but that fix does not resolve the WARN.
> 
> * Since ENCLS[ELDU] returns #GP on MAC verification failure a possible
>   cause could be if a problem occurs while loading the page from
>   backing store.
>   sgx_encl_get_backing_page() is used to both allocate backing storage
>   in the ENCLS[EWB] flow as well as to lookup backing storage in the
>   ENCLS[ELDU] flow.
>   As part of the investigation the interface was split to ensure that
>   backing storage is only allocated during the ENCLS[EWB] flow and only
>   lookup occurs during the ENCLS[ELDU] flow. This can be found in
>   patch 4.
>   This change revealed that there are cases during ENCLS[ELDU] flow
>   when the backing page is not present - attempting to lookup a
>   backing page returns -ENOENT. Before patch 4 a new backing page
>   would be allocated and thus trigger a #GP when ENCLS[ELDU] is
>   attempted.
>   This change is not a fix, the code path just fails earlier now, but
>   it reveals a cause of the WARNs encountered (MAC verification will
>   fail on a newly allocated page) but not why the pages cannot be found
>   in the backing store. With this change the number of WARNs are
>   reduced significantly but not completely. Even with this patch
>   applied the WARN was encountered once while running the stress
>   selftests.
> 
> Could you please advise on how to proceed? Do you perhaps have ideas
> why the backing store could not contain a page when it is loaded back
> during the ENCLS[ELDU] flow?
> There is some interesting text in the comments within shmem_fault()
> that hints that faulting pages should be avoided into holes as they
> are being punched ... but I do not understand those details and
> would really appreciate guidance on how to understand what is going on
> here.
> 
> These SGX2 tests exercise the concurrent operation of reclaimer and page
> fault handler and have a good track record of locating issues. Thanks
> to it we already have:
> 8795359e35bc ("x86/sgx: Silence softlockup detection when releasing large enclaves")
> ac5d272a0ad0 ("x86/sgx: Fix free page accounting")
> While this issue is triggered by the SGX2 tests I cannot find a connection
> with the SGX2 code paths. I have made a few attempts to create an
> equivalent test for SGX1 but have not yet had success to create a SGX1
> test that can trigger this issue.
> 
> These patches are based on v5.18-rc4 with the SGX2 series applied but
> do not touch SGX2 code paths. Patch 1,2, and 3 also apply cleanly to
> v5.18-rc4 while patch 4 has a conflict in arch/x86/kernel/cpu/sgx/encl.h
> due to the SGX2 declarations.
> 
> Your feedback will be greatly appreciated.
> Thank you very much
> 
> Reinette

Hi, sorry for the response lag. I was on sick leave for the 2nd half of
last week. Looking into this.

BR, Jarkko