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.
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.
