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