On Wed, May 11, 2022 at 11:02:47AM -0700, Reinette Chatre wrote:
> Hi Jarkko,
>
> On 5/11/2022 4:13 AM, Jarkko Sakkinen wrote:
> > On Mon, May 09, 2022 at 02:48:01PM -0700, Reinette Chatre wrote:
>
> ...
>
> >> diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> >> index fad3d6c4756e..a60f8b2780fb 100644
> >> --- a/arch/x86/kernel/cpu/sgx/main.c
> >> +++ b/arch/x86/kernel/cpu/sgx/main.c
> >> @@ -310,6 +310,7 @@ static void sgx_reclaimer_write(struct sgx_epc_page *epc_page,
> >> sgx_encl_ewb(epc_page, backing);
> >> encl_page->epc_page = NULL;
> >> encl->secs_child_cnt--;
> >> + sgx_encl_put_backing(backing);
> >>
> >> if (!encl->secs_child_cnt && test_bit(SGX_ENCL_INITIALIZED, &encl->flags)) {
> >> ret = sgx_encl_get_backing(encl, PFN_DOWN(encl->size),
> >> @@ -381,11 +382,14 @@ static void sgx_reclaim_pages(void)
> >> goto skip;
> >>
> >> page_index = PFN_DOWN(encl_page->desc - encl_page->encl->base);
> >> +
> >> + mutex_lock(&encl_page->encl->lock);
> >> ret = sgx_encl_get_backing(encl_page->encl, page_index, &backing[i]);
> >> - if (ret)
> >> + if (ret) {
> >> + mutex_unlock(&encl_page->encl->lock);
> >> goto skip;
> >> + }
> >>
> >> - mutex_lock(&encl_page->encl->lock);
> >> encl_page->desc |= SGX_ENCL_PAGE_BEING_RECLAIMED;
> >> mutex_unlock(&encl_page->encl->lock);
> >> continue;
> >> @@ -413,7 +417,6 @@ static void sgx_reclaim_pages(void)
> >>
> >> encl_page = epc_page->owner;
> >> sgx_reclaimer_write(epc_page, &backing[i]);
> >> - sgx_encl_put_backing(&backing[i]);
> >>
> >> kref_put(&encl_page->encl->refcount, sgx_encl_release);
> >> epc_page->flags &= ~SGX_EPC_PAGE_RECLAIMER_TRACKED;
> >> --
> >> 2.25.1
> >>
> >
> > I get the locking part but why is the move of sgx_encl_put_backing
> > relevant?
>
> Moving sgx_encl_put_backing() accomplishes the locking goal.
>
> Before the patch:
>
> sgx_reclaim_pages() {
> ...
> sgx_reclaimer_write() {
> mutex_lock(&encl->lock);
> ...
> mutex_unlock(&encl->lock);
> }
> sgx_encl_put_backing(); /* Not protected by enclave mutex */
> }
>
> After the patch:
>
> sgx_reclaim_pages() {
> ...
> sgx_reclaimer_write() {
> mutex_lock(&encl->lock);
> ...
> sgx_encl_put_backing(); /* Protected by enclave mutex */
> ...
> mutex_unlock(&encl->lock);
> }
>
> }
Right.
> Even so, because of patch 1/1 the first scenario described in the
> changelog is no longer valid since the page is marked as dirty
> with the enclave mutex held. It may thus not be required
> to call sgx_encl_put_backing() with enclave mutex held but it
> remains important for sgx_encl_get_backing() to be called with
> enclave mutex held since it ensures that SGX_ENCL_PAGE_BEING_RECLAIMED
> can be used (in patch 4/5) to reliably reflect references to the
> backing storage.
> Considering that I would like to continue to consistently protect
> sgx_encl_get_backing()/sgx_encl_put_backing() with the enclave mutex.
I fully agree with your conclusion.
> Reinette
Reviewed-by: Jarkko Sakkinen <jarkko@xxxxxxxxxx>
Also
Tested-by: Jarkko Sakkinen <jarkko@xxxxxxxxxx>
BR, Jarkko
