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

[Dave Hansen <dave.hansen@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 1519 bytes --]

On 4/29/22 11:50, Reinette Chatre wrote:
> I am not familiar with this area - is the PFN expected to be consistent? Do
> you perhaps have an idea why the PFN of the PCMD page may have changed? This
> is running with this series applied so the ELDU flow would do a lookup of the
> PCMD page and not attempt to allocate a new one.

First of all, cool!  This is really good progress!

It's possible for the PCMD shmem page to be swapped out and swapped back
in.  The pfn would be likely to change in that case.  But, that seems
highly unlikely in that short of a period of time.

I'd dump out two more things:

First, dump out page_pcmd_off, just like you're doing with page_index in
case page_pcmd_off itself is getting botched.  I looked but couldn't
find anything obvious.

Second, dump out the pfn in sgx_encl_truncate_backing_page().  It's
possible that something is getting overly-aggressive and zapping the
PCMD page too early.  That would be easy to explain with that PCMD
locking issue you discovered.  But, your patch should have fixed that issue.

For debugging, could you double-check that the PCMD page *is* empty
around sgx_encl_truncate_backing_page()?  If there's a race here you can
also enlarge the race window by adding an msleep() or a spin loop
somewhere after the memchr_inv().

You could also hold an extra reference on the PCMD page, maybe something
like the attached patch.  That will let you inspect the actual page
after it is *actually* truncated.  There should never be data in the
page there.

[-- Attachment #2: pcmd.diff --]
[-- Type: text/x-patch, Size: 773 bytes --]

diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
index 7c63a1911fae..309ffae43e0f 100644
--- a/arch/x86/kernel/cpu/sgx/encl.c
+++ b/arch/x86/kernel/cpu/sgx/encl.c
@@ -91,15 +91,20 @@ static int __sgx_encl_eldu(struct sgx_encl_page *encl_page,
 	 */
 	pcmd_page_empty = !memchr_inv(pcmd_page, 0, PAGE_SIZE);
 
-	kunmap_atomic(pcmd_page);
 	kunmap_atomic((void *)(unsigned long)pginfo.contents);
 
+	get_page(b.pcmd);
 	sgx_encl_put_backing(&b, false);
 
 	sgx_encl_truncate_backing_page(encl, page_index);
 
-	if (pcmd_page_empty)
+	if (pcmd_page_empty) {
 		sgx_encl_truncate_backing_page(encl, PFN_DOWN(page_pcmd_off));
+		WARN_ON(memchr_inv(pcmd_page, 0, PAGE_SIZE);
+	}
+
+	kunmap_atomic(pcmd_page);
+	put_page(pcmd_page);
 
 	return ret;
 }