[PATCH v2 0/6] x86/sgx: A collection of tests and fixes

[PATCH v2 0/6] x86/sgx: A collection of tests and fixes

[Jarkko Sakkinen]

Haitao Huang (2):
  x86/sgx: Handle VA page allocation failure for EAUG on PF.
  selftests/sgx: retry the ioctls returned with EAGAIN

Jarkko Sakkinen (2):
  x86/sgx: Do not consider unsanitized pages an error
  selftests/sgx: Add a bpftrace script for tracking allocation errors

Kristen Carlson Accardi (1):
  selftests/sgx: Ignore OpenSSL 3.0 deprecated functions warning

Vijay Dhanraj (1):
  selftests/sgx: Add SGX selftest augment_via_eaccept_long

 arch/x86/kernel/cpu/sgx/encl.c             |   5 +-
 arch/x86/kernel/cpu/sgx/main.c             |  42 ++++-
 tools/testing/selftests/sgx/alloc-error.bt |   9 +
 tools/testing/selftests/sgx/load.c         |   5 +-
 tools/testing/selftests/sgx/main.c         | 185 ++++++++++++++++++---
 tools/testing/selftests/sgx/main.h         |   3 +-
 tools/testing/selftests/sgx/sigstruct.c    |   6 +
 7 files changed, 218 insertions(+), 37 deletions(-)
 create mode 100644 tools/testing/selftests/sgx/alloc-error.bt

-- 
2.37.2

[PATCH v2 1/6] selftests/sgx: Ignore OpenSSL 3.0 deprecated functions warning

[Jarkko Sakkinen]

From: Kristen Carlson Accardi <kristen@linux.intel.com>

OpenSSL 3.0 deprecates some of the functions used in the SGX
selftests, causing build errors on new distros. For now ignore
the warnings until support for the functions is no longer
available and mark FIXME so that it can be clear this should
be removed at some point.

Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
v2:
- Kept because does not exist in tip/x86/sgx, which is the
  maintainer branch for SGX, and is required for selftests.
---
 tools/testing/selftests/sgx/sigstruct.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/testing/selftests/sgx/sigstruct.c b/tools/testing/selftests/sgx/sigstruct.c
index 50c5ab1aa6fa..a07896a46364 100644
--- a/tools/testing/selftests/sgx/sigstruct.c
+++ b/tools/testing/selftests/sgx/sigstruct.c
@@ -17,6 +17,12 @@
 #include "defines.h"
 #include "main.h"
 
+/*
+ * FIXME: OpenSSL 3.0 has deprecated some functions. For now just ignore
+ * the warnings.
+ */
+#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
+
 struct q1q2_ctx {
 	BN_CTX *bn_ctx;
 	BIGNUM *m;
-- 
2.37.2

[PATCH v2 2/6] x86/sgx: Do not consider unsanitized pages an error

[Jarkko Sakkinen]

In sgx_init(), if misc_register() fails or misc_register() succeeds but
neither sgx_drv_init() nor sgx_vepc_init() succeeds, then ksgxd will be
prematurely stopped. This may leave some unsanitized pages, which does
not matter, because SGX will be disabled for the whole power cycle.

This triggers WARN_ON() because sgx_dirty_page_list ends up being
non-empty, and dumps the call stack:

[    0.268103] sgx: EPC section 0x40200000-0x45f7ffff
[    0.268591] ------------[ cut here ]------------
[    0.268592] WARNING: CPU: 6 PID: 83 at
arch/x86/kernel/cpu/sgx/main.c:401 ksgxd+0x1b7/0x1d0
[    0.268598] Modules linked in:
[    0.268600] CPU: 6 PID: 83 Comm: ksgxd Not tainted 6.0.0-rc2 #382
[    0.268603] Hardware name: Dell Inc. XPS 13 9370/0RMYH9, BIOS 1.21.0
07/06/2022
[    0.268604] RIP: 0010:ksgxd+0x1b7/0x1d0
[    0.268607] Code: ff e9 f2 fe ff ff 48 89 df e8 75 07 0e 00 84 c0 0f
84 c3 fe ff ff 31 ff e8 e6 07 0e 00 84 c0 0f 85 94 fe ff ff e9 af fe ff
ff <0f> 0b e9 7f fe ff ff e8 dd 9c 95 00 66 66 2e 0f 1f 84 00 00 00 00
[    0.268608] RSP: 0000:ffffb6c7404f3ed8 EFLAGS: 00010287
[    0.268610] RAX: ffffb6c740431a10 RBX: ffff8dcd8117b400 RCX:
0000000000000000
[    0.268612] RDX: 0000000080000000 RSI: ffffb6c7404319d0 RDI:
00000000ffffffff
[    0.268613] RBP: ffff8dcd820a4d80 R08: ffff8dcd820a4180 R09:
ffff8dcd820a4180
[    0.268614] R10: 0000000000000000 R11: 0000000000000006 R12:
ffffb6c74006bce0
[    0.268615] R13: ffff8dcd80e63880 R14: ffffffffa8a60f10 R15:
0000000000000000
[    0.268616] FS:  0000000000000000(0000) GS:ffff8dcf25580000(0000)
knlGS:0000000000000000
[    0.268617] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    0.268619] CR2: 0000000000000000 CR3: 0000000213410001 CR4:
00000000003706e0
[    0.268620] DR0: 0000000000000000 DR1: 0000000000000000 DR2:
0000000000000000
[    0.268621] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7:
0000000000000400
[    0.268622] Call Trace:
[    0.268624]  <TASK>
[    0.268627]  ? _raw_spin_lock_irqsave+0x24/0x60
[    0.268632]  ? _raw_spin_unlock_irqrestore+0x23/0x40
[    0.268634]  ? __kthread_parkme+0x36/0x90
[    0.268637]  kthread+0xe5/0x110
[    0.268639]  ? kthread_complete_and_exit+0x20/0x20
[    0.268642]  ret_from_fork+0x1f/0x30
[    0.268647]  </TASK>
[    0.268648] ---[ end trace 0000000000000000 ]---

Ultimately this can crash the kernel, if the following is set:

	/proc/sys/kernel/panic_on_warn

In premature stop, print nothing, as the number is by practical means a
random number. Otherwise, it is an indicator of a bug in the driver, and
therefore print the number of unsanitized pages with pr_err().

Link: https://lore.kernel.org/linux-sgx/20220825051827.246698-1-jarkko@kernel.org/T/#u
Fixes: 51ab30eb2ad4 ("x86/sgx: Replace section->init_laundry_list with sgx_dirty_page_list")
Cc: stable@vger.kernel.org # v5.13+
Reported-by: Paul Menzel <pmenzel@molgen.mpg.de>
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
v6:
- Address Reinette's feedback:
  https://lore.kernel.org/linux-sgx/Yw6%2FiTzSdSw%2FY%2FVO@kernel.org/

v5:
- Add the klog dump and sysctl option to the commit message.

v4:
- Explain expectations for dirty_page_list in the function header, instead
  of an inline comment.
- Improve commit message to explain the conditions better.
- Return the number of pages left dirty to ksgxd() and print warning after
  the 2nd call, if there are any.

v3:
- Remove WARN_ON().
- Tuned comments and the commit message a bit.

v2:
- Replaced WARN_ON() with optional pr_info() inside
  __sgx_sanitize_pages().
- Rewrote the commit message.
- Added the fixes tag.
---
 arch/x86/kernel/cpu/sgx/main.c | 42 ++++++++++++++++++++++++++++------
 1 file changed, 35 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
index 515e2a5f25bb..bcd6b64961bd 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -49,17 +49,20 @@ static LIST_HEAD(sgx_dirty_page_list);
  * 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.
+ *
+ * Contents of the @dirty_page_list must be thread-local, i.e.
+ * not shared by multiple threads.
  */
-static void __sgx_sanitize_pages(struct list_head *dirty_page_list)
+static long __sgx_sanitize_pages(struct list_head *dirty_page_list)
 {
 	struct sgx_epc_page *page;
+	long left_dirty = 0;
 	LIST_HEAD(dirty);
 	int ret;
 
-	/* dirty_page_list is thread-local, no need for a lock: */
 	while (!list_empty(dirty_page_list)) {
 		if (kthread_should_stop())
-			return;
+			return -ECANCELED;
 
 		page = list_first_entry(dirty_page_list, struct sgx_epc_page, list);
 
@@ -92,12 +95,14 @@ static void __sgx_sanitize_pages(struct list_head *dirty_page_list)
 		} else {
 			/* The page is not yet clean - move to the dirty list. */
 			list_move_tail(&page->list, &dirty);
+			left_dirty++;
 		}
 
 		cond_resched();
 	}
 
 	list_splice(&dirty, dirty_page_list);
+	return left_dirty;
 }
 
 static bool sgx_reclaimer_age(struct sgx_epc_page *epc_page)
@@ -388,17 +393,40 @@ void sgx_reclaim_direct(void)
 
 static int ksgxd(void *p)
 {
+	long ret;
+
 	set_freezable();
 
 	/*
 	 * Sanitize pages in order to recover from kexec(). The 2nd pass is
 	 * required for SECS pages, whose child pages blocked EREMOVE.
 	 */
-	__sgx_sanitize_pages(&sgx_dirty_page_list);
-	__sgx_sanitize_pages(&sgx_dirty_page_list);
+	ret = __sgx_sanitize_pages(&sgx_dirty_page_list);
+	if (ret == -ECANCELED)
+		/* kthread stopped */
+		return 0;
 
-	/* sanity check: */
-	WARN_ON(!list_empty(&sgx_dirty_page_list));
+	ret = __sgx_sanitize_pages(&sgx_dirty_page_list);
+	switch (ret) {
+	case 0:
+		/* success, no unsanitized pages */
+		break;
+
+	case -ECANCELED:
+		/* kthread stopped */
+		return 0;
+
+	default:
+		/*
+		 * Never expected to happen in a working driver. If it happens
+		 * the bug is expected to be in the sanitization process, but
+		 * successfully sanitized pages are still valid and driver can
+		 * be used and most importantly debugged without issues. To put
+		 * short, the global state of kernel is not corrupted so no
+		 * reason to do any more complicated rollback.
+		 */
+		pr_err("%ld unsanitized pages\n", ret);
+	}
 
 	while (!kthread_should_stop()) {
 		if (try_to_freeze())
-- 
2.37.2

Re: [PATCH v2 2/6] x86/sgx: Do not consider unsanitized pages an error

[Reinette Chatre]

Hi Jarkko,

On 8/31/2022 10:38 AM, Jarkko Sakkinen wrote:
> In sgx_init(), if misc_register() fails or misc_register() succeeds but
> neither sgx_drv_init() nor sgx_vepc_init() succeeds, then ksgxd will be
> prematurely stopped. This may leave some unsanitized pages, which does
> not matter, because SGX will be disabled for the whole power cycle.
> 
> This triggers WARN_ON() because sgx_dirty_page_list ends up being
> non-empty, and dumps the call stack:
> 
> [    0.268103] sgx: EPC section 0x40200000-0x45f7ffff
> [    0.268591] ------------[ cut here ]------------
> [    0.268592] WARNING: CPU: 6 PID: 83 at
> arch/x86/kernel/cpu/sgx/main.c:401 ksgxd+0x1b7/0x1d0
> [    0.268598] Modules linked in:
> [    0.268600] CPU: 6 PID: 83 Comm: ksgxd Not tainted 6.0.0-rc2 #382
> [    0.268603] Hardware name: Dell Inc. XPS 13 9370/0RMYH9, BIOS 1.21.0
> 07/06/2022
> [    0.268604] RIP: 0010:ksgxd+0x1b7/0x1d0
> [    0.268607] Code: ff e9 f2 fe ff ff 48 89 df e8 75 07 0e 00 84 c0 0f
> 84 c3 fe ff ff 31 ff e8 e6 07 0e 00 84 c0 0f 85 94 fe ff ff e9 af fe ff
> ff <0f> 0b e9 7f fe ff ff e8 dd 9c 95 00 66 66 2e 0f 1f 84 00 00 00 00
> [    0.268608] RSP: 0000:ffffb6c7404f3ed8 EFLAGS: 00010287
> [    0.268610] RAX: ffffb6c740431a10 RBX: ffff8dcd8117b400 RCX:
> 0000000000000000
> [    0.268612] RDX: 0000000080000000 RSI: ffffb6c7404319d0 RDI:
> 00000000ffffffff
> [    0.268613] RBP: ffff8dcd820a4d80 R08: ffff8dcd820a4180 R09:
> ffff8dcd820a4180
> [    0.268614] R10: 0000000000000000 R11: 0000000000000006 R12:
> ffffb6c74006bce0
> [    0.268615] R13: ffff8dcd80e63880 R14: ffffffffa8a60f10 R15:
> 0000000000000000
> [    0.268616] FS:  0000000000000000(0000) GS:ffff8dcf25580000(0000)
> knlGS:0000000000000000
> [    0.268617] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [    0.268619] CR2: 0000000000000000 CR3: 0000000213410001 CR4:
> 00000000003706e0
> [    0.268620] DR0: 0000000000000000 DR1: 0000000000000000 DR2:
> 0000000000000000
> [    0.268621] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7:
> 0000000000000400
> [    0.268622] Call Trace:
> [    0.268624]  <TASK>
> [    0.268627]  ? _raw_spin_lock_irqsave+0x24/0x60
> [    0.268632]  ? _raw_spin_unlock_irqrestore+0x23/0x40
> [    0.268634]  ? __kthread_parkme+0x36/0x90
> [    0.268637]  kthread+0xe5/0x110
> [    0.268639]  ? kthread_complete_and_exit+0x20/0x20
> [    0.268642]  ret_from_fork+0x1f/0x30
> [    0.268647]  </TASK>
> [    0.268648] ---[ end trace 0000000000000000 ]---
> 

Are you still planning to trim this?

> Ultimately this can crash the kernel, if the following is set:
> 
> 	/proc/sys/kernel/panic_on_warn
> 
> In premature stop, print nothing, as the number is by practical means a
> random number. Otherwise, it is an indicator of a bug in the driver, and
> therefore print the number of unsanitized pages with pr_err().

I think that "print the number of unsanitized pages with pr_err()" 
contradicts the patch subject of "Do not consider unsanitized pages
an error".

...

> @@ -388,17 +393,40 @@ void sgx_reclaim_direct(void)
>  
>  static int ksgxd(void *p)
>  {
> +	long ret;
> +
>  	set_freezable();
>  
>  	/*
>  	 * Sanitize pages in order to recover from kexec(). The 2nd pass is
>  	 * required for SECS pages, whose child pages blocked EREMOVE.
>  	 */
> -	__sgx_sanitize_pages(&sgx_dirty_page_list);
> -	__sgx_sanitize_pages(&sgx_dirty_page_list);
> +	ret = __sgx_sanitize_pages(&sgx_dirty_page_list);
> +	if (ret == -ECANCELED)
> +		/* kthread stopped */
> +		return 0;
>  
> -	/* sanity check: */
> -	WARN_ON(!list_empty(&sgx_dirty_page_list));
> +	ret = __sgx_sanitize_pages(&sgx_dirty_page_list);
> +	switch (ret) {
> +	case 0:
> +		/* success, no unsanitized pages */
> +		break;
> +
> +	case -ECANCELED:
> +		/* kthread stopped */
> +		return 0;
> +
> +	default:
> +		/*
> +		 * Never expected to happen in a working driver. If it happens
> +		 * the bug is expected to be in the sanitization process, but
> +		 * successfully sanitized pages are still valid and driver can
> +		 * be used and most importantly debugged without issues. To put
> +		 * short, the global state of kernel is not corrupted so no
> +		 * reason to do any more complicated rollback.
> +		 */
> +		pr_err("%ld unsanitized pages\n", ret);
> +	}
>  
>  	while (!kthread_should_stop()) {
>  		if (try_to_freeze())


I think I am missing something here. A lot of logic is added here but I
do not see why it is necessary.  ksgxd() knows via kthread_should_stop() if
the reclaimer was canceled. I am thus wondering, could the above not be
simplified to something similar to V1:

@@ -388,6 +393,8 @@ void sgx_reclaim_direct(void)
 
 static int ksgxd(void *p)
 {
+	unsigned long left_dirty;
+
 	set_freezable();
 
 	/*
@@ -395,10 +402,10 @@ static int ksgxd(void *p)
 	 * required for SECS pages, whose child pages blocked EREMOVE.
 	 */
 	__sgx_sanitize_pages(&sgx_dirty_page_list);
-	__sgx_sanitize_pages(&sgx_dirty_page_list);
 
-	/* sanity check: */
-	WARN_ON(!list_empty(&sgx_dirty_page_list));
+	left_dirty = __sgx_sanitize_pages(&sgx_dirty_page_list);
+	if (left_dirty && !kthread_should_stop())
+		pr_err("%lu unsanitized pages\n", left_dirty);
 
 	while (!kthread_should_stop()) {
 		if (try_to_freeze())


Reinette

Re: [PATCH v2 2/6] x86/sgx: Do not consider unsanitized pages an error

[Huang, Kai]

On Wed, 2022-08-31 at 13:39 -0700, Reinette Chatre wrote:
> >   static int ksgxd(void *p)
> >   {
> > +	long ret;
> > +
> >   	set_freezable();
> >   
> >   	/*
> >   	 * Sanitize pages in order to recover from kexec(). The 2nd pass is
> >   	 * required for SECS pages, whose child pages blocked EREMOVE.
> >   	 */
> > -	__sgx_sanitize_pages(&sgx_dirty_page_list);
> > -	__sgx_sanitize_pages(&sgx_dirty_page_list);
> > +	ret = __sgx_sanitize_pages(&sgx_dirty_page_list);
> > +	if (ret == -ECANCELED)
> > +		/* kthread stopped */
> > +		return 0;
> >   
> > -	/* sanity check: */
> > -	WARN_ON(!list_empty(&sgx_dirty_page_list));
> > +	ret = __sgx_sanitize_pages(&sgx_dirty_page_list);
> > +	switch (ret) {
> > +	case 0:
> > +		/* success, no unsanitized pages */
> > +		break;
> > +
> > +	case -ECANCELED:
> > +		/* kthread stopped */
> > +		return 0;
> > +
> > +	default:
> > +		/*
> > +		 * Never expected to happen in a working driver. If it
> > happens
> > +		 * the bug is expected to be in the sanitization process,
> > but
> > +		 * successfully sanitized pages are still valid and driver
> > can
> > +		 * be used and most importantly debugged without issues. To
> > put
> > +		 * short, the global state of kernel is not corrupted so no
> > +		 * reason to do any more complicated rollback.
> > +		 */
> > +		pr_err("%ld unsanitized pages\n", ret);
> > +	}
> >   
> >   	while (!kthread_should_stop()) {
> >   		if (try_to_freeze())
> 
> 
> I think I am missing something here. A lot of logic is added here but I
> do not see why it is necessary.  ksgxd() knows via kthread_should_stop() if
> the reclaimer was canceled. I am thus wondering, could the above not be
> simplified to something similar to V1:
> 
> @@ -388,6 +393,8 @@ void sgx_reclaim_direct(void)
>  
>  static int ksgxd(void *p)
>  {
> +	unsigned long left_dirty;
> +
>  	set_freezable();
>  
>  	/*
> @@ -395,10 +402,10 @@ static int ksgxd(void *p)
>  	 * required for SECS pages, whose child pages blocked EREMOVE.
>  	 */
>  	__sgx_sanitize_pages(&sgx_dirty_page_list);
> -	__sgx_sanitize_pages(&sgx_dirty_page_list);
>  
> -	/* sanity check: */
> -	WARN_ON(!list_empty(&sgx_dirty_page_list));
> +	left_dirty = __sgx_sanitize_pages(&sgx_dirty_page_list);
> +	if (left_dirty && !kthread_should_stop())
> +		pr_err("%lu unsanitized pages\n", left_dirty);
>  

This basically means driver bug if I understand correctly.  To be consistent
with the behaviour of existing code, how about just WARN()?
	
	...
	left_dirty = __sgx_sanitize_pages(&sgx_dirty_page_list);
	WARN_ON(left_dirty && !kthread_should_stop());

It seems there's little value to print out the unsanitized pages here.  The
existing code doesn't print it anyway.

-- 
Thanks,
-Kai

Re: [PATCH v2 2/6] x86/sgx: Do not consider unsanitized pages an error

[jarkko]

On Thu, Sep 01, 2022 at 10:50:07AM +0000, Huang, Kai wrote:
> On Wed, 2022-08-31 at 13:39 -0700, Reinette Chatre wrote:
> > >   static int ksgxd(void *p)
> > >   {
> > > +	long ret;
> > > +
> > >   	set_freezable();
> > >   
> > >   	/*
> > >   	 * Sanitize pages in order to recover from kexec(). The 2nd pass is
> > >   	 * required for SECS pages, whose child pages blocked EREMOVE.
> > >   	 */
> > > -	__sgx_sanitize_pages(&sgx_dirty_page_list);
> > > -	__sgx_sanitize_pages(&sgx_dirty_page_list);
> > > +	ret = __sgx_sanitize_pages(&sgx_dirty_page_list);
> > > +	if (ret == -ECANCELED)
> > > +		/* kthread stopped */
> > > +		return 0;
> > >   
> > > -	/* sanity check: */
> > > -	WARN_ON(!list_empty(&sgx_dirty_page_list));
> > > +	ret = __sgx_sanitize_pages(&sgx_dirty_page_list);
> > > +	switch (ret) {
> > > +	case 0:
> > > +		/* success, no unsanitized pages */
> > > +		break;
> > > +
> > > +	case -ECANCELED:
> > > +		/* kthread stopped */
> > > +		return 0;
> > > +
> > > +	default:
> > > +		/*
> > > +		 * Never expected to happen in a working driver. If it
> > > happens
> > > +		 * the bug is expected to be in the sanitization process,
> > > but
> > > +		 * successfully sanitized pages are still valid and driver
> > > can
> > > +		 * be used and most importantly debugged without issues. To
> > > put
> > > +		 * short, the global state of kernel is not corrupted so no
> > > +		 * reason to do any more complicated rollback.
> > > +		 */
> > > +		pr_err("%ld unsanitized pages\n", ret);
> > > +	}
> > >   
> > >   	while (!kthread_should_stop()) {
> > >   		if (try_to_freeze())
> > 
> > 
> > I think I am missing something here. A lot of logic is added here but I
> > do not see why it is necessary.  ksgxd() knows via kthread_should_stop() if
> > the reclaimer was canceled. I am thus wondering, could the above not be
> > simplified to something similar to V1:
> > 
> > @@ -388,6 +393,8 @@ void sgx_reclaim_direct(void)
> >  
> >  static int ksgxd(void *p)
> >  {
> > +	unsigned long left_dirty;
> > +
> >  	set_freezable();
> >  
> >  	/*
> > @@ -395,10 +402,10 @@ static int ksgxd(void *p)
> >  	 * required for SECS pages, whose child pages blocked EREMOVE.
> >  	 */
> >  	__sgx_sanitize_pages(&sgx_dirty_page_list);
> > -	__sgx_sanitize_pages(&sgx_dirty_page_list);
> >  
> > -	/* sanity check: */
> > -	WARN_ON(!list_empty(&sgx_dirty_page_list));
> > +	left_dirty = __sgx_sanitize_pages(&sgx_dirty_page_list);
> > +	if (left_dirty && !kthread_should_stop())
> > +		pr_err("%lu unsanitized pages\n", left_dirty);
> >  
> 
> This basically means driver bug if I understand correctly.  To be consistent
> with the behaviour of existing code, how about just WARN()?
> 	
> 	...
> 	left_dirty = __sgx_sanitize_pages(&sgx_dirty_page_list);
> 	WARN_ON(left_dirty && !kthread_should_stop());
> 
> It seems there's little value to print out the unsanitized pages here.  The
> existing code doesn't print it anyway.

Using WARN IMHO here is too strong measure, given that
it tear down the whole kernel, if panic_on_warn is enabled.

For debugging, any information is useful information, so
would not make sense not print the number of pages, if 
that is available. That could very well point out the
issue why all pages are not sanitized if there was a bug.

BR, Jarkko

Re: [PATCH v2 2/6] x86/sgx: Do not consider unsanitized pages an error

[Jarkko Sakkinen]

On Wed, Aug 31, 2022 at 01:39:53PM -0700, Reinette Chatre wrote:
> Hi Jarkko,
> 
> On 8/31/2022 10:38 AM, Jarkko Sakkinen wrote:
> > In sgx_init(), if misc_register() fails or misc_register() succeeds but
> > neither sgx_drv_init() nor sgx_vepc_init() succeeds, then ksgxd will be
> > prematurely stopped. This may leave some unsanitized pages, which does
> > not matter, because SGX will be disabled for the whole power cycle.
> > 
> > This triggers WARN_ON() because sgx_dirty_page_list ends up being
> > non-empty, and dumps the call stack:
> > 
> > [    0.268103] sgx: EPC section 0x40200000-0x45f7ffff
> > [    0.268591] ------------[ cut here ]------------
> > [    0.268592] WARNING: CPU: 6 PID: 83 at
> > arch/x86/kernel/cpu/sgx/main.c:401 ksgxd+0x1b7/0x1d0
> > [    0.268598] Modules linked in:
> > [    0.268600] CPU: 6 PID: 83 Comm: ksgxd Not tainted 6.0.0-rc2 #382
> > [    0.268603] Hardware name: Dell Inc. XPS 13 9370/0RMYH9, BIOS 1.21.0
> > 07/06/2022
> > [    0.268604] RIP: 0010:ksgxd+0x1b7/0x1d0
> > [    0.268607] Code: ff e9 f2 fe ff ff 48 89 df e8 75 07 0e 00 84 c0 0f
> > 84 c3 fe ff ff 31 ff e8 e6 07 0e 00 84 c0 0f 85 94 fe ff ff e9 af fe ff
> > ff <0f> 0b e9 7f fe ff ff e8 dd 9c 95 00 66 66 2e 0f 1f 84 00 00 00 00
> > [    0.268608] RSP: 0000:ffffb6c7404f3ed8 EFLAGS: 00010287
> > [    0.268610] RAX: ffffb6c740431a10 RBX: ffff8dcd8117b400 RCX:
> > 0000000000000000
> > [    0.268612] RDX: 0000000080000000 RSI: ffffb6c7404319d0 RDI:
> > 00000000ffffffff
> > [    0.268613] RBP: ffff8dcd820a4d80 R08: ffff8dcd820a4180 R09:
> > ffff8dcd820a4180
> > [    0.268614] R10: 0000000000000000 R11: 0000000000000006 R12:
> > ffffb6c74006bce0
> > [    0.268615] R13: ffff8dcd80e63880 R14: ffffffffa8a60f10 R15:
> > 0000000000000000
> > [    0.268616] FS:  0000000000000000(0000) GS:ffff8dcf25580000(0000)
> > knlGS:0000000000000000
> > [    0.268617] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > [    0.268619] CR2: 0000000000000000 CR3: 0000000213410001 CR4:
> > 00000000003706e0
> > [    0.268620] DR0: 0000000000000000 DR1: 0000000000000000 DR2:
> > 0000000000000000
> > [    0.268621] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7:
> > 0000000000000400
> > [    0.268622] Call Trace:
> > [    0.268624]  <TASK>
> > [    0.268627]  ? _raw_spin_lock_irqsave+0x24/0x60
> > [    0.268632]  ? _raw_spin_unlock_irqrestore+0x23/0x40
> > [    0.268634]  ? __kthread_parkme+0x36/0x90
> > [    0.268637]  kthread+0xe5/0x110
> > [    0.268639]  ? kthread_complete_and_exit+0x20/0x20
> > [    0.268642]  ret_from_fork+0x1f/0x30
> > [    0.268647]  </TASK>
> > [    0.268648] ---[ end trace 0000000000000000 ]---
> > 
> 
> Are you still planning to trim this?
> 
> > Ultimately this can crash the kernel, if the following is set:
> > 
> > 	/proc/sys/kernel/panic_on_warn
> > 
> > In premature stop, print nothing, as the number is by practical means a
> > random number. Otherwise, it is an indicator of a bug in the driver, and
> > therefore print the number of unsanitized pages with pr_err().
> 
> I think that "print the number of unsanitized pages with pr_err()" 
> contradicts the patch subject of "Do not consider unsanitized pages
> an error".
> 
> ...
> 
> > @@ -388,17 +393,40 @@ void sgx_reclaim_direct(void)
> >  
> >  static int ksgxd(void *p)
> >  {
> > +	long ret;
> > +
> >  	set_freezable();
> >  
> >  	/*
> >  	 * Sanitize pages in order to recover from kexec(). The 2nd pass is
> >  	 * required for SECS pages, whose child pages blocked EREMOVE.
> >  	 */
> > -	__sgx_sanitize_pages(&sgx_dirty_page_list);
> > -	__sgx_sanitize_pages(&sgx_dirty_page_list);
> > +	ret = __sgx_sanitize_pages(&sgx_dirty_page_list);
> > +	if (ret == -ECANCELED)
> > +		/* kthread stopped */
> > +		return 0;
> >  
> > -	/* sanity check: */
> > -	WARN_ON(!list_empty(&sgx_dirty_page_list));
> > +	ret = __sgx_sanitize_pages(&sgx_dirty_page_list);
> > +	switch (ret) {
> > +	case 0:
> > +		/* success, no unsanitized pages */
> > +		break;
> > +
> > +	case -ECANCELED:
> > +		/* kthread stopped */
> > +		return 0;
> > +
> > +	default:
> > +		/*
> > +		 * Never expected to happen in a working driver. If it happens
> > +		 * the bug is expected to be in the sanitization process, but
> > +		 * successfully sanitized pages are still valid and driver can
> > +		 * be used and most importantly debugged without issues. To put
> > +		 * short, the global state of kernel is not corrupted so no
> > +		 * reason to do any more complicated rollback.
> > +		 */
> > +		pr_err("%ld unsanitized pages\n", ret);
> > +	}
> >  
> >  	while (!kthread_should_stop()) {
> >  		if (try_to_freeze())
> 
> 
> I think I am missing something here. A lot of logic is added here but I
> do not see why it is necessary.  ksgxd() knows via kthread_should_stop() if
> the reclaimer was canceled. I am thus wondering, could the above not be
> simplified to something similar to V1:
> 
> @@ -388,6 +393,8 @@ void sgx_reclaim_direct(void)
>  
>  static int ksgxd(void *p)
>  {
> +	unsigned long left_dirty;
> +
>  	set_freezable();
>  
>  	/*
> @@ -395,10 +402,10 @@ static int ksgxd(void *p)
>  	 * required for SECS pages, whose child pages blocked EREMOVE.
>  	 */
>  	__sgx_sanitize_pages(&sgx_dirty_page_list);

IMHO, would make sense also to have here:

        if (!kthread_should_stop())
                return 0;

> -	__sgx_sanitize_pages(&sgx_dirty_page_list);
>  
> -	/* sanity check: */
> -	WARN_ON(!list_empty(&sgx_dirty_page_list));
> +	left_dirty = __sgx_sanitize_pages(&sgx_dirty_page_list);
> +	if (left_dirty && !kthread_should_stop())
> +		pr_err("%lu unsanitized pages\n", left_dirty);

That would be incorrect, if the function returned
because of kthread stopped.

If you do the check here you already have a window
where kthread could have been stopped anyhow.

So even this would be less correct:

        if (kthreas_should_stop()) {
                return 0;
        }  else if (left_dirty) {
                pr_err("%lu unsanitized pages\n", left_dirty);
        }

So in the end you end as complicated and less correct
fix. This all is explained in the commit message.

If you unconditionally print error, you don't have
a meaning for the number of unsanitized pags.

BR, Jarkko

Re: [PATCH v2 2/6] x86/sgx: Do not consider unsanitized pages an error

[Jarkko Sakkinen]

On Fri, Sep 02, 2022 at 12:53:55AM +0300, Jarkko Sakkinen wrote:
> On Wed, Aug 31, 2022 at 01:39:53PM -0700, Reinette Chatre wrote:
> > Hi Jarkko,
> > 
> > On 8/31/2022 10:38 AM, Jarkko Sakkinen wrote:
> > > In sgx_init(), if misc_register() fails or misc_register() succeeds but
> > > neither sgx_drv_init() nor sgx_vepc_init() succeeds, then ksgxd will be
> > > prematurely stopped. This may leave some unsanitized pages, which does
> > > not matter, because SGX will be disabled for the whole power cycle.
> > > 
> > > This triggers WARN_ON() because sgx_dirty_page_list ends up being
> > > non-empty, and dumps the call stack:
> > > 
> > > [    0.268103] sgx: EPC section 0x40200000-0x45f7ffff
> > > [    0.268591] ------------[ cut here ]------------
> > > [    0.268592] WARNING: CPU: 6 PID: 83 at
> > > arch/x86/kernel/cpu/sgx/main.c:401 ksgxd+0x1b7/0x1d0
> > > [    0.268598] Modules linked in:
> > > [    0.268600] CPU: 6 PID: 83 Comm: ksgxd Not tainted 6.0.0-rc2 #382
> > > [    0.268603] Hardware name: Dell Inc. XPS 13 9370/0RMYH9, BIOS 1.21.0
> > > 07/06/2022
> > > [    0.268604] RIP: 0010:ksgxd+0x1b7/0x1d0
> > > [    0.268607] Code: ff e9 f2 fe ff ff 48 89 df e8 75 07 0e 00 84 c0 0f
> > > 84 c3 fe ff ff 31 ff e8 e6 07 0e 00 84 c0 0f 85 94 fe ff ff e9 af fe ff
> > > ff <0f> 0b e9 7f fe ff ff e8 dd 9c 95 00 66 66 2e 0f 1f 84 00 00 00 00
> > > [    0.268608] RSP: 0000:ffffb6c7404f3ed8 EFLAGS: 00010287
> > > [    0.268610] RAX: ffffb6c740431a10 RBX: ffff8dcd8117b400 RCX:
> > > 0000000000000000
> > > [    0.268612] RDX: 0000000080000000 RSI: ffffb6c7404319d0 RDI:
> > > 00000000ffffffff
> > > [    0.268613] RBP: ffff8dcd820a4d80 R08: ffff8dcd820a4180 R09:
> > > ffff8dcd820a4180
> > > [    0.268614] R10: 0000000000000000 R11: 0000000000000006 R12:
> > > ffffb6c74006bce0
> > > [    0.268615] R13: ffff8dcd80e63880 R14: ffffffffa8a60f10 R15:
> > > 0000000000000000
> > > [    0.268616] FS:  0000000000000000(0000) GS:ffff8dcf25580000(0000)
> > > knlGS:0000000000000000
> > > [    0.268617] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > [    0.268619] CR2: 0000000000000000 CR3: 0000000213410001 CR4:
> > > 00000000003706e0
> > > [    0.268620] DR0: 0000000000000000 DR1: 0000000000000000 DR2:
> > > 0000000000000000
> > > [    0.268621] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7:
> > > 0000000000000400
> > > [    0.268622] Call Trace:
> > > [    0.268624]  <TASK>
> > > [    0.268627]  ? _raw_spin_lock_irqsave+0x24/0x60
> > > [    0.268632]  ? _raw_spin_unlock_irqrestore+0x23/0x40
> > > [    0.268634]  ? __kthread_parkme+0x36/0x90
> > > [    0.268637]  kthread+0xe5/0x110
> > > [    0.268639]  ? kthread_complete_and_exit+0x20/0x20
> > > [    0.268642]  ret_from_fork+0x1f/0x30
> > > [    0.268647]  </TASK>
> > > [    0.268648] ---[ end trace 0000000000000000 ]---
> > > 
> > 
> > Are you still planning to trim this?
> > 
> > > Ultimately this can crash the kernel, if the following is set:
> > > 
> > > 	/proc/sys/kernel/panic_on_warn
> > > 
> > > In premature stop, print nothing, as the number is by practical means a
> > > random number. Otherwise, it is an indicator of a bug in the driver, and
> > > therefore print the number of unsanitized pages with pr_err().
> > 
> > I think that "print the number of unsanitized pages with pr_err()" 
> > contradicts the patch subject of "Do not consider unsanitized pages
> > an error".
> > 
> > ...
> > 
> > > @@ -388,17 +393,40 @@ void sgx_reclaim_direct(void)
> > >  
> > >  static int ksgxd(void *p)
> > >  {
> > > +	long ret;
> > > +
> > >  	set_freezable();
> > >  
> > >  	/*
> > >  	 * Sanitize pages in order to recover from kexec(). The 2nd pass is
> > >  	 * required for SECS pages, whose child pages blocked EREMOVE.
> > >  	 */
> > > -	__sgx_sanitize_pages(&sgx_dirty_page_list);
> > > -	__sgx_sanitize_pages(&sgx_dirty_page_list);
> > > +	ret = __sgx_sanitize_pages(&sgx_dirty_page_list);
> > > +	if (ret == -ECANCELED)
> > > +		/* kthread stopped */
> > > +		return 0;
> > >  
> > > -	/* sanity check: */
> > > -	WARN_ON(!list_empty(&sgx_dirty_page_list));
> > > +	ret = __sgx_sanitize_pages(&sgx_dirty_page_list);
> > > +	switch (ret) {
> > > +	case 0:
> > > +		/* success, no unsanitized pages */
> > > +		break;
> > > +
> > > +	case -ECANCELED:
> > > +		/* kthread stopped */
> > > +		return 0;
> > > +
> > > +	default:
> > > +		/*
> > > +		 * Never expected to happen in a working driver. If it happens
> > > +		 * the bug is expected to be in the sanitization process, but
> > > +		 * successfully sanitized pages are still valid and driver can
> > > +		 * be used and most importantly debugged without issues. To put
> > > +		 * short, the global state of kernel is not corrupted so no
> > > +		 * reason to do any more complicated rollback.
> > > +		 */
> > > +		pr_err("%ld unsanitized pages\n", ret);
> > > +	}
> > >  
> > >  	while (!kthread_should_stop()) {
> > >  		if (try_to_freeze())
> > 
> > 
> > I think I am missing something here. A lot of logic is added here but I
> > do not see why it is necessary.  ksgxd() knows via kthread_should_stop() if
> > the reclaimer was canceled. I am thus wondering, could the above not be
> > simplified to something similar to V1:
> > 
> > @@ -388,6 +393,8 @@ void sgx_reclaim_direct(void)
> >  
> >  static int ksgxd(void *p)
> >  {
> > +	unsigned long left_dirty;
> > +
> >  	set_freezable();
> >  
> >  	/*
> > @@ -395,10 +402,10 @@ static int ksgxd(void *p)
> >  	 * required for SECS pages, whose child pages blocked EREMOVE.
> >  	 */
> >  	__sgx_sanitize_pages(&sgx_dirty_page_list);
> 
> IMHO, would make sense also to have here:
> 
>         if (!kthread_should_stop())
>                 return 0;
> 
> > -	__sgx_sanitize_pages(&sgx_dirty_page_list);
> >  
> > -	/* sanity check: */
> > -	WARN_ON(!list_empty(&sgx_dirty_page_list));
> > +	left_dirty = __sgx_sanitize_pages(&sgx_dirty_page_list);
> > +	if (left_dirty && !kthread_should_stop())
> > +		pr_err("%lu unsanitized pages\n", left_dirty);
> 
> That would be incorrect, if the function returned
> because of kthread stopped.
> 
> If you do the check here you already have a window
> where kthread could have been stopped anyhow.
> 
> So even this would be less correct:
> 
>         if (kthreas_should_stop()) {
>                 return 0;
>         }  else if (left_dirty) {
>                 pr_err("%lu unsanitized pages\n", left_dirty);
>         }
> 
> So in the end you end as complicated and less correct
> fix. This all is explained in the commit message.
> 
> If you unconditionally print error, you don't have
> a meaning for the number of unsanitized pags.

If you add my long comment explaining the error case, the SLOC
size is almost the same. That takes most space in my patch.

BR, Jarkko

Re: [PATCH v2 2/6] x86/sgx: Do not consider unsanitized pages an error

[Jarkko Sakkinen]

On Fri, Sep 02, 2022 at 12:56:34AM +0300, Jarkko Sakkinen wrote:
> On Fri, Sep 02, 2022 at 12:53:55AM +0300, Jarkko Sakkinen wrote:
> > On Wed, Aug 31, 2022 at 01:39:53PM -0700, Reinette Chatre wrote:
> > > Hi Jarkko,
> > > 
> > > On 8/31/2022 10:38 AM, Jarkko Sakkinen wrote:
> > > > In sgx_init(), if misc_register() fails or misc_register() succeeds but
> > > > neither sgx_drv_init() nor sgx_vepc_init() succeeds, then ksgxd will be
> > > > prematurely stopped. This may leave some unsanitized pages, which does
> > > > not matter, because SGX will be disabled for the whole power cycle.
> > > > 
> > > > This triggers WARN_ON() because sgx_dirty_page_list ends up being
> > > > non-empty, and dumps the call stack:
> > > > 
> > > > [    0.268103] sgx: EPC section 0x40200000-0x45f7ffff
> > > > [    0.268591] ------------[ cut here ]------------
> > > > [    0.268592] WARNING: CPU: 6 PID: 83 at
> > > > arch/x86/kernel/cpu/sgx/main.c:401 ksgxd+0x1b7/0x1d0
> > > > [    0.268598] Modules linked in:
> > > > [    0.268600] CPU: 6 PID: 83 Comm: ksgxd Not tainted 6.0.0-rc2 #382
> > > > [    0.268603] Hardware name: Dell Inc. XPS 13 9370/0RMYH9, BIOS 1.21.0
> > > > 07/06/2022
> > > > [    0.268604] RIP: 0010:ksgxd+0x1b7/0x1d0
> > > > [    0.268607] Code: ff e9 f2 fe ff ff 48 89 df e8 75 07 0e 00 84 c0 0f
> > > > 84 c3 fe ff ff 31 ff e8 e6 07 0e 00 84 c0 0f 85 94 fe ff ff e9 af fe ff
> > > > ff <0f> 0b e9 7f fe ff ff e8 dd 9c 95 00 66 66 2e 0f 1f 84 00 00 00 00
> > > > [    0.268608] RSP: 0000:ffffb6c7404f3ed8 EFLAGS: 00010287
> > > > [    0.268610] RAX: ffffb6c740431a10 RBX: ffff8dcd8117b400 RCX:
> > > > 0000000000000000
> > > > [    0.268612] RDX: 0000000080000000 RSI: ffffb6c7404319d0 RDI:
> > > > 00000000ffffffff
> > > > [    0.268613] RBP: ffff8dcd820a4d80 R08: ffff8dcd820a4180 R09:
> > > > ffff8dcd820a4180
> > > > [    0.268614] R10: 0000000000000000 R11: 0000000000000006 R12:
> > > > ffffb6c74006bce0
> > > > [    0.268615] R13: ffff8dcd80e63880 R14: ffffffffa8a60f10 R15:
> > > > 0000000000000000
> > > > [    0.268616] FS:  0000000000000000(0000) GS:ffff8dcf25580000(0000)
> > > > knlGS:0000000000000000
> > > > [    0.268617] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > > [    0.268619] CR2: 0000000000000000 CR3: 0000000213410001 CR4:
> > > > 00000000003706e0
> > > > [    0.268620] DR0: 0000000000000000 DR1: 0000000000000000 DR2:
> > > > 0000000000000000
> > > > [    0.268621] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7:
> > > > 0000000000000400
> > > > [    0.268622] Call Trace:
> > > > [    0.268624]  <TASK>
> > > > [    0.268627]  ? _raw_spin_lock_irqsave+0x24/0x60
> > > > [    0.268632]  ? _raw_spin_unlock_irqrestore+0x23/0x40
> > > > [    0.268634]  ? __kthread_parkme+0x36/0x90
> > > > [    0.268637]  kthread+0xe5/0x110
> > > > [    0.268639]  ? kthread_complete_and_exit+0x20/0x20
> > > > [    0.268642]  ret_from_fork+0x1f/0x30
> > > > [    0.268647]  </TASK>
> > > > [    0.268648] ---[ end trace 0000000000000000 ]---
> > > > 
> > > 
> > > Are you still planning to trim this?
> > > 
> > > > Ultimately this can crash the kernel, if the following is set:
> > > > 
> > > > 	/proc/sys/kernel/panic_on_warn
> > > > 
> > > > In premature stop, print nothing, as the number is by practical means a
> > > > random number. Otherwise, it is an indicator of a bug in the driver, and
> > > > therefore print the number of unsanitized pages with pr_err().
> > > 
> > > I think that "print the number of unsanitized pages with pr_err()" 
> > > contradicts the patch subject of "Do not consider unsanitized pages
> > > an error".
> > > 
> > > ...
> > > 
> > > > @@ -388,17 +393,40 @@ void sgx_reclaim_direct(void)
> > > >  
> > > >  static int ksgxd(void *p)
> > > >  {
> > > > +	long ret;
> > > > +
> > > >  	set_freezable();
> > > >  
> > > >  	/*
> > > >  	 * Sanitize pages in order to recover from kexec(). The 2nd pass is
> > > >  	 * required for SECS pages, whose child pages blocked EREMOVE.
> > > >  	 */
> > > > -	__sgx_sanitize_pages(&sgx_dirty_page_list);
> > > > -	__sgx_sanitize_pages(&sgx_dirty_page_list);
> > > > +	ret = __sgx_sanitize_pages(&sgx_dirty_page_list);
> > > > +	if (ret == -ECANCELED)
> > > > +		/* kthread stopped */
> > > > +		return 0;
> > > >  
> > > > -	/* sanity check: */
> > > > -	WARN_ON(!list_empty(&sgx_dirty_page_list));
> > > > +	ret = __sgx_sanitize_pages(&sgx_dirty_page_list);
> > > > +	switch (ret) {
> > > > +	case 0:
> > > > +		/* success, no unsanitized pages */
> > > > +		break;
> > > > +
> > > > +	case -ECANCELED:
> > > > +		/* kthread stopped */
> > > > +		return 0;
> > > > +
> > > > +	default:
> > > > +		/*
> > > > +		 * Never expected to happen in a working driver. If it happens
> > > > +		 * the bug is expected to be in the sanitization process, but
> > > > +		 * successfully sanitized pages are still valid and driver can
> > > > +		 * be used and most importantly debugged without issues. To put
> > > > +		 * short, the global state of kernel is not corrupted so no
> > > > +		 * reason to do any more complicated rollback.
> > > > +		 */
> > > > +		pr_err("%ld unsanitized pages\n", ret);
> > > > +	}
> > > >  
> > > >  	while (!kthread_should_stop()) {
> > > >  		if (try_to_freeze())
> > > 
> > > 
> > > I think I am missing something here. A lot of logic is added here but I
> > > do not see why it is necessary.  ksgxd() knows via kthread_should_stop() if
> > > the reclaimer was canceled. I am thus wondering, could the above not be
> > > simplified to something similar to V1:
> > > 
> > > @@ -388,6 +393,8 @@ void sgx_reclaim_direct(void)
> > >  
> > >  static int ksgxd(void *p)
> > >  {
> > > +	unsigned long left_dirty;
> > > +
> > >  	set_freezable();
> > >  
> > >  	/*
> > > @@ -395,10 +402,10 @@ static int ksgxd(void *p)
> > >  	 * required for SECS pages, whose child pages blocked EREMOVE.
> > >  	 */
> > >  	__sgx_sanitize_pages(&sgx_dirty_page_list);
> > 
> > IMHO, would make sense also to have here:
> > 
> >         if (!kthread_should_stop())
> >                 return 0;
> > 
> > > -	__sgx_sanitize_pages(&sgx_dirty_page_list);
> > >  
> > > -	/* sanity check: */
> > > -	WARN_ON(!list_empty(&sgx_dirty_page_list));
> > > +	left_dirty = __sgx_sanitize_pages(&sgx_dirty_page_list);
> > > +	if (left_dirty && !kthread_should_stop())
> > > +		pr_err("%lu unsanitized pages\n", left_dirty);
> > 
> > That would be incorrect, if the function returned
> > because of kthread stopped.
> > 
> > If you do the check here you already have a window
> > where kthread could have been stopped anyhow.
> > 
> > So even this would be less correct:
> > 
> >         if (kthreas_should_stop()) {
> >                 return 0;
> >         }  else if (left_dirty) {
> >                 pr_err("%lu unsanitized pages\n", left_dirty);
> >         }
> > 
> > So in the end you end as complicated and less correct
> > fix. This all is explained in the commit message.
> > 
> > If you unconditionally print error, you don't have
> > a meaning for the number of unsanitized pags.
> 
> If you add my long comment explaining the error case, the SLOC
> size is almost the same. That takes most space in my patch.

Printing pages if the thread was stopped is just making
the bug look different and have less output, it does not
fix anything, except prevent kernel panic.

BR, Jarkko

Re: [PATCH v2 2/6] x86/sgx: Do not consider unsanitized pages an error

[Reinette Chatre]

Hi Jarkko,

On 9/1/2022 2:53 PM, Jarkko Sakkinen wrote:
> On Wed, Aug 31, 2022 at 01:39:53PM -0700, Reinette Chatre wrote:
>> On 8/31/2022 10:38 AM, Jarkko Sakkinen wrote:

>> I think I am missing something here. A lot of logic is added here but I
>> do not see why it is necessary.  ksgxd() knows via kthread_should_stop() if
>> the reclaimer was canceled. I am thus wondering, could the above not be
>> simplified to something similar to V1:
>>
>> @@ -388,6 +393,8 @@ void sgx_reclaim_direct(void)
>>  
>>  static int ksgxd(void *p)
>>  {
>> +	unsigned long left_dirty;
>> +
>>  	set_freezable();
>>  
>>  	/*
>> @@ -395,10 +402,10 @@ static int ksgxd(void *p)
>>  	 * required for SECS pages, whose child pages blocked EREMOVE.
>>  	 */
>>  	__sgx_sanitize_pages(&sgx_dirty_page_list);
> 
> IMHO, would make sense also to have here:
> 
>         if (!kthread_should_stop())
>                 return 0;
> 

Would this not prematurely stop the thread when it should not be?

>> -	__sgx_sanitize_pages(&sgx_dirty_page_list);
>>  
>> -	/* sanity check: */
>> -	WARN_ON(!list_empty(&sgx_dirty_page_list));
>> +	left_dirty = __sgx_sanitize_pages(&sgx_dirty_page_list);
>> +	if (left_dirty && !kthread_should_stop())
>> +		pr_err("%lu unsanitized pages\n", left_dirty);
> 
> That would be incorrect, if the function returned
> because of kthread stopped.


I should have highlighted this but in my example I changed
left_dirty to be "unsigned long" with the intention that the
"return -ECANCELED" is replaced with "return 0".

__sgx_sanitize_pages() returns 0 when it exits because of
kthread stopped.

To elaborate I was thinking about:

+static unsigned long __sgx_sanitize_pages(struct list_head *dirty_page_list)
 {
+	unsigned long left_dirty = 0;
 	struct sgx_epc_page *page;
 	LIST_HEAD(dirty);
 	int ret;
 
-	/* dirty_page_list is thread-local, no need for a lock: */
 	while (!list_empty(dirty_page_list)) {
 		if (kthread_should_stop())
-			return;
+			return 0;
 
 		page = list_first_entry(dirty_page_list, struct sgx_epc_page, list);
 
@@ -92,12 +95,14 @@ static void __sgx_sanitize_pages(struct list_head *dirty_page_list)
 		} else {
 			/* The page is not yet clean - move to the dirty list. */
 			list_move_tail(&page->list, &dirty);
+			left_dirty++;
 		}
 
 		cond_resched();
 	}
 
 	list_splice(&dirty, dirty_page_list);
+	return left_dirty;
 }


and then with what I had in previous email the checks should work:

@@ -388,6 +393,8 @@ void sgx_reclaim_direct(void)
 
 static int ksgxd(void *p)
 {
+	unsigned long left_dirty;
+
 	set_freezable();
 
 	/*
@@ -395,10 +402,10 @@ static int ksgxd(void *p)
 	 * required for SECS pages, whose child pages blocked EREMOVE.
 	 */
 	__sgx_sanitize_pages(&sgx_dirty_page_list);
-	__sgx_sanitize_pages(&sgx_dirty_page_list);
 
-	/* sanity check: */
-	WARN_ON(!list_empty(&sgx_dirty_page_list));
+	left_dirty = __sgx_sanitize_pages(&sgx_dirty_page_list);
+	if (left_dirty && !kthread_should_stop())
+		pr_err("%lu unsanitized pages\n", left_dirty);
 
 	while (!kthread_should_stop()) {
 		if (try_to_freeze())


> 
> If you do the check here you already have a window
> where kthread could have been stopped anyhow.
> 
> So even this would be less correct:
> 
>         if (kthreas_should_stop()) {
>                 return 0;
>         }  else if (left_dirty) {
>                 pr_err("%lu unsanitized pages\n", left_dirty);
>         }
> 
> So in the end you end as complicated and less correct
> fix. This all is explained in the commit message.
> 
> If you unconditionally print error, you don't have
> a meaning for the number of unsanitized pags.

Understood that the goal is to only print the
number of unsanitized pages if ksgxd has not been
stopped prematurely.

Reinette 

Re: [PATCH v2 2/6] x86/sgx: Do not consider unsanitized pages an error

[Jarkko Sakkinen]

On Thu, Sep 01, 2022 at 03:34:10PM -0700, Reinette Chatre wrote:
> Hi Jarkko,
> 
> On 9/1/2022 2:53 PM, Jarkko Sakkinen wrote:
> > On Wed, Aug 31, 2022 at 01:39:53PM -0700, Reinette Chatre wrote:
> >> On 8/31/2022 10:38 AM, Jarkko Sakkinen wrote:
> 
> >> I think I am missing something here. A lot of logic is added here but I
> >> do not see why it is necessary.  ksgxd() knows via kthread_should_stop() if
> >> the reclaimer was canceled. I am thus wondering, could the above not be
> >> simplified to something similar to V1:
> >>
> >> @@ -388,6 +393,8 @@ void sgx_reclaim_direct(void)
> >>  
> >>  static int ksgxd(void *p)
> >>  {
> >> +	unsigned long left_dirty;
> >> +
> >>  	set_freezable();
> >>  
> >>  	/*
> >> @@ -395,10 +402,10 @@ static int ksgxd(void *p)
> >>  	 * required for SECS pages, whose child pages blocked EREMOVE.
> >>  	 */
> >>  	__sgx_sanitize_pages(&sgx_dirty_page_list);
> > 
> > IMHO, would make sense also to have here:
> > 
> >         if (!kthread_should_stop())
> >                 return 0;
> > 
> 
> Would this not prematurely stop the thread when it should not be?
> 
> >> -	__sgx_sanitize_pages(&sgx_dirty_page_list);
> >>  
> >> -	/* sanity check: */
> >> -	WARN_ON(!list_empty(&sgx_dirty_page_list));
> >> +	left_dirty = __sgx_sanitize_pages(&sgx_dirty_page_list);
> >> +	if (left_dirty && !kthread_should_stop())
> >> +		pr_err("%lu unsanitized pages\n", left_dirty);
> > 
> > That would be incorrect, if the function returned
> > because of kthread stopped.
> 
> 
> I should have highlighted this but in my example I changed
> left_dirty to be "unsigned long" with the intention that the
> "return -ECANCELED" is replaced with "return 0".

It wasn't supposed to be, it's an error. Thanks for spotting
that.

> 
> __sgx_sanitize_pages() returns 0 when it exits because of
> kthread stopped.
> 
> To elaborate I was thinking about:
> 
> +static unsigned long __sgx_sanitize_pages(struct list_head *dirty_page_list)
>  {
> +	unsigned long left_dirty = 0;
>  	struct sgx_epc_page *page;
>  	LIST_HEAD(dirty);
>  	int ret;
>  
> -	/* dirty_page_list is thread-local, no need for a lock: */
>  	while (!list_empty(dirty_page_list)) {
>  		if (kthread_should_stop())
> -			return;
> +			return 0;
>  
>  		page = list_first_entry(dirty_page_list, struct sgx_epc_page, list);
>  
> @@ -92,12 +95,14 @@ static void __sgx_sanitize_pages(struct list_head *dirty_page_list)
>  		} else {
>  			/* The page is not yet clean - move to the dirty list. */
>  			list_move_tail(&page->list, &dirty);
> +			left_dirty++;
>  		}
>  
>  		cond_resched();
>  	}
>  
>  	list_splice(&dirty, dirty_page_list);
> +	return left_dirty;
>  }
> 
> 
> and then with what I had in previous email the checks should work:
> 
> @@ -388,6 +393,8 @@ void sgx_reclaim_direct(void)
>  
>  static int ksgxd(void *p)
>  {
> +	unsigned long left_dirty;
> +
>  	set_freezable();
>  
>  	/*
> @@ -395,10 +402,10 @@ static int ksgxd(void *p)
>  	 * required for SECS pages, whose child pages blocked EREMOVE.
>  	 */
>  	__sgx_sanitize_pages(&sgx_dirty_page_list);
> -	__sgx_sanitize_pages(&sgx_dirty_page_list);
>  
> -	/* sanity check: */
> -	WARN_ON(!list_empty(&sgx_dirty_page_list));
> +	left_dirty = __sgx_sanitize_pages(&sgx_dirty_page_list);
> +	if (left_dirty && !kthread_should_stop())
> +		pr_err("%lu unsanitized pages\n", left_dirty);
>  
>  	while (!kthread_should_stop()) {
>  		if (try_to_freeze())
> 
> 
> > 
> > If you do the check here you already have a window
> > where kthread could have been stopped anyhow.
> > 
> > So even this would be less correct:
> > 
> >         if (kthreas_should_stop()) {
> >                 return 0;
> >         }  else if (left_dirty) {
> >                 pr_err("%lu unsanitized pages\n", left_dirty);
> >         }
> > 
> > So in the end you end as complicated and less correct
> > fix. This all is explained in the commit message.
> > 
> > If you unconditionally print error, you don't have
> > a meaning for the number of unsanitized pags.
> 
> Understood that the goal is to only print the
> number of unsanitized pages if ksgxd has not been
> stopped prematurely.

Yeah, and thus give as useful information for sysadmin/developer
as we can.

BR, Jarkko

Re: [PATCH v2 2/6] x86/sgx: Do not consider unsanitized pages an error

[Jarkko Sakkinen]

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

On Fri, Sep 02, 2022 at 02:57:01AM +0300, Jarkko Sakkinen wrote:
> > Understood that the goal is to only print the
> > number of unsanitized pages if ksgxd has not been
> > stopped prematurely.
> 
> Yeah, and thus give as useful information for sysadmin/developer
> as we can.

I figured out how to get best of both worlds. See the
attached patch. I'll send formal series later on.

Keeps the accurancy, just postpones the exit.

BR, Jarkko

[-- Attachment #2: tmp.patch --]
[-- Type: text/plain, Size: 6614 bytes --]

From 31c43c3276667cef0a7f0687d489552f26da877b Mon Sep 17 00:00:00 2001
From: Jarkko Sakkinen <jarkko@kernel.org>
Date: Thu, 25 Aug 2022 08:12:30 +0300
Subject: [PATCH] x86/sgx: Do not consider unsanitized pages an error

In sgx_init(), if misc_register() fails or misc_register() succeeds but
neither sgx_drv_init() nor sgx_vepc_init() succeeds, then ksgxd will be
prematurely stopped. This may leave some unsanitized pages, which does
not matter, because SGX will be disabled for the whole power cycle.

This triggers WARN_ON() because sgx_dirty_page_list ends up being
non-empty, and dumps the call stack:

[    0.268103] sgx: EPC section 0x40200000-0x45f7ffff
[    0.268591] ------------[ cut here ]------------
[    0.268592] WARNING: CPU: 6 PID: 83 at
arch/x86/kernel/cpu/sgx/main.c:401 ksgxd+0x1b7/0x1d0
[    0.268598] Modules linked in:
[    0.268600] CPU: 6 PID: 83 Comm: ksgxd Not tainted 6.0.0-rc2 #382
[    0.268603] Hardware name: Dell Inc. XPS 13 9370/0RMYH9, BIOS 1.21.0
07/06/2022
[    0.268604] RIP: 0010:ksgxd+0x1b7/0x1d0
[    0.268607] Code: ff e9 f2 fe ff ff 48 89 df e8 75 07 0e 00 84 c0 0f
84 c3 fe ff ff 31 ff e8 e6 07 0e 00 84 c0 0f 85 94 fe ff ff e9 af fe ff
ff <0f> 0b e9 7f fe ff ff e8 dd 9c 95 00 66 66 2e 0f 1f 84 00 00 00 00
[    0.268608] RSP: 0000:ffffb6c7404f3ed8 EFLAGS: 00010287
[    0.268610] RAX: ffffb6c740431a10 RBX: ffff8dcd8117b400 RCX:
0000000000000000
[    0.268612] RDX: 0000000080000000 RSI: ffffb6c7404319d0 RDI:
00000000ffffffff
[    0.268613] RBP: ffff8dcd820a4d80 R08: ffff8dcd820a4180 R09:
ffff8dcd820a4180
[    0.268614] R10: 0000000000000000 R11: 0000000000000006 R12:
ffffb6c74006bce0
[    0.268615] R13: ffff8dcd80e63880 R14: ffffffffa8a60f10 R15:
0000000000000000
[    0.268616] FS:  0000000000000000(0000) GS:ffff8dcf25580000(0000)
knlGS:0000000000000000
[    0.268617] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    0.268619] CR2: 0000000000000000 CR3: 0000000213410001 CR4:
00000000003706e0
[    0.268620] DR0: 0000000000000000 DR1: 0000000000000000 DR2:
0000000000000000
[    0.268621] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7:
0000000000000400
[    0.268622] Call Trace:
[    0.268624]  <TASK>
[    0.268627]  ? _raw_spin_lock_irqsave+0x24/0x60
[    0.268632]  ? _raw_spin_unlock_irqrestore+0x23/0x40
[    0.268634]  ? __kthread_parkme+0x36/0x90
[    0.268637]  kthread+0xe5/0x110
[    0.268639]  ? kthread_complete_and_exit+0x20/0x20
[    0.268642]  ret_from_fork+0x1f/0x30
[    0.268647]  </TASK>
[    0.268648] ---[ end trace 0000000000000000 ]---

Ultimately this can crash the kernel, if the following is set:

	/proc/sys/kernel/panic_on_warn

In premature stop, print nothing, as the number is by practical means a
random number. Otherwise, it is an indicator of a bug in the driver, and
therefore print the number of unsanitized pages with pr_err().

Link: https://lore.kernel.org/linux-sgx/20220825051827.246698-1-jarkko@kernel.org/T/#u
Fixes: 51ab30eb2ad4 ("x86/sgx: Replace section->init_laundry_list with sgx_dirty_page_list")
Cc: stable@vger.kernel.org # v5.13+
Reported-by: Paul Menzel <pmenzel@molgen.mpg.de>
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>

v6:
- Address Reinette's feedback:
  https://lore.kernel.org/linux-sgx/Yw6%2FiTzSdSw%2FY%2FVO@kernel.org/

v5:
- Add the klog dump and sysctl option to the commit message.

v4:
- Explain expectations for dirty_page_list in the function header, instead
  of an inline comment.
- Improve commit message to explain the conditions better.
- Return the number of pages left dirty to ksgxd() and print warning after
  the 2nd call, if there are any.

v3:
- Remove WARN_ON().
- Tuned comments and the commit message a bit.

v2:
- Replaced WARN_ON() with optional pr_info() inside
  __sgx_sanitize_pages().
- Rewrote the commit message.
- Added the fixes tag.
---
 arch/x86/kernel/cpu/sgx/main.c | 33 ++++++++++++++++++++++++++-------
 1 file changed, 26 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
index 515e2a5f25bb..f29dcaddd140 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -49,17 +49,23 @@ static LIST_HEAD(sgx_dirty_page_list);
  * 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.
+ *
+ * Contents of the @dirty_page_list must be thread-local, i.e.
+ * not shared by multiple threads.
+ *
+ * Return 0 when sanitization was successful or kthread was stopped, and the
+ * number of unsanitized pages otherwise.
  */
-static void __sgx_sanitize_pages(struct list_head *dirty_page_list)
+static unsigned long __sgx_sanitize_pages(struct list_head *dirty_page_list)
 {
 	struct sgx_epc_page *page;
+	long left_dirty = 0;
 	LIST_HEAD(dirty);
 	int ret;
 
-	/* dirty_page_list is thread-local, no need for a lock: */
 	while (!list_empty(dirty_page_list)) {
 		if (kthread_should_stop())
-			return;
+			return 0;
 
 		page = list_first_entry(dirty_page_list, struct sgx_epc_page, list);
 
@@ -92,12 +98,14 @@ static void __sgx_sanitize_pages(struct list_head *dirty_page_list)
 		} else {
 			/* The page is not yet clean - move to the dirty list. */
 			list_move_tail(&page->list, &dirty);
+			left_dirty++;
 		}
 
 		cond_resched();
 	}
 
 	list_splice(&dirty, dirty_page_list);
+	return left_dirty;
 }
 
 static bool sgx_reclaimer_age(struct sgx_epc_page *epc_page)
@@ -388,17 +396,28 @@ void sgx_reclaim_direct(void)
 
 static int ksgxd(void *p)
 {
+	unsigned long left_dirty;
+
 	set_freezable();
 
 	/*
 	 * Sanitize pages in order to recover from kexec(). The 2nd pass is
 	 * required for SECS pages, whose child pages blocked EREMOVE.
 	 */
-	__sgx_sanitize_pages(&sgx_dirty_page_list);
-	__sgx_sanitize_pages(&sgx_dirty_page_list);
+	left_dirty = __sgx_sanitize_pages(&sgx_dirty_page_list);
+	pr_debug("%ld unsanitized pages\n", left_dirty);
 
-	/* sanity check: */
-	WARN_ON(!list_empty(&sgx_dirty_page_list));
+	left_dirty = __sgx_sanitize_pages(&sgx_dirty_page_list);
+	/*
+	 * Never expected to happen in a working driver. If it happens the bug
+	 * is expected to be in the sanitization process, but successfully
+	 * sanitized pages are still valid and driver can be used and most
+	 * importantly debugged without issues. To put short, the global state
+	 * of kernel is not corrupted so no reason to do any more complicated
+	 * rollback.
+	 */
+	if (ret)
+		pr_err("%ld unsanitized pages\n", left_dirty);
 
 	while (!kthread_should_stop()) {
 		if (try_to_freeze())
-- 
2.37.2

Re: [PATCH v2 2/6] x86/sgx: Do not consider unsanitized pages an error

[Jarkko Sakkinen]

On Fri, Sep 02, 2022 at 04:26:51PM +0300, Jarkko Sakkinen wrote:
> +	if (ret)
> +		pr_err("%ld unsanitized pages\n", left_dirty);

Yeah, I know, should be 'left_dirty'. I just quickly drafted
the patch for the email.

BR, Jarkko

Re: [PATCH v2 2/6] x86/sgx: Do not consider unsanitized pages an error

[Reinette Chatre]

Hi Jarkko,

On 9/2/2022 8:53 AM, Jarkko Sakkinen wrote:
> On Fri, Sep 02, 2022 at 04:26:51PM +0300, Jarkko Sakkinen wrote:
>> +	if (ret)
>> +		pr_err("%ld unsanitized pages\n", left_dirty);
> 
> Yeah, I know, should be 'left_dirty'. I just quickly drafted
> the patch for the email.
> 

No problem - you did mention that it was an informal patch.

(btw ... also watch out for the long local parameter returned
as an unsigned long and the signed vs unsigned printing
format string.) I also continue to recommend that you trim
that backtrace ... this patch is heading to x86 area where
this is required.

Reinette

Re: [PATCH v2 2/6] x86/sgx: Do not consider unsanitized pages an error

[Jarkko Sakkinen]

On Fri, Sep 02, 2022 at 09:08:23AM -0700, Reinette Chatre wrote:
> Hi Jarkko,
> 
> On 9/2/2022 8:53 AM, Jarkko Sakkinen wrote:
> > On Fri, Sep 02, 2022 at 04:26:51PM +0300, Jarkko Sakkinen wrote:
> >> +	if (ret)
> >> +		pr_err("%ld unsanitized pages\n", left_dirty);
> > 
> > Yeah, I know, should be 'left_dirty'. I just quickly drafted
> > the patch for the email.
> > 
> 
> No problem - you did mention that it was an informal patch.
> 
> (btw ... also watch out for the long local parameter returned
> as an unsigned long and the signed vs unsigned printing
> format string.) I also continue to recommend that you trim

Point taken.

> that backtrace ... this patch is heading to x86 area where
> this is required.

Should I just cut the whole stack trace, and leave the
part before it?

BR, Jarkko

Re: [PATCH v2 2/6] x86/sgx: Do not consider unsanitized pages an error

[Reinette Chatre]

Hi Jarkko,

On 9/2/2022 9:30 AM, Jarkko Sakkinen wrote:
> On Fri, Sep 02, 2022 at 09:08:23AM -0700, Reinette Chatre wrote:
>> Hi Jarkko,
>>
>> On 9/2/2022 8:53 AM, Jarkko Sakkinen wrote:
>>> On Fri, Sep 02, 2022 at 04:26:51PM +0300, Jarkko Sakkinen wrote:
>>>> +	if (ret)
>>>> +		pr_err("%ld unsanitized pages\n", left_dirty);
>>>
>>> Yeah, I know, should be 'left_dirty'. I just quickly drafted
>>> the patch for the email.
>>>
>>
>> No problem - you did mention that it was an informal patch.
>>
>> (btw ... also watch out for the long local parameter returned
>> as an unsigned long and the signed vs unsigned printing
>> format string.) I also continue to recommend that you trim
> 
> Point taken.
> 
>> that backtrace ... this patch is heading to x86 area where
>> this is required.
> 
> Should I just cut the whole stack trace, and leave the
> part before it?

The trace is printed because of a WARN_ON() in the code.
I do not think there is anything very helpful in that trace.
I think the only helpful parts are the WARN itself that includes
the line number and then information on which kernel it was
encountered on.

How about something like (please note the FIXME within):

"
Paul reported the following WARN while running kernel vFIXME:
  WARNING: CPU: 6 PID: 83 at arch/x86/kernel/cpu/sgx/main.c:401 ksgxd+0x1b7/0x1d0

This is the WARN_ON() within ksgxd() that is triggered when
sgx_dirty_page_list (the list containing unsanitized pages) is non-empty.

In sgx_init(), if misc_register() fails or misc_register() succeeds but
neither sgx_drv_init() nor sgx_vepc_init() succeeds, then ksgxd will be
prematurely stopped. This may leave some unsanitized pages, which does
not matter, because SGX will be disabled for the whole power cycle.

Ultimately this can crash the kernel, if the following is set:

	/proc/sys/kernel/panic_on_warn

In premature stop, print nothing, as the number is by practical means a
random number. Otherwise, it is an indicator of a bug in the driver, and
therefore print the number of unsanitized pages with pr_err().
"

Reinette

Re: [PATCH v2 2/6] x86/sgx: Do not consider unsanitized pages an error

[Jarkko Sakkinen]

On Fri, Sep 02, 2022 at 10:38:34AM -0700, Reinette Chatre wrote:
> Hi Jarkko,
> 
> On 9/2/2022 9:30 AM, Jarkko Sakkinen wrote:
> > On Fri, Sep 02, 2022 at 09:08:23AM -0700, Reinette Chatre wrote:
> >> Hi Jarkko,
> >>
> >> On 9/2/2022 8:53 AM, Jarkko Sakkinen wrote:
> >>> On Fri, Sep 02, 2022 at 04:26:51PM +0300, Jarkko Sakkinen wrote:
> >>>> +	if (ret)
> >>>> +		pr_err("%ld unsanitized pages\n", left_dirty);
> >>>
> >>> Yeah, I know, should be 'left_dirty'. I just quickly drafted
> >>> the patch for the email.
> >>>
> >>
> >> No problem - you did mention that it was an informal patch.
> >>
> >> (btw ... also watch out for the long local parameter returned
> >> as an unsigned long and the signed vs unsigned printing
> >> format string.) I also continue to recommend that you trim
> > 
> > Point taken.
> > 
> >> that backtrace ... this patch is heading to x86 area where
> >> this is required.
> > 
> > Should I just cut the whole stack trace, and leave the
> > part before it?
> 
> The trace is printed because of a WARN_ON() in the code.
> I do not think there is anything very helpful in that trace.
> I think the only helpful parts are the WARN itself that includes
> the line number and then information on which kernel it was
> encountered on.
> 
> How about something like (please note the FIXME within):
> 
> "
> Paul reported the following WARN while running kernel vFIXME:
>   WARNING: CPU: 6 PID: 83 at arch/x86/kernel/cpu/sgx/main.c:401 ksgxd+0x1b7/0x1d0

Yeah, this is a great idea, the use of WARN() is the whole point.
Thank you.

BR, Jarkko

[PATCH v2 3/6] x86/sgx: Handle VA page allocation failure for EAUG on PF.

[Jarkko Sakkinen]

From: Haitao Huang <haitao.huang@linux.intel.com>

VM_FAULT_NOPAGE is expected behaviour for -EBUSY failure path, when
augmenting a page, as this means that the reclaimer thread has been
triggered, and the intention is just to round-trip in ring-3, and
retry with a new page fault.

Fixes: 5a90d2c3f5ef ("x86/sgx: Support adding of pages to an initialized enclave")
Signed-off-by: Haitao Huang <haitao.huang@linux.intel.com>
Tested-by: Vijay Dhanraj <vijay.dhanraj@intel.com>
Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
v3:
* Added Reinette's ack.

v2:
* Removed reviewed-by, no other changes.
---
 arch/x86/kernel/cpu/sgx/encl.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
index f40d64206ded..c0fd98a1c658 100644
--- a/arch/x86/kernel/cpu/sgx/encl.c
+++ b/arch/x86/kernel/cpu/sgx/encl.c
@@ -347,8 +347,11 @@ static vm_fault_t sgx_encl_eaug_page(struct vm_area_struct *vma,
 	}
 
 	va_page = sgx_encl_grow(encl, false);
-	if (IS_ERR(va_page))
+	if (IS_ERR(va_page)) {
+		if (PTR_ERR(va_page) == -EBUSY)
+			vmret =  VM_FAULT_NOPAGE;
 		goto err_out_epc;
+	}
 
 	if (va_page)
 		list_add(&va_page->list, &encl->va_pages);
-- 
2.37.2

Re: [PATCH v2 3/6] x86/sgx: Handle VA page allocation failure for EAUG on PF.

[Reinette Chatre]

Hi Jarkko,

On 8/31/2022 10:38 AM, Jarkko Sakkinen wrote:
> From: Haitao Huang <haitao.huang@linux.intel.com>
> 
> VM_FAULT_NOPAGE is expected behaviour for -EBUSY failure path, when
> augmenting a page, as this means that the reclaimer thread has been
> triggered, and the intention is just to round-trip in ring-3, and
> retry with a new page fault.
> 
> Fixes: 5a90d2c3f5ef ("x86/sgx: Support adding of pages to an initialized enclave")
> Signed-off-by: Haitao Huang <haitao.huang@linux.intel.com>
> Tested-by: Vijay Dhanraj <vijay.dhanraj@intel.com>
> Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> ---
> v3:
> * Added Reinette's ack.

The ack came with a caveat related to a white space issue that
you did not fix in this version.

Reinette

Re: [PATCH v2 3/6] x86/sgx: Handle VA page allocation failure for EAUG on PF.

[Jarkko Sakkinen]

On Wed, Aug 31, 2022 at 11:08:33AM -0700, Reinette Chatre wrote:
> Hi Jarkko,
> 
> On 8/31/2022 10:38 AM, Jarkko Sakkinen wrote:
> > From: Haitao Huang <haitao.huang@linux.intel.com>
> > 
> > VM_FAULT_NOPAGE is expected behaviour for -EBUSY failure path, when
> > augmenting a page, as this means that the reclaimer thread has been
> > triggered, and the intention is just to round-trip in ring-3, and
> > retry with a new page fault.
> > 
> > Fixes: 5a90d2c3f5ef ("x86/sgx: Support adding of pages to an initialized enclave")
> > Signed-off-by: Haitao Huang <haitao.huang@linux.intel.com>
> > Tested-by: Vijay Dhanraj <vijay.dhanraj@intel.com>
> > Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>
> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > ---
> > v3:
> > * Added Reinette's ack.
> 
> The ack came with a caveat related to a white space issue that
> you did not fix in this version.

Reinette, I'm sorry but I might possibly have some sort of blind
spot here.

I looked at the diff and I could not see any issues, and did not
run into issues with checkpath.

Can you point out what exactly is the whitespace issue?

BR, Jarkko

Re: [PATCH v2 3/6] x86/sgx: Handle VA page allocation failure for EAUG on PF.

[Reinette Chatre]

Hi Jarkko,

On 8/31/2022 11:21 AM, Jarkko Sakkinen wrote:
> 
> Can you point out what exactly is the whitespace issue?

There is an extra space after the "=" in:
       vmret =  VM_FAULT_NOPAGE;

Reinette

Re: [PATCH v2 3/6] x86/sgx: Handle VA page allocation failure for EAUG on PF.

[Jarkko Sakkinen]

On Wed, Aug 31, 2022 at 11:33:26AM -0700, Reinette Chatre wrote:
> Hi Jarkko,
> 
> On 8/31/2022 11:21 AM, Jarkko Sakkinen wrote:
> > 
> > Can you point out what exactly is the whitespace issue?
> 
> There is an extra space after the "=" in:
>        vmret =  VM_FAULT_NOPAGE;

Duh, I did look at it about million times but honestly
did not see it before you pointed it out.

BR, Jarkko

[PATCH v2 4/6] selftests/sgx: Add SGX selftest augment_via_eaccept_long

[Jarkko Sakkinen]

From: Vijay Dhanraj <vijay.dhanraj@intel.com>

Add a new test case which is same as augment_via_eaccept but adds a
larger number of EPC pages to stress test EAUG via EACCEPT.

Signed-off-by: Vijay Dhanraj <vijay.dhanraj@intel.com>
Co-developed-by: Jarkko Sakkinen <jarkko@kernel.org>
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
v3:
- Addressed Reinette's feedback:
  https://lore.kernel.org/linux-sgx/bd5285dd-d6dd-8a46-fca9-728db5e2f369@intel.com/
v2:
- Addressed Reinette's feedback:
  https://lore.kernel.org/linux-sgx/24bd8e42-ff4e-0090-d9e1-cd81e4807f21@intel.com/
---
 tools/testing/selftests/sgx/load.c |   5 +-
 tools/testing/selftests/sgx/main.c | 143 +++++++++++++++++++++++++----
 tools/testing/selftests/sgx/main.h |   3 +-
 3 files changed, 130 insertions(+), 21 deletions(-)

diff --git a/tools/testing/selftests/sgx/load.c b/tools/testing/selftests/sgx/load.c
index 94bdeac1cf04..47b2786d6a77 100644
--- a/tools/testing/selftests/sgx/load.c
+++ b/tools/testing/selftests/sgx/load.c
@@ -171,7 +171,8 @@ uint64_t encl_get_entry(struct encl *encl, const char *symbol)
 	return 0;
 }
 
-bool encl_load(const char *path, struct encl *encl, unsigned long heap_size)
+bool encl_load(const char *path, struct encl *encl, unsigned long heap_size,
+	       unsigned long edmm_size)
 {
 	const char device_path[] = "/dev/sgx_enclave";
 	struct encl_segment *seg;
@@ -300,7 +301,7 @@ bool encl_load(const char *path, struct encl *encl, unsigned long heap_size)
 
 	encl->src_size = encl->segment_tbl[j].offset + encl->segment_tbl[j].size;
 
-	for (encl->encl_size = 4096; encl->encl_size < encl->src_size; )
+	for (encl->encl_size = 4096; encl->encl_size < encl->src_size + edmm_size;)
 		encl->encl_size <<= 1;
 
 	return true;
diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
index 9820b3809c69..c5aa9f323745 100644
--- a/tools/testing/selftests/sgx/main.c
+++ b/tools/testing/selftests/sgx/main.c
@@ -23,6 +23,10 @@
 
 static const uint64_t MAGIC = 0x1122334455667788ULL;
 static const uint64_t MAGIC2 = 0x8877665544332211ULL;
+/* Message-ID: <DM8PR11MB55912A7F47A84EC9913A6352F6999@DM8PR11MB5591.namprd11.prod.outlook.com> */
+static const uint64_t EDMM_SIZE_LONG = 8L * 1024L * 1024L * 1024L;
+static const uint64_t TIMEOUT_LONG = 900; /* seconds */
+
 vdso_sgx_enter_enclave_t vdso_sgx_enter_enclave;
 
 /*
@@ -173,7 +177,8 @@ FIXTURE(enclave) {
 };
 
 static bool setup_test_encl(unsigned long heap_size, struct encl *encl,
-			    struct __test_metadata *_metadata)
+			    struct __test_metadata *_metadata,
+			    unsigned long edmm_size)
 {
 	Elf64_Sym *sgx_enter_enclave_sym = NULL;
 	struct vdso_symtab symtab;
@@ -183,7 +188,7 @@ static bool setup_test_encl(unsigned long heap_size, struct encl *encl,
 	unsigned int i;
 	void *addr;
 
-	if (!encl_load("test_encl.elf", encl, heap_size)) {
+	if (!encl_load("test_encl.elf", encl, heap_size, edmm_size)) {
 		encl_delete(encl);
 		TH_LOG("Failed to load the test enclave.");
 		return false;
@@ -284,7 +289,7 @@ TEST_F(enclave, unclobbered_vdso)
 	struct encl_op_get_from_buf get_op;
 	struct encl_op_put_to_buf put_op;
 
-	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata));
+	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata, 0));
 
 	memset(&self->run, 0, sizeof(self->run));
 	self->run.tcs = self->encl.encl_base;
@@ -357,7 +362,7 @@ TEST_F(enclave, unclobbered_vdso_oversubscribed)
 
 	total_mem = get_total_epc_mem();
 	ASSERT_NE(total_mem, 0);
-	ASSERT_TRUE(setup_test_encl(total_mem, &self->encl, _metadata));
+	ASSERT_TRUE(setup_test_encl(total_mem, &self->encl, _metadata, 0));
 
 	memset(&self->run, 0, sizeof(self->run));
 	self->run.tcs = self->encl.encl_base;
@@ -401,7 +406,7 @@ TEST_F_TIMEOUT(enclave, unclobbered_vdso_oversubscribed_remove, 900)
 	ASSERT_NE(total_mem, 0);
 	TH_LOG("Creating an enclave with %lu bytes heap may take a while ...",
 	       total_mem);
-	ASSERT_TRUE(setup_test_encl(total_mem, &self->encl, _metadata));
+	ASSERT_TRUE(setup_test_encl(total_mem, &self->encl, _metadata, 0));
 
 	/*
 	 * Hardware (SGX2) and kernel support is needed for this test. Start
@@ -506,7 +511,7 @@ TEST_F(enclave, clobbered_vdso)
 	struct encl_op_get_from_buf get_op;
 	struct encl_op_put_to_buf put_op;
 
-	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata));
+	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata, 0));
 
 	memset(&self->run, 0, sizeof(self->run));
 	self->run.tcs = self->encl.encl_base;
@@ -542,7 +547,7 @@ TEST_F(enclave, clobbered_vdso_and_user_function)
 	struct encl_op_get_from_buf get_op;
 	struct encl_op_put_to_buf put_op;
 
-	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata));
+	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata, 0));
 
 	memset(&self->run, 0, sizeof(self->run));
 	self->run.tcs = self->encl.encl_base;
@@ -575,7 +580,7 @@ TEST_F(enclave, tcs_entry)
 {
 	struct encl_op_header op;
 
-	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata));
+	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata, 0));
 
 	memset(&self->run, 0, sizeof(self->run));
 	self->run.tcs = self->encl.encl_base;
@@ -620,7 +625,7 @@ TEST_F(enclave, pte_permissions)
 	unsigned long data_start;
 	int ret;
 
-	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata));
+	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata, 0));
 
 	memset(&self->run, 0, sizeof(self->run));
 	self->run.tcs = self->encl.encl_base;
@@ -722,7 +727,7 @@ TEST_F(enclave, tcs_permissions)
 	struct sgx_enclave_restrict_permissions ioc;
 	int ret, errno_save;
 
-	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata));
+	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata, 0));
 
 	memset(&self->run, 0, sizeof(self->run));
 	self->run.tcs = self->encl.encl_base;
@@ -785,7 +790,7 @@ TEST_F(enclave, epcm_permissions)
 	unsigned long data_start;
 	int ret, errno_save;
 
-	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata));
+	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata, 0));
 
 	memset(&self->run, 0, sizeof(self->run));
 	self->run.tcs = self->encl.encl_base;
@@ -986,7 +991,7 @@ TEST_F(enclave, augment)
 	if (!sgx2_supported())
 		SKIP(return, "SGX2 not supported");
 
-	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata));
+	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata, 0));
 
 	memset(&self->run, 0, sizeof(self->run));
 	self->run.tcs = self->encl.encl_base;
@@ -1116,7 +1121,7 @@ TEST_F(enclave, augment_via_eaccept)
 	if (!sgx2_supported())
 		SKIP(return, "SGX2 not supported");
 
-	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata));
+	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata, 0));
 
 	memset(&self->run, 0, sizeof(self->run));
 	self->run.tcs = self->encl.encl_base;
@@ -1210,6 +1215,108 @@ TEST_F(enclave, augment_via_eaccept)
 	munmap(addr, PAGE_SIZE);
 }
 
+/*
+ * Test for the addition of large number of pages to an initialized enclave via
+ * a pre-emptive run of EACCEPT on every page to be added.
+ */
+TEST_F_TIMEOUT(enclave, augment_via_eaccept_long, TIMEOUT_LONG)
+{
+	struct encl_op_get_from_addr get_addr_op;
+	struct encl_op_put_to_addr put_addr_op;
+	struct encl_op_eaccept eaccept_op;
+	size_t total_size = 0;
+	unsigned long i;
+	void *addr;
+
+	if (!sgx2_supported())
+		SKIP(return, "SGX2 not supported");
+
+	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata,
+				    EDMM_SIZE_LONG));
+
+	memset(&self->run, 0, sizeof(self->run));
+	self->run.tcs = self->encl.encl_base;
+
+	for (i = 0; i < self->encl.nr_segments; i++) {
+		struct encl_segment *seg = &self->encl.segment_tbl[i];
+
+		total_size += seg->size;
+	}
+
+	/*
+	 * mmap() every page at end of existing enclave to be used for
+	 * EDMM.
+	 */
+	addr = mmap((void *)self->encl.encl_base + total_size, EDMM_SIZE_LONG,
+			PROT_READ | PROT_WRITE | PROT_EXEC, MAP_SHARED | MAP_FIXED,
+			self->encl.fd, 0);
+	EXPECT_NE(addr, MAP_FAILED);
+
+	self->run.exception_vector = 0;
+	self->run.exception_error_code = 0;
+	self->run.exception_addr = 0;
+
+	/*
+	 * Run EACCEPT on every page to trigger the #PF->EAUG->EACCEPT(again
+	 * without a #PF). All should be transparent to userspace.
+	 */
+	eaccept_op.flags = SGX_SECINFO_R | SGX_SECINFO_W | SGX_SECINFO_REG | SGX_SECINFO_PENDING;
+	eaccept_op.ret = 0;
+	eaccept_op.header.type = ENCL_OP_EACCEPT;
+
+	for (i = 0; i < EDMM_SIZE_LONG; i += 4096) {
+		eaccept_op.epc_addr = (uint64_t)(addr + i);
+
+		EXPECT_EQ(ENCL_CALL(&eaccept_op, &self->run, true), 0);
+		if (self->run.exception_vector == 14 &&
+			self->run.exception_error_code == 4 &&
+			self->run.exception_addr == self->encl.encl_base) {
+			munmap(addr, EDMM_SIZE_LONG);
+			SKIP(return, "Kernel does not support adding pages to initialized enclave");
+		}
+
+		EXPECT_EQ(self->run.exception_vector, 0);
+		EXPECT_EQ(self->run.exception_error_code, 0);
+		EXPECT_EQ(self->run.exception_addr, 0);
+		ASSERT_EQ(eaccept_op.ret, 0);
+		ASSERT_EQ(self->run.function, EEXIT);
+	}
+
+	/*
+	 * Pool of pages were successfully added to enclave. Perform sanity
+	 * check on first page of the pool only to ensure data can be written
+	 * to and read from a dynamically added enclave page.
+	 */
+	put_addr_op.value = MAGIC;
+	put_addr_op.addr = (unsigned long)addr;
+	put_addr_op.header.type = ENCL_OP_PUT_TO_ADDRESS;
+
+	EXPECT_EQ(ENCL_CALL(&put_addr_op, &self->run, true), 0);
+
+	EXPECT_EEXIT(&self->run);
+	EXPECT_EQ(self->run.exception_vector, 0);
+	EXPECT_EQ(self->run.exception_error_code, 0);
+	EXPECT_EQ(self->run.exception_addr, 0);
+
+	/*
+	 * Read memory from newly added page that was just written to,
+	 * confirming that data previously written (MAGIC) is present.
+	 */
+	get_addr_op.value = 0;
+	get_addr_op.addr = (unsigned long)addr;
+	get_addr_op.header.type = ENCL_OP_GET_FROM_ADDRESS;
+
+	EXPECT_EQ(ENCL_CALL(&get_addr_op, &self->run, true), 0);
+
+	EXPECT_EQ(get_addr_op.value, MAGIC);
+	EXPECT_EEXIT(&self->run);
+	EXPECT_EQ(self->run.exception_vector, 0);
+	EXPECT_EQ(self->run.exception_error_code, 0);
+	EXPECT_EQ(self->run.exception_addr, 0);
+
+	munmap(addr, EDMM_SIZE_LONG);
+}
+
 /*
  * SGX2 page type modification test in two phases:
  * Phase 1:
@@ -1238,7 +1345,7 @@ TEST_F(enclave, tcs_create)
 	int ret, i;
 
 	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl,
-				    _metadata));
+				    _metadata, 0));
 
 	memset(&self->run, 0, sizeof(self->run));
 	self->run.tcs = self->encl.encl_base;
@@ -1568,7 +1675,7 @@ TEST_F(enclave, remove_added_page_no_eaccept)
 	unsigned long data_start;
 	int ret, errno_save;
 
-	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata));
+	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata, 0));
 
 	memset(&self->run, 0, sizeof(self->run));
 	self->run.tcs = self->encl.encl_base;
@@ -1679,7 +1786,7 @@ TEST_F(enclave, remove_added_page_invalid_access)
 	unsigned long data_start;
 	int ret, errno_save;
 
-	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata));
+	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata, 0));
 
 	memset(&self->run, 0, sizeof(self->run));
 	self->run.tcs = self->encl.encl_base;
@@ -1794,7 +1901,7 @@ TEST_F(enclave, remove_added_page_invalid_access_after_eaccept)
 	unsigned long data_start;
 	int ret, errno_save;
 
-	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata));
+	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata, 0));
 
 	memset(&self->run, 0, sizeof(self->run));
 	self->run.tcs = self->encl.encl_base;
@@ -1918,7 +2025,7 @@ TEST_F(enclave, remove_untouched_page)
 	unsigned long data_start;
 	int ret, errno_save;
 
-	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata));
+	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata, 0));
 
 	/*
 	 * Hardware (SGX2) and kernel support is needed for this test. Start
diff --git a/tools/testing/selftests/sgx/main.h b/tools/testing/selftests/sgx/main.h
index fc585be97e2f..5c190e21a5ff 100644
--- a/tools/testing/selftests/sgx/main.h
+++ b/tools/testing/selftests/sgx/main.h
@@ -35,7 +35,8 @@ extern unsigned char sign_key[];
 extern unsigned char sign_key_end[];
 
 void encl_delete(struct encl *ctx);
-bool encl_load(const char *path, struct encl *encl, unsigned long heap_size);
+bool encl_load(const char *path, struct encl *encl, unsigned long heap_size,
+	       unsigned long edmm_size);
 bool encl_measure(struct encl *encl);
 bool encl_build(struct encl *encl);
 uint64_t encl_get_entry(struct encl *encl, const char *symbol);
-- 
2.37.2

Re: [PATCH v2 4/6] selftests/sgx: Add SGX selftest augment_via_eaccept_long

[Reinette Chatre]

Hi Jarkko,

On 8/31/2022 10:38 AM, Jarkko Sakkinen wrote:
> From: Vijay Dhanraj <vijay.dhanraj@intel.com>
> 
> Add a new test case which is same as augment_via_eaccept but adds a
> larger number of EPC pages to stress test EAUG via EACCEPT.
> 
> Signed-off-by: Vijay Dhanraj <vijay.dhanraj@intel.com>
> Co-developed-by: Jarkko Sakkinen <jarkko@kernel.org>
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> ---
> v3:
> - Addressed Reinette's feedback:
>   https://lore.kernel.org/linux-sgx/bd5285dd-d6dd-8a46-fca9-728db5e2f369@intel.com/
> v2:
> - Addressed Reinette's feedback:
>   https://lore.kernel.org/linux-sgx/24bd8e42-ff4e-0090-d9e1-cd81e4807f21@intel.com/
> ---
>  tools/testing/selftests/sgx/load.c |   5 +-
>  tools/testing/selftests/sgx/main.c | 143 +++++++++++++++++++++++++----
>  tools/testing/selftests/sgx/main.h |   3 +-

Is this test passing on your system? This version is missing the change to
mrenclave_ecreate() that causes SGX_IOC_ENCLAVE_INIT to fail when I try it out.

>  3 files changed, 130 insertions(+), 21 deletions(-)
> 
> diff --git a/tools/testing/selftests/sgx/load.c b/tools/testing/selftests/sgx/load.c
> index 94bdeac1cf04..47b2786d6a77 100644
> --- a/tools/testing/selftests/sgx/load.c
> +++ b/tools/testing/selftests/sgx/load.c
> @@ -171,7 +171,8 @@ uint64_t encl_get_entry(struct encl *encl, const char *symbol)
>  	return 0;
>  }
>  
> -bool encl_load(const char *path, struct encl *encl, unsigned long heap_size)
> +bool encl_load(const char *path, struct encl *encl, unsigned long heap_size,
> +	       unsigned long edmm_size)
>  {
>  	const char device_path[] = "/dev/sgx_enclave";
>  	struct encl_segment *seg;
> @@ -300,7 +301,7 @@ bool encl_load(const char *path, struct encl *encl, unsigned long heap_size)
>  
>  	encl->src_size = encl->segment_tbl[j].offset + encl->segment_tbl[j].size;
>  
> -	for (encl->encl_size = 4096; encl->encl_size < encl->src_size; )
> +	for (encl->encl_size = 4096; encl->encl_size < encl->src_size + edmm_size;)
>  		encl->encl_size <<= 1;
>  
>  	return true;
> diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
> index 9820b3809c69..c5aa9f323745 100644
> --- a/tools/testing/selftests/sgx/main.c
> +++ b/tools/testing/selftests/sgx/main.c
> @@ -23,6 +23,10 @@
>  
>  static const uint64_t MAGIC = 0x1122334455667788ULL;
>  static const uint64_t MAGIC2 = 0x8877665544332211ULL;
> +/* Message-ID: <DM8PR11MB55912A7F47A84EC9913A6352F6999@DM8PR11MB5591.namprd11.prod.outlook.com> */
> +static const uint64_t EDMM_SIZE_LONG = 8L * 1024L * 1024L * 1024L;
> +static const uint64_t TIMEOUT_LONG = 900; /* seconds */
> +

Apologies if my feedback was vague - I actually think that the comments in V1 added
valuable information, it was just the variation in formatting that was distracting.


Reinette

Re: [PATCH v2 4/6] selftests/sgx: Add SGX selftest augment_via_eaccept_long

[Jarkko Sakkinen]

On Wed, Aug 31, 2022 at 01:07:35PM -0700, Reinette Chatre wrote:
> Hi Jarkko,
> 
> On 8/31/2022 10:38 AM, Jarkko Sakkinen wrote:
> > From: Vijay Dhanraj <vijay.dhanraj@intel.com>
> > 
> > Add a new test case which is same as augment_via_eaccept but adds a
> > larger number of EPC pages to stress test EAUG via EACCEPT.
> > 
> > Signed-off-by: Vijay Dhanraj <vijay.dhanraj@intel.com>
> > Co-developed-by: Jarkko Sakkinen <jarkko@kernel.org>
> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > ---
> > v3:
> > - Addressed Reinette's feedback:
> >   https://lore.kernel.org/linux-sgx/bd5285dd-d6dd-8a46-fca9-728db5e2f369@intel.com/
> > v2:
> > - Addressed Reinette's feedback:
> >   https://lore.kernel.org/linux-sgx/24bd8e42-ff4e-0090-d9e1-cd81e4807f21@intel.com/
> > ---
> >  tools/testing/selftests/sgx/load.c |   5 +-
> >  tools/testing/selftests/sgx/main.c | 143 +++++++++++++++++++++++++----
> >  tools/testing/selftests/sgx/main.h |   3 +-
> 
> Is this test passing on your system? This version is missing the change to
> mrenclave_ecreate() that causes SGX_IOC_ENCLAVE_INIT to fail when I try it out.

I *did* get a pass in my test machine. Hmm... I'll check if
the kernel tree was out-of-sync, which could be the reason.

I do not compile kernel on that machine but have the kernel
tree for running selftests. So there is a possiblity for
a human error. Thanks for pointing this out.

> 
> >  3 files changed, 130 insertions(+), 21 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/sgx/load.c b/tools/testing/selftests/sgx/load.c
> > index 94bdeac1cf04..47b2786d6a77 100644
> > --- a/tools/testing/selftests/sgx/load.c
> > +++ b/tools/testing/selftests/sgx/load.c
> > @@ -171,7 +171,8 @@ uint64_t encl_get_entry(struct encl *encl, const char *symbol)
> >  	return 0;
> >  }
> >  
> > -bool encl_load(const char *path, struct encl *encl, unsigned long heap_size)
> > +bool encl_load(const char *path, struct encl *encl, unsigned long heap_size,
> > +	       unsigned long edmm_size)
> >  {
> >  	const char device_path[] = "/dev/sgx_enclave";
> >  	struct encl_segment *seg;
> > @@ -300,7 +301,7 @@ bool encl_load(const char *path, struct encl *encl, unsigned long heap_size)
> >  
> >  	encl->src_size = encl->segment_tbl[j].offset + encl->segment_tbl[j].size;
> >  
> > -	for (encl->encl_size = 4096; encl->encl_size < encl->src_size; )
> > +	for (encl->encl_size = 4096; encl->encl_size < encl->src_size + edmm_size;)
> >  		encl->encl_size <<= 1;
> >  
> >  	return true;
> > diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
> > index 9820b3809c69..c5aa9f323745 100644
> > --- a/tools/testing/selftests/sgx/main.c
> > +++ b/tools/testing/selftests/sgx/main.c
> > @@ -23,6 +23,10 @@
> >  
> >  static const uint64_t MAGIC = 0x1122334455667788ULL;
> >  static const uint64_t MAGIC2 = 0x8877665544332211ULL;
> > +/* Message-ID: <DM8PR11MB55912A7F47A84EC9913A6352F6999@DM8PR11MB5591.namprd11.prod.outlook.com> */
> > +static const uint64_t EDMM_SIZE_LONG = 8L * 1024L * 1024L * 1024L;
> > +static const uint64_t TIMEOUT_LONG = 900; /* seconds */
> > +
> 
> Apologies if my feedback was vague - I actually think that the comments in V1 added
> valuable information, it was just the variation in formatting that was distracting.

IMHO message ID is pretty good reference. Can you
propose how would you redo it to minimize the number
of iterations in the series?

> Reinette

BR, Jarkko

Re: [PATCH v2 4/6] selftests/sgx: Add SGX selftest augment_via_eaccept_long

[Reinette Chatre]

Hi Jarkko,

On 9/1/2022 3:22 PM, Jarkko Sakkinen wrote:
> On Wed, Aug 31, 2022 at 01:07:35PM -0700, Reinette Chatre wrote:
>> On 8/31/2022 10:38 AM, Jarkko Sakkinen wrote:

...

>>>  tools/testing/selftests/sgx/load.c |   5 +-
>>>  tools/testing/selftests/sgx/main.c | 143 +++++++++++++++++++++++++----
>>>  tools/testing/selftests/sgx/main.h |   3 +-
>>
>> Is this test passing on your system? This version is missing the change to
>> mrenclave_ecreate() that causes SGX_IOC_ENCLAVE_INIT to fail when I try it out.
> 
> I *did* get a pass in my test machine. Hmm... I'll check if
> the kernel tree was out-of-sync, which could be the reason.
> 
> I do not compile kernel on that machine but have the kernel
> tree for running selftests. So there is a possiblity for
> a human error. Thanks for pointing this out.

On my system I encounter the failure below (V1 of this series
did not have this problem):

[SNIP]
ok 11 enclave.augment_via_eaccept
#  RUN           enclave.augment_via_eaccept_long ...
SGX_IOC_ENCLAVE_INIT failed: Operation not permitted
# main.c:236:augment_via_eaccept_long:0x0000000000000000 0x0000000000002000 0x03
# main.c:236:augment_via_eaccept_long:0x0000000000002000 0x0000000000001000 0x05
# main.c:236:augment_via_eaccept_long:0x0000000000003000 0x0000000000006000 0x03
# main.c:236:augment_via_eaccept_long:0x0000000000009000 0x0000000000001000 0x03
# main.c:251:augment_via_eaccept_long:Failed to initialize the test enclave.
# main.c:1260:augment_via_eaccept_long:Expected 0 (0) != setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata, EDMM_SIZE_LONG) (0)
# augment_via_eaccept_long: Test terminated by assertion
#          FAIL  enclave.augment_via_eaccept_long
not ok 12 enclave.augment_via_eaccept_long
[SNIP]

...

>>>  
>>>  static const uint64_t MAGIC = 0x1122334455667788ULL;
>>>  static const uint64_t MAGIC2 = 0x8877665544332211ULL;
>>> +/* Message-ID: <DM8PR11MB55912A7F47A84EC9913A6352F6999@DM8PR11MB5591.namprd11.prod.outlook.com> */
>>> +static const uint64_t EDMM_SIZE_LONG = 8L * 1024L * 1024L * 1024L;
>>> +static const uint64_t TIMEOUT_LONG = 900; /* seconds */
>>> +
>>
>> Apologies if my feedback was vague - I actually think that the comments in V1 added
>> valuable information, it was just the variation in formatting that was distracting.
> 
> IMHO message ID is pretty good reference. Can you
> propose how would you redo it to minimize the number
> of iterations in the series?

The message ID is a good reference but it points to an email thread
and as used here it is unclear what part of that thread is referred to.
What you had in V1 was very helpful so it could be:

/*
 * The size was chosen based on a bug report:
 * Message-ID: <DM8PR11MB55912A7F47A84EC9913A6352F6999@DM8PR11MB5591.namprd11.prod.outlook.com>
 */

I am not sure about Message-ID vs url. The latter may be more
convenient since the user needs to first search which inbox the message-ID belongs
to before the message can be accessed. Not a big deal though so I think
either works.

Reinette

Re: [PATCH v2 4/6] selftests/sgx: Add SGX selftest augment_via_eaccept_long

[Jarkko Sakkinen]

On Thu, Sep 01, 2022 at 04:12:22PM -0700, Reinette Chatre wrote:
> Hi Jarkko,
> 
> On 9/1/2022 3:22 PM, Jarkko Sakkinen wrote:
> > On Wed, Aug 31, 2022 at 01:07:35PM -0700, Reinette Chatre wrote:
> >> On 8/31/2022 10:38 AM, Jarkko Sakkinen wrote:
> 
> ...
> 
> >>>  tools/testing/selftests/sgx/load.c |   5 +-
> >>>  tools/testing/selftests/sgx/main.c | 143 +++++++++++++++++++++++++----
> >>>  tools/testing/selftests/sgx/main.h |   3 +-
> >>
> >> Is this test passing on your system? This version is missing the change to
> >> mrenclave_ecreate() that causes SGX_IOC_ENCLAVE_INIT to fail when I try it out.
> > 
> > I *did* get a pass in my test machine. Hmm... I'll check if
> > the kernel tree was out-of-sync, which could be the reason.
> > 
> > I do not compile kernel on that machine but have the kernel
> > tree for running selftests. So there is a possiblity for
> > a human error. Thanks for pointing this out.
> 
> On my system I encounter the failure below (V1 of this series
> did not have this problem):
> 
> [SNIP]
> ok 11 enclave.augment_via_eaccept
> #  RUN           enclave.augment_via_eaccept_long ...
> SGX_IOC_ENCLAVE_INIT failed: Operation not permitted
> # main.c:236:augment_via_eaccept_long:0x0000000000000000 0x0000000000002000 0x03
> # main.c:236:augment_via_eaccept_long:0x0000000000002000 0x0000000000001000 0x05
> # main.c:236:augment_via_eaccept_long:0x0000000000003000 0x0000000000006000 0x03
> # main.c:236:augment_via_eaccept_long:0x0000000000009000 0x0000000000001000 0x03
> # main.c:251:augment_via_eaccept_long:Failed to initialize the test enclave.
> # main.c:1260:augment_via_eaccept_long:Expected 0 (0) != setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata, EDMM_SIZE_LONG) (0)
> # augment_via_eaccept_long: Test terminated by assertion
> #          FAIL  enclave.augment_via_eaccept_long
> not ok 12 enclave.augment_via_eaccept_long
> [SNIP]
> 
> ...
> 
> >>>  
> >>>  static const uint64_t MAGIC = 0x1122334455667788ULL;
> >>>  static const uint64_t MAGIC2 = 0x8877665544332211ULL;
> >>> +/* Message-ID: <DM8PR11MB55912A7F47A84EC9913A6352F6999@DM8PR11MB5591.namprd11.prod.outlook.com> */
> >>> +static const uint64_t EDMM_SIZE_LONG = 8L * 1024L * 1024L * 1024L;
> >>> +static const uint64_t TIMEOUT_LONG = 900; /* seconds */
> >>> +
> >>
> >> Apologies if my feedback was vague - I actually think that the comments in V1 added
> >> valuable information, it was just the variation in formatting that was distracting.
> > 
> > IMHO message ID is pretty good reference. Can you
> > propose how would you redo it to minimize the number
> > of iterations in the series?
> 
> The message ID is a good reference but it points to an email thread
> and as used here it is unclear what part of that thread is referred to.
> What you had in V1 was very helpful so it could be:
> 
> /*
>  * The size was chosen based on a bug report:
>  * Message-ID: <DM8PR11MB55912A7F47A84EC9913A6352F6999@DM8PR11MB5591.namprd11.prod.outlook.com>
>  */
> 
> I am not sure about Message-ID vs url. The latter may be more
> convenient since the user needs to first search which inbox the message-ID belongs
> to before the message can be accessed. Not a big deal though so I think
> either works.

This is definitely better, I'll use it. Thanks.

> 
> Reinette

BR, Jarkko

Re: [PATCH v2 4/6] selftests/sgx: Add SGX selftest augment_via_eaccept_long

[Jarkko Sakkinen]

On Fri, Sep 02, 2022 at 01:22:59AM +0300, Jarkko Sakkinen wrote:
> > Is this test passing on your system? This version is missing the change to
> > mrenclave_ecreate() that causes SGX_IOC_ENCLAVE_INIT to fail when I try it out.
> 
> I *did* get a pass in my test machine. Hmm... I'll check if
> the kernel tree was out-of-sync, which could be the reason.
> 
> I do not compile kernel on that machine but have the kernel
> tree for running selftests. So there is a possiblity for
> a human error. Thanks for pointing this out.

Apparently, v1 and v2 break the encl->src_size calculation:
the dynamic heap size is not added.

So, in order to revert sigstruct change:

diff --git a/tools/testing/selftests/sgx/load.c b/tools/testing/selftests/sgx/load.c
index 47b2786d6a77..0e4e12e1e3eb 100644
--- a/tools/testing/selftests/sgx/load.c
+++ b/tools/testing/selftests/sgx/load.c
@@ -172,7 +172,7 @@ uint64_t encl_get_entry(struct encl *encl, const char *symbol)
 }

 bool encl_load(const char *path, struct encl *encl, unsigned long heap_size,
-              unsigned long edmm_size)
+              unsigned long dynamic_heap_size)
 {
        const char device_path[] = "/dev/sgx_enclave";
        struct encl_segment *seg;
@@ -299,9 +299,9 @@ bool encl_load(const char *path, struct encl *encl, unsigned long heap_size,
        if (seg->src == MAP_FAILED)
                goto err;

-       encl->src_size = encl->segment_tbl[j].offset + encl->segment_tbl[j].size;
+       encl->src_size = encl->segment_tbl[j].offset + encl->segment_tbl[j].size + dynamic_heap_size;

-       for (encl->encl_size = 4096; encl->encl_size < encl->src_size + edmm_size;)
+       for (encl->encl_size = 4096; encl->encl_size < encl->src_size;)
                encl->encl_size <<= 1;

        return true;

BR, Jarkko

Re: [PATCH v2 4/6] selftests/sgx: Add SGX selftest augment_via_eaccept_long

[Jarkko Sakkinen]

On Sun, Sep 04, 2022 at 07:02:08AM +0300, Jarkko Sakkinen wrote:
> On Fri, Sep 02, 2022 at 01:22:59AM +0300, Jarkko Sakkinen wrote:
> > > Is this test passing on your system? This version is missing the change to
> > > mrenclave_ecreate() that causes SGX_IOC_ENCLAVE_INIT to fail when I try it out.
> > 
> > I *did* get a pass in my test machine. Hmm... I'll check if
> > the kernel tree was out-of-sync, which could be the reason.
> > 
> > I do not compile kernel on that machine but have the kernel
> > tree for running selftests. So there is a possiblity for
> > a human error. Thanks for pointing this out.
> 
> Apparently, v1 and v2 break the encl->src_size calculation:
> the dynamic heap size is not added.
> 
> So, in order to revert sigstruct change:
> 
> diff --git a/tools/testing/selftests/sgx/load.c b/tools/testing/selftests/sgx/load.c
> index 47b2786d6a77..0e4e12e1e3eb 100644
> --- a/tools/testing/selftests/sgx/load.c
> +++ b/tools/testing/selftests/sgx/load.c
> @@ -172,7 +172,7 @@ uint64_t encl_get_entry(struct encl *encl, const char *symbol)
>  }
> 
>  bool encl_load(const char *path, struct encl *encl, unsigned long heap_size,
> -              unsigned long edmm_size)
> +              unsigned long dynamic_heap_size)
>  {
>         const char device_path[] = "/dev/sgx_enclave";
>         struct encl_segment *seg;
> @@ -299,9 +299,9 @@ bool encl_load(const char *path, struct encl *encl, unsigned long heap_size,
>         if (seg->src == MAP_FAILED)
>                 goto err;
> 
> -       encl->src_size = encl->segment_tbl[j].offset + encl->segment_tbl[j].size;
> +       encl->src_size = encl->segment_tbl[j].offset + encl->segment_tbl[j].size + dynamic_heap_size;
> 
> -       for (encl->encl_size = 4096; encl->encl_size < encl->src_size + edmm_size;)
> +       for (encl->encl_size = 4096; encl->encl_size < encl->src_size;)
>                 encl->encl_size <<= 1;

Actually, it is correct after all how Vijay changed it. We should use
the final pre-calculated enclave address range in sigstruct.c. It's the
re-calculation of that in sigstruct is a reminiscent of it being a
separate command-line utility, instead of calculating the sigstruct
on-fly. I.e. there has been sane reasons why it has been like that.

BR, Jarkko

[PATCH v2 5/6] selftests/sgx: retry the ioctls returned with EAGAIN

[Jarkko Sakkinen]

From: Haitao Huang <haitao.huang@linux.intel.com>

For EMODT and EREMOVE ioctls with a large range, kernel
may not finish in one shot and return EAGAIN error code
and count of bytes of EPC pages on that operations are
finished successfully.

Change the unclobbered_vdso_oversubscribed_remove test
to rerun the ioctls in a loop, updating offset and length
using the byte count returned in each iteration.

Signed-off-by: Haitao Huang <haitao.huang@linux.intel.com>
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
v2:
* Changed branching in EAGAIN condition so that else branch
  is not required.
* Addressed Reinette's feedback:
  https://lore.kernel.org/linux-sgx/5d19be91-3aef-5cbe-6063-3ff3dbd5572b@intel.com/
---
 tools/testing/selftests/sgx/main.c | 42 ++++++++++++++++++++++++------
 1 file changed, 34 insertions(+), 8 deletions(-)

diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
index c5aa9f323745..f42941da3bbe 100644
--- a/tools/testing/selftests/sgx/main.c
+++ b/tools/testing/selftests/sgx/main.c
@@ -395,6 +395,7 @@ TEST_F_TIMEOUT(enclave, unclobbered_vdso_oversubscribed_remove, 900)
 	struct encl_segment *heap;
 	unsigned long total_mem;
 	int ret, errno_save;
+	unsigned long count;
 	unsigned long addr;
 	unsigned long i;
 
@@ -458,16 +459,30 @@ TEST_F_TIMEOUT(enclave, unclobbered_vdso_oversubscribed_remove, 900)
 	modt_ioc.offset = heap->offset;
 	modt_ioc.length = heap->size;
 	modt_ioc.page_type = SGX_PAGE_TYPE_TRIM;
-
+	count = 0;
 	TH_LOG("Changing type of %zd bytes to trimmed may take a while ...",
 	       heap->size);
-	ret = ioctl(self->encl.fd, SGX_IOC_ENCLAVE_MODIFY_TYPES, &modt_ioc);
-	errno_save = ret == -1 ? errno : 0;
+	do {
+		ret = ioctl(self->encl.fd, SGX_IOC_ENCLAVE_MODIFY_TYPES, &modt_ioc);
+
+		errno_save = ret == -1 ? errno : 0;
+		if (errno_save != EAGAIN)
+			break;
+
+		EXPECT_EQ(modt_ioc.result, 0);
+
+		count += modt_ioc.count;
+		modt_ioc.offset += modt_ioc.count;
+		modt_ioc.length -= modt_ioc.count;
+		modt_ioc.result = 0;
+		modt_ioc.count = 0;
+	} while (modt_ioc.length != 0);
 
 	EXPECT_EQ(ret, 0);
 	EXPECT_EQ(errno_save, 0);
 	EXPECT_EQ(modt_ioc.result, 0);
-	EXPECT_EQ(modt_ioc.count, heap->size);
+	count += modt_ioc.count;
+	EXPECT_EQ(count, heap->size);
 
 	/* EACCEPT all removed pages. */
 	addr = self->encl.encl_base + heap->offset;
@@ -495,15 +510,26 @@ TEST_F_TIMEOUT(enclave, unclobbered_vdso_oversubscribed_remove, 900)
 
 	remove_ioc.offset = heap->offset;
 	remove_ioc.length = heap->size;
-
+	count = 0;
 	TH_LOG("Removing %zd bytes from enclave may take a while ...",
 	       heap->size);
-	ret = ioctl(self->encl.fd, SGX_IOC_ENCLAVE_REMOVE_PAGES, &remove_ioc);
-	errno_save = ret == -1 ? errno : 0;
+	do {
+		ret = ioctl(self->encl.fd, SGX_IOC_ENCLAVE_REMOVE_PAGES, &remove_ioc);
+
+		errno_save = ret == -1 ? errno : 0;
+		if (errno_save != EAGAIN)
+			break;
+
+		count += remove_ioc.count;
+		remove_ioc.offset += remove_ioc.count;
+		remove_ioc.length -= remove_ioc.count;
+		remove_ioc.count = 0;
+	} while (remove_ioc.length != 0);
 
 	EXPECT_EQ(ret, 0);
 	EXPECT_EQ(errno_save, 0);
-	EXPECT_EQ(remove_ioc.count, heap->size);
+	count += remove_ioc.count;
+	EXPECT_EQ(count, heap->size);
 }
 
 TEST_F(enclave, clobbered_vdso)
-- 
2.37.2

Re: [PATCH v2 5/6] selftests/sgx: retry the ioctls returned with EAGAIN

[Reinette Chatre]

Hi Jarkko,

On 8/31/2022 10:38 AM, Jarkko Sakkinen wrote:
> @@ -458,16 +459,30 @@ TEST_F_TIMEOUT(enclave, unclobbered_vdso_oversubscribed_remove, 900)
>  	modt_ioc.offset = heap->offset;
>  	modt_ioc.length = heap->size;
>  	modt_ioc.page_type = SGX_PAGE_TYPE_TRIM;
> -
> +	count = 0;
>  	TH_LOG("Changing type of %zd bytes to trimmed may take a while ...",
>  	       heap->size);
> -	ret = ioctl(self->encl.fd, SGX_IOC_ENCLAVE_MODIFY_TYPES, &modt_ioc);
> -	errno_save = ret == -1 ? errno : 0;
> +	do {
> +		ret = ioctl(self->encl.fd, SGX_IOC_ENCLAVE_MODIFY_TYPES, &modt_ioc);
> +
> +		errno_save = ret == -1 ? errno : 0;
> +		if (errno_save != EAGAIN)
> +			break;
> +
> +		EXPECT_EQ(modt_ioc.result, 0);
> +
> +		count += modt_ioc.count;
> +		modt_ioc.offset += modt_ioc.count;
> +		modt_ioc.length -= modt_ioc.count;
> +		modt_ioc.result = 0;

From the discussion in V1 it seemed that you were planning to add a check
on the result value instead of just overwriting it. Are you still planning to
do this?

Reinette

[PATCH v2 6/6] selftests/sgx: Add a bpftrace script for tracking allocation errors

[Jarkko Sakkinen]

Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
v2:
* Added comments.
---
 tools/testing/selftests/sgx/alloc-error.bt | 9 +++++++++
 1 file changed, 9 insertions(+)
 create mode 100644 tools/testing/selftests/sgx/alloc-error.bt

diff --git a/tools/testing/selftests/sgx/alloc-error.bt b/tools/testing/selftests/sgx/alloc-error.bt
new file mode 100644
index 000000000000..0cc8b2e41852
--- /dev/null
+++ b/tools/testing/selftests/sgx/alloc-error.bt
@@ -0,0 +1,9 @@
+/* EPC allocation */
+kr:sgx_alloc_epc_page /(uint64)retval >= (uint64)(-4095)/ {
+		 printf("sgx_alloc_epc_page: retval=%d\n", (int64)retval);
+}
+
+/* kzalloc for struct sgx_encl_page */
+kr:sgx_encl_page_alloc /(uint64)retval >= (uint64)(-4095)/ {
+		 printf("sgx_encl_page_alloc: retval=%d\n", (int64)retval);
+}
-- 
2.37.2

Re: [PATCH v2 6/6] selftests/sgx: Add a bpftrace script for tracking allocation errors

[Reinette Chatre]

Hi Jarkko,

On 8/31/2022 10:38 AM, Jarkko Sakkinen wrote:
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> ---
> v2:
> * Added comments.
> ---
>  tools/testing/selftests/sgx/alloc-error.bt | 9 +++++++++
>  1 file changed, 9 insertions(+)
>  create mode 100644 tools/testing/selftests/sgx/alloc-error.bt
> 
> diff --git a/tools/testing/selftests/sgx/alloc-error.bt b/tools/testing/selftests/sgx/alloc-error.bt
> new file mode 100644
> index 000000000000..0cc8b2e41852
> --- /dev/null
> +++ b/tools/testing/selftests/sgx/alloc-error.bt
> @@ -0,0 +1,9 @@
> +/* EPC allocation */
> +kr:sgx_alloc_epc_page /(uint64)retval >= (uint64)(-4095)/ {
> +		 printf("sgx_alloc_epc_page: retval=%d\n", (int64)retval);
> +}
> +
> +/* kzalloc for struct sgx_encl_page */
> +kr:sgx_encl_page_alloc /(uint64)retval >= (uint64)(-4095)/ {
> +		 printf("sgx_encl_page_alloc: retval=%d\n", (int64)retval);
> +}

I did see you response in [1]. I continue to find this
addition very cryptic in that it assumes global familiarity
with BPF scripting which I do not think can be assumed. I
still think this script can benefit from a comment to describe
what the script does and how to use it.


Reinette


[1] https://lore.kernel.org/linux-sgx/Yw+nCBVYfueEXVZK@kernel.org/

Re: [PATCH v2 6/6] selftests/sgx: Add a bpftrace script for tracking allocation errors

[Jarkko Sakkinen]

On Wed, Aug 31, 2022 at 01:09:21PM -0700, Reinette Chatre wrote:
> Hi Jarkko,
> 
> On 8/31/2022 10:38 AM, Jarkko Sakkinen wrote:
> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > ---
> > v2:
> > * Added comments.
> > ---
> >  tools/testing/selftests/sgx/alloc-error.bt | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >  create mode 100644 tools/testing/selftests/sgx/alloc-error.bt
> > 
> > diff --git a/tools/testing/selftests/sgx/alloc-error.bt b/tools/testing/selftests/sgx/alloc-error.bt
> > new file mode 100644
> > index 000000000000..0cc8b2e41852
> > --- /dev/null
> > +++ b/tools/testing/selftests/sgx/alloc-error.bt
> > @@ -0,0 +1,9 @@
> > +/* EPC allocation */
> > +kr:sgx_alloc_epc_page /(uint64)retval >= (uint64)(-4095)/ {
> > +		 printf("sgx_alloc_epc_page: retval=%d\n", (int64)retval);
> > +}
> > +
> > +/* kzalloc for struct sgx_encl_page */
> > +kr:sgx_encl_page_alloc /(uint64)retval >= (uint64)(-4095)/ {
> > +		 printf("sgx_encl_page_alloc: retval=%d\n", (int64)retval);
> > +}
> 
> I did see you response in [1]. I continue to find this
> addition very cryptic in that it assumes global familiarity
> with BPF scripting which I do not think can be assumed. I
> still think this script can benefit from a comment to describe
> what the script does and how to use it.


I think I follow Dave's advice to use perf here. I'll
make a header which explains what the script does.

BR, Jarkko

Re: [PATCH v2 0/6] x86/sgx: A collection of tests and fixes

[Dave Hansen]

Jarkko,

What is your expectations of this series?  There's nothing in the cover
letter.

Do you think all of this is urgent material that needs to go to Linus
for 6.0?  Or is it all 6.1 material?

Re: [PATCH v2 0/6] x86/sgx: A collection of tests and fixes

[Jarkko Sakkinen]

On Wed, Aug 31, 2022 at 10:43:05AM -0700, Dave Hansen wrote:
> Jarkko,
> 
> What is your expectations of this series?  There's nothing in the cover
> letter.
> 
> Do you think all of this is urgent material that needs to go to Linus
> for 6.0?  Or is it all 6.1 material?

Hmm... Let me think.

I would pick exactly 2/6 and 3/6 and postpone rest for 6.1.

2/6 is IMHO critical fix and 3/6 is fix for a commit that
is part of 6.0.

BR, Jarkko

Re: [PATCH v2 0/6] x86/sgx: A collection of tests and fixes

[Dave Hansen]

On 8/31/22 11:11, Jarkko Sakkinen wrote:
> On Wed, Aug 31, 2022 at 10:43:05AM -0700, Dave Hansen wrote:
>> Jarkko,
>>
>> What is your expectations of this series?  There's nothing in the cover
>> letter.
>>
>> Do you think all of this is urgent material that needs to go to Linus
>> for 6.0?  Or is it all 6.1 material?
> Hmm... Let me think.
> 
> I would pick exactly 2/6 and 3/6 and postpone rest for 6.1.
> 
> 2/6 is IMHO critical fix and 3/6 is fix for a commit that
> is part of 6.0.

That's really great context, thanks!  Next time, could you please
include that in your cover letter(s) and split the patches up between
urgent non-urgent bits?

Re: [PATCH v2 0/6] x86/sgx: A collection of tests and fixes

[Jarkko Sakkinen]

On Wed, Aug 31, 2022 at 11:24:52AM -0700, Dave Hansen wrote:
> On 8/31/22 11:11, Jarkko Sakkinen wrote:
> > On Wed, Aug 31, 2022 at 10:43:05AM -0700, Dave Hansen wrote:
> >> Jarkko,
> >>
> >> What is your expectations of this series?  There's nothing in the cover
> >> letter.
> >>
> >> Do you think all of this is urgent material that needs to go to Linus
> >> for 6.0?  Or is it all 6.1 material?
> > Hmm... Let me think.
> > 
> > I would pick exactly 2/6 and 3/6 and postpone rest for 6.1.
> > 
> > 2/6 is IMHO critical fix and 3/6 is fix for a commit that
> > is part of 6.0.
> 
> That's really great context, thanks!  Next time, could you please
> include that in your cover letter(s) and split the patches up between
> urgent non-urgent bits?

I can, no problem.

BR, Jarkko