x86/sgx: A collection of tests and fixes

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().

arch/x86/kernel/cpu/sgx/main.c +20 -4

@@ -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

Quoted 44 lines

  static int ksgxd(void *p)
  {

(quoted diff) +2 -0

+	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. */

arch/x86/kernel/cpu/sgx/main.c +4 -2

-	__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;

arch/x86/kernel/cpu/sgx/main.c +13 -2

-	/* 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

(quoted diff) +1 -0

+		 * the bug is expected to be in the sanitization process,

but

(quoted diff) +1 -0

+		 * successfully sanitized pages are still valid and driver

can

(quoted diff) +1 -0

+		 * be used and most importantly debugged without issues. To

put

arch/x86/kernel/cpu/sgx/main.c +5 -0

+		 * 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);
+	}

Quoted 3 lines

   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:

(quoted diff) +0 -0

@@ -388,6 +393,8 @@ void sgx_reclaim_direct(void)

 static int ksgxd(void *p)
 {

(quoted diff) +2 -0

+	unsigned long left_dirty;
+

  set_freezable();

  /*

arch/x86/kernel/cpu/sgx/main.c +0 -1

@@ -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);

(quoted diff) +3 -2

-	/* 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);

Quoted 74 lines · Reinette Chatre

Quoted 44 lines

  static int ksgxd(void *p)
  {

(quoted diff) +2 -0

+	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. */

arch/x86/kernel/cpu/sgx/main.c +4 -2

-	__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;

arch/x86/kernel/cpu/sgx/main.c +13 -2

-	/* 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

(quoted diff) +1 -0

+		 * the bug is expected to be in the sanitization process,

but

(quoted diff) +1 -0

+		 * successfully sanitized pages are still valid and driver

can

(quoted diff) +1 -0

+		 * be used and most importantly debugged without issues. To

put

arch/x86/kernel/cpu/sgx/main.c +5 -0

+		 * 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);
+	}

Quoted 3 lines

   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:

(quoted diff) +0 -0

@@ -388,6 +393,8 @@ void sgx_reclaim_direct(void)

 static int ksgxd(void *p)
 {

(quoted diff) +2 -0

+	unsigned long left_dirty;
+

  set_freezable();

  /*

arch/x86/kernel/cpu/sgx/main.c +0 -1

@@ -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);

(quoted diff) +3 -2

-	/* 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);

Quoted 1 line

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.

Hi Jarkko,

Quoted 42 lines · 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>

Quoted 9 lines

[    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?

Quoted 7 lines

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".

...

Quoted 35 lines

arch/x86/kernel/cpu/sgx/main.c +20 -4

@@ -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

arch/x86/kernel/cpu/sgx/main.c void sgx_reclaim_direct(void) · line 432

Quoted 10 lines

arch/x86/kernel/cpu/sgx/main.c +7 -0

+		 * 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:

arch/x86/kernel/cpu/sgx/main.c +2 -0

@@ -388,6 +393,8 @@ void sgx_reclaim_direct(void)
 
 static int ksgxd(void *p)
 {
+	unsigned long left_dirty;
+
 	set_freezable();
 
 	/*

Quoted 133 lines · Reinette Chatre

Hi Jarkko,

Quoted 42 lines · 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>

Quoted 9 lines

[    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?

Quoted 7 lines

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".

...

Quoted 35 lines

arch/x86/kernel/cpu/sgx/main.c +20 -4

@@ -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

arch/x86/kernel/cpu/sgx/main.c void sgx_reclaim_direct(void) · line 432

Quoted 10 lines

arch/x86/kernel/cpu/sgx/main.c +7 -0

+		 * 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:

arch/x86/kernel/cpu/sgx/main.c +2 -0

@@ -388,6 +393,8 @@ void sgx_reclaim_direct(void)
 
 static int ksgxd(void *p)
 {
+	unsigned long left_dirty;
+
 	set_freezable();
 
 	/*

arch/x86/kernel/cpu/sgx/main.c static int ksgxd(void *p) · line 404

Quoted 3 lines

arch/x86/kernel/cpu/sgx/main.c +0 -0

@@ -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;

arch/x86/kernel/cpu/sgx/main.c void sgx_reclaim_direct(void)

Quoted 7 lines

arch/x86/kernel/cpu/sgx/main.c +3 -3

-	__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.

Quoted 165 lines · Jarkko Sakkinen

Quoted 133 lines · Reinette Chatre

Hi Jarkko,

Quoted 42 lines · 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>

Quoted 9 lines

[    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?

Quoted 7 lines

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".

...

Quoted 35 lines

arch/x86/kernel/cpu/sgx/main.c +20 -4

@@ -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

arch/x86/kernel/cpu/sgx/main.c void sgx_reclaim_direct(void) · line 432

Quoted 10 lines

arch/x86/kernel/cpu/sgx/main.c +7 -0

+		 * 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:

arch/x86/kernel/cpu/sgx/main.c +2 -0

@@ -388,6 +393,8 @@ void sgx_reclaim_direct(void)
 
 static int ksgxd(void *p)
 {
+	unsigned long left_dirty;
+
 	set_freezable();
 
 	/*

arch/x86/kernel/cpu/sgx/main.c static int ksgxd(void *p) · line 404

Quoted 3 lines

arch/x86/kernel/cpu/sgx/main.c +0 -0

@@ -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;

arch/x86/kernel/cpu/sgx/main.c void sgx_reclaim_direct(void)

Quoted 7 lines

arch/x86/kernel/cpu/sgx/main.c +3 -3

-	__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);
        }

Quoted 5 lines

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.

Quoted 17 lines · Reinette Chatre

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:

arch/x86/kernel/cpu/sgx/main.c +2 -0

@@ -388,6 +393,8 @@ void sgx_reclaim_direct(void)
 
 static int ksgxd(void *p)
 {
+	unsigned long left_dirty;
+
 	set_freezable();
 
 	/*

arch/x86/kernel/cpu/sgx/main.c static int ksgxd(void *p) · line 404

Quoted 3 lines

arch/x86/kernel/cpu/sgx/main.c +0 -0

@@ -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;

arch/x86/kernel/cpu/sgx/main.c void sgx_reclaim_direct(void)

Quoted 7 lines

arch/x86/kernel/cpu/sgx/main.c +3 -3

-	__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.

Hi Jarkko,

Quoted 24 lines · Jarkko Sakkinen

Quoted 17 lines · Reinette Chatre

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:

arch/x86/kernel/cpu/sgx/main.c +2 -0

@@ -388,6 +393,8 @@ void sgx_reclaim_direct(void)
 
 static int ksgxd(void *p)
 {
+	unsigned long left_dirty;
+
 	set_freezable();
 
 	/*

arch/x86/kernel/cpu/sgx/main.c static int ksgxd(void *p) · line 404

Quoted 3 lines

arch/x86/kernel/cpu/sgx/main.c +0 -0

@@ -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:

Quoted 3 lines

        if (!kthread_should_stop())
                return 0;

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

Quoted 10 lines

arch/x86/kernel/cpu/sgx/main.c void sgx_reclaim_direct(void)

Quoted 7 lines

arch/x86/kernel/cpu/sgx/main.c +3 -3

-	__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:

arch/x86/kernel/cpu/sgx/main.c +5 -2

+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:

arch/x86/kernel/cpu/sgx/main.c +5 -3

@@ -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())

Quoted 17 lines

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.

Quoted 2 lines

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.

Quoted 3 lines

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.

arch/x86/kernel/cpu/sgx/main.c void sgx_reclaim_direct(void) · line 420

Quoted 2 lines · Jarkko Sakkinen

arch/x86/kernel/cpu/sgx/main.c +2 -0

+	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.

Hi Jarkko,

Quoted 7 lines · Jarkko Sakkinen

arch/x86/kernel/cpu/sgx/main.c void sgx_reclaim_direct(void) · line 420

Quoted 2 lines · Jarkko Sakkinen

arch/x86/kernel/cpu/sgx/main.c +2 -0

+	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.

Quoted 11 lines · Reinette Chatre

Hi Jarkko,

Quoted 7 lines · Jarkko Sakkinen

arch/x86/kernel/cpu/sgx/main.c void sgx_reclaim_direct(void) · line 420

Quoted 2 lines · Jarkko Sakkinen

arch/x86/kernel/cpu/sgx/main.c +2 -0

+	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.

Quoted 5 lines

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.

Quoted 2 lines

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?

Hi Jarkko,

Quoted 18 lines · Jarkko Sakkinen

Quoted 11 lines · Reinette Chatre

Hi Jarkko,

Quoted 7 lines · Jarkko Sakkinen

arch/x86/kernel/cpu/sgx/main.c void sgx_reclaim_direct(void) · line 420

Quoted 2 lines · Jarkko Sakkinen

arch/x86/kernel/cpu/sgx/main.c +2 -0

+	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.

Quoted 5 lines

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

Quoted 7 lines

Point taken.

Quoted 2 lines

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

From: Haitao Huang <[email protected]>

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 <[email protected]>

Tested-by Vijay Dhanraj <[email protected]>

Reviewed-by Reinette Chatre <[email protected]>

Signed-off-by Jarkko Sakkinen <[email protected]>

---
v3:
* Added Reinette's ack.

Hi Jarkko,

Quoted 7 lines · Jarkko Sakkinen

From: Haitao Huang <[email protected]>

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.

Quoted 8 lines

Fixes 5a90d2c3f5ef ("x86/sgx: Support adding of pages to an initialized enclave")

Signed-off-by Haitao Huang <[email protected]>

Tested-by Vijay Dhanraj <[email protected]>

Reviewed-by Reinette Chatre <[email protected]>

Signed-off-by Jarkko Sakkinen <[email protected]>

---
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.

Can you point out what exactly is the whitespace issue?

Hi Jarkko,

Quoted 2 lines · Jarkko Sakkinen

Can you point out what exactly is the whitespace issue?

There is an extra space after the "=" in:

       vmret =  VM_FAULT_NOPAGE;

From: Vijay Dhanraj <[email protected]>

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 <[email protected]>

Co-developed-by Jarkko Sakkinen <[email protected]>

Signed-off-by Jarkko Sakkinen <[email protected]>

---
v3:
- Addressed Reinette's feedback:
https://lore.kernel.org/linux-sgx/[email protected]/
v2:
- Addressed Reinette's feedback:
https://lore.kernel.org/linux-sgx/[email protected]/

---
 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(-)

tools/testing/selftests/sgx/load.c +3 -2

index 94bdeac1cf04..47b2786d6a77 100644
@@ -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;

tools/testing/selftests/sgx/main.c +0 -0

index 9820b3809c69..c5aa9f323745 100644

Hi Jarkko,

Quoted 15 lines · Jarkko Sakkinen

From: Vijay Dhanraj <[email protected]>

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 <[email protected]>

Co-developed-by Jarkko Sakkinen <[email protected]>

Signed-off-by Jarkko Sakkinen <[email protected]>

---
v3:
- Addressed Reinette's feedback:
https://lore.kernel.org/linux-sgx/[email protected]/
v2:
- Addressed Reinette's feedback:
https://lore.kernel.org/linux-sgx/[email protected]/

Quoted 4 lines

---
 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.

Quoted 32 lines

 3 files changed, 130 insertions(+), 21 deletions(-)

tools/testing/selftests/sgx/load.c +3 -2

index 94bdeac1cf04..47b2786d6a77 100644
@@ -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;

tools/testing/selftests/sgx/main.c +0 -0

index 9820b3809c69..c5aa9f323745 100644
@@ -23,6 +23,10 @@
 

tools/testing/selftests/sgx/main.c line 29

Quoted 6 lines

tools/testing/selftests/sgx/main.c +4 -0

 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

Quoted 1 line · Reinette Chatre

On 8/31/2022 10:38 AM, Jarkko Sakkinen wrote:

Quoted 6 lines

Quoted 3 lines

 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.

Quoted 10 lines

tools/testing/selftests/sgx/main.c line 29

Quoted 7 lines

static const uint64_t MAGIC = 0x1122334455667788ULL; static const uint64_t MAGIC2 = 0x8877665544332211ULL;

tools/testing/selftests/sgx/main.c +4 -0

+/* 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?

Hi Jarkko,

Quoted 2 lines · Jarkko Sakkinen

Quoted 1 line · Reinette Chatre

On 8/31/2022 10:38 AM, Jarkko Sakkinen wrote:

...

Quoted 7 lines

Quoted 6 lines

Quoted 3 lines

 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.

Quoted 6 lines

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]

...

Quoted 11 lines

Quoted 10 lines

tools/testing/selftests/sgx/main.c line 29

Quoted 7 lines

static const uint64_t MAGIC = 0x1122334455667788ULL; static const uint64_t MAGIC2 = 0x8877665544332211ULL;

tools/testing/selftests/sgx/main.c +4 -0

+/* 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.

Quoted 3 lines

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

Quoted 2 lines

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.

Quoted 9 lines · Jarkko Sakkinen

Quoted 2 lines

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:

tools/testing/selftests/sgx/load.c +1 -1

index 47b2786d6a77..0e4e12e1e3eb 100644
@@ -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;

tools/testing/selftests/sgx/main.c +4 -3

@@ -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);
+

Signed-off-by Jarkko Sakkinen <[email protected]>

---
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

tools/testing/selftests/sgx/alloc-error.bt new file +0 -0

new file mode 100644
index 000000000000..0cc8b2e41852

Hi Jarkko,

Quoted 22 lines · Jarkko Sakkinen

Signed-off-by Jarkko Sakkinen <[email protected]>

---
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

tools/testing/selftests/sgx/alloc-error.bt new file +7 -0

new file mode 100644
index 000000000000..0cc8b2e41852
@@ -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)/ {

tools/testing/selftests/sgx/alloc-error.bt line 9

Quoted 2 lines

tools/testing/selftests/sgx/alloc-error.bt new file +2 -0

+		 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.

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?

Quoted 5 lines · Dave Hansen

Jarkko,

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

Quoted 2 lines

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.

Quoted 10 lines · Jarkko Sakkinen

Quoted 7 lines · 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?

Hmm... Let me think.

Quoted 4 lines

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?