Linux-Sgx Archive on lore.kernel.org
 help / color / Atom feed
From: Dave Hansen <dave.hansen@intel.com>
To: jarkko@kernel.org
Cc: akpm@linux-foundation.org, andriy.shevchenko@linux.intel.com,
	asapek@google.com, bp@alien8.de, cedric.xing@intel.com,
	chenalexchen@google.com, conradparker@google.com,
	cyhanish@google.com, dave.hansen@intel.com,
	haitao.huang@intel.com, jethro@fortanix.com, kai.huang@intel.com,
	kai.svahn@intel.com, kmoy@google.com,
	linux-kernel@vger.kernel.org, linux-sgx@vger.kernel.org,
	ludloff@google.com, luto@kernel.org, mikko.ylinen@intel.com,
	nhorman@redhat.com, npmccallum@redhat.com, puiterwijk@redhat.com,
	rientjes@google.com, sean.j.christopherson@intel.com,
	serge.ayoun@intel.com, tglx@linutronix.de, x86@kernel.org,
	yaozhangx@google.com, Dave Hansen <dave.hansen@linux.intel.com>
Subject: [PATCH] x86/sgx: clarify 'laundry_list' locking
Date: Mon, 16 Nov 2020 14:25:31 -0800
Message-ID: <20201116222531.4834-1-dave.hansen@intel.com> (raw)
In-Reply-To: <20201112220135.165028-6-jarkko@kernel.org>

From: Dave Hansen <dave.hansen@linux.intel.com>

Short Version:

The SGX section->laundry_list structure is effectively thread-local,
but declared next to some shared structures.  Its semantics are clear
as mud.  Fix that.  No functional changes.  Compile tested only.

Long Version:

The SGX hardware keeps per-page metadata.  This can provide things like
permissions, integrity and replay protection.  It also prevents things
like having an enclave page mapped multiple times or shared between
enclaves.

But, that presents a problem for kexec()'d kernels (or any other kernel
that does not run immediately after a hardware reset).  This is because
the last kernel may have been rude and forgotten to reset pages, which
would trigger the the "shared page" sanity check.

To fix this, the SGX code "launders" the pages by running the EREMOVE
instruction on all pages at boot.  This is slow and can take a long
time, so it is performed off in the SGX-specific ksgxd instead of being
synchronous at boot.  The init code hands the list of pages to launder
in a per-SGX-section list: ->laundry_list.  The only code to touch this
list is the init code and ksgxd.  This means that no locking is
necessary for ->laundry_list.

However, a lock is required for section->page_list, which is accessed
while creating enclaves and by ksgxd.  This lock (section->lock is
acquired by ksgxd while also processing ->laundry_list.  It is easy
to confuse the purpose of the locking as being for ->laundry_list
and ->page_list.

Rename ->laundry_list to ->init_laundry_list to make it clear that
this is not normally used at runtime.  Also add some comments
clarifying the locking, and reorganize 'sgx_epc_section' to put 'lock'
near the things it protects.

Note: init_laundry_list is 128 bytes of wasted space at runtime.  It
could theoretically be dynamically allocated and then freed after the
laundering process.  But, I suspect it would take nearly 128 bytes
of extra instructions to do that.

Cc: Jethro Beekman <jethro@fortanix.com>
Cc: Serge Ayoun <serge.ayoun@intel.com>
Cc: Jarkko Sakkinen <jarkko@kernel.org>
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
---

 b/arch/x86/kernel/cpu/sgx/main.c |   14 ++++++++------
 b/arch/x86/kernel/cpu/sgx/sgx.h  |   15 ++++++++++++---
 2 files changed, 20 insertions(+), 9 deletions(-)

diff -puN arch/x86/kernel/cpu/sgx/main.c~sgx-laundry-comments arch/x86/kernel/cpu/sgx/main.c
--- a/arch/x86/kernel/cpu/sgx/main.c~sgx-laundry-comments	2020-11-16 13:55:42.624972349 -0800
+++ b/arch/x86/kernel/cpu/sgx/main.c	2020-11-16 13:58:10.652971980 -0800
@@ -36,13 +36,15 @@ static void sgx_sanitize_section(struct
 	LIST_HEAD(dirty);
 	int ret;
 
-	while (!list_empty(&section->laundry_list)) {
+	/* init_laundry_list is thread-local, no need for a lock: */
+	while (!list_empty(&section->init_laundry_list)) {
 		if (kthread_should_stop())
 			return;
 
+		/* needed for access to ->page_list: */
 		spin_lock(&section->lock);
 
-		page = list_first_entry(&section->laundry_list,
+		page = list_first_entry(&section->init_laundry_list,
 					struct sgx_epc_page, list);
 
 		ret = __eremove(sgx_get_epc_virt_addr(page));
@@ -56,7 +58,7 @@ static void sgx_sanitize_section(struct
 		cond_resched();
 	}
 
-	list_splice(&dirty, &section->laundry_list);
+	list_splice(&dirty, &section->init_laundry_list);
 }
 
 static bool sgx_reclaimer_age(struct sgx_epc_page *epc_page)
@@ -418,7 +420,7 @@ static int ksgxswapd(void *p)
 		sgx_sanitize_section(&sgx_epc_sections[i]);
 
 		/* Should never happen. */
-		if (!list_empty(&sgx_epc_sections[i].laundry_list))
+		if (!list_empty(&sgx_epc_sections[i].init_laundry_list))
 			WARN(1, "EPC section %d has unsanitized pages.\n", i);
 	}
 
@@ -632,13 +634,13 @@ static bool __init sgx_setup_epc_section
 	section->phys_addr = phys_addr;
 	spin_lock_init(&section->lock);
 	INIT_LIST_HEAD(&section->page_list);
-	INIT_LIST_HEAD(&section->laundry_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->laundry_list);
+		list_add_tail(&section->pages[i].list, &section->init_laundry_list);
 	}
 
 	section->free_cnt = nr_pages;
diff -puN arch/x86/kernel/cpu/sgx/sgx.h~sgx-laundry-comments arch/x86/kernel/cpu/sgx/sgx.h
--- a/arch/x86/kernel/cpu/sgx/sgx.h~sgx-laundry-comments	2020-11-16 13:55:42.627972349 -0800
+++ b/arch/x86/kernel/cpu/sgx/sgx.h	2020-11-16 13:55:42.631972349 -0800
@@ -34,15 +34,24 @@ struct sgx_epc_page {
  * physical memory e.g. for memory areas of the each node. This structure is
  * used to store EPC pages for one EPC section and virtual memory area where
  * the pages have been mapped.
+ *
+ * 'lock' must be held before accessing 'page_list' or 'free_cnt'.
  */
 struct sgx_epc_section {
 	unsigned long phys_addr;
 	void *virt_addr;
-	struct list_head page_list;
-	struct list_head laundry_list;
 	struct sgx_epc_page *pages;
-	unsigned long free_cnt;
+
 	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];
_

  reply index

Thread overview: 55+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-12 22:01 [PATCH v41 00/24] Intel SGX foundations Jarkko Sakkinen
2020-11-12 22:01 ` [PATCH v41 01/24] x86/sgx: Add SGX architectural data structures Jarkko Sakkinen
2020-11-12 22:01 ` [PATCH v41 02/24] x86/sgx: Add wrappers for ENCLS functions Jarkko Sakkinen
2020-11-12 22:01 ` [PATCH v41 03/24] x86/cpufeatures: x86/msr: Add Intel SGX hardware bits Jarkko Sakkinen
2020-11-12 22:01 ` [PATCH v41 04/24] x86/cpufeatures: x86/msr: Add Intel SGX Launch Control " Jarkko Sakkinen
2020-11-12 22:01 ` [PATCH v41 05/24] x86/sgx: Initialize metadata for Enclave Page Cache (EPC) sections Jarkko Sakkinen
2020-11-16 22:25   ` Dave Hansen [this message]
2020-11-17 19:29     ` [PATCH] x86/sgx: clarify 'laundry_list' locking Jarkko Sakkinen
2020-11-12 22:01 ` [PATCH v41 06/24] x86/mm: x86/sgx: Signal SIGSEGV with PF_SGX Jarkko Sakkinen
2020-11-12 22:01 ` [PATCH v41 07/24] x86/cpu/intel: Detect SGX support Jarkko Sakkinen
2020-11-12 22:01 ` [PATCH v41 08/24] x86/cpu/intel: Add nosgx kernel parameter Jarkko Sakkinen
2020-11-12 22:01 ` [PATCH v41 09/24] x86/sgx: Add SGX page allocator functions Jarkko Sakkinen
2020-11-12 22:01 ` [PATCH v41 10/24] mm: Add 'mprotect' hook to struct vm_operations_struct Jarkko Sakkinen
2020-11-13 10:25   ` Mel Gorman
2020-11-17 18:16     ` Jarkko Sakkinen
2020-11-15 17:08   ` Dr. Greg
2020-11-15 17:32   ` Matthew Wilcox
2020-11-15 18:36     ` Dave Hansen
2020-11-16 10:09       ` Mel Gorman
2020-11-17 19:15         ` Jarkko Sakkinen
2020-11-12 22:01 ` [PATCH v41 11/24] x86/sgx: Add SGX misc driver interface Jarkko Sakkinen
2020-11-12 22:01 ` [PATCH v41 12/24] x86/sgx: Add SGX_IOC_ENCLAVE_CREATE Jarkko Sakkinen
     [not found]   ` <20201115044044.11040-1-hdanton@sina.com>
2020-11-17 17:35     ` Jarkko Sakkinen
2020-11-12 22:01 ` [PATCH v41 13/24] x86/sgx: Add SGX_IOC_ENCLAVE_ADD_PAGES Jarkko Sakkinen
2020-11-12 22:01 ` [PATCH v41 14/24] x86/sgx: Add SGX_IOC_ENCLAVE_INIT Jarkko Sakkinen
2020-11-12 22:01 ` [PATCH v41 15/24] x86/sgx: Add SGX_IOC_ENCLAVE_PROVISION Jarkko Sakkinen
2020-11-12 22:01 ` [PATCH v41 16/24] x86/vdso: Add support for exception fixup in vDSO functions Jarkko Sakkinen
2020-11-12 22:01 ` [PATCH v41 17/24] x86/fault: Add helper function to sanitize error code Jarkko Sakkinen
2020-11-12 22:01 ` [PATCH v41 18/24] x86/traps: Attempt to fixup exceptions in vDSO before signaling Jarkko Sakkinen
2020-11-12 22:01 ` [PATCH v41 19/24] x86/vdso: Implement a vDSO for Intel SGX enclave call Jarkko Sakkinen
2020-11-17 13:14   ` Borislav Petkov
2020-11-17 19:41     ` Jarkko Sakkinen
2020-11-12 22:01 ` [PATCH v41 20/24] selftests/x86: Add a selftest for SGX Jarkko Sakkinen
2020-11-16 18:19   ` Shuah Khan
2020-11-17 13:22     ` Borislav Petkov
2020-11-17 19:42     ` Jarkko Sakkinen
2020-11-17 17:26   ` Borislav Petkov
2020-11-17 21:27     ` Jarkko Sakkinen
2020-11-17 21:38     ` Jarkko Sakkinen
2020-11-12 22:01 ` [PATCH v41 21/24] x86/sgx: Add a page reclaimer Jarkko Sakkinen
2020-11-12 22:01 ` [PATCH v41 22/24] x86/sgx: Add ptrace() support for the SGX driver Jarkko Sakkinen
2020-11-12 22:01 ` [PATCH v41 23/24] docs: x86/sgx: Document SGX kernel architecture Jarkko Sakkinen
2020-11-12 22:01 ` [PATCH v41 24/24] x86/sgx: Update MAINTAINERS Jarkko Sakkinen
2020-11-16 16:55 ` [PATCH v41 00/24] Intel SGX foundations Borislav Petkov
2020-11-16 17:21   ` Dave Hansen
2020-11-16 17:28     ` Borislav Petkov
2020-11-17 19:20       ` Jarkko Sakkinen
     [not found] ` <20201114084211.5284-1-hdanton@sina.com>
2020-11-16 18:33   ` [PATCH v41 05/24] x86/sgx: Initialize metadata for Enclave Page Cache (EPC) sections Dave Hansen
     [not found] ` <20201115040127.7804-1-hdanton@sina.com>
2020-11-16 21:11   ` [PATCH v41 11/24] x86/sgx: Add SGX misc driver interface Dave Hansen
     [not found] ` <20201114090708.8684-1-hdanton@sina.com>
2020-11-17 18:12   ` [PATCH v41 06/24] x86/mm: x86/sgx: Signal SIGSEGV with PF_SGX Jarkko Sakkinen
     [not found] ` <20201114093256.7800-1-hdanton@sina.com>
2020-11-17 18:14   ` [PATCH v41 09/24] x86/sgx: Add SGX page allocator functions Jarkko Sakkinen
     [not found] ` <20201115030548.1572-1-hdanton@sina.com>
2020-11-17 18:22   ` [PATCH v41 10/24] mm: Add 'mprotect' hook to struct vm_operations_struct Jarkko Sakkinen
2020-12-15  5:38 ` [PATCH v41 00/24] Intel SGX foundations Hui, Chunyang
2020-12-15  5:43 ` Hui, Chunyang
2020-12-15 15:58   ` Jarkko Sakkinen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201116222531.4834-1-dave.hansen@intel.com \
    --to=dave.hansen@intel.com \
    --cc=akpm@linux-foundation.org \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=asapek@google.com \
    --cc=bp@alien8.de \
    --cc=cedric.xing@intel.com \
    --cc=chenalexchen@google.com \
    --cc=conradparker@google.com \
    --cc=cyhanish@google.com \
    --cc=dave.hansen@linux.intel.com \
    --cc=haitao.huang@intel.com \
    --cc=jarkko@kernel.org \
    --cc=jethro@fortanix.com \
    --cc=kai.huang@intel.com \
    --cc=kai.svahn@intel.com \
    --cc=kmoy@google.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-sgx@vger.kernel.org \
    --cc=ludloff@google.com \
    --cc=luto@kernel.org \
    --cc=mikko.ylinen@intel.com \
    --cc=nhorman@redhat.com \
    --cc=npmccallum@redhat.com \
    --cc=puiterwijk@redhat.com \
    --cc=rientjes@google.com \
    --cc=sean.j.christopherson@intel.com \
    --cc=serge.ayoun@intel.com \
    --cc=tglx@linutronix.de \
    --cc=x86@kernel.org \
    --cc=yaozhangx@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Sgx Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-sgx/0 linux-sgx/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-sgx linux-sgx/ https://lore.kernel.org/linux-sgx \
		linux-sgx@vger.kernel.org
	public-inbox-index linux-sgx

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-sgx


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git