Re: [PATCH V2] x86/sgx: Fix free page accounting
From: Reinette Chatre
Date: Wed Nov 10 2021 - 14:01:06 EST
Hi Jarkko,
On 11/10/2021 7:11 AM, Jarkko Sakkinen wrote:
On Tue, 2021-11-09 at 12:00 -0800, Reinette Chatre wrote:
The SGX driver maintains a single global free page counter,
sgx_nr_free_pages, that reflects the number of free pages available
across all NUMA nodes. Correspondingly, a list of free pages is
associated with each NUMA node and sgx_nr_free_pages is updated
every time a page is added or removed from any of the free page
lists. The main usage of sgx_nr_free_pages is by the reclaimer
that will run when it (sgx_nr_free_pages) goes below a watermark
to ensure that there are always some free pages available to, for
example, support efficient page faults.
With sgx_nr_free_pages accessed and modified from a few places
it is essential to ensure that these accesses are done safely but
this is not the case. sgx_nr_free_pages is read without any
protection and updated with inconsistent protection by any one
of the spin locks associated with the individual NUMA nodes.
For example:
CPU_A CPU_B
----- -----
spin_lock(&nodeA->lock); spin_lock(&nodeB->lock);
... ...
sgx_nr_free_pages--; /* NOT SAFE */ sgx_nr_free_pages--;
spin_unlock(&nodeA->lock); spin_unlock(&nodeB->lock);
I don't understand the "NOT SAFE" part here. It is safe to access
the variable, even when it is not atomic...
The "NOT SAFE" part is because in the above example (that reflects the
current code behavior) the updates to sgx_nr_free_pages is "protected"
by two _different_ spin locks and thus not actually protected.
I don't understand what the sequence above should tell me.
The consequence of sgx_nr_free_pages not being protected is that
its value may not accurately reflect the actual number of free
pages on the system, impacting the availability of free pages in
support of many flows. The problematic scenario isu when the
In non-atomicity is not a problem, when it is not a problem :-)
reclaimer does not run because it believes there to be sufficient
free pages while any attempt to allocate a page fails because there
are no free pages available.
The worst scenario observed was a user space hang because of
repeated page faults caused by no free pages made available.
The following flow was encountered:
asm_exc_page_fault
...
sgx_vma_fault()
sgx_encl_load_page()
sgx_encl_eldu() // Encrypted page needs to be loaded from backing
// storage into newly allocated SGX memory page
sgx_alloc_epc_page() // Allocate a page of SGX memory
__sgx_alloc_epc_page() // Fails, no free SGX memory
...
if (sgx_should_reclaim(SGX_NR_LOW_PAGES)) // Wake reclaimer
wake_up(&ksgxd_waitq);
return -EBUSY; // Return -EBUSY giving reclaimer time to run
return -EBUSY;
return -EBUSY;
return VM_FAULT_NOPAGE;
The reclaimer is triggered in above flow with the following code:
static bool sgx_should_reclaim(unsigned long watermark)
{
return sgx_nr_free_pages < watermark &&
!list_empty(&sgx_active_page_list);
}
In the problematic scenario there were no free pages available yet the
value of sgx_nr_free_pages was above the watermark. The allocation of
SGX memory thus always failed because of a lack of free pages while no
free pages were made available because the reclaimer is never started
because of sgx_nr_free_pages' incorrect value. The consequence was that
user space kept encountering VM_FAULT_NOPAGE that caused the same
address to be accessed repeatedly with the same result.
That causes sgx_should_reclaim() executed to be multiple times as the
fault is retried. Eventually it should be successful.
sgx_should_reclaim() would only succeed when sgx_nr_free_pages goes
below the watermark. Once sgx_nr_free_pages becomes corrupted there is
no clear way in which it can correct itself since it is only ever
incremented or decremented.
It may indeed be possible for the reclaimer to eventually get a chance
to run with a corrupted sgx_nr_free_pages if it is not wrong by more
than SGX_NR_LOW_PAGES when there are enough free pages available to have
the reclaimer run when it is almost depleted. Unfortunately, as in the
scenario I encountered, it is also possible for the free pages to be
depleted while sgx_nr_free_pages is above the watermark and in this case
there is not a way for the reclaimer to ever run.
On the system I tested with there was two nodes with about 64GB of SGX
memory per node and the test created an enclave that consumed all memory
across both nodes. The test then accessed all this memory three times,
once to change the type of each page, once for a read access to each
page from within the enclave, once to remove the page. With these many
accesses and unsafe updating of sgx_nr_free_pages it seems to be enough
to trigger the scenario where sgx_nr_free_pages has a value that is off
by more than SGX_NR_LOW_PAGES (which is just 32).
Change the global free page counter to an atomic type that
ensures simultaneous updates are done safely. While doing so, move
the updating of the variable outside of the spin lock critical
section to which it does not belong.
Cc: stable@xxxxxxxxxxxxxxx
Fixes: 901ddbb9ecf5 ("x86/sgx: Add a basic NUMA allocation scheme to sgx_alloc_epc_page()")
Suggested-by: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>
Reviewed-by: Tony Luck <tony.luck@xxxxxxxxx>
Signed-off-by: Reinette Chatre <reinette.chatre@xxxxxxxxx>
I'm not yet sure if this a bug, even if it'd be a improvement
to the performance.
Please let me know what additional data would convince you. The traces I
provided earlier show that without this patch the system spends almost
100% of time in the page fault handler while the user space application
hangs, with this patch the traces show that the system divides its time
between the page fault handler and the reclaimer and the user space
application is able to complete.
Reinette