[PATCH 0/3] Minor improvements to sgx_init()

[PATCH 0/3] Minor improvements to sgx_init()

[Kai Huang]

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

[PATCH 1/3] x86/sgx: Start the ksgxd() at the end of sgx_init()

[Kai Huang]

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

[PATCH 2/3] x86/sgx: Only run the reclaimer when the native SGX driver is enabled

[Kai Huang]

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

[RESEND PATCH 3/3] x86/sgx: Add xa_store_range() return value check in sgx_setup_epc_section()

[Kai Huang]

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

Re: [RESEND PATCH 3/3] x86/sgx: Add xa_store_range() return value check in sgx_setup_epc_section()

[Jarkko Sakkinen]

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

Re: [RESEND PATCH 3/3] x86/sgx: Add xa_store_range() return value check in sgx_setup_epc_section()

[Huang, Kai]

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

Re: [RESEND PATCH 3/3] x86/sgx: Add xa_store_range() return value check in sgx_setup_epc_section()

[Huang, Kai]

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