diff mbox series

[1/2] x86/sgx: Replace section->init_laundry_list with sgx_dirty_page_list

Message ID 20210317235332.362001-1-jarkko.sakkinen@intel.com (mailing list archive)
State New, archived
Headers show
Series [1/2] x86/sgx: Replace section->init_laundry_list with sgx_dirty_page_list | expand

Commit Message

Jarkko Sakkinen March 17, 2021, 11:53 p.m. UTC 
From: Jarkko Sakkinen <jarkko@kernel.org>

During normal runtime, the "ksgxd" daemon behaves like a  version of
kswapd just for SGX.  But, before it starts acting like kswapd, its
first job is to initialize enclave memory.

Currently, the SGX boot code places each enclave page on a
epc_section->init_laundry_list.  Once it starts up, the ksgxd code walks
over that list and populates the actual SGX page allocator.

However, the per-section structures are going away to make way for the SGX
NUMA allocator.  There's also little need to have a per-section structure;
the enclave pages are all treated identically, and they can be placed on
the correct allocator list from metadata stored in the enclave page
(struct sgx_epc_page) itself.

Modify sgx_sanitize_section() to take a single page list instead of taking
a section and deriving the list from there.

Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
Acked-by: Dave Hansen <dave.hansen@linux.intel.com>
---

v5
* Refine the commit message.
* Refine inline comments.
* Encapsulate a sanitization pass into __sgx_sanitize_pages().

v4:
* Open coded sgx_santize_section() to ksgxd().
* Rewrote the commit message.

 arch/x86/kernel/cpu/sgx/main.c | 54 ++++++++++++++++------------------
 arch/x86/kernel/cpu/sgx/sgx.h  |  7 -----
 2 files changed, 25 insertions(+), 36 deletions(-)

Comments

Borislav Petkov March 18, 2021, 5:40 p.m. UTC | #1 
On Thu, Mar 18, 2021 at 01:53:30AM +0200, Jarkko Sakkinen wrote:
> From: Jarkko Sakkinen <jarkko@kernel.org>
> 
> During normal runtime, the "ksgxd" daemon behaves like a  version of
> kswapd just for SGX.  But, before it starts acting like kswapd, its
> first job is to initialize enclave memory.
> 
> Currently, the SGX boot code places each enclave page on a
> epc_section->init_laundry_list.  Once it starts up, the ksgxd code walks
> over that list and populates the actual SGX page allocator.
> 
> However, the per-section structures are going away to make way for the SGX
> NUMA allocator.  There's also little need to have a per-section structure;
> the enclave pages are all treated identically, and they can be placed on
> the correct allocator list from metadata stored in the enclave page
> (struct sgx_epc_page) itself.
> 
> Modify sgx_sanitize_section() to take a single page list instead of taking
> a section and deriving the list from there.
> 
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> Acked-by: Dave Hansen <dave.hansen@linux.intel.com>
> ---
> 
> v5
> * Refine the commit message.
> * Refine inline comments.
> * Encapsulate a sanitization pass into __sgx_sanitize_pages().
> 
> v4:
> * Open coded sgx_santize_section() to ksgxd().
> * Rewrote the commit message.
> 
>  arch/x86/kernel/cpu/sgx/main.c | 54 ++++++++++++++++------------------
>  arch/x86/kernel/cpu/sgx/sgx.h  |  7 -----
>  2 files changed, 25 insertions(+), 36 deletions(-)

So both patches look ok to me but the sgx test case fails on -rc3 with and
without those patches on my box:

./test_sgx 
0x0000000000000000 0x0000000000002000 0x03
0x0000000000002000 0x0000000000001000 0x05
0x0000000000003000 0x0000000000003000 0x03
mmap() failed, errno=1.

Box is:

[    0.138402] smpboot: CPU0: Intel(R) Core(TM) i5-9600K CPU @ 3.70GHz (family: 0x6, model: 0x9e, stepping: 0xc)
[    0.693947] sgx: EPC section 0x80200000-0x85ffffff

And AFAIR that test used to pass there...
Dave Hansen March 18, 2021, 6:34 p.m. UTC | #2 
On 3/18/21 10:40 AM, Borislav Petkov wrote:
> So both patches look ok to me but the sgx test case fails on -rc3 with and
> without those patches on my box:
> 
> ./test_sgx 
> 0x0000000000000000 0x0000000000002000 0x03
> 0x0000000000002000 0x0000000000001000 0x05
> 0x0000000000003000 0x0000000000003000 0x03
> mmap() failed, errno=1.

I usually get that after I reboot.  I have to do this:

	chmod 755 /dev/sgx_enclave
	mount -o remount,exec /dev

BTW, that *IS* going to get tripped over all the time.  The selftest
needs a better error message.  I'll send a patch.
Borislav Petkov March 18, 2021, 7:01 p.m. UTC | #3 
On Thu, Mar 18, 2021 at 11:34:10AM -0700, Dave Hansen wrote:
> I usually get that after I reboot.  I have to do this:
> 
> 	chmod 755 /dev/sgx_enclave
> 	mount -o remount,exec /dev

Yap, that did it:

0x0000000000000000 0x0000000000002000 0x03
0x0000000000002000 0x0000000000001000 0x05
0x0000000000003000 0x0000000000003000 0x03
SUCCESS

> BTW, that *IS* going to get tripped over all the time.

Yap, and I saw it last time and we talked about it and I forgot again.
Conveniently.

> The selftest needs a better error message.  I'll send a patch.

Please do.

Thx.
Jarkko Sakkinen March 18, 2021, 7:28 p.m. UTC | #4 
On Thu, Mar 18, 2021 at 08:01:38PM +0100, Borislav Petkov wrote:
> On Thu, Mar 18, 2021 at 11:34:10AM -0700, Dave Hansen wrote:
> > I usually get that after I reboot.  I have to do this:
> > 
> > 	chmod 755 /dev/sgx_enclave
> > 	mount -o remount,exec /dev
> 
> Yap, that did it:
> 
> 0x0000000000000000 0x0000000000002000 0x03
> 0x0000000000002000 0x0000000000001000 0x05
> 0x0000000000003000 0x0000000000003000 0x03
> SUCCESS
> 
> > BTW, that *IS* going to get tripped over all the time.
> 
> Yap, and I saw it last time and we talked about it and I forgot again.
> Conveniently.
> 
> > The selftest needs a better error message.  I'll send a patch.
> 
> Please do.

Yeah, would make sense. Not longer than two or three weeks ago
I wondered what is wrong with my system, up until I realized that
I forgot to remount :-)

Thanks for taking the patches.

> Thx.

 /Jarkko
diff mbox series

Patch

diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
index 8df81a3ed945..f3a5cd2d27ef 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -26,39 +26,43 @@  static LIST_HEAD(sgx_active_page_list);
 
 static DEFINE_SPINLOCK(sgx_reclaimer_lock);
 
+static LIST_HEAD(sgx_dirty_page_list);
+
 /*
- * Reset dirty EPC pages to uninitialized state. Laundry can be left with SECS
- * pages whose child pages blocked EREMOVE.
+ * Reset post-kexec EPC pages to the uninitialized state. The pages are removed
+ * from the input list, and made available for the page allocator. SECS pages
+ * prepending their children in the input list are left intact.
  */
-static void sgx_sanitize_section(struct sgx_epc_section *section)
+static void __sgx_sanitize_pages(struct list_head *dirty_page_list)
 {
 	struct sgx_epc_page *page;
 	LIST_HEAD(dirty);
 	int ret;
 
-	/* init_laundry_list is thread-local, no need for a lock: */
-	while (!list_empty(&section->init_laundry_list)) {
+	/* dirty_page_list is thread-local, no need for a lock: */
+	while (!list_empty(dirty_page_list)) {
 		if (kthread_should_stop())
 			return;
 
-		/* needed for access to ->page_list: */
-		spin_lock(&section->lock);
-
-		page = list_first_entry(&section->init_laundry_list,
-					struct sgx_epc_page, list);
+		page = list_first_entry(dirty_page_list, struct sgx_epc_page, list);
 
 		ret = __eremove(sgx_get_epc_virt_addr(page));
-		if (!ret)
-			list_move(&page->list, &section->page_list);
-		else
+		if (!ret) {
+			/*
+			 * page is now sanitized.  Make it available via the SGX
+			 * page allocator:
+			 */
+			list_del(&page->list);
+			sgx_free_epc_page(page);
+		} else {
+			/* The page is not yet clean - move to the dirty list. */
 			list_move_tail(&page->list, &dirty);
-
-		spin_unlock(&section->lock);
+		}
 
 		cond_resched();
 	}
 
-	list_splice(&dirty, &section->init_laundry_list);
+	list_splice(&dirty, dirty_page_list);
 }
 
 static bool sgx_reclaimer_age(struct sgx_epc_page *epc_page)
@@ -405,24 +409,17 @@  static bool sgx_should_reclaim(unsigned long watermark)
 
 static int ksgxd(void *p)
 {
-	int i;
-
 	set_freezable();
 
 	/*
 	 * Sanitize pages in order to recover from kexec(). The 2nd pass is
 	 * required for SECS pages, whose child pages blocked EREMOVE.
 	 */
-	for (i = 0; i < sgx_nr_epc_sections; i++)
-		sgx_sanitize_section(&sgx_epc_sections[i]);
-
-	for (i = 0; i < sgx_nr_epc_sections; i++) {
-		sgx_sanitize_section(&sgx_epc_sections[i]);
+	__sgx_sanitize_pages(&sgx_dirty_page_list);
+	__sgx_sanitize_pages(&sgx_dirty_page_list);
 
-		/* Should never happen. */
-		if (!list_empty(&sgx_epc_sections[i].init_laundry_list))
-			WARN(1, "EPC section %d has unsanitized pages.\n", i);
-	}
+	/* sanity check: */
+	WARN_ON(!list_empty(&sgx_dirty_page_list));
 
 	while (!kthread_should_stop()) {
 		if (try_to_freeze())
@@ -637,13 +634,12 @@  static bool __init sgx_setup_epc_section(u64 phys_addr, u64 size,
 	section->phys_addr = phys_addr;
 	spin_lock_init(&section->lock);
 	INIT_LIST_HEAD(&section->page_list);
-	INIT_LIST_HEAD(&section->init_laundry_list);
 
 	for (i = 0; i < nr_pages; i++) {
 		section->pages[i].section = index;
 		section->pages[i].flags = 0;
 		section->pages[i].owner = NULL;
-		list_add_tail(&section->pages[i].list, &section->init_laundry_list);
+		list_add_tail(&section->pages[i].list, &sgx_dirty_page_list);
 	}
 
 	section->free_cnt = nr_pages;
diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h
index 5fa42d143feb..bc8af0428640 100644
--- a/arch/x86/kernel/cpu/sgx/sgx.h
+++ b/arch/x86/kernel/cpu/sgx/sgx.h
@@ -45,13 +45,6 @@  struct sgx_epc_section {
 	spinlock_t lock;
 	struct list_head page_list;
 	unsigned long free_cnt;
-
-	/*
-	 * Pages which need EREMOVE run on them before they can be
-	 * used.  Only safe to be accessed in ksgxd and init code.
-	 * Not protected by locks.
-	 */
-	struct list_head init_laundry_list;
 };
 
 extern struct sgx_epc_section sgx_epc_sections[SGX_MAX_EPC_SECTIONS];