* [PATCH v8 2/5] selftests/sgx: Migrate to kselftest harness
2021-06-10 8:30 [PATCH v8 1/5] selftests/sgx: Rename 'eenter' and 'sgx_call_vdso' Jarkko Sakkinen
@ 2021-06-10 8:30 ` Jarkko Sakkinen
2021-06-10 8:30 ` [PATCH v8 3/5] selftests/sgx: Dump enclave memory map Jarkko Sakkinen
` (3 subsequent siblings)
4 siblings, 0 replies; 18+ messages in thread
From: Jarkko Sakkinen @ 2021-06-10 8:30 UTC (permalink / raw)
To: shuah
Cc: linux-kselftest, linux-sgx, Reinette Chatre, Jarkko Sakkinen,
Dave Hansen, linux-kernel
Migrate to kselftest harness. Use a fixture test with enclave initialized
and de-initialized for each of the existing three tests, in other words:
1. One FIXTURE() for managing the enclave life-cycle.
2. Three TEST_F()'s, one for each test case.
Dump lines of /proc/self/maps matching "sgx" in FIXTURE_SETUP() as this
can be very useful debugging information later on.
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
v8:
* Replace inline function call with ENCL_CALL() macro.
v7:
* Implement encl_call() that wraps the logic of invoking the vDSO for the
test enclave.
v6:
* Remove extra '\n' from TH_LOG() calls.
v5:
* Use TH_LOG() for printing enclave address ranges instead of printf(),
based on Reinette's remark.
v4:
* Refine to take better use of the kselftest harness macros.
* Fix: TCS base address was not initialized for a run struct.
v3:
* Use helper macros.
v2:
* Add the missing string argument to ksft_test_result_pass() and
ksft_test_result_fail() calls.
tools/testing/selftests/sgx/load.c | 3 -
tools/testing/selftests/sgx/main.c | 177 +++++++++++++++--------------
2 files changed, 92 insertions(+), 88 deletions(-)
diff --git a/tools/testing/selftests/sgx/load.c b/tools/testing/selftests/sgx/load.c
index f441ac34b4d4..00928be57fc4 100644
--- a/tools/testing/selftests/sgx/load.c
+++ b/tools/testing/selftests/sgx/load.c
@@ -239,9 +239,6 @@ bool encl_load(const char *path, struct encl *encl)
seg->offset = (phdr->p_offset & PAGE_MASK) - src_offset;
seg->size = (phdr->p_filesz + PAGE_SIZE - 1) & PAGE_MASK;
- printf("0x%016lx 0x%016lx 0x%02x\n", seg->offset, seg->size,
- seg->prot);
-
j++;
}
diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
index 43da68388e25..6da19b6bf287 100644
--- a/tools/testing/selftests/sgx/main.c
+++ b/tools/testing/selftests/sgx/main.c
@@ -17,8 +17,8 @@
#include <sys/types.h>
#include <sys/auxv.h>
#include "defines.h"
+#include "../kselftest_harness.h"
#include "main.h"
-#include "../kselftest.h"
static const uint64_t MAGIC = 0x1122334455667788ULL;
vdso_sgx_enter_enclave_t vdso_sgx_enter_enclave;
@@ -107,85 +107,49 @@ static Elf64_Sym *vdso_symtab_get(struct vdso_symtab *symtab, const char *name)
return NULL;
}
-bool report_results(struct sgx_enclave_run *run, int ret, uint64_t result,
- const char *test)
-{
- bool valid = true;
-
- if (ret) {
- printf("FAIL: %s() returned: %d\n", test, ret);
- valid = false;
- }
-
- if (run->function != EEXIT) {
- printf("FAIL: %s() function, expected: %u, got: %u\n", test, EEXIT,
- run->function);
- valid = false;
- }
-
- if (result != MAGIC) {
- printf("FAIL: %s(), expected: 0x%lx, got: 0x%lx\n", test, MAGIC,
- result);
- valid = false;
- }
-
- if (run->user_data) {
- printf("FAIL: %s() user data, expected: 0x0, got: 0x%llx\n",
- test, run->user_data);
- valid = false;
- }
-
- return valid;
-}
-
-static int user_handler(long rdi, long rsi, long rdx, long ursp, long r8, long r9,
- struct sgx_enclave_run *run)
-{
- run->user_data = 0;
- return 0;
-}
+FIXTURE(enclave) {
+ struct encl encl;
+ struct sgx_enclave_run run;
+};
-int main(int argc, char *argv[])
+FIXTURE_SETUP(enclave)
{
- struct sgx_enclave_run run;
+ Elf64_Sym *sgx_enter_enclave_sym = NULL;
struct vdso_symtab symtab;
- Elf64_Sym *sgx_enter_enclave_sym;
- uint64_t result = 0;
- struct encl encl;
+ struct encl_segment *seg;
unsigned int i;
void *addr;
- int ret;
-
- memset(&run, 0, sizeof(run));
- if (!encl_load("test_encl.elf", &encl)) {
- encl_delete(&encl);
+ if (!encl_load("test_encl.elf", &self->encl)) {
+ encl_delete(&self->encl);
ksft_exit_skip("cannot load enclaves\n");
}
- if (!encl_measure(&encl))
+ for (i = 0; i < self->encl.nr_segments; i++) {
+ seg = &self->encl.segment_tbl[i];
+
+ TH_LOG("0x%016lx 0x%016lx 0x%02x", seg->offset, seg->size, seg->prot);
+ }
+
+ if (!encl_measure(&self->encl))
goto err;
- if (!encl_build(&encl))
+ if (!encl_build(&self->encl))
goto err;
/*
* An enclave consumer only must do this.
*/
- for (i = 0; i < encl.nr_segments; i++) {
- struct encl_segment *seg = &encl.segment_tbl[i];
-
- addr = mmap((void *)encl.encl_base + seg->offset, seg->size,
- seg->prot, MAP_SHARED | MAP_FIXED, encl.fd, 0);
- if (addr == MAP_FAILED) {
- perror("mmap() segment failed");
- exit(KSFT_FAIL);
- }
+ for (i = 0; i < self->encl.nr_segments; i++) {
+ struct encl_segment *seg = &self->encl.segment_tbl[i];
+
+ addr = mmap((void *)self->encl.encl_base + seg->offset, seg->size,
+ seg->prot, MAP_SHARED | MAP_FIXED, self->encl.fd, 0);
+ EXPECT_NE(addr, MAP_FAILED);
+ if (addr == MAP_FAILED)
+ goto err;
}
- memset(&run, 0, sizeof(run));
- run.tcs = encl.encl_base;
-
/* Get vDSO base address */
addr = (void *)getauxval(AT_SYSINFO_EHDR);
if (!addr)
@@ -200,32 +164,75 @@ int main(int argc, char *argv[])
vdso_sgx_enter_enclave = addr + sgx_enter_enclave_sym->st_value;
- ret = sgx_enter_enclave((void *)&MAGIC, &result, 0, EENTER,
- NULL, NULL, &run);
- if (!report_results(&run, ret, result, "sgx_enter_enclave_unclobbered"))
- goto err;
+ memset(&self->run, 0, sizeof(self->run));
+ self->run.tcs = self->encl.encl_base;
+err:
+ if (!sgx_enter_enclave_sym)
+ encl_delete(&self->encl);
- /* Invoke the vDSO directly. */
- result = 0;
- ret = vdso_sgx_enter_enclave((unsigned long)&MAGIC, (unsigned long)&result,
- 0, EENTER, 0, 0, &run);
- if (!report_results(&run, ret, result, "sgx_enter_enclave"))
- goto err;
+ ASSERT_NE(sgx_enter_enclave_sym, NULL);
+}
- /* And with an exit handler. */
- run.user_handler = (__u64)user_handler;
- run.user_data = 0xdeadbeef;
- ret = vdso_sgx_enter_enclave((unsigned long)&MAGIC, (unsigned long)&result,
- 0, EENTER, 0, 0, &run);
- if (!report_results(&run, ret, result, "user_handler"))
- goto err;
+FIXTURE_TEARDOWN(enclave)
+{
+ encl_delete(&self->encl);
+}
- printf("SUCCESS\n");
- encl_delete(&encl);
- exit(KSFT_PASS);
+#define ENCL_CALL(in, out, run, clobbered) \
+ ({ \
+ int ret; \
+ if ((clobbered)) \
+ ret = vdso_sgx_enter_enclave((unsigned long)(in), (unsigned long)(out), 0, \
+ EENTER, 0, 0, (run)); \
+ else \
+ ret = sgx_enter_enclave((void *)(in), (void *)(out), 0, EENTER, NULL, NULL, \
+ (run)); \
+ ret; \
+ })
+
+TEST_F(enclave, unclobbered_vdso)
+{
+ uint64_t result = 0;
-err:
- encl_delete(&encl);
- exit(KSFT_FAIL);
+ EXPECT_EQ(ENCL_CALL(&MAGIC, &result, &self->run, false), 0);
+
+ EXPECT_EQ(result, MAGIC);
+ EXPECT_EQ(self->run.function, EEXIT);
+ EXPECT_EQ(self->run.user_data, 0);
+}
+
+TEST_F(enclave, clobbered_vdso)
+{
+ uint64_t result = 0;
+
+ EXPECT_EQ(ENCL_CALL(&MAGIC, &result, &self->run, true), 0);
+
+ EXPECT_EQ(result, MAGIC);
+ EXPECT_EQ(self->run.function, EEXIT);
+ EXPECT_EQ(self->run.user_data, 0);
}
+
+static int test_handler(long rdi, long rsi, long rdx, long ursp, long r8, long r9,
+ struct sgx_enclave_run *run)
+{
+ run->user_data = 0;
+
+ return 0;
+}
+
+TEST_F(enclave, clobbered_vdso_and_user_function)
+{
+ uint64_t result = 0;
+
+ self->run.user_handler = (__u64)test_handler;
+ self->run.user_data = 0xdeadbeef;
+
+ EXPECT_EQ(ENCL_CALL(&MAGIC, &result, &self->run, true), 0);
+
+ EXPECT_EQ(result, MAGIC);
+ EXPECT_EQ(self->run.function, EEXIT);
+ EXPECT_EQ(self->run.user_data, 0);
+}
+
+TEST_HARNESS_MAIN
--
2.31.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH v8 3/5] selftests/sgx: Dump enclave memory map
2021-06-10 8:30 [PATCH v8 1/5] selftests/sgx: Rename 'eenter' and 'sgx_call_vdso' Jarkko Sakkinen
2021-06-10 8:30 ` [PATCH v8 2/5] selftests/sgx: Migrate to kselftest harness Jarkko Sakkinen
@ 2021-06-10 8:30 ` Jarkko Sakkinen
2021-06-11 22:45 ` Shuah Khan
2021-06-10 8:30 ` [PATCH v8 4/5] selftests/sgx: Add EXPECT_EEXIT() macro Jarkko Sakkinen
` (2 subsequent siblings)
4 siblings, 1 reply; 18+ messages in thread
From: Jarkko Sakkinen @ 2021-06-10 8:30 UTC (permalink / raw)
To: shuah
Cc: linux-kselftest, linux-sgx, Reinette Chatre, Jarkko Sakkinen,
Dave Hansen, linux-kernel
Often, it's useful to check whether /proc/self/maps looks sane when
dealing with memory mapped objects, especially when they are JIT'ish
dynamically constructed objects. Therefore, dump "/dev/sgx_enclave"
matching lines from the memory map in FIXTURE_SETUP().
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
tools/testing/selftests/sgx/main.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
index 6da19b6bf287..14030f8b85ff 100644
--- a/tools/testing/selftests/sgx/main.c
+++ b/tools/testing/selftests/sgx/main.c
@@ -117,6 +117,8 @@ FIXTURE_SETUP(enclave)
Elf64_Sym *sgx_enter_enclave_sym = NULL;
struct vdso_symtab symtab;
struct encl_segment *seg;
+ char maps_line[256];
+ FILE *maps_file;
unsigned int i;
void *addr;
@@ -167,6 +169,18 @@ FIXTURE_SETUP(enclave)
memset(&self->run, 0, sizeof(self->run));
self->run.tcs = self->encl.encl_base;
+ maps_file = fopen("/proc/self/maps", "r");
+ if (maps_file != NULL) {
+ while (fgets(maps_line, sizeof(maps_line), maps_file) != NULL) {
+ maps_line[strlen(maps_line) - 1] = '\0';
+
+ if (strstr(maps_line, "/dev/sgx_enclave"))
+ TH_LOG("%s", maps_line);
+ }
+
+ fclose(maps_file);
+ }
+
err:
if (!sgx_enter_enclave_sym)
encl_delete(&self->encl);
--
2.31.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v8 3/5] selftests/sgx: Dump enclave memory map
2021-06-10 8:30 ` [PATCH v8 3/5] selftests/sgx: Dump enclave memory map Jarkko Sakkinen
@ 2021-06-11 22:45 ` Shuah Khan
2021-06-12 0:34 ` Dave Hansen
2021-06-12 4:27 ` Jarkko Sakkinen
0 siblings, 2 replies; 18+ messages in thread
From: Shuah Khan @ 2021-06-11 22:45 UTC (permalink / raw)
To: Jarkko Sakkinen, shuah
Cc: linux-kselftest, linux-sgx, Reinette Chatre, Dave Hansen,
linux-kernel, Shuah Khan
On 6/10/21 2:30 AM, Jarkko Sakkinen wrote:
> Often, it's useful to check whether /proc/self/maps looks sane when
> dealing with memory mapped objects, especially when they are JIT'ish
> dynamically constructed objects. Therefore, dump "/dev/sgx_enclave"
> matching lines from the memory map in FIXTURE_SETUP().
>
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> ---
> tools/testing/selftests/sgx/main.c | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
> index 6da19b6bf287..14030f8b85ff 100644
> --- a/tools/testing/selftests/sgx/main.c
> +++ b/tools/testing/selftests/sgx/main.c
> @@ -117,6 +117,8 @@ FIXTURE_SETUP(enclave)
> Elf64_Sym *sgx_enter_enclave_sym = NULL;
> struct vdso_symtab symtab;
> struct encl_segment *seg;
> + char maps_line[256];
> + FILE *maps_file;
> unsigned int i;
> void *addr;
>
> @@ -167,6 +169,18 @@ FIXTURE_SETUP(enclave)
> memset(&self->run, 0, sizeof(self->run));
> self->run.tcs = self->encl.encl_base;
>
> + maps_file = fopen("/proc/self/maps", "r");
I almost applied these. Does this require root access, if so,
please add logic to skip the test if non-root user runs it.
Same comments for all other paths that might require root access.
thanks,
-- Shuah
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v8 3/5] selftests/sgx: Dump enclave memory map
2021-06-11 22:45 ` Shuah Khan
@ 2021-06-12 0:34 ` Dave Hansen
2021-06-12 4:27 ` Jarkko Sakkinen
1 sibling, 0 replies; 18+ messages in thread
From: Dave Hansen @ 2021-06-12 0:34 UTC (permalink / raw)
To: Shuah Khan, Jarkko Sakkinen, shuah
Cc: linux-kselftest, linux-sgx, Reinette Chatre, Dave Hansen, linux-kernel
On 6/11/21 3:45 PM, Shuah Khan wrote:
>>
>> @@ -167,6 +169,18 @@ FIXTURE_SETUP(enclave)
>> memset(&self->run, 0, sizeof(self->run));
>> self->run.tcs = self->encl.encl_base;
>> + maps_file = fopen("/proc/self/maps", "r");
>
> I almost applied these. Does this require root access, if so,
> please add logic to skip the test if non-root user runs it.
>
> Same comments for all other paths that might require root access.
This specifically doesn't require root. Anything can read its own
/proc/self/maps file.
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v8 3/5] selftests/sgx: Dump enclave memory map
2021-06-11 22:45 ` Shuah Khan
2021-06-12 0:34 ` Dave Hansen
@ 2021-06-12 4:27 ` Jarkko Sakkinen
2021-06-14 16:45 ` Shuah Khan
1 sibling, 1 reply; 18+ messages in thread
From: Jarkko Sakkinen @ 2021-06-12 4:27 UTC (permalink / raw)
To: Shuah Khan
Cc: shuah, linux-kselftest, linux-sgx, Reinette Chatre, Dave Hansen,
linux-kernel
On Fri, Jun 11, 2021 at 04:45:19PM -0600, Shuah Khan wrote:
> On 6/10/21 2:30 AM, Jarkko Sakkinen wrote:
> > Often, it's useful to check whether /proc/self/maps looks sane when
> > dealing with memory mapped objects, especially when they are JIT'ish
> > dynamically constructed objects. Therefore, dump "/dev/sgx_enclave"
> > matching lines from the memory map in FIXTURE_SETUP().
> >
> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > ---
> > tools/testing/selftests/sgx/main.c | 14 ++++++++++++++
> > 1 file changed, 14 insertions(+)
> >
> > diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
> > index 6da19b6bf287..14030f8b85ff 100644
> > --- a/tools/testing/selftests/sgx/main.c
> > +++ b/tools/testing/selftests/sgx/main.c
> > @@ -117,6 +117,8 @@ FIXTURE_SETUP(enclave)
> > Elf64_Sym *sgx_enter_enclave_sym = NULL;
> > struct vdso_symtab symtab;
> > struct encl_segment *seg;
> > + char maps_line[256];
> > + FILE *maps_file;
> > unsigned int i;
> > void *addr;
> > @@ -167,6 +169,18 @@ FIXTURE_SETUP(enclave)
> > memset(&self->run, 0, sizeof(self->run));
> > self->run.tcs = self->encl.encl_base;
> > + maps_file = fopen("/proc/self/maps", "r");
>
> I almost applied these. Does this require root access, if so,
> please add logic to skip the test if non-root user runs it.
>
> Same comments for all other paths that might require root access.
As Dave stated, it does not. A process can inspect its own state
through /proc/self path. E.g. Chrome web browser uses /proc/self/exe
to initialize multiple instances of itself for browser tabs...
As far as other things go, this patch set does not bind to any other
new OS resources.
> thanks,
> -- Shuah
/Jarkko
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v8 3/5] selftests/sgx: Dump enclave memory map
2021-06-12 4:27 ` Jarkko Sakkinen
@ 2021-06-14 16:45 ` Shuah Khan
2021-06-15 13:07 ` Jarkko Sakkinen
0 siblings, 1 reply; 18+ messages in thread
From: Shuah Khan @ 2021-06-14 16:45 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: shuah, linux-kselftest, linux-sgx, Reinette Chatre, Dave Hansen,
linux-kernel, Shuah Khan
On 6/11/21 10:27 PM, Jarkko Sakkinen wrote:
> On Fri, Jun 11, 2021 at 04:45:19PM -0600, Shuah Khan wrote:
>> On 6/10/21 2:30 AM, Jarkko Sakkinen wrote:
>>> Often, it's useful to check whether /proc/self/maps looks sane when
>>> dealing with memory mapped objects, especially when they are JIT'ish
>>> dynamically constructed objects. Therefore, dump "/dev/sgx_enclave"
>>> matching lines from the memory map in FIXTURE_SETUP().
>>>
>>> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
>>> ---
>>> tools/testing/selftests/sgx/main.c | 14 ++++++++++++++
>>> 1 file changed, 14 insertions(+)
>>>
>>> diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
>>> index 6da19b6bf287..14030f8b85ff 100644
>>> --- a/tools/testing/selftests/sgx/main.c
>>> +++ b/tools/testing/selftests/sgx/main.c
>>> @@ -117,6 +117,8 @@ FIXTURE_SETUP(enclave)
>>> Elf64_Sym *sgx_enter_enclave_sym = NULL;
>>> struct vdso_symtab symtab;
>>> struct encl_segment *seg;
>>> + char maps_line[256];
>>> + FILE *maps_file;
>>> unsigned int i;
>>> void *addr;
>>> @@ -167,6 +169,18 @@ FIXTURE_SETUP(enclave)
>>> memset(&self->run, 0, sizeof(self->run));
>>> self->run.tcs = self->encl.encl_base;
>>> + maps_file = fopen("/proc/self/maps", "r");
>>
>> I almost applied these. Does this require root access, if so,
>> please add logic to skip the test if non-root user runs it.
>>
>> Same comments for all other paths that might require root access.
>
> As Dave stated, it does not. A process can inspect its own state
> through /proc/self path. E.g. Chrome web browser uses /proc/self/exe
> to initialize multiple instances of itself for browser tabs...
>
Ah yes. I missed the self part. Thanks for clarifying.
> As far as other things go, this patch set does not bind to any other
> new OS resources.
>
Thank you. I will apply these for 5.14-rc1.
thanks,
-- Shuah
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v8 3/5] selftests/sgx: Dump enclave memory map
2021-06-14 16:45 ` Shuah Khan
@ 2021-06-15 13:07 ` Jarkko Sakkinen
0 siblings, 0 replies; 18+ messages in thread
From: Jarkko Sakkinen @ 2021-06-15 13:07 UTC (permalink / raw)
To: Shuah Khan
Cc: shuah, linux-kselftest, linux-sgx, Reinette Chatre, Dave Hansen,
linux-kernel
On Mon, Jun 14, 2021 at 10:45:43AM -0600, Shuah Khan wrote:
> On 6/11/21 10:27 PM, Jarkko Sakkinen wrote:
> > On Fri, Jun 11, 2021 at 04:45:19PM -0600, Shuah Khan wrote:
> > > On 6/10/21 2:30 AM, Jarkko Sakkinen wrote:
> > > > Often, it's useful to check whether /proc/self/maps looks sane when
> > > > dealing with memory mapped objects, especially when they are JIT'ish
> > > > dynamically constructed objects. Therefore, dump "/dev/sgx_enclave"
> > > > matching lines from the memory map in FIXTURE_SETUP().
> > > >
> > > > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > > > ---
> > > > tools/testing/selftests/sgx/main.c | 14 ++++++++++++++
> > > > 1 file changed, 14 insertions(+)
> > > >
> > > > diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
> > > > index 6da19b6bf287..14030f8b85ff 100644
> > > > --- a/tools/testing/selftests/sgx/main.c
> > > > +++ b/tools/testing/selftests/sgx/main.c
> > > > @@ -117,6 +117,8 @@ FIXTURE_SETUP(enclave)
> > > > Elf64_Sym *sgx_enter_enclave_sym = NULL;
> > > > struct vdso_symtab symtab;
> > > > struct encl_segment *seg;
> > > > + char maps_line[256];
> > > > + FILE *maps_file;
> > > > unsigned int i;
> > > > void *addr;
> > > > @@ -167,6 +169,18 @@ FIXTURE_SETUP(enclave)
> > > > memset(&self->run, 0, sizeof(self->run));
> > > > self->run.tcs = self->encl.encl_base;
> > > > + maps_file = fopen("/proc/self/maps", "r");
> > >
> > > I almost applied these. Does this require root access, if so,
> > > please add logic to skip the test if non-root user runs it.
> > >
> > > Same comments for all other paths that might require root access.
> >
> > As Dave stated, it does not. A process can inspect its own state
> > through /proc/self path. E.g. Chrome web browser uses /proc/self/exe
> > to initialize multiple instances of itself for browser tabs...
> >
>
> Ah yes. I missed the self part. Thanks for clarifying.
>
> > As far as other things go, this patch set does not bind to any other
> > new OS resources.
> >
>
> Thank you. I will apply these for 5.14-rc1.
Thank you!
> thanks,
> -- Shuah
/Jarkko
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v8 4/5] selftests/sgx: Add EXPECT_EEXIT() macro
2021-06-10 8:30 [PATCH v8 1/5] selftests/sgx: Rename 'eenter' and 'sgx_call_vdso' Jarkko Sakkinen
2021-06-10 8:30 ` [PATCH v8 2/5] selftests/sgx: Migrate to kselftest harness Jarkko Sakkinen
2021-06-10 8:30 ` [PATCH v8 3/5] selftests/sgx: Dump enclave memory map Jarkko Sakkinen
@ 2021-06-10 8:30 ` Jarkko Sakkinen
2021-06-10 8:30 ` [PATCH v8 5/5] selftests/sgx: Refine the test enclave to have storage Jarkko Sakkinen
2021-06-10 15:45 ` [PATCH v8 1/5] selftests/sgx: Rename 'eenter' and 'sgx_call_vdso' Dave Hansen
4 siblings, 0 replies; 18+ messages in thread
From: Jarkko Sakkinen @ 2021-06-10 8:30 UTC (permalink / raw)
To: shuah
Cc: linux-kselftest, linux-sgx, Reinette Chatre, Jarkko Sakkinen,
Dave Hansen, linux-kernel
Add EXPECT_EEXIT() macro, which will conditionally print the exception
information, in addition to
EXPECT_EQ(self->run.function, EEXIT);
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
tools/testing/selftests/sgx/main.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
index 14030f8b85ff..bcd0257f48e0 100644
--- a/tools/testing/selftests/sgx/main.c
+++ b/tools/testing/selftests/sgx/main.c
@@ -205,6 +205,14 @@ FIXTURE_TEARDOWN(enclave)
ret; \
})
+#define EXPECT_EEXIT(run) \
+ do { \
+ EXPECT_EQ((run)->function, EEXIT); \
+ if ((run)->function != EEXIT) \
+ TH_LOG("0x%02x 0x%02x 0x%016llx", (run)->exception_vector, \
+ (run)->exception_error_code, (run)->exception_addr); \
+ } while (0)
+
TEST_F(enclave, unclobbered_vdso)
{
uint64_t result = 0;
@@ -212,7 +220,7 @@ TEST_F(enclave, unclobbered_vdso)
EXPECT_EQ(ENCL_CALL(&MAGIC, &result, &self->run, false), 0);
EXPECT_EQ(result, MAGIC);
- EXPECT_EQ(self->run.function, EEXIT);
+ EXPECT_EEXIT(&self->run);
EXPECT_EQ(self->run.user_data, 0);
}
@@ -223,7 +231,7 @@ TEST_F(enclave, clobbered_vdso)
EXPECT_EQ(ENCL_CALL(&MAGIC, &result, &self->run, true), 0);
EXPECT_EQ(result, MAGIC);
- EXPECT_EQ(self->run.function, EEXIT);
+ EXPECT_EEXIT(&self->run);
EXPECT_EQ(self->run.user_data, 0);
}
@@ -245,7 +253,7 @@ TEST_F(enclave, clobbered_vdso_and_user_function)
EXPECT_EQ(ENCL_CALL(&MAGIC, &result, &self->run, true), 0);
EXPECT_EQ(result, MAGIC);
- EXPECT_EQ(self->run.function, EEXIT);
+ EXPECT_EEXIT(&self->run);
EXPECT_EQ(self->run.user_data, 0);
}
--
2.31.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH v8 5/5] selftests/sgx: Refine the test enclave to have storage
2021-06-10 8:30 [PATCH v8 1/5] selftests/sgx: Rename 'eenter' and 'sgx_call_vdso' Jarkko Sakkinen
` (2 preceding siblings ...)
2021-06-10 8:30 ` [PATCH v8 4/5] selftests/sgx: Add EXPECT_EEXIT() macro Jarkko Sakkinen
@ 2021-06-10 8:30 ` Jarkko Sakkinen
2021-06-14 20:16 ` Shuah Khan
2021-06-10 15:45 ` [PATCH v8 1/5] selftests/sgx: Rename 'eenter' and 'sgx_call_vdso' Dave Hansen
4 siblings, 1 reply; 18+ messages in thread
From: Jarkko Sakkinen @ 2021-06-10 8:30 UTC (permalink / raw)
To: shuah
Cc: linux-kselftest, linux-sgx, Reinette Chatre, Jarkko Sakkinen,
Dave Hansen, linux-kernel
Extend the enclave to have two operations: ENCL_OP_PUT and ENCL_OP_GET.
ENCL_OP_PUT stores value inside the enclave address space and
ENCL_OP_GET reads it. The internal buffer can be later extended to be
variable size, and allow reclaimer tests.
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
tools/testing/selftests/sgx/defines.h | 10 ++++
tools/testing/selftests/sgx/main.c | 57 ++++++++++++++++++-----
tools/testing/selftests/sgx/test_encl.c | 19 +++++++-
tools/testing/selftests/sgx/test_encl.lds | 3 +-
4 files changed, 74 insertions(+), 15 deletions(-)
diff --git a/tools/testing/selftests/sgx/defines.h b/tools/testing/selftests/sgx/defines.h
index 0bd73428d2f3..f88562afcaa0 100644
--- a/tools/testing/selftests/sgx/defines.h
+++ b/tools/testing/selftests/sgx/defines.h
@@ -18,4 +18,14 @@
#include "../../../../arch/x86/include/asm/enclu.h"
#include "../../../../arch/x86/include/uapi/asm/sgx.h"
+enum encl_op_type {
+ ENCL_OP_PUT,
+ ENCL_OP_GET,
+};
+
+struct encl_op {
+ uint64_t type;
+ uint64_t buffer;
+};
+
#endif /* DEFINES_H */
diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
index bcd0257f48e0..e252015e0c15 100644
--- a/tools/testing/selftests/sgx/main.c
+++ b/tools/testing/selftests/sgx/main.c
@@ -193,14 +193,14 @@ FIXTURE_TEARDOWN(enclave)
encl_delete(&self->encl);
}
-#define ENCL_CALL(in, out, run, clobbered) \
+#define ENCL_CALL(op, run, clobbered) \
({ \
int ret; \
if ((clobbered)) \
- ret = vdso_sgx_enter_enclave((unsigned long)(in), (unsigned long)(out), 0, \
+ ret = vdso_sgx_enter_enclave((unsigned long)(op), 0, 0, \
EENTER, 0, 0, (run)); \
else \
- ret = sgx_enter_enclave((void *)(in), (void *)(out), 0, EENTER, NULL, NULL, \
+ ret = sgx_enter_enclave((void *)(op), NULL, 0, EENTER, NULL, NULL, \
(run)); \
ret; \
})
@@ -215,22 +215,44 @@ FIXTURE_TEARDOWN(enclave)
TEST_F(enclave, unclobbered_vdso)
{
- uint64_t result = 0;
+ struct encl_op op;
- EXPECT_EQ(ENCL_CALL(&MAGIC, &result, &self->run, false), 0);
+ op.type = ENCL_OP_PUT;
+ op.buffer = MAGIC;
+
+ EXPECT_EQ(ENCL_CALL(&op, &self->run, false), 0);
- EXPECT_EQ(result, MAGIC);
+ EXPECT_EEXIT(&self->run);
+ EXPECT_EQ(self->run.user_data, 0);
+
+ op.type = ENCL_OP_GET;
+ op.buffer = 0;
+
+ EXPECT_EQ(ENCL_CALL(&op, &self->run, false), 0);
+
+ EXPECT_EQ(op.buffer, MAGIC);
EXPECT_EEXIT(&self->run);
EXPECT_EQ(self->run.user_data, 0);
}
TEST_F(enclave, clobbered_vdso)
{
- uint64_t result = 0;
+ struct encl_op op;
+
+ op.type = ENCL_OP_PUT;
+ op.buffer = MAGIC;
+
+ EXPECT_EQ(ENCL_CALL(&op, &self->run, true), 0);
+
+ EXPECT_EEXIT(&self->run);
+ EXPECT_EQ(self->run.user_data, 0);
+
+ op.type = ENCL_OP_GET;
+ op.buffer = 0;
- EXPECT_EQ(ENCL_CALL(&MAGIC, &result, &self->run, true), 0);
+ EXPECT_EQ(ENCL_CALL(&op, &self->run, true), 0);
- EXPECT_EQ(result, MAGIC);
+ EXPECT_EQ(op.buffer, MAGIC);
EXPECT_EEXIT(&self->run);
EXPECT_EQ(self->run.user_data, 0);
}
@@ -245,14 +267,25 @@ static int test_handler(long rdi, long rsi, long rdx, long ursp, long r8, long r
TEST_F(enclave, clobbered_vdso_and_user_function)
{
- uint64_t result = 0;
+ struct encl_op op;
self->run.user_handler = (__u64)test_handler;
self->run.user_data = 0xdeadbeef;
- EXPECT_EQ(ENCL_CALL(&MAGIC, &result, &self->run, true), 0);
+ op.type = ENCL_OP_PUT;
+ op.buffer = MAGIC;
+
+ EXPECT_EQ(ENCL_CALL(&op, &self->run, true), 0);
+
+ EXPECT_EEXIT(&self->run);
+ EXPECT_EQ(self->run.user_data, 0);
+
+ op.type = ENCL_OP_GET;
+ op.buffer = 0;
+
+ EXPECT_EQ(ENCL_CALL(&op, &self->run, true), 0);
- EXPECT_EQ(result, MAGIC);
+ EXPECT_EQ(op.buffer, MAGIC);
EXPECT_EEXIT(&self->run);
EXPECT_EQ(self->run.user_data, 0);
}
diff --git a/tools/testing/selftests/sgx/test_encl.c b/tools/testing/selftests/sgx/test_encl.c
index cf25b5dc1e03..734ea52f9924 100644
--- a/tools/testing/selftests/sgx/test_encl.c
+++ b/tools/testing/selftests/sgx/test_encl.c
@@ -4,6 +4,8 @@
#include <stddef.h>
#include "defines.h"
+static uint8_t encl_buffer[8192] = { 1 };
+
static void *memcpy(void *dest, const void *src, size_t n)
{
size_t i;
@@ -14,7 +16,20 @@ static void *memcpy(void *dest, const void *src, size_t n)
return dest;
}
-void encl_body(void *rdi, void *rsi)
+void encl_body(void *rdi, void *rsi)
{
- memcpy(rsi, rdi, 8);
+ struct encl_op *op = (struct encl_op *)rdi;
+
+ switch (op->type) {
+ case ENCL_OP_PUT:
+ memcpy(&encl_buffer[0], &op->buffer, 8);
+ break;
+
+ case ENCL_OP_GET:
+ memcpy(&op->buffer, &encl_buffer[0], 8);
+ break;
+
+ default:
+ break;
+ }
}
diff --git a/tools/testing/selftests/sgx/test_encl.lds b/tools/testing/selftests/sgx/test_encl.lds
index 0fbbda7e665e..a1ec64f7d91f 100644
--- a/tools/testing/selftests/sgx/test_encl.lds
+++ b/tools/testing/selftests/sgx/test_encl.lds
@@ -18,9 +18,10 @@ SECTIONS
.text : {
*(.text*)
*(.rodata*)
+ FILL(0xDEADBEEF);
+ . = ALIGN(4096);
} : text
- . = ALIGN(4096);
.data : {
*(.data*)
} : data
--
2.31.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v8 5/5] selftests/sgx: Refine the test enclave to have storage
2021-06-10 8:30 ` [PATCH v8 5/5] selftests/sgx: Refine the test enclave to have storage Jarkko Sakkinen
@ 2021-06-14 20:16 ` Shuah Khan
2021-06-15 13:13 ` Jarkko Sakkinen
0 siblings, 1 reply; 18+ messages in thread
From: Shuah Khan @ 2021-06-14 20:16 UTC (permalink / raw)
To: Jarkko Sakkinen, shuah
Cc: linux-kselftest, linux-sgx, Reinette Chatre, Dave Hansen,
linux-kernel, Shuah Khan
On 6/10/21 2:30 AM, Jarkko Sakkinen wrote:
> Extend the enclave to have two operations: ENCL_OP_PUT and ENCL_OP_GET.
> ENCL_OP_PUT stores value inside the enclave address space and
> ENCL_OP_GET reads it. The internal buffer can be later extended to be
> variable size, and allow reclaimer tests.
>
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> ---
> tools/testing/selftests/sgx/defines.h | 10 ++++
> tools/testing/selftests/sgx/main.c | 57 ++++++++++++++++++-----
> tools/testing/selftests/sgx/test_encl.c | 19 +++++++-
> tools/testing/selftests/sgx/test_encl.lds | 3 +-
> 4 files changed, 74 insertions(+), 15 deletions(-)
>
Test output before applying the series:
TAP version 13
1..1
# selftests: sgx: test_sgx
# Unable to open /dev/sgx_enclave: No such file or directory
# 1..0 # SKIP cannot load enclaves
ok 1 selftests: sgx: test_sgx # SKIP
Test output after applying second patch
selftests/sgx: Migrate to kselftest harness
Output changes to the following. It doesn't look like the second
patch adds any new tests. What is the point in running the tests
that fail if /dev/sgx_enclave is missing.
Unfortunately this series doesn't have a cover letter that explains
what this series is doing. I don't like the fact that the test
output and behavior changes when migrating the test to kselftest
harness. Shouldn't the output stay the same as in skip the tests
if /dev/sgx_enclave fails.
TAP version 13
1..1
# selftests: sgx: test_sgx
# TAP version 13
# 1..3
# # Starting 3 tests from 2 test cases.
# # RUN enclave.unclobbered_vdso ...
# Unable to open /dev/sgx_enclave: No such file or directory
# ok 2 # SKIP cannot load enclaves
# # Planned tests != run tests (3 != 1)
# # Totals: pass:0 fail:0 xfail:0 xpass:0 skip:1 error:0
# # unclobbered_vdso: Test failed at step #4
# # FAIL enclave.unclobbered_vdso
# not ok 1 enclave.unclobbered_vdso
# # RUN enclave.clobbered_vdso ...
# Unable to open /dev/sgx_enclave: No such file or directory
# ok 3 # SKIP cannot load enclaves
# # Planned tests != run tests (3 != 2)
# # Totals: pass:0 fail:1 xfail:0 xpass:0 skip:1 error:0
# # clobbered_vdso: Test failed at step #4
# # FAIL enclave.clobbered_vdso
# not ok 2 enclave.clobbered_vdso
# # RUN enclave.clobbered_vdso_and_user_function ...
# Unable to open /dev/sgx_enclave: No such file or directory
# ok 4 # SKIP cannot load enclaves
# # Totals: pass:0 fail:2 xfail:0 xpass:0 skip:1 error:0
# # clobbered_vdso_and_user_function: Test failed at step #4
# # FAIL enclave.clobbered_vdso_and_user_function
# not ok 3 enclave.clobbered_vdso_and_user_function
# # FAILED: 0 / 3 tests passed.
# # Totals: pass:0 fail:3 xfail:0 xpass:0 skip:0 error:0
not ok 1 selftests: sgx: test_sgx # exit=1
thanks,
-- Shuah
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v8 5/5] selftests/sgx: Refine the test enclave to have storage
2021-06-14 20:16 ` Shuah Khan
@ 2021-06-15 13:13 ` Jarkko Sakkinen
2021-06-15 13:15 ` Jarkko Sakkinen
0 siblings, 1 reply; 18+ messages in thread
From: Jarkko Sakkinen @ 2021-06-15 13:13 UTC (permalink / raw)
To: Shuah Khan
Cc: shuah, linux-kselftest, linux-sgx, Reinette Chatre, Dave Hansen,
linux-kernel
On Mon, Jun 14, 2021 at 02:16:15PM -0600, Shuah Khan wrote:
> On 6/10/21 2:30 AM, Jarkko Sakkinen wrote:
> > Extend the enclave to have two operations: ENCL_OP_PUT and ENCL_OP_GET.
> > ENCL_OP_PUT stores value inside the enclave address space and
> > ENCL_OP_GET reads it. The internal buffer can be later extended to be
> > variable size, and allow reclaimer tests.
> >
> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > ---
> > tools/testing/selftests/sgx/defines.h | 10 ++++
> > tools/testing/selftests/sgx/main.c | 57 ++++++++++++++++++-----
> > tools/testing/selftests/sgx/test_encl.c | 19 +++++++-
> > tools/testing/selftests/sgx/test_encl.lds | 3 +-
> > 4 files changed, 74 insertions(+), 15 deletions(-)
> >
>
> Test output before applying the series:
>
> TAP version 13
> 1..1
> # selftests: sgx: test_sgx
> # Unable to open /dev/sgx_enclave: No such file or directory
> # 1..0 # SKIP cannot load enclaves
> ok 1 selftests: sgx: test_sgx # SKIP
>
> Test output after applying second patch
>
> selftests/sgx: Migrate to kselftest harness
>
> Output changes to the following. It doesn't look like the second
> patch adds any new tests. What is the point in running the tests
> that fail if /dev/sgx_enclave is missing.
>
> Unfortunately this series doesn't have a cover letter that explains
> what this series is doing. I don't like the fact that the test
> output and behavior changes when migrating the test to kselftest
> harness. Shouldn't the output stay the same as in skip the tests
> if /dev/sgx_enclave fails.
I get what you are saying but actually I do not know how with
fixtures I can skip "the rest" when FIXTURE_SETUP() fails.
The reason for the output below is that with fixtures for all
tests enclave is initialized for each test case. And it kind of
makes sense because all tests start from the clean expected
state.
I don't how to do that with zero change in the output.
The reason to do this change is to make it easy to add more tests,
and return correct status codes to the framework.
> TAP version 13
> 1..1
> # selftests: sgx: test_sgx
> # TAP version 13
> # 1..3
> # # Starting 3 tests from 2 test cases.
> # # RUN enclave.unclobbered_vdso ...
> # Unable to open /dev/sgx_enclave: No such file or directory
> # ok 2 # SKIP cannot load enclaves
> # # Planned tests != run tests (3 != 1)
> # # Totals: pass:0 fail:0 xfail:0 xpass:0 skip:1 error:0
> # # unclobbered_vdso: Test failed at step #4
> # # FAIL enclave.unclobbered_vdso
> # not ok 1 enclave.unclobbered_vdso
> # # RUN enclave.clobbered_vdso ...
> # Unable to open /dev/sgx_enclave: No such file or directory
> # ok 3 # SKIP cannot load enclaves
> # # Planned tests != run tests (3 != 2)
> # # Totals: pass:0 fail:1 xfail:0 xpass:0 skip:1 error:0
> # # clobbered_vdso: Test failed at step #4
> # # FAIL enclave.clobbered_vdso
> # not ok 2 enclave.clobbered_vdso
> # # RUN enclave.clobbered_vdso_and_user_function ...
> # Unable to open /dev/sgx_enclave: No such file or directory
> # ok 4 # SKIP cannot load enclaves
> # # Totals: pass:0 fail:2 xfail:0 xpass:0 skip:1 error:0
> # # clobbered_vdso_and_user_function: Test failed at step #4
> # # FAIL enclave.clobbered_vdso_and_user_function
> # not ok 3 enclave.clobbered_vdso_and_user_function
> # # FAILED: 0 / 3 tests passed.
> # # Totals: pass:0 fail:3 xfail:0 xpass:0 skip:0 error:0
> not ok 1 selftests: sgx: test_sgx # exit=1
>
> thanks,
> -- Shuah
/Jarkko
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v8 5/5] selftests/sgx: Refine the test enclave to have storage
2021-06-15 13:13 ` Jarkko Sakkinen
@ 2021-06-15 13:15 ` Jarkko Sakkinen
2021-06-15 21:55 ` Shuah Khan
0 siblings, 1 reply; 18+ messages in thread
From: Jarkko Sakkinen @ 2021-06-15 13:15 UTC (permalink / raw)
To: Shuah Khan
Cc: shuah, linux-kselftest, linux-sgx, Reinette Chatre, Dave Hansen,
linux-kernel
On Tue, Jun 15, 2021 at 04:14:02PM +0300, Jarkko Sakkinen wrote:
> On Mon, Jun 14, 2021 at 02:16:15PM -0600, Shuah Khan wrote:
> > On 6/10/21 2:30 AM, Jarkko Sakkinen wrote:
> > > Extend the enclave to have two operations: ENCL_OP_PUT and ENCL_OP_GET.
> > > ENCL_OP_PUT stores value inside the enclave address space and
> > > ENCL_OP_GET reads it. The internal buffer can be later extended to be
> > > variable size, and allow reclaimer tests.
> > >
> > > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > > ---
> > > tools/testing/selftests/sgx/defines.h | 10 ++++
> > > tools/testing/selftests/sgx/main.c | 57 ++++++++++++++++++-----
> > > tools/testing/selftests/sgx/test_encl.c | 19 +++++++-
> > > tools/testing/selftests/sgx/test_encl.lds | 3 +-
> > > 4 files changed, 74 insertions(+), 15 deletions(-)
> > >
> >
> > Test output before applying the series:
> >
> > TAP version 13
> > 1..1
> > # selftests: sgx: test_sgx
> > # Unable to open /dev/sgx_enclave: No such file or directory
> > # 1..0 # SKIP cannot load enclaves
> > ok 1 selftests: sgx: test_sgx # SKIP
> >
> > Test output after applying second patch
> >
> > selftests/sgx: Migrate to kselftest harness
> >
> > Output changes to the following. It doesn't look like the second
> > patch adds any new tests. What is the point in running the tests
> > that fail if /dev/sgx_enclave is missing.
> >
> > Unfortunately this series doesn't have a cover letter that explains
> > what this series is doing. I don't like the fact that the test
> > output and behavior changes when migrating the test to kselftest
> > harness. Shouldn't the output stay the same as in skip the tests
> > if /dev/sgx_enclave fails.
>
> I get what you are saying but actually I do not know how with
> fixtures I can skip "the rest" when FIXTURE_SETUP() fails.
>
> The reason for the output below is that with fixtures for all
> tests enclave is initialized for each test case. And it kind of
> makes sense because all tests start from the clean expected
> state.
>
> I don't how to do that with zero change in the output.
>
> The reason to do this change is to make it easy to add more tests,
> and return correct status codes to the framework.
To add: everything I did I based purely to the existing kernel
documentation, following the examples on how to use fixture.
/Jarkko
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v8 5/5] selftests/sgx: Refine the test enclave to have storage
2021-06-15 13:15 ` Jarkko Sakkinen
@ 2021-06-15 21:55 ` Shuah Khan
2021-06-18 9:07 ` Jarkko Sakkinen
0 siblings, 1 reply; 18+ messages in thread
From: Shuah Khan @ 2021-06-15 21:55 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: shuah, linux-kselftest, linux-sgx, Reinette Chatre, Dave Hansen,
linux-kernel, Shuah Khan
On 6/15/21 7:15 AM, Jarkko Sakkinen wrote:
> On Tue, Jun 15, 2021 at 04:14:02PM +0300, Jarkko Sakkinen wrote:
>> On Mon, Jun 14, 2021 at 02:16:15PM -0600, Shuah Khan wrote:
>>> On 6/10/21 2:30 AM, Jarkko Sakkinen wrote:
>>>> Extend the enclave to have two operations: ENCL_OP_PUT and ENCL_OP_GET.
>>>> ENCL_OP_PUT stores value inside the enclave address space and
>>>> ENCL_OP_GET reads it. The internal buffer can be later extended to be
>>>> variable size, and allow reclaimer tests.
>>>>
>>>> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
>>>> ---
>>>> tools/testing/selftests/sgx/defines.h | 10 ++++
>>>> tools/testing/selftests/sgx/main.c | 57 ++++++++++++++++++-----
>>>> tools/testing/selftests/sgx/test_encl.c | 19 +++++++-
>>>> tools/testing/selftests/sgx/test_encl.lds | 3 +-
>>>> 4 files changed, 74 insertions(+), 15 deletions(-)
>>>>
>>>
>>> Test output before applying the series:
>>>
>>> TAP version 13
>>> 1..1
>>> # selftests: sgx: test_sgx
>>> # Unable to open /dev/sgx_enclave: No such file or directory
>>> # 1..0 # SKIP cannot load enclaves
>>> ok 1 selftests: sgx: test_sgx # SKIP
>>>
>>> Test output after applying second patch
>>>
>>> selftests/sgx: Migrate to kselftest harness
>>>
>>> Output changes to the following. It doesn't look like the second
>>> patch adds any new tests. What is the point in running the tests
>>> that fail if /dev/sgx_enclave is missing.
>>>
>>> Unfortunately this series doesn't have a cover letter that explains
>>> what this series is doing. I don't like the fact that the test
>>> output and behavior changes when migrating the test to kselftest
>>> harness. Shouldn't the output stay the same as in skip the tests
>>> if /dev/sgx_enclave fails.
>>
>> I get what you are saying but actually I do not know how with
>> fixtures I can skip "the rest" when FIXTURE_SETUP() fails.
>>
>> The reason for the output below is that with fixtures for all
>> tests enclave is initialized for each test case. And it kind of
>> makes sense because all tests start from the clean expected
>> state.
>>
>> I don't how to do that with zero change in the output.
>>
Yeah. I took a look at the FIXTURE. Doesn't look like it is possible.
>> The reason to do this change is to make it easy to add more tests,
>> and return correct status codes to the framework.
>
> To add: everything I did I based purely to the existing kernel
> documentation, following the examples on how to use fixture.
>
I will pick these up and will add a note to the last commit that
output changes, so test rings that run kselftest are aware of the
change.
thanks,
-- Shuah
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v8 5/5] selftests/sgx: Refine the test enclave to have storage
2021-06-15 21:55 ` Shuah Khan
@ 2021-06-18 9:07 ` Jarkko Sakkinen
0 siblings, 0 replies; 18+ messages in thread
From: Jarkko Sakkinen @ 2021-06-18 9:07 UTC (permalink / raw)
To: Shuah Khan
Cc: shuah, linux-kselftest, linux-sgx, Reinette Chatre, Dave Hansen,
linux-kernel
On Tue, Jun 15, 2021 at 03:55:25PM -0600, Shuah Khan wrote:
> On 6/15/21 7:15 AM, Jarkko Sakkinen wrote:
> > On Tue, Jun 15, 2021 at 04:14:02PM +0300, Jarkko Sakkinen wrote:
> > > On Mon, Jun 14, 2021 at 02:16:15PM -0600, Shuah Khan wrote:
> > > > On 6/10/21 2:30 AM, Jarkko Sakkinen wrote:
> > > > > Extend the enclave to have two operations: ENCL_OP_PUT and ENCL_OP_GET.
> > > > > ENCL_OP_PUT stores value inside the enclave address space and
> > > > > ENCL_OP_GET reads it. The internal buffer can be later extended to be
> > > > > variable size, and allow reclaimer tests.
> > > > >
> > > > > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > > > > ---
> > > > > tools/testing/selftests/sgx/defines.h | 10 ++++
> > > > > tools/testing/selftests/sgx/main.c | 57 ++++++++++++++++++-----
> > > > > tools/testing/selftests/sgx/test_encl.c | 19 +++++++-
> > > > > tools/testing/selftests/sgx/test_encl.lds | 3 +-
> > > > > 4 files changed, 74 insertions(+), 15 deletions(-)
> > > > >
> > > >
> > > > Test output before applying the series:
> > > >
> > > > TAP version 13
> > > > 1..1
> > > > # selftests: sgx: test_sgx
> > > > # Unable to open /dev/sgx_enclave: No such file or directory
> > > > # 1..0 # SKIP cannot load enclaves
> > > > ok 1 selftests: sgx: test_sgx # SKIP
> > > >
> > > > Test output after applying second patch
> > > >
> > > > selftests/sgx: Migrate to kselftest harness
> > > >
> > > > Output changes to the following. It doesn't look like the second
> > > > patch adds any new tests. What is the point in running the tests
> > > > that fail if /dev/sgx_enclave is missing.
> > > >
> > > > Unfortunately this series doesn't have a cover letter that explains
> > > > what this series is doing. I don't like the fact that the test
> > > > output and behavior changes when migrating the test to kselftest
> > > > harness. Shouldn't the output stay the same as in skip the tests
> > > > if /dev/sgx_enclave fails.
> > >
> > > I get what you are saying but actually I do not know how with
> > > fixtures I can skip "the rest" when FIXTURE_SETUP() fails.
> > >
> > > The reason for the output below is that with fixtures for all
> > > tests enclave is initialized for each test case. And it kind of
> > > makes sense because all tests start from the clean expected
> > > state.
> > >
> > > I don't how to do that with zero change in the output.
> > >
>
> Yeah. I took a look at the FIXTURE. Doesn't look like it is possible.
>
> > > The reason to do this change is to make it easy to add more tests,
> > > and return correct status codes to the framework.
> >
> > To add: everything I did I based purely to the existing kernel
> > documentation, following the examples on how to use fixture.
> >
>
> I will pick these up and will add a note to the last commit that
> output changes, so test rings that run kselftest are aware of the
> change.
>
> thanks,
> -- Shuah
OK, great, thank you!
/Jarkko
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v8 1/5] selftests/sgx: Rename 'eenter' and 'sgx_call_vdso'
2021-06-10 8:30 [PATCH v8 1/5] selftests/sgx: Rename 'eenter' and 'sgx_call_vdso' Jarkko Sakkinen
` (3 preceding siblings ...)
2021-06-10 8:30 ` [PATCH v8 5/5] selftests/sgx: Refine the test enclave to have storage Jarkko Sakkinen
@ 2021-06-10 15:45 ` Dave Hansen
2021-06-11 17:35 ` Shuah Khan
4 siblings, 1 reply; 18+ messages in thread
From: Dave Hansen @ 2021-06-10 15:45 UTC (permalink / raw)
To: Jarkko Sakkinen, shuah
Cc: linux-kselftest, linux-sgx, Reinette Chatre, Dave Hansen, linux-kernel
On 6/10/21 1:30 AM, Jarkko Sakkinen wrote:
> Rename symbols for better clarity:
>
> * 'eenter' might be confused for directly calling ENCLU[EENTER]. It does
> not. It calls into the VDSO, which actually has the EENTER instruction.
> * 'sgx_call_vdso' is *only* used for entering the enclave. It's not some
> generic SGX call into the VDSO.
>
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
These all look fine to me. Feel free to add my ack on them.
Since these are pure x86 selftests and the initial code went through the
x86 maintainers, should these got through them as well? Or, since this
is only selftest code, should Shuah pick them up?
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v8 1/5] selftests/sgx: Rename 'eenter' and 'sgx_call_vdso'
2021-06-10 15:45 ` [PATCH v8 1/5] selftests/sgx: Rename 'eenter' and 'sgx_call_vdso' Dave Hansen
@ 2021-06-11 17:35 ` Shuah Khan
2021-06-11 22:47 ` Shuah Khan
0 siblings, 1 reply; 18+ messages in thread
From: Shuah Khan @ 2021-06-11 17:35 UTC (permalink / raw)
To: Dave Hansen, Jarkko Sakkinen, shuah
Cc: linux-kselftest, linux-sgx, Reinette Chatre, Dave Hansen,
linux-kernel, Shuah Khan
On 6/10/21 9:45 AM, Dave Hansen wrote:
> On 6/10/21 1:30 AM, Jarkko Sakkinen wrote:
>> Rename symbols for better clarity:
>>
>> * 'eenter' might be confused for directly calling ENCLU[EENTER]. It does
>> not. It calls into the VDSO, which actually has the EENTER instruction.
>> * 'sgx_call_vdso' is *only* used for entering the enclave. It's not some
>> generic SGX call into the VDSO.
>>
>> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
>
> These all look fine to me. Feel free to add my ack on them.
>
> Since these are pure x86 selftests and the initial code went through the
> x86 maintainers, should these got through them as well? Or, since this
> is only selftest code, should Shuah pick them up?
>
I will queue these up for 5.14-rc1
thanks,
-- Shuah
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v8 1/5] selftests/sgx: Rename 'eenter' and 'sgx_call_vdso'
2021-06-11 17:35 ` Shuah Khan
@ 2021-06-11 22:47 ` Shuah Khan
0 siblings, 0 replies; 18+ messages in thread
From: Shuah Khan @ 2021-06-11 22:47 UTC (permalink / raw)
To: Dave Hansen, Jarkko Sakkinen, shuah
Cc: linux-kselftest, linux-sgx, Reinette Chatre, Dave Hansen,
linux-kernel, Shuah Khan
On 6/11/21 11:35 AM, Shuah Khan wrote:
> On 6/10/21 9:45 AM, Dave Hansen wrote:
>> On 6/10/21 1:30 AM, Jarkko Sakkinen wrote:
>>> Rename symbols for better clarity:
>>>
>>> * 'eenter' might be confused for directly calling ENCLU[EENTER]. It
>>> does
>>> not. It calls into the VDSO, which actually has the EENTER
>>> instruction.
>>> * 'sgx_call_vdso' is *only* used for entering the enclave. It's not
>>> some
>>> generic SGX call into the VDSO.
>>>
>>> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
>>
>> These all look fine to me. Feel free to add my ack on them.
>>
>> Since these are pure x86 selftests and the initial code went through the
>> x86 maintainers, should these got through them as well? Or, since this
>> is only selftest code, should Shuah pick them up?
>>
>
> I will queue these up for 5.14-rc1
>
I almost applied these. Does this require root access, if so,
please add logic to skip the test if non-root user runs it.
Please check for code paths that require root access and skip
the tests.
thanks,
-- Shuah
^ permalink raw reply [flat|nested] 18+ messages in thread