linux-sgx.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] Minor improvements to sgx_init()
@ 2022-10-03 22:04 Kai Huang
  2022-10-03 22:04 ` [PATCH 1/3] x86/sgx: Start the ksgxd() at the end of sgx_init() Kai Huang
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Kai Huang @ 2022-10-03 22:04 UTC (permalink / raw)
  To: linux-sgx; +Cc: dave.hansen, jarkko, tony.luck, linux-kernel

Hi,

Patch 1 and 2 are the improvements around EPC page reclaimer (as I
mentioned to do couple of weeks ago).  Patch 3 is the RESEND of a
previous patch to add a missing error check.

This series is based on latest Linus's upstream tree, but not
tip/x86/sgx, as the latter doesn't have 2 patches that Dave sent to
Linus for 6.0-rc8.

Kai Huang (3):
  x86/sgx: Start the ksgxd() at the end of sgx_init()
  x86/sgx: Only run the reclaimer when the native SGX driver is enabled
  x86/sgx: Add xa_store_range() return value check in
    sgx_setup_epc_section()

 arch/x86/kernel/cpu/sgx/driver.c | 13 ++++---
 arch/x86/kernel/cpu/sgx/driver.h |  1 +
 arch/x86/kernel/cpu/sgx/main.c   | 58 +++++++++++++++++++-------------
 3 files changed, 44 insertions(+), 28 deletions(-)


base-commit: f3dfe925f9548a4337883926db542ccf4ca55fe1
-- 
2.37.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 1/3] x86/sgx: Start the ksgxd() at the end of sgx_init()
  2022-10-03 22:04 [PATCH 0/3] Minor improvements to sgx_init() Kai Huang
@ 2022-10-03 22:04 ` Kai Huang
  2022-10-03 22:04 ` [PATCH 2/3] x86/sgx: Only run the reclaimer when the native SGX driver is enabled Kai Huang
  2022-10-03 22:04 ` [RESEND PATCH 3/3] x86/sgx: Add xa_store_range() return value check in sgx_setup_epc_section() Kai Huang
  2 siblings, 0 replies; 7+ messages in thread
From: Kai Huang @ 2022-10-03 22:04 UTC (permalink / raw)
  To: linux-sgx; +Cc: dave.hansen, jarkko, tony.luck, linux-kernel

The ksgxd() kernel thread basically does two things: 1) sanitize all EPC
pages; 2) start the page reclaimer.  Currently it is created and started
before initializing both the native SGX driver and the KVM driver, but
there's no reason to do that.  It only needs to be started when at least
one of the native and the KVM driver has been initialized.

Move creating and running the ksgxd() to the end of sgx_init() after at
least one of the native and the KVM driver has been initialized.  Also,
when kernel fails to create the ksgxd(), opportunistically improve the
behaviour to not disable SGX completely, but to continue to sanitize EPC
pages and run w/o reclaimer.  This allows SGX to continue to work when
kernel is not running out of EPC (this is especially reasonable for KVM
virtual EPC driver as virtual EPC pages cannot be reclaimed anyway).

With above change, just remove the sgx_page_reclaimer_init() and open
code its logic at the end of sgx_init() as this way is more clear.

Signed-off-by: Kai Huang <kai.huang@intel.com>
---
 arch/x86/kernel/cpu/sgx/main.c | 41 ++++++++++++++++------------------
 1 file changed, 19 insertions(+), 22 deletions(-)

diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
index 0aad028f04d4..713ca09f6d6e 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -420,19 +420,6 @@ static int ksgxd(void *p)
 	return 0;
 }
 
-static bool __init sgx_page_reclaimer_init(void)
-{
-	struct task_struct *tsk;
-
-	tsk = kthread_run(ksgxd, NULL, "ksgxd");
-	if (IS_ERR(tsk))
-		return false;
-
-	ksgxd_tsk = tsk;
-
-	return true;
-}
-
 bool current_is_ksgxd(void)
 {
 	return current == ksgxd_tsk;
@@ -921,14 +908,9 @@ static int __init sgx_init(void)
 	if (!sgx_page_cache_init())
 		return -ENOMEM;
 
-	if (!sgx_page_reclaimer_init()) {
-		ret = -ENOMEM;
-		goto err_page_cache;
-	}
-
 	ret = misc_register(&sgx_dev_provision);
 	if (ret)
-		goto err_kthread;
+		goto err_page_cache;
 
 	/*
 	 * Always try to initialize the native *and* KVM drivers.
@@ -943,14 +925,29 @@ static int __init sgx_init(void)
 	if (sgx_vepc_init() && ret)
 		goto err_provision;
 
+	/*
+	 * At least one of the native and the KVM driver has been
+	 * initialized.  Start the ksgxd().
+	 */
+	ksgxd_tsk = kthread_run(ksgxd, NULL, "ksgxd");
+
+	/*
+	 * If unable to create the ksgxd() thread, don't disable
+	 * SGX completely.  Instead, continue to sanitize all EPC
+	 * pages and run w/o reclaimer.
+	 */
+	if (IS_ERR(ksgxd_tsk)) {
+		ksgxd_tsk = NULL;
+		__sgx_sanitize_pages(&sgx_dirty_page_list);
+		WARN_ON(__sgx_sanitize_pages(&sgx_dirty_page_list));
+		pr_info("Running SGX w/o EPC page reclaimer.\n");
+	}
+
 	return 0;
 
 err_provision:
 	misc_deregister(&sgx_dev_provision);
 
-err_kthread:
-	kthread_stop(ksgxd_tsk);
-
 err_page_cache:
 	for (i = 0; i < sgx_nr_epc_sections; i++) {
 		vfree(sgx_epc_sections[i].pages);
-- 
2.37.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 2/3] x86/sgx: Only run the reclaimer when the native SGX driver is enabled
  2022-10-03 22:04 [PATCH 0/3] Minor improvements to sgx_init() Kai Huang
  2022-10-03 22:04 ` [PATCH 1/3] x86/sgx: Start the ksgxd() at the end of sgx_init() Kai Huang
@ 2022-10-03 22:04 ` Kai Huang
  2022-10-03 22:04 ` [RESEND PATCH 3/3] x86/sgx: Add xa_store_range() return value check in sgx_setup_epc_section() Kai Huang
  2 siblings, 0 replies; 7+ messages in thread
From: Kai Huang @ 2022-10-03 22:04 UTC (permalink / raw)
  To: linux-sgx; +Cc: dave.hansen, jarkko, tony.luck, linux-kernel

Currently the EPC pages assigned to KVM guests cannot be reclaimed, so
there's no point to run the reclaimer when the native SGX driver is not
enabled.

Add a function to indicate whether the native SGX driver has been
initialized, and in ksgxd(), avoid running the reclaimer when it is
false.

In sgx_drv_init(), move the register of "/dev/sgx_enclave" misc device
before initializing sgx_attributes_reserved_mask (and the other two
masks) so that the new function can just use it to determine whether the
SGX driver has been initialized w/o introducing a new boolean.

Signed-off-by: Kai Huang <kai.huang@intel.com>
---
 arch/x86/kernel/cpu/sgx/driver.c | 13 +++++++++----
 arch/x86/kernel/cpu/sgx/driver.h |  1 +
 arch/x86/kernel/cpu/sgx/main.c   | 11 ++++++++++-
 3 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/cpu/sgx/driver.c b/arch/x86/kernel/cpu/sgx/driver.c
index aa9b8b868867..b5e754632eed 100644
--- a/arch/x86/kernel/cpu/sgx/driver.c
+++ b/arch/x86/kernel/cpu/sgx/driver.c
@@ -160,6 +160,10 @@ int __init sgx_drv_init(void)
 		return -ENODEV;
 	}
 
+	ret = misc_register(&sgx_dev_enclave);
+	if (ret)
+		return ret;
+
 	sgx_misc_reserved_mask = ~ebx | SGX_MISC_RESERVED_MASK;
 
 	cpuid_count(SGX_CPUID, 1, &eax, &ebx, &ecx, &edx);
@@ -172,9 +176,10 @@ int __init sgx_drv_init(void)
 		sgx_xfrm_reserved_mask = ~xfrm_mask;
 	}
 
-	ret = misc_register(&sgx_dev_enclave);
-	if (ret)
-		return ret;
-
 	return 0;
 }
+
+bool sgx_drv_inited(void)
+{
+	return !!sgx_attributes_reserved_mask;
+}
diff --git a/arch/x86/kernel/cpu/sgx/driver.h b/arch/x86/kernel/cpu/sgx/driver.h
index 4eddb4d571ef..159cc066e9cb 100644
--- a/arch/x86/kernel/cpu/sgx/driver.h
+++ b/arch/x86/kernel/cpu/sgx/driver.h
@@ -25,5 +25,6 @@ extern const struct file_operations sgx_provision_fops;
 long sgx_ioctl(struct file *filep, unsigned int cmd, unsigned long arg);
 
 int sgx_drv_init(void);
+bool sgx_drv_inited(void);
 
 #endif /* __ARCH_X86_SGX_DRIVER_H__ */
diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
index 713ca09f6d6e..0fdbc490b0f8 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -403,6 +403,14 @@ static int ksgxd(void *p)
 	__sgx_sanitize_pages(&sgx_dirty_page_list);
 	WARN_ON(__sgx_sanitize_pages(&sgx_dirty_page_list));
 
+	/*
+	 * EPC pages assigned to KVM guests cannot be reclaimed.  There's
+	 * no reason to run the reclaimer if the native SGX driver isn't
+	 * initialized successfully (i.e. on the machines w/o SGX_LC).
+	 */
+	if (!sgx_drv_inited())
+		return 0;
+
 	while (!kthread_should_stop()) {
 		if (try_to_freeze())
 			continue;
@@ -940,7 +948,8 @@ static int __init sgx_init(void)
 		ksgxd_tsk = NULL;
 		__sgx_sanitize_pages(&sgx_dirty_page_list);
 		WARN_ON(__sgx_sanitize_pages(&sgx_dirty_page_list));
-		pr_info("Running SGX w/o EPC page reclaimer.\n");
+		if (sgx_drv_inited())
+			pr_info("Running native SGX driver w/o EPC page reclaimer.\n");
 	}
 
 	return 0;
-- 
2.37.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [RESEND PATCH 3/3] x86/sgx: Add xa_store_range() return value check in sgx_setup_epc_section()
  2022-10-03 22:04 [PATCH 0/3] Minor improvements to sgx_init() Kai Huang
  2022-10-03 22:04 ` [PATCH 1/3] x86/sgx: Start the ksgxd() at the end of sgx_init() Kai Huang
  2022-10-03 22:04 ` [PATCH 2/3] x86/sgx: Only run the reclaimer when the native SGX driver is enabled Kai Huang
@ 2022-10-03 22:04 ` Kai Huang
  2022-10-04 22:21   ` Jarkko Sakkinen
  2 siblings, 1 reply; 7+ messages in thread
From: Kai Huang @ 2022-10-03 22:04 UTC (permalink / raw)
  To: linux-sgx; +Cc: dave.hansen, jarkko, tony.luck, linux-kernel

In sgx_setup_epc_section(), xa_store_range() is called to store EPC
pages' owner section to an Xarray using physical addresses of those EPC
pages as index.  Currently, the return value of xa_store_range() is not
checked, but actually it can fail (i.e. due to -ENOMEM).

Not checking the return value of xa_store_range() would result in the
EPC section being used by SGX driver (and KVM SGX guests), but part or
all of its EPC pages not being handled by the memory failure handling of
EPC page.  Such inconsistency should be avoided, even at the cost that
this section won't be used by the kernel.

Add the missing check of the return value of xa_store_range(), and when
it fails, clean up and fail to initialize the EPC section.

Fixes: 40e0e7843e23 ("x86/sgx: Add infrastructure to identify SGX EPC pages")
Signed-off-by: Kai Huang <kai.huang@intel.com>
---
 arch/x86/kernel/cpu/sgx/main.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
index 0fdbc490b0f8..5ddf9d9296f4 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -630,8 +630,12 @@ static bool __init sgx_setup_epc_section(u64 phys_addr, u64 size,
 	}
 
 	section->phys_addr = phys_addr;
-	xa_store_range(&sgx_epc_address_space, section->phys_addr,
-		       phys_addr + size - 1, section, GFP_KERNEL);
+	if (xa_err(xa_store_range(&sgx_epc_address_space, section->phys_addr,
+		       phys_addr + size - 1, section, GFP_KERNEL))) {
+		vfree(section->pages);
+		memunmap(section->virt_addr);
+		return false;
+	}
 
 	for (i = 0; i < nr_pages; i++) {
 		section->pages[i].section = index;
-- 
2.37.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [RESEND PATCH 3/3] x86/sgx: Add xa_store_range() return value check in sgx_setup_epc_section()
  2022-10-03 22:04 ` [RESEND PATCH 3/3] x86/sgx: Add xa_store_range() return value check in sgx_setup_epc_section() Kai Huang
@ 2022-10-04 22:21   ` Jarkko Sakkinen
  2022-10-04 22:42     ` Huang, Kai
  0 siblings, 1 reply; 7+ messages in thread
From: Jarkko Sakkinen @ 2022-10-04 22:21 UTC (permalink / raw)
  To: Kai Huang, dave.hansen
  Cc: linux-sgx, tony.luck, linux-kernel, reinette.chatre

On Tue, Oct 04, 2022 at 11:04:29AM +1300, Kai Huang wrote:
> In sgx_setup_epc_section(), xa_store_range() is called to store EPC
> pages' owner section to an Xarray using physical addresses of those EPC
> pages as index.  Currently, the return value of xa_store_range() is not
> checked, but actually it can fail (i.e. due to -ENOMEM).
> 
> Not checking the return value of xa_store_range() would result in the
> EPC section being used by SGX driver (and KVM SGX guests), but part or
> all of its EPC pages not being handled by the memory failure handling of
> EPC page.  Such inconsistency should be avoided, even at the cost that
> this section won't be used by the kernel.
> 
> Add the missing check of the return value of xa_store_range(), and when
> it fails, clean up and fail to initialize the EPC section.
> 
> Fixes: 40e0e7843e23 ("x86/sgx: Add infrastructure to identify SGX EPC pages")
> Signed-off-by: Kai Huang <kai.huang@intel.com>

Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

This needs:

Cc: stable@vger.kernel.org # v5.17+

Dave, can you pick this independently of rest of the patch set
(unless ofc you have change suggestions)?

BR, Jarkko

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RESEND PATCH 3/3] x86/sgx: Add xa_store_range() return value check in sgx_setup_epc_section()
  2022-10-04 22:21   ` Jarkko Sakkinen
@ 2022-10-04 22:42     ` Huang, Kai
  2022-11-01  2:41       ` Huang, Kai
  0 siblings, 1 reply; 7+ messages in thread
From: Huang, Kai @ 2022-10-04 22:42 UTC (permalink / raw)
  To: jarkko, dave.hansen; +Cc: linux-sgx, Luck, Tony, Chatre, Reinette, linux-kernel

On Wed, 2022-10-05 at 01:21 +0300, Jarkko Sakkinen wrote:
> On Tue, Oct 04, 2022 at 11:04:29AM +1300, Kai Huang wrote:
> > In sgx_setup_epc_section(), xa_store_range() is called to store EPC
> > pages' owner section to an Xarray using physical addresses of those EPC
> > pages as index.  Currently, the return value of xa_store_range() is not
> > checked, but actually it can fail (i.e. due to -ENOMEM).
> > 
> > Not checking the return value of xa_store_range() would result in the
> > EPC section being used by SGX driver (and KVM SGX guests), but part or
> > all of its EPC pages not being handled by the memory failure handling of
> > EPC page.  Such inconsistency should be avoided, even at the cost that
> > this section won't be used by the kernel.
> > 
> > Add the missing check of the return value of xa_store_range(), and when
> > it fails, clean up and fail to initialize the EPC section.
> > 
> > Fixes: 40e0e7843e23 ("x86/sgx: Add infrastructure to identify SGX EPC pages")
> > Signed-off-by: Kai Huang <kai.huang@intel.com>
> 
> Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
> 
> This needs:
> 
> Cc: stable@vger.kernel.org # v5.17+
> 
> Dave, can you pick this independently of rest of the patch set
> (unless ofc you have change suggestions)?
> 
> BR, Jarkko

Thanks Jarkko.  I will add the "Cc stable" part if I need to send out a new
version.
 
-- 
Thanks,
-Kai



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RESEND PATCH 3/3] x86/sgx: Add xa_store_range() return value check in sgx_setup_epc_section()
  2022-10-04 22:42     ` Huang, Kai
@ 2022-11-01  2:41       ` Huang, Kai
  0 siblings, 0 replies; 7+ messages in thread
From: Huang, Kai @ 2022-11-01  2:41 UTC (permalink / raw)
  To: jarkko, dave.hansen; +Cc: linux-sgx, Luck, Tony, Chatre, Reinette, linux-kernel

On Tue, 2022-10-04 at 22:42 +0000, Huang, Kai wrote:
> On Wed, 2022-10-05 at 01:21 +0300, Jarkko Sakkinen wrote:
> > On Tue, Oct 04, 2022 at 11:04:29AM +1300, Kai Huang wrote:
> > > In sgx_setup_epc_section(), xa_store_range() is called to store EPC
> > > pages' owner section to an Xarray using physical addresses of those EPC
> > > pages as index.  Currently, the return value of xa_store_range() is not
> > > checked, but actually it can fail (i.e. due to -ENOMEM).
> > > 
> > > Not checking the return value of xa_store_range() would result in the
> > > EPC section being used by SGX driver (and KVM SGX guests), but part or
> > > all of its EPC pages not being handled by the memory failure handling of
> > > EPC page.  Such inconsistency should be avoided, even at the cost that
> > > this section won't be used by the kernel.
> > > 
> > > Add the missing check of the return value of xa_store_range(), and when
> > > it fails, clean up and fail to initialize the EPC section.
> > > 
> > > Fixes: 40e0e7843e23 ("x86/sgx: Add infrastructure to identify SGX EPC pages")
> > > Signed-off-by: Kai Huang <kai.huang@intel.com>
> > 
> > Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
> > 
> > This needs:
> > 
> > Cc: stable@vger.kernel.org # v5.17+
> > 
> > Dave, can you pick this independently of rest of the patch set
> > (unless ofc you have change suggestions)?
> > 
> > BR, Jarkko
> 
> Thanks Jarkko.  I will add the "Cc stable" part if I need to send out a new
> version.
>  
> -- 
> Thanks,
> -Kai
> 

Hi Dave,

Is this patch worth to do?  For now this should be more like a theoretical issue
that I just saw when scanning the code.  If it is worth to do I'll send out a
new one with Jarkko's reviewed-by and CC stable tag.

-- 
Thanks,
-Kai



^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2022-11-01  2:41 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-03 22:04 [PATCH 0/3] Minor improvements to sgx_init() Kai Huang
2022-10-03 22:04 ` [PATCH 1/3] x86/sgx: Start the ksgxd() at the end of sgx_init() Kai Huang
2022-10-03 22:04 ` [PATCH 2/3] x86/sgx: Only run the reclaimer when the native SGX driver is enabled Kai Huang
2022-10-03 22:04 ` [RESEND PATCH 3/3] x86/sgx: Add xa_store_range() return value check in sgx_setup_epc_section() Kai Huang
2022-10-04 22:21   ` Jarkko Sakkinen
2022-10-04 22:42     ` Huang, Kai
2022-11-01  2:41       ` Huang, Kai

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).