Re: [RFC PATCH 0/4] SGX shmem backing store issue

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Dave,

On 5/2/2022 7:36 AM, Dave Hansen wrote:
> On 4/29/22 20:22, Reinette Chatre wrote:
>> -	if (pcmd_page_empty) {
>> +	if (pcmd_page_empty && && put_page_testzero(b.pcmd)) {
>>  		ida_free(&encl->pcmd_in_backing, PFN_DOWN(page_pcmd_off));
>>  		sgx_encl_truncate_backing_page(encl, PFN_DOWN(page_pcmd_off));
>>  	}
> 
> Reinette, nice work on tracking this down!
> 
> One comment on the fix though:  Will this put_page_testzero() ever
> actually trigger?  The page cache itself holds a reference, which is
> released in this case via:
> 
> 	shmem_truncate_range() ->
> 	truncate_inode_pages_range() ->
> 	truncate_inode_folio() ->
> 	filemap_remove_folio() ->
> 	filemap_free_folio() ->
> 	folio_put_refs()
> 
> So I think the lowest the page refcount can actually be at the point of
> put_page_testzero() is 1.

oh, I see, thank you for pointing that out. We then need something like
"put_page_testone()".

> 
> I suspect the end result of that patch would actually just be that PCMD
> pages are never freed at runtime, which would also work around the bug
> you discovered.
> 
> No matter what we do here, we need to ensure that there is no race between:
> 
>         pcmd_page_empty = !memchr_inv(pcmd_page, 0, PAGE_SIZE);
> and
> 	sgx_encl_truncate_backing_page(encl, PFN_DOWN(page_pcmd_off));
> 
> Holding the encl->lock over that section of code would work.  It also
> would not be awful to add a special "truncation lock" which is taken
> before putting data in a pcmd page and is taken over the
> memchr()->truncate window as well.

&encl->lock is already held over that section of the code so there
never is a race between the empty check and the truncate call.

The problem as I see it is that at the time __sgx_encl_eldu() is called
by the page fault handler (with enclave mutex held), the reclaimer already
has a reference to the PCMD page (but of course does _not_ have the enclave
mutex). The reclaimer expects to write to the PCMD page later when it obtains
the enclave mutex again and expects it to be safe since it has a reference
to the PCMD page.

The page fault handler will find the PCMD page empty when it removes
the last entry and thus truncate the page.

When reclaimer runs again it writes to the just-truncated PCMD page and
when it releases its reference the page is removed and the just written
data is lost.

My goal was to prevent the page fault handler from doing a "is the PCMD
page empty" if the reclaimer has a reference to it. Even if the PCMD page
is empty when the page fault handler checks it the page is expected to
get data right when reclaimer can get the mutex back since the reclaimer
already has a reference to the page.

Regarding your other suggestion, to use a "truncation lock". SGX just
gets the page pointer from shmem every time it is needed so there is
no room as I see it to add metadata to a PCMD page. One possibility
that we could do is for the page fault handler to check if any enclave
page sharing the PCMD page is being reclaimed (has the SGX_ENCL_PAGE_BEING_RECLAIMED
flag set). The reclaimer sets SGX_ENCL_PAGE_BEING_RECLAIMED for an enclave
page when it obtains the reference to its PCMD page and clears the flag
when it releases the reference.

Reinette