[PATCH v2 1/5] selftests/sgx: Retry the ioctl()'s returned with EAGAIN

[PATCH v2 1/5] selftests/sgx: Retry the ioctl()'s returned with EAGAIN

[Jarkko Sakkinen]

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

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

Change the unclobbered_vdso_oversubscribed_remove test
to rerun the ioctl()'s in a loop, updating offset and length
using the byte count returned in each iteration.

Fixes: 6507cce561b4 ("selftests/sgx: Page removal stress test")
Signed-off-by: Haitao Huang <haitao.huang@linux.intel.com>
Tested-by: Jarkko Sakkinen <jarkko@kernel.org>
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
v3:
* Added a fixes tag. The bug is in v6.0 patches.
* Added my tested-by (the bug reproduced in my NUC often).
v2:
* Changed branching in EAGAIN condition so that else branch
  is not required.
* Addressed Reinette's feedback:
---
 tools/testing/selftests/sgx/main.c | 42 ++++++++++++++++++++++++------
 1 file changed, 34 insertions(+), 8 deletions(-)

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

Re: [PATCH v2 1/5] selftests/sgx: Retry the ioctl()'s returned with EAGAIN

[Reinette Chatre]

Hi Jarkko and Haitao,

On 9/4/2022 7:04 PM, Jarkko Sakkinen wrote:
> From: Haitao Huang <haitao.huang@linux.intel.com>
> 
> For EMODT and EREMOVE ioctl()'s with a large range, kernel
> may not finish in one shot and return EAGAIN error code
> and count of bytes of EPC pages on that operations are
> finished successfully.
> 
> Change the unclobbered_vdso_oversubscribed_remove test
> to rerun the ioctl()'s in a loop, updating offset and length
> using the byte count returned in each iteration.
> 
> Fixes: 6507cce561b4 ("selftests/sgx: Page removal stress test")

Should this patch be moved to the "critical fixes for v6.0" series?

> Signed-off-by: Haitao Huang <haitao.huang@linux.intel.com>
> Tested-by: Jarkko Sakkinen <jarkko@kernel.org>
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> ---
> v3:
> * Added a fixes tag. The bug is in v6.0 patches.
> * Added my tested-by (the bug reproduced in my NUC often).
> v2:
> * Changed branching in EAGAIN condition so that else branch
>   is not required.
> * Addressed Reinette's feedback:
> ---
>  tools/testing/selftests/sgx/main.c | 42 ++++++++++++++++++++++++------
>  1 file changed, 34 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
> index 9820b3809c69..59cca806eda1 100644
> --- a/tools/testing/selftests/sgx/main.c
> +++ b/tools/testing/selftests/sgx/main.c
> @@ -390,6 +390,7 @@ TEST_F_TIMEOUT(enclave, unclobbered_vdso_oversubscribed_remove, 900)
>  	struct encl_segment *heap;
>  	unsigned long total_mem;
>  	int ret, errno_save;
> +	unsigned long count;
>  	unsigned long addr;
>  	unsigned long i;
>  
> @@ -453,16 +454,30 @@ TEST_F_TIMEOUT(enclave, unclobbered_vdso_oversubscribed_remove, 900)
>  	modt_ioc.offset = heap->offset;
>  	modt_ioc.length = heap->size;
>  	modt_ioc.page_type = SGX_PAGE_TYPE_TRIM;
> -
> +	count = 0;
>  	TH_LOG("Changing type of %zd bytes to trimmed may take a while ...",
>  	       heap->size);
> -	ret = ioctl(self->encl.fd, SGX_IOC_ENCLAVE_MODIFY_TYPES, &modt_ioc);
> -	errno_save = ret == -1 ? errno : 0;
> +	do {
> +		ret = ioctl(self->encl.fd, SGX_IOC_ENCLAVE_MODIFY_TYPES, &modt_ioc);
> +
> +		errno_save = ret == -1 ? errno : 0;
> +		if (errno_save != EAGAIN)
> +			break;
> +
> +		EXPECT_EQ(modt_ioc.result, 0);

If this check triggers then there is something seriously wrong and in that case
it may also be that this loop may be unable to terminate or the error condition would
keep appearing until the loop terminates (which may be many iterations). Considering
the severity and risk I do think that ASSERT_EQ() would be more appropriate,
similar to how ASSERT_EQ() is used in patch 5/5.

Apart from that I think that this looks good.

Thank you very much for adding this.

Reinette

Re: [PATCH v2 1/5] selftests/sgx: Retry the ioctl()'s returned with EAGAIN

[Jarkko Sakkinen]

On Thu, Sep 08, 2022 at 03:43:06PM -0700, Reinette Chatre wrote:
> Hi Jarkko and Haitao,
> 
> On 9/4/2022 7:04 PM, Jarkko Sakkinen wrote:
> > From: Haitao Huang <haitao.huang@linux.intel.com>
> > 
> > For EMODT and EREMOVE ioctl()'s with a large range, kernel
> > may not finish in one shot and return EAGAIN error code
> > and count of bytes of EPC pages on that operations are
> > finished successfully.
> > 
> > Change the unclobbered_vdso_oversubscribed_remove test
> > to rerun the ioctl()'s in a loop, updating offset and length
> > using the byte count returned in each iteration.
> > 
> > Fixes: 6507cce561b4 ("selftests/sgx: Page removal stress test")
> 
> Should this patch be moved to the "critical fixes for v6.0" series?

I think not because it does not risk stability of the
kernel itself. It's "nice to have" but not mandatory.

> 
> > Signed-off-by: Haitao Huang <haitao.huang@linux.intel.com>
> > Tested-by: Jarkko Sakkinen <jarkko@kernel.org>
> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > ---
> > v3:
> > * Added a fixes tag. The bug is in v6.0 patches.
> > * Added my tested-by (the bug reproduced in my NUC often).
> > v2:
> > * Changed branching in EAGAIN condition so that else branch
> >   is not required.
> > * Addressed Reinette's feedback:
> > ---
> >  tools/testing/selftests/sgx/main.c | 42 ++++++++++++++++++++++++------
> >  1 file changed, 34 insertions(+), 8 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
> > index 9820b3809c69..59cca806eda1 100644
> > --- a/tools/testing/selftests/sgx/main.c
> > +++ b/tools/testing/selftests/sgx/main.c
> > @@ -390,6 +390,7 @@ TEST_F_TIMEOUT(enclave, unclobbered_vdso_oversubscribed_remove, 900)
> >  	struct encl_segment *heap;
> >  	unsigned long total_mem;
> >  	int ret, errno_save;
> > +	unsigned long count;
> >  	unsigned long addr;
> >  	unsigned long i;
> >  
> > @@ -453,16 +454,30 @@ TEST_F_TIMEOUT(enclave, unclobbered_vdso_oversubscribed_remove, 900)
> >  	modt_ioc.offset = heap->offset;
> >  	modt_ioc.length = heap->size;
> >  	modt_ioc.page_type = SGX_PAGE_TYPE_TRIM;
> > -
> > +	count = 0;
> >  	TH_LOG("Changing type of %zd bytes to trimmed may take a while ...",
> >  	       heap->size);
> > -	ret = ioctl(self->encl.fd, SGX_IOC_ENCLAVE_MODIFY_TYPES, &modt_ioc);
> > -	errno_save = ret == -1 ? errno : 0;
> > +	do {
> > +		ret = ioctl(self->encl.fd, SGX_IOC_ENCLAVE_MODIFY_TYPES, &modt_ioc);
> > +
> > +		errno_save = ret == -1 ? errno : 0;
> > +		if (errno_save != EAGAIN)
> > +			break;
> > +
> > +		EXPECT_EQ(modt_ioc.result, 0);
> 
> If this check triggers then there is something seriously wrong and in that case
> it may also be that this loop may be unable to terminate or the error condition would
> keep appearing until the loop terminates (which may be many iterations). Considering
> the severity and risk I do think that ASSERT_EQ() would be more appropriate,
> similar to how ASSERT_EQ() is used in patch 5/5.
> 
> Apart from that I think that this looks good.
> 
> Thank you very much for adding this.
> 
> Reinette

Hmm... I could along the lines:

/*
 * Get time since Epoch is milliseconds.
 */
unsigned long get_time(void)
{
    struct timeval start;

    gettimeofday(&start, NULL);

    return (unsigneg long)start.tv_sec * 1000L + (unsigned long)start.tv_usec / 1000L;
}

and

#define IOCTL_RETRY_TIMEOUT 100

In the test function:

        unsigned long start_time;

        /* ... */

        start_time = get_time();
        do {
                EXPECT_LT(get_time() - start_time(), IOCTL_RETRY_TIMEOUT);

                /* ... */
        }

        /* ... */

What do you think?

BR, Jarkko

Re: [PATCH v2 1/5] selftests/sgx: Retry the ioctl()'s returned with EAGAIN

[Jarkko Sakkinen]

On Fri, Sep 09, 2022 at 02:19:40AM +0300, Jarkko Sakkinen wrote:
>                 EXPECT_LT(get_time() - start_time(), IOCTL_RETRY_TIMEOUT);
                                                   ~~
                                                   typo

BR, Jarkko

Re: [PATCH v2 1/5] selftests/sgx: Retry the ioctl()'s returned with EAGAIN

[Jarkko Sakkinen]

On Fri, Sep 09, 2022 at 02:20:22AM +0300, Jarkko Sakkinen wrote:
> On Fri, Sep 09, 2022 at 02:19:40AM +0300, Jarkko Sakkinen wrote:
> >                 EXPECT_LT(get_time() - start_time(), IOCTL_RETRY_TIMEOUT);
>                                                    ~~
>                                                    typo
> 
> BR, Jarkko

Also, the timeout can probably be like one second.

BR, Jarkko

Re: [PATCH v2 1/5] selftests/sgx: Retry the ioctl()'s returned with EAGAIN

[Reinette Chatre]

Hi Jarkko,

On 9/8/2022 4:19 PM, Jarkko Sakkinen wrote:
> On Thu, Sep 08, 2022 at 03:43:06PM -0700, Reinette Chatre wrote:
>> Hi Jarkko and Haitao,
>>
>> On 9/4/2022 7:04 PM, Jarkko Sakkinen wrote:
>>> From: Haitao Huang <haitao.huang@linux.intel.com>
>>>
>>> For EMODT and EREMOVE ioctl()'s with a large range, kernel
>>> may not finish in one shot and return EAGAIN error code
>>> and count of bytes of EPC pages on that operations are
>>> finished successfully.
>>>
>>> Change the unclobbered_vdso_oversubscribed_remove test
>>> to rerun the ioctl()'s in a loop, updating offset and length
>>> using the byte count returned in each iteration.
>>>
>>> Fixes: 6507cce561b4 ("selftests/sgx: Page removal stress test")
>>
>> Should this patch be moved to the "critical fixes for v6.0" series?
> 
> I think not because it does not risk stability of the
> kernel itself. It's "nice to have" but not mandatory.

ok, thank you for considering it.

...

>>> @@ -453,16 +454,30 @@ TEST_F_TIMEOUT(enclave, unclobbered_vdso_oversubscribed_remove, 900)
>>>  	modt_ioc.offset = heap->offset;
>>>  	modt_ioc.length = heap->size;
>>>  	modt_ioc.page_type = SGX_PAGE_TYPE_TRIM;
>>> -
>>> +	count = 0;
>>>  	TH_LOG("Changing type of %zd bytes to trimmed may take a while ...",
>>>  	       heap->size);
>>> -	ret = ioctl(self->encl.fd, SGX_IOC_ENCLAVE_MODIFY_TYPES, &modt_ioc);
>>> -	errno_save = ret == -1 ? errno : 0;
>>> +	do {
>>> +		ret = ioctl(self->encl.fd, SGX_IOC_ENCLAVE_MODIFY_TYPES, &modt_ioc);
>>> +
>>> +		errno_save = ret == -1 ? errno : 0;
>>> +		if (errno_save != EAGAIN)
>>> +			break;
>>> +
>>> +		EXPECT_EQ(modt_ioc.result, 0);
>>
>> If this check triggers then there is something seriously wrong and in that case
>> it may also be that this loop may be unable to terminate or the error condition would
>> keep appearing until the loop terminates (which may be many iterations). Considering
>> the severity and risk I do think that ASSERT_EQ() would be more appropriate,
>> similar to how ASSERT_EQ() is used in patch 5/5.
>>
>> Apart from that I think that this looks good.
>>
>> Thank you very much for adding this.
>>
>> Reinette
> 
> Hmm... I could along the lines:
> 
> /*
>  * Get time since Epoch is milliseconds.
>  */
> unsigned long get_time(void)
> {
>     struct timeval start;
> 
>     gettimeofday(&start, NULL);
> 
>     return (unsigneg long)start.tv_sec * 1000L + (unsigned long)start.tv_usec / 1000L;
> }
> 
> and
> 
> #define IOCTL_RETRY_TIMEOUT 100
> 
> In the test function:
> 
>         unsigned long start_time;
> 
>         /* ... */
> 
>         start_time = get_time();
>         do {
>                 EXPECT_LT(get_time() - start_time(), IOCTL_RETRY_TIMEOUT);
> 
>                 /* ... */
>         }
> 
>         /* ... */
> 
> What do you think?

I do think that your proposal can be considered for an additional check in this
test but the way I understand it it does not address my feedback.

In this patch the flow is:

	do {
		ret = ioctl(self->encl.fd, SGX_IOC_ENCLAVE_MODIFY_TYPES, &modt_ioc);

		errno_save = ret == -1 ? errno : 0;
		if (errno_save != EAGAIN)
			break;

		EXPECT_EQ(modt_ioc.result, 0);
		...
	} while ...


If this EXPECT_EQ() check fails then it means that errno_save is EAGAIN
and modt_ioc.result != 0. This should never happen because in the kernel
(sgx_enclave_modify_types()) the only time modt_ioc.result can be set is
when the ioctl() returns EFAULT.

In my opinion this check should be changed to:
		ASSERT_EQ(modt_ioc.result, 0);

This is my opinion because this check indicates a kernel bug and I do
not see value in continuing the test after a kernel bug is encountered.
My expectation is that this test is of value to folks modifying
the kernel code.

As for the new check you are proposing - it seems to me that kselftest
with TIMEOUT_DEFAULT already covers this.

Reinette

Re: [PATCH v2 1/5] selftests/sgx: Retry the ioctl()'s returned with EAGAIN

[Jarkko Sakkinen]

On Thu, Sep 08, 2022 at 05:06:58PM -0700, Reinette Chatre wrote:
> Hi Jarkko,
> 
> On 9/8/2022 4:19 PM, Jarkko Sakkinen wrote:
> > On Thu, Sep 08, 2022 at 03:43:06PM -0700, Reinette Chatre wrote:
> >> Hi Jarkko and Haitao,
> >>
> >> On 9/4/2022 7:04 PM, Jarkko Sakkinen wrote:
> >>> From: Haitao Huang <haitao.huang@linux.intel.com>
> >>>
> >>> For EMODT and EREMOVE ioctl()'s with a large range, kernel
> >>> may not finish in one shot and return EAGAIN error code
> >>> and count of bytes of EPC pages on that operations are
> >>> finished successfully.
> >>>
> >>> Change the unclobbered_vdso_oversubscribed_remove test
> >>> to rerun the ioctl()'s in a loop, updating offset and length
> >>> using the byte count returned in each iteration.
> >>>
> >>> Fixes: 6507cce561b4 ("selftests/sgx: Page removal stress test")
> >>
> >> Should this patch be moved to the "critical fixes for v6.0" series?
> > 
> > I think not because it does not risk stability of the
> > kernel itself. It's "nice to have" but not mandatory.
> 
> ok, thank you for considering it.
> 
> ...
> 
> >>> @@ -453,16 +454,30 @@ TEST_F_TIMEOUT(enclave, unclobbered_vdso_oversubscribed_remove, 900)
> >>>  	modt_ioc.offset = heap->offset;
> >>>  	modt_ioc.length = heap->size;
> >>>  	modt_ioc.page_type = SGX_PAGE_TYPE_TRIM;
> >>> -
> >>> +	count = 0;
> >>>  	TH_LOG("Changing type of %zd bytes to trimmed may take a while ...",
> >>>  	       heap->size);
> >>> -	ret = ioctl(self->encl.fd, SGX_IOC_ENCLAVE_MODIFY_TYPES, &modt_ioc);
> >>> -	errno_save = ret == -1 ? errno : 0;
> >>> +	do {
> >>> +		ret = ioctl(self->encl.fd, SGX_IOC_ENCLAVE_MODIFY_TYPES, &modt_ioc);
> >>> +
> >>> +		errno_save = ret == -1 ? errno : 0;
> >>> +		if (errno_save != EAGAIN)
> >>> +			break;
> >>> +
> >>> +		EXPECT_EQ(modt_ioc.result, 0);
> >>
> >> If this check triggers then there is something seriously wrong and in that case
> >> it may also be that this loop may be unable to terminate or the error condition would
> >> keep appearing until the loop terminates (which may be many iterations). Considering
> >> the severity and risk I do think that ASSERT_EQ() would be more appropriate,
> >> similar to how ASSERT_EQ() is used in patch 5/5.
> >>
> >> Apart from that I think that this looks good.
> >>
> >> Thank you very much for adding this.
> >>
> >> Reinette
> > 
> > Hmm... I could along the lines:
> > 
> > /*
> >  * Get time since Epoch is milliseconds.
> >  */
> > unsigned long get_time(void)
> > {
> >     struct timeval start;
> > 
> >     gettimeofday(&start, NULL);
> > 
> >     return (unsigneg long)start.tv_sec * 1000L + (unsigned long)start.tv_usec / 1000L;
> > }
> > 
> > and
> > 
> > #define IOCTL_RETRY_TIMEOUT 100
> > 
> > In the test function:
> > 
> >         unsigned long start_time;
> > 
> >         /* ... */
> > 
> >         start_time = get_time();
> >         do {
> >                 EXPECT_LT(get_time() - start_time(), IOCTL_RETRY_TIMEOUT);
> > 
> >                 /* ... */
> >         }
> > 
> >         /* ... */
> > 
> > What do you think?
> 
> I do think that your proposal can be considered for an additional check in this
> test but the way I understand it it does not address my feedback.
> 
> In this patch the flow is:
> 
> 	do {
> 		ret = ioctl(self->encl.fd, SGX_IOC_ENCLAVE_MODIFY_TYPES, &modt_ioc);
> 
> 		errno_save = ret == -1 ? errno : 0;
> 		if (errno_save != EAGAIN)
> 			break;
> 
> 		EXPECT_EQ(modt_ioc.result, 0);
> 		...
> 	} while ...
> 
> 
> If this EXPECT_EQ() check fails then it means that errno_save is EAGAIN
> and modt_ioc.result != 0. This should never happen because in the kernel
> (sgx_enclave_modify_types()) the only time modt_ioc.result can be set is
> when the ioctl() returns EFAULT.
> 
> In my opinion this check should be changed to:
> 		ASSERT_EQ(modt_ioc.result, 0);

Right, I missed this. It should be definitely ASSERT_EQ(().

BR, Jarkko

Re: [PATCH v2 1/5] selftests/sgx: Retry the ioctl()'s returned with EAGAIN

[Jarkko Sakkinen]

On Fri, Sep 09, 2022 at 07:01:36AM +0300, Jarkko Sakkinen wrote:
> On Thu, Sep 08, 2022 at 05:06:58PM -0700, Reinette Chatre wrote:
> > Hi Jarkko,
> > 
> > On 9/8/2022 4:19 PM, Jarkko Sakkinen wrote:
> > > On Thu, Sep 08, 2022 at 03:43:06PM -0700, Reinette Chatre wrote:
> > >> Hi Jarkko and Haitao,
> > >>
> > >> On 9/4/2022 7:04 PM, Jarkko Sakkinen wrote:
> > >>> From: Haitao Huang <haitao.huang@linux.intel.com>
> > >>>
> > >>> For EMODT and EREMOVE ioctl()'s with a large range, kernel
> > >>> may not finish in one shot and return EAGAIN error code
> > >>> and count of bytes of EPC pages on that operations are
> > >>> finished successfully.
> > >>>
> > >>> Change the unclobbered_vdso_oversubscribed_remove test
> > >>> to rerun the ioctl()'s in a loop, updating offset and length
> > >>> using the byte count returned in each iteration.
> > >>>
> > >>> Fixes: 6507cce561b4 ("selftests/sgx: Page removal stress test")
> > >>
> > >> Should this patch be moved to the "critical fixes for v6.0" series?
> > > 
> > > I think not because it does not risk stability of the
> > > kernel itself. It's "nice to have" but not mandatory.
> > 
> > ok, thank you for considering it.
> > 
> > ...
> > 
> > >>> @@ -453,16 +454,30 @@ TEST_F_TIMEOUT(enclave, unclobbered_vdso_oversubscribed_remove, 900)
> > >>>  	modt_ioc.offset = heap->offset;
> > >>>  	modt_ioc.length = heap->size;
> > >>>  	modt_ioc.page_type = SGX_PAGE_TYPE_TRIM;
> > >>> -
> > >>> +	count = 0;
> > >>>  	TH_LOG("Changing type of %zd bytes to trimmed may take a while ...",
> > >>>  	       heap->size);
> > >>> -	ret = ioctl(self->encl.fd, SGX_IOC_ENCLAVE_MODIFY_TYPES, &modt_ioc);
> > >>> -	errno_save = ret == -1 ? errno : 0;
> > >>> +	do {
> > >>> +		ret = ioctl(self->encl.fd, SGX_IOC_ENCLAVE_MODIFY_TYPES, &modt_ioc);
> > >>> +
> > >>> +		errno_save = ret == -1 ? errno : 0;
> > >>> +		if (errno_save != EAGAIN)
> > >>> +			break;
> > >>> +
> > >>> +		EXPECT_EQ(modt_ioc.result, 0);
> > >>
> > >> If this check triggers then there is something seriously wrong and in that case
> > >> it may also be that this loop may be unable to terminate or the error condition would
> > >> keep appearing until the loop terminates (which may be many iterations). Considering
> > >> the severity and risk I do think that ASSERT_EQ() would be more appropriate,
> > >> similar to how ASSERT_EQ() is used in patch 5/5.
> > >>
> > >> Apart from that I think that this looks good.
> > >>
> > >> Thank you very much for adding this.
> > >>
> > >> Reinette
> > > 
> > > Hmm... I could along the lines:
> > > 
> > > /*
> > >  * Get time since Epoch is milliseconds.
> > >  */
> > > unsigned long get_time(void)
> > > {
> > >     struct timeval start;
> > > 
> > >     gettimeofday(&start, NULL);
> > > 
> > >     return (unsigneg long)start.tv_sec * 1000L + (unsigned long)start.tv_usec / 1000L;
> > > }
> > > 
> > > and
> > > 
> > > #define IOCTL_RETRY_TIMEOUT 100
> > > 
> > > In the test function:
> > > 
> > >         unsigned long start_time;
> > > 
> > >         /* ... */
> > > 
> > >         start_time = get_time();
> > >         do {
> > >                 EXPECT_LT(get_time() - start_time(), IOCTL_RETRY_TIMEOUT);
> > > 
> > >                 /* ... */
> > >         }
> > > 
> > >         /* ... */
> > > 
> > > What do you think?
> > 
> > I do think that your proposal can be considered for an additional check in this
> > test but the way I understand it it does not address my feedback.
> > 
> > In this patch the flow is:
> > 
> > 	do {
> > 		ret = ioctl(self->encl.fd, SGX_IOC_ENCLAVE_MODIFY_TYPES, &modt_ioc);
> > 
> > 		errno_save = ret == -1 ? errno : 0;
> > 		if (errno_save != EAGAIN)
> > 			break;
> > 
> > 		EXPECT_EQ(modt_ioc.result, 0);
> > 		...
> > 	} while ...
> > 
> > 
> > If this EXPECT_EQ() check fails then it means that errno_save is EAGAIN
> > and modt_ioc.result != 0. This should never happen because in the kernel
> > (sgx_enclave_modify_types()) the only time modt_ioc.result can be set is
> > when the ioctl() returns EFAULT.
> > 
> > In my opinion this check should be changed to:
> > 		ASSERT_EQ(modt_ioc.result, 0);
> 
> Right, I missed this. It should be definitely ASSERT_EQ(().

I was thinking to add patch, which adds helper to calculate static
content length from the last item of the segment table (offset + size)
and replace total_length calculations in various tests. 

I won't send a new version this week because I'm at Open Source Summit
EU and Linux Security Summit EU.

BR, Jarkko

[PATCH v2 2/5] selftests/sgx: Move ENCL_HEAP_SIZE_DEFAULT to main.c

[Jarkko Sakkinen]

Move ENCL_HEAP_SIZE_DEFAULT to main.c because all the other constants
are also there, and it is only used locally there.

Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
 tools/testing/selftests/sgx/main.c | 1 +
 tools/testing/selftests/sgx/main.h | 2 --
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
index 59cca806eda1..a1850e139c99 100644
--- a/tools/testing/selftests/sgx/main.c
+++ b/tools/testing/selftests/sgx/main.c
@@ -21,6 +21,7 @@
 #include "../kselftest_harness.h"
 #include "main.h"
 
+static const size_t ENCL_HEAP_SIZE_DEFAULT = PAGE_SIZE;
 static const uint64_t MAGIC = 0x1122334455667788ULL;
 static const uint64_t MAGIC2 = 0x8877665544332211ULL;
 vdso_sgx_enter_enclave_t vdso_sgx_enter_enclave;
diff --git a/tools/testing/selftests/sgx/main.h b/tools/testing/selftests/sgx/main.h
index fc585be97e2f..82b33f8db048 100644
--- a/tools/testing/selftests/sgx/main.h
+++ b/tools/testing/selftests/sgx/main.h
@@ -6,8 +6,6 @@
 #ifndef MAIN_H
 #define MAIN_H
 
-#define ENCL_HEAP_SIZE_DEFAULT	4096
-
 struct encl_segment {
 	void *src;
 	off_t offset;
-- 
2.37.2

Re: [PATCH v2 2/5] selftests/sgx: Move ENCL_HEAP_SIZE_DEFAULT to main.c

[Reinette Chatre]

On 9/4/2022 7:04 PM, Jarkko Sakkinen wrote:
> Move ENCL_HEAP_SIZE_DEFAULT to main.c because all the other constants
> are also there, and it is only used locally there.
> 
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>

Acked-by: Reinette Chatre <reinette.chatre@intel.com>

Reinette

[PATCH v2 3/5] selftests/sgx: Use encl->encl_size in sigstruct.c

[Jarkko Sakkinen]

The final enclave address range (referred as ELRANGE in Intel SDM)
calculation is a reminiscent of signing tool being a separate command-line
utility, and sigstruct being produced during the compilation. Given that
nowadays the sigstruct is calculated on-fly, use the readily calculated
encl->encl_size instead, in order to remove duplicate code.

Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
 tools/testing/selftests/sgx/load.c      | 5 +++--
 tools/testing/selftests/sgx/main.h      | 1 -
 tools/testing/selftests/sgx/sigstruct.c | 8 ++------
 3 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/tools/testing/selftests/sgx/load.c b/tools/testing/selftests/sgx/load.c
index 94bdeac1cf04..3b4e2422fb09 100644
--- a/tools/testing/selftests/sgx/load.c
+++ b/tools/testing/selftests/sgx/load.c
@@ -174,6 +174,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)
 {
 	const char device_path[] = "/dev/sgx_enclave";
+	unsigned long contents_size;
 	struct encl_segment *seg;
 	Elf64_Phdr *phdr_tbl;
 	off_t src_offset;
@@ -298,9 +299,9 @@ bool encl_load(const char *path, struct encl *encl, unsigned long heap_size)
 	if (seg->src == MAP_FAILED)
 		goto err;
 
-	encl->src_size = encl->segment_tbl[j].offset + encl->segment_tbl[j].size;
+	contents_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 < contents_size; )
 		encl->encl_size <<= 1;
 
 	return true;
diff --git a/tools/testing/selftests/sgx/main.h b/tools/testing/selftests/sgx/main.h
index 82b33f8db048..9c1bc0d9b43c 100644
--- a/tools/testing/selftests/sgx/main.h
+++ b/tools/testing/selftests/sgx/main.h
@@ -20,7 +20,6 @@ struct encl {
 	void *bin;
 	off_t bin_size;
 	void *src;
-	size_t src_size;
 	size_t encl_size;
 	off_t encl_base;
 	unsigned int nr_segments;
diff --git a/tools/testing/selftests/sgx/sigstruct.c b/tools/testing/selftests/sgx/sigstruct.c
index 50c5ab1aa6fa..0c7678d2594b 100644
--- a/tools/testing/selftests/sgx/sigstruct.c
+++ b/tools/testing/selftests/sgx/sigstruct.c
@@ -212,13 +212,9 @@ struct mrecreate {
 } __attribute__((__packed__));
 
 
-static bool mrenclave_ecreate(EVP_MD_CTX *ctx, uint64_t blob_size)
+static bool mrenclave_ecreate(EVP_MD_CTX *ctx, uint64_t encl_size)
 {
 	struct mrecreate mrecreate;
-	uint64_t encl_size;
-
-	for (encl_size = 0x1000; encl_size < blob_size; )
-		encl_size <<= 1;
 
 	memset(&mrecreate, 0, sizeof(mrecreate));
 	mrecreate.tag = MRECREATE;
@@ -343,7 +339,7 @@ bool encl_measure(struct encl *encl)
 	if (!ctx)
 		goto err;
 
-	if (!mrenclave_ecreate(ctx, encl->src_size))
+	if (!mrenclave_ecreate(ctx, encl->encl_size))
 		goto err;
 
 	for (i = 0; i < encl->nr_segments; i++) {
-- 
2.37.2

Re: [PATCH v2 3/5] selftests/sgx: Use encl->encl_size in sigstruct.c

[Reinette Chatre]

Hi Jarkko,

On 9/4/2022 7:04 PM, Jarkko Sakkinen wrote:
> The final enclave address range (referred as ELRANGE in Intel SDM)
> calculation is a reminiscent of signing tool being a separate command-line
> utility, and sigstruct being produced during the compilation. Given that
> nowadays the sigstruct is calculated on-fly, use the readily calculated
> encl->encl_size instead, in order to remove duplicate code.
> 
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>

Thank you very much.

Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>

Reinette

[PATCH v2 4/5] selftests/sgx: Include the dynamic heap size to the ELRANGE calculation

[Jarkko Sakkinen]

When calculating ELRANGE, i.e. the address range defined for an enclave,
and represented by encl->encl_size, also dynamic memory should be taken
into account. Implement setup_test_encl_dynamic() with dynamic_size
parameter for the dynamic heap size, and use it in 'augment_via_eaccept'
and 'augment' tests.

Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
v2:
* Specify a dynamic heap of three pages for tcs_create test.
* Specify required dynamic heap inside the test cases instead of
  ENCL_DYNAMIC_SIZE_DEFAULT because the dynamic heaps size
  varies between the test cases.
---
 tools/testing/selftests/sgx/load.c |  5 +++--
 tools/testing/selftests/sgx/main.c | 22 +++++++++++++++-------
 tools/testing/selftests/sgx/main.h |  3 ++-
 3 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/tools/testing/selftests/sgx/load.c b/tools/testing/selftests/sgx/load.c
index 3b4e2422fb09..963a5c6bbbdc 100644
--- a/tools/testing/selftests/sgx/load.c
+++ b/tools/testing/selftests/sgx/load.c
@@ -171,7 +171,8 @@ uint64_t encl_get_entry(struct encl *encl, const char *symbol)
 	return 0;
 }
 
-bool encl_load(const char *path, struct encl *encl, unsigned long heap_size)
+bool encl_load(const char *path, struct encl *encl, unsigned long heap_size,
+	       unsigned long dynamic_size)
 {
 	const char device_path[] = "/dev/sgx_enclave";
 	unsigned long contents_size;
@@ -299,7 +300,7 @@ bool encl_load(const char *path, struct encl *encl, unsigned long heap_size)
 	if (seg->src == MAP_FAILED)
 		goto err;
 
-	contents_size = encl->segment_tbl[j].offset + encl->segment_tbl[j].size;
+	contents_size = encl->segment_tbl[j].offset + encl->segment_tbl[j].size + dynamic_size;
 
 	for (encl->encl_size = 4096; encl->encl_size < contents_size; )
 		encl->encl_size <<= 1;
diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
index a1850e139c99..78c3b913ce10 100644
--- a/tools/testing/selftests/sgx/main.c
+++ b/tools/testing/selftests/sgx/main.c
@@ -173,8 +173,8 @@ FIXTURE(enclave) {
 	struct sgx_enclave_run run;
 };
 
-static bool setup_test_encl(unsigned long heap_size, struct encl *encl,
-			    struct __test_metadata *_metadata)
+static bool setup_test_encl_dynamic(unsigned long heap_size, unsigned long dynamic_size,
+				    struct encl *encl, struct __test_metadata *_metadata)
 {
 	Elf64_Sym *sgx_enter_enclave_sym = NULL;
 	struct vdso_symtab symtab;
@@ -184,7 +184,7 @@ static bool setup_test_encl(unsigned long heap_size, struct encl *encl,
 	unsigned int i;
 	void *addr;
 
-	if (!encl_load("test_encl.elf", encl, heap_size)) {
+	if (!encl_load("test_encl.elf", encl, heap_size, dynamic_size)) {
 		encl_delete(encl);
 		TH_LOG("Failed to load the test enclave.");
 		return false;
@@ -251,6 +251,12 @@ static bool setup_test_encl(unsigned long heap_size, struct encl *encl,
 	return false;
 }
 
+static bool setup_test_encl(unsigned long heap_size, struct encl *encl,
+			    struct __test_metadata *_metadata)
+{
+	return setup_test_encl_dynamic(heap_size, 0, encl, _metadata);
+}
+
 FIXTURE_SETUP(enclave)
 {
 }
@@ -1013,7 +1019,8 @@ TEST_F(enclave, augment)
 	if (!sgx2_supported())
 		SKIP(return, "SGX2 not supported");
 
-	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata));
+	ASSERT_TRUE(setup_test_encl_dynamic(ENCL_HEAP_SIZE_DEFAULT, PAGE_SIZE, &self->encl,
+					    _metadata));
 
 	memset(&self->run, 0, sizeof(self->run));
 	self->run.tcs = self->encl.encl_base;
@@ -1143,7 +1150,8 @@ TEST_F(enclave, augment_via_eaccept)
 	if (!sgx2_supported())
 		SKIP(return, "SGX2 not supported");
 
-	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata));
+	ASSERT_TRUE(setup_test_encl_dynamic(ENCL_HEAP_SIZE_DEFAULT, PAGE_SIZE, &self->encl,
+					    _metadata));
 
 	memset(&self->run, 0, sizeof(self->run));
 	self->run.tcs = self->encl.encl_base;
@@ -1264,8 +1272,8 @@ TEST_F(enclave, tcs_create)
 	int errno_save;
 	int ret, i;
 
-	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl,
-				    _metadata));
+	ASSERT_TRUE(setup_test_encl_dynamic(ENCL_HEAP_SIZE_DEFAULT, 3 * PAGE_SIZE, &self->encl,
+					    _metadata));
 
 	memset(&self->run, 0, sizeof(self->run));
 	self->run.tcs = self->encl.encl_base;
diff --git a/tools/testing/selftests/sgx/main.h b/tools/testing/selftests/sgx/main.h
index 9c1bc0d9b43c..8f77ce56ad09 100644
--- a/tools/testing/selftests/sgx/main.h
+++ b/tools/testing/selftests/sgx/main.h
@@ -32,7 +32,8 @@ extern unsigned char sign_key[];
 extern unsigned char sign_key_end[];
 
 void encl_delete(struct encl *ctx);
-bool encl_load(const char *path, struct encl *encl, unsigned long heap_size);
+bool encl_load(const char *path, struct encl *encl, unsigned long heap_size,
+	       unsigned long dynamic_size);
 bool encl_measure(struct encl *encl);
 bool encl_build(struct encl *encl);
 uint64_t encl_get_entry(struct encl *encl, const char *symbol);
-- 
2.37.2

Re: [PATCH v2 4/5] selftests/sgx: Include the dynamic heap size to the ELRANGE calculation

[Reinette Chatre]

Hi Jarkko,

On 9/4/2022 7:04 PM, Jarkko Sakkinen wrote:
> When calculating ELRANGE, i.e. the address range defined for an enclave,
> and represented by encl->encl_size, also dynamic memory should be taken
> into account. Implement setup_test_encl_dynamic() with dynamic_size
> parameter for the dynamic heap size, and use it in 'augment_via_eaccept'
> and 'augment' tests.
> 
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>

This is a constructive addition, thank you very much.

Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>

Reinette

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

[Jarkko Sakkinen]

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

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

Signed-off-by: Vijay Dhanraj <vijay.dhanraj@intel.com>
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
v8:
- Specify dynamic heap size in side the test case.

v7:
- Contains now only the test case. Support for dynamic heap is
  prepared in prepending patches.

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

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

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

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

v2:
- Replaced WARN_ON() with optional pr_info() inside
  __sgx_sanitize_pages().
- Rewrote the commit message.
- Added the fixes tag.
---
 tools/testing/selftests/sgx/main.c | 112 ++++++++++++++++++++++++++++-
 1 file changed, 111 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
index 78c3b913ce10..e596b45bc5f8 100644
--- a/tools/testing/selftests/sgx/main.c
+++ b/tools/testing/selftests/sgx/main.c
@@ -22,8 +22,10 @@
 #include "main.h"
 
 static const size_t ENCL_HEAP_SIZE_DEFAULT = PAGE_SIZE;
+static const unsigned long TIMEOUT_DEFAULT = 900;
 static const uint64_t MAGIC = 0x1122334455667788ULL;
 static const uint64_t MAGIC2 = 0x8877665544332211ULL;
+
 vdso_sgx_enter_enclave_t vdso_sgx_enter_enclave;
 
 /*
@@ -387,7 +389,7 @@ TEST_F(enclave, unclobbered_vdso_oversubscribed)
 	EXPECT_EQ(self->run.user_data, 0);
 }
 
-TEST_F_TIMEOUT(enclave, unclobbered_vdso_oversubscribed_remove, 900)
+TEST_F_TIMEOUT(enclave, unclobbered_vdso_oversubscribed_remove, TIMEOUT_DEFAULT)
 {
 	struct sgx_enclave_remove_pages remove_ioc;
 	struct sgx_enclave_modify_types modt_ioc;
@@ -1245,6 +1247,114 @@ TEST_F(enclave, augment_via_eaccept)
 	munmap(addr, PAGE_SIZE);
 }
 
+/*
+ * Test for the addition of large number of pages to an initialized enclave via
+ * a pre-emptive run of EACCEPT on every page to be added.
+ */
+TEST_F_TIMEOUT(enclave, augment_via_eaccept_long, TIMEOUT_DEFAULT)
+{
+	/*
+	 * The dynamic heap size was chosen based on a bug report:
+	 * Message-ID:
+	 * <DM8PR11MB55912A7F47A84EC9913A6352F6999@DM8PR11MB5591.namprd11.prod.outlook.com>
+	 */
+	static const unsigned long DYNAMIC_HEAP_SIZE = 0x200000L * PAGE_SIZE;
+	struct encl_op_get_from_addr get_addr_op;
+	struct encl_op_put_to_addr put_addr_op;
+	struct encl_op_eaccept eaccept_op;
+	size_t total_size = 0;
+	unsigned long i;
+	void *addr;
+
+	if (!sgx2_supported())
+		SKIP(return, "SGX2 not supported");
+
+	ASSERT_TRUE(setup_test_encl_dynamic(ENCL_HEAP_SIZE_DEFAULT, DYNAMIC_HEAP_SIZE,
+					    &self->encl, _metadata));
+
+	memset(&self->run, 0, sizeof(self->run));
+	self->run.tcs = self->encl.encl_base;
+
+	for (i = 0; i < self->encl.nr_segments; i++) {
+		struct encl_segment *seg = &self->encl.segment_tbl[i];
+
+		total_size += seg->size;
+	}
+
+	/*
+	 * mmap() every page at end of existing enclave to be used for
+	 * EDMM.
+	 */
+	addr = mmap((void *)self->encl.encl_base + total_size, DYNAMIC_HEAP_SIZE,
+		    PROT_READ | PROT_WRITE | PROT_EXEC, MAP_SHARED | MAP_FIXED,
+		    self->encl.fd, 0);
+	EXPECT_NE(addr, MAP_FAILED);
+
+	self->run.exception_vector = 0;
+	self->run.exception_error_code = 0;
+	self->run.exception_addr = 0;
+
+	/*
+	 * Run EACCEPT on every page to trigger the #PF->EAUG->EACCEPT(again
+	 * without a #PF). All should be transparent to userspace.
+	 */
+	eaccept_op.flags = SGX_SECINFO_R | SGX_SECINFO_W | SGX_SECINFO_REG | SGX_SECINFO_PENDING;
+	eaccept_op.ret = 0;
+	eaccept_op.header.type = ENCL_OP_EACCEPT;
+
+	for (i = 0; i < DYNAMIC_HEAP_SIZE; i += PAGE_SIZE) {
+		eaccept_op.epc_addr = (uint64_t)(addr + i);
+
+		EXPECT_EQ(ENCL_CALL(&eaccept_op, &self->run, true), 0);
+		if (self->run.exception_vector == 14 &&
+		    self->run.exception_error_code == 4 &&
+		    self->run.exception_addr == self->encl.encl_base) {
+			munmap(addr, DYNAMIC_HEAP_SIZE);
+			SKIP(return, "Kernel does not support adding pages to initialized enclave");
+		}
+
+		EXPECT_EQ(self->run.exception_vector, 0);
+		EXPECT_EQ(self->run.exception_error_code, 0);
+		EXPECT_EQ(self->run.exception_addr, 0);
+		ASSERT_EQ(eaccept_op.ret, 0);
+		ASSERT_EQ(self->run.function, EEXIT);
+	}
+
+	/*
+	 * Pool of pages were successfully added to enclave. Perform sanity
+	 * check on first page of the pool only to ensure data can be written
+	 * to and read from a dynamically added enclave page.
+	 */
+	put_addr_op.value = MAGIC;
+	put_addr_op.addr = (unsigned long)addr;
+	put_addr_op.header.type = ENCL_OP_PUT_TO_ADDRESS;
+
+	EXPECT_EQ(ENCL_CALL(&put_addr_op, &self->run, true), 0);
+
+	EXPECT_EEXIT(&self->run);
+	EXPECT_EQ(self->run.exception_vector, 0);
+	EXPECT_EQ(self->run.exception_error_code, 0);
+	EXPECT_EQ(self->run.exception_addr, 0);
+
+	/*
+	 * Read memory from newly added page that was just written to,
+	 * confirming that data previously written (MAGIC) is present.
+	 */
+	get_addr_op.value = 0;
+	get_addr_op.addr = (unsigned long)addr;
+	get_addr_op.header.type = ENCL_OP_GET_FROM_ADDRESS;
+
+	EXPECT_EQ(ENCL_CALL(&get_addr_op, &self->run, true), 0);
+
+	EXPECT_EQ(get_addr_op.value, MAGIC);
+	EXPECT_EEXIT(&self->run);
+	EXPECT_EQ(self->run.exception_vector, 0);
+	EXPECT_EQ(self->run.exception_error_code, 0);
+	EXPECT_EQ(self->run.exception_addr, 0);
+
+	munmap(addr, DYNAMIC_HEAP_SIZE);
+}
+
 /*
  * SGX2 page type modification test in two phases:
  * Phase 1:
-- 
2.37.2

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

[Jarkko Sakkinen]

On Mon, Sep 05, 2022 at 05:04:11AM +0300, Jarkko Sakkinen wrote:
> +	for (i = 0; i < self->encl.nr_segments; i++) {
> +		struct encl_segment *seg = &self->encl.segment_tbl[i];
> +
> +		total_size += seg->size;
> +	}

This is actually same as:

        struct encl_segment *seg = &self->encl.segment_tbl[self->encl.nr_segments - 1];
        
        total_size = seg->offset + seg->size;

Should I update?

BR, Jarkko

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

[Reinette Chatre]

Hi Jarkko and Vijay,

On 9/4/2022 7:04 PM, Jarkko Sakkinen wrote:
> From: Vijay Dhanraj <vijay.dhanraj@intel.com>
> 
> Add a new test case which is same as augment_via_eaccept but adds a
> larger number of EPC pages to stress test EAUG via EACCEPT.
> 
> Signed-off-by: Vijay Dhanraj <vijay.dhanraj@intel.com>
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> ---

...

> diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
> index 78c3b913ce10..e596b45bc5f8 100644
> --- a/tools/testing/selftests/sgx/main.c
> +++ b/tools/testing/selftests/sgx/main.c
> @@ -22,8 +22,10 @@
>  #include "main.h"
>  
>  static const size_t ENCL_HEAP_SIZE_DEFAULT = PAGE_SIZE;
> +static const unsigned long TIMEOUT_DEFAULT = 900;

I am not sure about the naming here ... it is _very_ close to
(and thus appears to be in the same namespace as)
TEST_TIMEOUT_DEFAULT from the included kselftest_harness.h.

This is surely a nitpick but how about SGX_TEST_TIMEOUT_DEFAULT?

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

There is an extra empty line here ... but it looks like intended code
organization?

>  vdso_sgx_enter_enclave_t vdso_sgx_enter_enclave;
>  
>  /*


Apart from the naming comment this addition looks good. 

This is a valuable addition to the SGX tests.

Reinette

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

[Jarkko Sakkinen]

On Thu, Sep 08, 2022 at 03:44:29PM -0700, Reinette Chatre wrote:
> Hi Jarkko and Vijay,
> 
> On 9/4/2022 7:04 PM, Jarkko Sakkinen wrote:
> > From: Vijay Dhanraj <vijay.dhanraj@intel.com>
> > 
> > Add a new test case which is same as augment_via_eaccept but adds a
> > larger number of EPC pages to stress test EAUG via EACCEPT.
> > 
> > Signed-off-by: Vijay Dhanraj <vijay.dhanraj@intel.com>
> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > ---
> 
> ...
> 
> > diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
> > index 78c3b913ce10..e596b45bc5f8 100644
> > --- a/tools/testing/selftests/sgx/main.c
> > +++ b/tools/testing/selftests/sgx/main.c
> > @@ -22,8 +22,10 @@
> >  #include "main.h"
> >  
> >  static const size_t ENCL_HEAP_SIZE_DEFAULT = PAGE_SIZE;
> > +static const unsigned long TIMEOUT_DEFAULT = 900;
> 
> I am not sure about the naming here ... it is _very_ close to
> (and thus appears to be in the same namespace as)
> TEST_TIMEOUT_DEFAULT from the included kselftest_harness.h.
> 
> This is surely a nitpick but how about SGX_TEST_TIMEOUT_DEFAULT?

Agreed, I would use ENCL_TEST_TIMEOUT_DEFAULT (better match
with existing names).

> 
> >  static const uint64_t MAGIC = 0x1122334455667788ULL;
> >  static const uint64_t MAGIC2 = 0x8877665544332211ULL;
> > +
> 
> There is an extra empty line here ... but it looks like intended code
> organization?

Yeah, I think it'd make sense to have it there and would
not be productive to make it a separate patch.

> 
> >  vdso_sgx_enter_enclave_t vdso_sgx_enter_enclave;
> >  
> >  /*
> 
> 
> Apart from the naming comment this addition looks good. 
> 
> This is a valuable addition to the SGX tests.
> 
> Reinette

Thank you.

BR, Jarkko