Date:   Wed, 10 Nov 2021 10:51:11 -0800
From:   Reinette Chatre <reinette.chatre@...el.com>
To:     Jarkko Sakkinen <jarkko@...nel.org>, <dave.hansen@...ux.intel.com>,
        <tglx@...utronix.de>, <bp@...en8.de>, <mingo@...hat.com>,
        <linux-sgx@...r.kernel.org>, <x86@...nel.org>
CC:     <seanjc@...gle.com>, <tony.luck@...el.com>, <hpa@...or.com>,
        <linux-kernel@...r.kernel.org>, <stable@...r.kernel.org>
Subject: Re: [PATCH V2] x86/sgx: Fix free page accounting

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@...r.kernel.org
>> Fixes: 901ddbb9ecf5 ("x86/sgx: Add a basic NUMA allocation scheme to sgx_alloc_epc_page()")
>> Suggested-by: Dave Hansen <dave.hansen@...ux.intel.com>
>> Reviewed-by: Tony Luck <tony.luck@...el.com>
>> Signed-off-by: Reinette Chatre <reinette.chatre@...el.com>
> 
> 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

Powered by blists - more mailing lists