From: Jarkko Sakkinen <jarkko@kernel.org>
To: Reinette Chatre <reinette.chatre@intel.com>
Cc: linux-sgx@vger.kernel.org,
Haitao Huang <haitao.huang@linux.intel.com>,
Vijay Dhanraj <vijay.dhanraj@intel.com>,
Dave Hansen <dave.hansen@linux.intel.com>,
Shuah Khan <shuah@kernel.org>,
"open list:KERNEL SELFTEST FRAMEWORK"
<linux-kselftest@vger.kernel.org>,
open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 1/5] selftests/sgx: Retry the ioctl()'s returned with EAGAIN
Date: Mon, 12 Sep 2022 13:40:33 +0300 [thread overview]
Message-ID: <Yx8MoYF3Jed5G+uo@kernel.org> (raw)
In-Reply-To: <Yxq6oAcGkg33tkb8@kernel.org>
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
next prev parent reply other threads:[~2022-09-12 10:40 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-05 2:04 [PATCH v2 0/5] Test a large dynamic heap Jarkko Sakkinen
2022-09-05 2:04 ` [PATCH v2 1/5] selftests/sgx: Retry the ioctl()'s returned with EAGAIN Jarkko Sakkinen
2022-09-08 22:43 ` Reinette Chatre
2022-09-08 23:19 ` Jarkko Sakkinen
2022-09-08 23:20 ` Jarkko Sakkinen
2022-09-08 23:31 ` Jarkko Sakkinen
2022-09-09 0:06 ` Reinette Chatre
2022-09-09 4:01 ` Jarkko Sakkinen
2022-09-12 10:40 ` Jarkko Sakkinen [this message]
2022-09-05 2:04 ` [PATCH v2 2/5] selftests/sgx: Move ENCL_HEAP_SIZE_DEFAULT to main.c Jarkko Sakkinen
2022-09-08 22:43 ` Reinette Chatre
2022-09-05 2:04 ` [PATCH v2 3/5] selftests/sgx: Use encl->encl_size in sigstruct.c Jarkko Sakkinen
2022-09-08 22:43 ` Reinette Chatre
2022-09-05 2:04 ` [PATCH v2 4/5] selftests/sgx: Include the dynamic heap size to the ELRANGE calculation Jarkko Sakkinen
2022-09-08 22:43 ` Reinette Chatre
2022-09-05 2:04 ` [PATCH v2 5/5] selftests/sgx: Add SGX selftest augment_via_eaccept_long Jarkko Sakkinen
2022-09-08 21:17 ` Jarkko Sakkinen
2022-09-08 22:44 ` Reinette Chatre
2022-09-08 23:28 ` Jarkko Sakkinen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Yx8MoYF3Jed5G+uo@kernel.org \
--to=jarkko@kernel.org \
--cc=dave.hansen@linux.intel.com \
--cc=haitao.huang@linux.intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-kselftest@vger.kernel.org \
--cc=linux-sgx@vger.kernel.org \
--cc=reinette.chatre@intel.com \
--cc=shuah@kernel.org \
--cc=vijay.dhanraj@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).