2022-01-20 21:25:17

by Kristen Carlson Accardi

[permalink] [raw]
Subject: [PATCH v3 0/2] x86/sgx: Limit EPC overcommit

SGX currently allows EPC pages to be overcommitted. If the system is
out of enclave memory, EPC pages are swapped to normal RAM via
a per enclave shared memory area. This shared memory is not charged
to the enclave or the task mapping it, making it hard to account
for using normal methods. Since SGX will allow EPC pages to be
overcommitted without limits, enclaves can consume system memory
for these backing pages without limits.

In order to prevent this, set a cap on the amount of overcommit SGX
allows. Whenever a backing page is requested by an enclave, track
the total amount of shared memory pages used across all enclaves and
return an error if the overcommit limit has been reached. This will
restrict the total amount of backing pages that all enclaves can
consume to a maximum amount, and prevent enclaves from consuming
all the system RAM for backing pages.

The overcommit percentage has a value of 150, which limits shared
memory page consumption to 1.5x the number of EPC pages in the system.

Changes from v2
---------------
* whitespace change
* use reverse christmas tree format for variable
* modify commit message to add information about user impact

Changes from v1
----------------
* removed module parameter and disable boolean
* increased over commit percentage to 150% from 100%

Kristen Carlson Accardi (2):
x86/sgx: Add accounting for tracking overcommit
x86/sgx: account backing pages

arch/x86/kernel/cpu/sgx/encl.c | 76 ++++++++++++++++++++++++++++++++--
arch/x86/kernel/cpu/sgx/encl.h | 6 ++-
arch/x86/kernel/cpu/sgx/main.c | 52 +++++++++++++++++++++--
arch/x86/kernel/cpu/sgx/sgx.h | 2 +
4 files changed, 128 insertions(+), 8 deletions(-)

--
2.20.1


Kristen Carlson Accardi (2):
x86/sgx: Add accounting for tracking overcommit
x86/sgx: account backing pages

arch/x86/kernel/cpu/sgx/encl.c | 76 ++++++++++++++++++++++++++++++++--
arch/x86/kernel/cpu/sgx/encl.h | 6 ++-
arch/x86/kernel/cpu/sgx/main.c | 51 +++++++++++++++++++++--
arch/x86/kernel/cpu/sgx/sgx.h | 2 +
4 files changed, 127 insertions(+), 8 deletions(-)

--
2.20.1


2022-01-20 21:25:28

by Kristen Carlson Accardi

[permalink] [raw]
Subject: [PATCH v3 1/2] x86/sgx: Add accounting for tracking overcommit

When the system runs out of enclave memory, SGX can reclaim EPC pages
by swapping to normal RAM. This normal RAM is allocated via a
per-enclave shared memory area. The shared memory area is not mapped
into the enclave or the task mapping it, which makes its memory use
opaque (including to the OOM killer). Having lots of hard to find
memory around is problematic, especially when there is no limit.

Introduce a global counter that can be used to limit the number of pages
that enclaves are able to consume for backing storage. This parameter
is a percentage value that is used in conjunction with the number of
EPC pages in the system to set a cap on the amount of backing RAM that
can be consumed.

The default for this value is 150, which limits the total number of
shared memory pages that may be consumed by all enclaves as backing
pages to 1.5X of EPC pages on the system. For example, on an SGX
system that has 128MB of EPC, this default would cap the amount of
normal RAM that SGX consumes for its shared memory areas at 192MB.
The value of 1.5x the number of EPC pages was chosen because it should
handle the most common case of a few enclaves that don't need much
overcommit without any impact to user space. In the less common case
where there are many enclaves, or a few large enclaves which need
a lot of overcommit due to large EPC memory requirements, the
reclaimer may fail to allocate a backing page for swapping if the
limit has been reached. In this case, the page will not be able
to allocate any new EPC pages. Any ioctl or call to add new EPC
pages will get -ENOMEM, so for example, new enclaves will fail to
load, and new EPC pages will not be able to be added.

The SGX overcommit_percent works differently than the core VM overcommit
limit. Enclaves request backing pages one page at a time, and the number
of in use backing pages that are allowed is a global resource that is
limited for all enclaves.

Introduce a pair of functions which can be used by callers when requesting
backing RAM pages. These functions are responsible for accounting the
page charges. A request may return an error if the request will cause the
counter to exceed the backing page cap.

Signed-off-by: Kristen Carlson Accardi <[email protected]>
Tested-by: Jarkko Sakkinen <[email protected]>
---
arch/x86/kernel/cpu/sgx/main.c | 45 ++++++++++++++++++++++++++++++++++
arch/x86/kernel/cpu/sgx/sgx.h | 2 ++
2 files changed, 47 insertions(+)

diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
index 2857a49f2335..261e3702aef9 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -43,6 +43,45 @@ static struct sgx_numa_node *sgx_numa_nodes;

static LIST_HEAD(sgx_dirty_page_list);

+/*
+ * Limits the amount of normal RAM that SGX can consume for EPC
+ * overcommit to the total EPC pages * sgx_overcommit_percent / 100
+ */
+static int sgx_overcommit_percent = 150;
+
+/* The number of pages that can be allocated globally for backing storage. */
+static atomic_long_t sgx_nr_available_backing_pages;
+
+/**
+ * sgx_charge_mem() - charge for a page used for backing storage
+ *
+ * Backing storage usage is capped by the sgx_nr_available_backing_pages.
+ * If the backing storage usage is over the overcommit limit,
+ * return an error.
+ *
+ * Return:
+ * 0: The page requested does not exceed the limit
+ * -ENOMEM: The page requested exceeds the overcommit limit
+ */
+int sgx_charge_mem(void)
+{
+ if (!atomic_long_add_unless(&sgx_nr_available_backing_pages, -1, 0))
+ return -ENOMEM;
+
+ return 0;
+}
+
+/**
+ * sgx_uncharge_mem() - uncharge a page previously used for backing storage
+ *
+ * When backing storage is no longer in use, increment the
+ * sgx_nr_available_backing_pages counter.
+ */
+void sgx_uncharge_mem(void)
+{
+ atomic_long_inc(&sgx_nr_available_backing_pages);
+}
+
/*
* Reset post-kexec EPC pages to the uninitialized state. The pages are removed
* from the input list, and made available for the page allocator. SECS pages
@@ -783,6 +822,8 @@ static inline u64 __init sgx_calc_section_metric(u64 low, u64 high)
static bool __init sgx_page_cache_init(void)
{
u32 eax, ebx, ecx, edx, type;
+ u64 available_backing_bytes;
+ u64 total_epc_bytes = 0;
u64 pa, size;
int nid;
int i;
@@ -830,6 +871,7 @@ static bool __init sgx_page_cache_init(void)

sgx_epc_sections[i].node = &sgx_numa_nodes[nid];
sgx_numa_nodes[nid].size += size;
+ total_epc_bytes += size;

sgx_nr_epc_sections++;
}
@@ -839,6 +881,9 @@ static bool __init sgx_page_cache_init(void)
return false;
}

+ available_backing_bytes = total_epc_bytes * (sgx_overcommit_percent / 100);
+ atomic_long_set(&sgx_nr_available_backing_pages, available_backing_bytes >> PAGE_SHIFT);
+
return true;
}

diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h
index 0f17def9fe6f..3507a9983fc1 100644
--- a/arch/x86/kernel/cpu/sgx/sgx.h
+++ b/arch/x86/kernel/cpu/sgx/sgx.h
@@ -89,6 +89,8 @@ void sgx_free_epc_page(struct sgx_epc_page *page);
void sgx_mark_page_reclaimable(struct sgx_epc_page *page);
int sgx_unmark_page_reclaimable(struct sgx_epc_page *page);
struct sgx_epc_page *sgx_alloc_epc_page(void *owner, bool reclaim);
+int sgx_charge_mem(void);
+void sgx_uncharge_mem(void);

#ifdef CONFIG_X86_SGX_KVM
int __init sgx_vepc_init(void);
--
2.20.1

2022-01-20 21:25:36

by Kristen Carlson Accardi

[permalink] [raw]
Subject: [PATCH v3 2/2] x86/sgx: account backing pages

SGX may allow EPC pages to be overcommitted. If the system is
out of enclave memory, EPC pages are swapped to normal RAM via
a per enclave shared memory area. This shared memory is not
charged to the enclave or the task mapping it, making it hard
to account for using normal methods.

In order to avoid unlimited usage of normal RAM, enclaves must be
charged for each new page used for backing storage, and uncharged
when they are no longer using a backing page.

Modify the existing flow for requesting backing pages to reduce the
available backing page counter and confirm that the limit has not
been exceeded. Backing page usage for loading EPC pages back out of
the shared memory do not incur a charge.

When a backing page is released from usage, increment the available
backing page counter.

When swapping EPC pages to RAM, in addition to storing the page
contents, SGX must store some additional metadata to protect
against a malicious kernel when the page is swapped back in. This
additional metadata is called Paging Crypto MetaData. PCMD is
allocated from the same shared memory area as the backing page
contents and consumes RAM the same way.

PCMD is 128 bytes in size, and there is one PCMD structure per
page written to shared RAM. The page index for the PCMD page is
calculated from the page index of the backing page, so it is possible
that the PCMD structures are not packed into the minimum number of
pages possible. If 32 PCMDs can fit onto a single page, then PCMD
usage is 1/32 of total EPC pages. In the worst case, PCMD can
consume the same amount of RAM as EPC backing pages (1:1). For
simplicity, this implementation does not account for PCMD page usage.

Signed-off-by: Kristen Carlson Accardi <[email protected]>
Tested-by: Jarkko Sakkinen <[email protected]>
---
arch/x86/kernel/cpu/sgx/encl.c | 76 ++++++++++++++++++++++++++++++++--
arch/x86/kernel/cpu/sgx/encl.h | 6 ++-
arch/x86/kernel/cpu/sgx/main.c | 6 +--
3 files changed, 80 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
index 001808e3901c..8be6f0592bdc 100644
--- a/arch/x86/kernel/cpu/sgx/encl.c
+++ b/arch/x86/kernel/cpu/sgx/encl.c
@@ -32,7 +32,7 @@ static int __sgx_encl_eldu(struct sgx_encl_page *encl_page,
else
page_index = PFN_DOWN(encl->size);

- ret = sgx_encl_get_backing(encl, page_index, &b);
+ ret = sgx_encl_lookup_backing(encl, page_index, &b);
if (ret)
return ret;

@@ -407,6 +407,12 @@ void sgx_encl_release(struct kref *ref)
sgx_encl_free_epc_page(entry->epc_page);
encl->secs_child_cnt--;
entry->epc_page = NULL;
+ } else {
+ /*
+ * If there is no epc_page, it means it has been
+ * swapped out. Uncharge the backing storage.
+ */
+ sgx_uncharge_mem();
}

kfree(entry);
@@ -574,8 +580,8 @@ static struct page *sgx_encl_get_backing_page(struct sgx_encl *encl,
* 0 on success,
* -errno otherwise.
*/
-int sgx_encl_get_backing(struct sgx_encl *encl, unsigned long page_index,
- struct sgx_backing *backing)
+static int sgx_encl_get_backing(struct sgx_encl *encl, unsigned long page_index,
+ struct sgx_backing *backing)
{
pgoff_t pcmd_index = PFN_DOWN(encl->size) + 1 + (page_index >> 5);
struct page *contents;
@@ -601,6 +607,62 @@ int sgx_encl_get_backing(struct sgx_encl *encl, unsigned long page_index,
return 0;
}

+/**
+ * sgx_encl_alloc_backing() - allocate a new backing storage page
+ * @encl: an enclave pointer
+ * @page_index: enclave page index
+ * @backing: data for accessing backing storage for the page
+ *
+ * Confirm that the global overcommit limit has not been reached before
+ * requesting a new backing storage page for storing the encrypted contents
+ * and Paging Crypto MetaData (PCMD) of an enclave page. This is called when
+ * there is no existing backing page, just before writing to the backing
+ * storage with EWB.
+ *
+ * Return:
+ * 0 on success,
+ * -errno otherwise.
+ */
+int sgx_encl_alloc_backing(struct sgx_encl *encl, unsigned long page_index,
+ struct sgx_backing *backing)
+{
+ int ret;
+
+ if (sgx_charge_mem())
+ return -ENOMEM;
+
+ ret = sgx_encl_get_backing(encl, page_index, backing);
+ if (ret)
+ sgx_uncharge_mem();
+
+ return ret;
+}
+
+/**
+ * sgx_encl_lookup_backing() - retrieve an existing backing storage page
+ * @encl: an enclave pointer
+ * @page_index: enclave page index
+ * @backing: data for accessing backing storage for the page
+ *
+ * Retrieve a backing page for loading data back into an EPC page with ELDU.
+ * This call does not cause a charge to the overcommit limit because a page
+ * has already been allocated, but has been swapped out or is in RAM
+ *
+ * It is the caller's responsibility to ensure that it is appropriate to
+ * use sgx_encl_lookup_backing() rather than sgx_encl_alloc_backing(). If
+ * lookup is not used correctly, this will cause an allocation that is
+ * not accounted for.
+ *
+ * Return:
+ * 0 on success,
+ * -errno otherwise.
+ */
+int sgx_encl_lookup_backing(struct sgx_encl *encl, unsigned long page_index,
+ struct sgx_backing *backing)
+{
+ return sgx_encl_get_backing(encl, page_index, backing);
+}
+
/**
* sgx_encl_put_backing() - Unpin the backing storage
* @backing: data for accessing backing storage for the page
@@ -608,9 +670,17 @@ int sgx_encl_get_backing(struct sgx_encl *encl, unsigned long page_index,
*/
void sgx_encl_put_backing(struct sgx_backing *backing, bool do_write)
{
+ /*
+ * If the page is being written to by the reclaimer, it is
+ * still in use and the backing page usage should not be
+ * uncharged. However, if the page is not being written to,
+ * it is no longer in use and may be uncharged.
+ */
if (do_write) {
set_page_dirty(backing->pcmd);
set_page_dirty(backing->contents);
+ } else {
+ sgx_uncharge_mem();
}

put_page(backing->pcmd);
diff --git a/arch/x86/kernel/cpu/sgx/encl.h b/arch/x86/kernel/cpu/sgx/encl.h
index fec43ca65065..8ffb8a83263f 100644
--- a/arch/x86/kernel/cpu/sgx/encl.h
+++ b/arch/x86/kernel/cpu/sgx/encl.h
@@ -105,8 +105,10 @@ int sgx_encl_may_map(struct sgx_encl *encl, unsigned long start,

void sgx_encl_release(struct kref *ref);
int sgx_encl_mm_add(struct sgx_encl *encl, struct mm_struct *mm);
-int sgx_encl_get_backing(struct sgx_encl *encl, unsigned long page_index,
- struct sgx_backing *backing);
+int sgx_encl_alloc_backing(struct sgx_encl *encl, unsigned long page_index,
+ struct sgx_backing *backing);
+int sgx_encl_lookup_backing(struct sgx_encl *encl, unsigned long page_index,
+ struct sgx_backing *backing);
void sgx_encl_put_backing(struct sgx_backing *backing, bool do_write);
int sgx_encl_test_and_clear_young(struct mm_struct *mm,
struct sgx_encl_page *page);
diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
index 261e3702aef9..990f341bbd30 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -347,8 +347,8 @@ static void sgx_reclaimer_write(struct sgx_epc_page *epc_page,
encl->secs_child_cnt--;

if (!encl->secs_child_cnt && test_bit(SGX_ENCL_INITIALIZED, &encl->flags)) {
- ret = sgx_encl_get_backing(encl, PFN_DOWN(encl->size),
- &secs_backing);
+ ret = sgx_encl_alloc_backing(encl, PFN_DOWN(encl->size),
+ &secs_backing);
if (ret)
goto out;

@@ -418,7 +418,7 @@ static void sgx_reclaim_pages(void)
goto skip;

page_index = PFN_DOWN(encl_page->desc - encl_page->encl->base);
- ret = sgx_encl_get_backing(encl_page->encl, page_index, &backing[i]);
+ ret = sgx_encl_alloc_backing(encl_page->encl, page_index, &backing[i]);
if (ret)
goto skip;

--
2.20.1

2022-01-21 22:05:15

by Jarkko Sakkinen

[permalink] [raw]
Subject: Re: [PATCH v3 2/2] x86/sgx: account backing pages

On Tue, 2022-01-18 at 09:57 -0800, Kristen Carlson Accardi wrote:
> SGX may allow EPC pages to be overcommitted. If the system is
> out of enclave memory, EPC pages are swapped to normal RAM via
> a per enclave shared memory area. This shared memory is not
> charged to the enclave or the task mapping it, making it hard
> to account for using normal methods.
>
> In order to avoid unlimited usage of normal RAM, enclaves must be
> charged for each new page used for backing storage, and uncharged
> when they are no longer using a backing page.
>
> Modify the existing flow for requesting backing pages to reduce the
> available backing page counter and confirm that the limit has not
> been exceeded. Backing page usage for loading EPC pages back out of
> the shared memory do not incur a charge.
>
> When a backing page is released from usage, increment the available
> backing page counter.
>
> When swapping EPC pages to RAM, in addition to storing the page
> contents, SGX must store some additional metadata to protect
> against a malicious kernel when the page is swapped back in. This
> additional metadata is called Paging Crypto MetaData. PCMD is
> allocated from the same shared memory area as the backing page
> contents and consumes RAM the same way.
>
> PCMD is 128 bytes in size, and there is one PCMD structure per
> page written to shared RAM. The page index for the PCMD page is
> calculatted from the page index of the backing page, so it is
> possible
> that the PCMD structures are not packed into the minimum number of
> pages possible. If 32 PCMDs can fit onto a single page, then PCMD
> usage is 1/32 of total EPC pages. In the worst case, PCMD can
> consume the same amount of RAM as EPC backing pages (1:1). For
> simplicity, this implementation does not account for PCMD page usage.
>
> Signed-off-by: Kristen Carlson Accardi <[email protected]>
> Tested-by: Jarkko Sakkinen <[email protected]>
> ---
>  arch/x86/kernel/cpu/sgx/encl.c | 76
> ++++++++++++++++++++++++++++++++--
>  arch/x86/kernel/cpu/sgx/encl.h |  6 ++-
>  arch/x86/kernel/cpu/sgx/main.c |  6 +--
>  3 files changed, 80 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/sgx/encl.c
> b/arch/x86/kernel/cpu/sgx/encl.c
> index 001808e3901c..8be6f0592bdc 100644
> --- a/arch/x86/kernel/cpu/sgx/encl.c
> +++ b/arch/x86/kernel/cpu/sgx/encl.c
> @@ -32,7 +32,7 @@ static int __sgx_encl_eldu(struct sgx_encl_page
> *encl_page,
>         else
>                 page_index = PFN_DOWN(encl->size);
>  
> -       ret = sgx_encl_get_backing(encl, page_index, &b);
> +       ret = sgx_encl_lookup_backing(encl, page_index, &b);
>         if (ret)
>                 return ret;
>  
> @@ -407,6 +407,12 @@ void sgx_encl_release(struct kref *ref)
>                         sgx_encl_free_epc_page(entry->epc_page);
>                         encl->secs_child_cnt--;
>                         entry->epc_page = NULL;
> +               } else {
> +                       /*
> +                        * If there is no epc_page, it means it has
> been
> +                        * swapped out. Uncharge the backing storage.
> +                        */
> +                       sgx_uncharge_mem();
>                 }
>  
>                 kfree(entry);
> @@ -574,8 +580,8 @@ static struct page
> *sgx_encl_get_backing_page(struct sgx_encl *encl,
>   *   0 on success,
>   *   -errno otherwise.
>   */
> -int sgx_encl_get_backing(struct sgx_encl *encl, unsigned long
> page_index,
> -                        struct sgx_backing *backing)
> +static int sgx_encl_get_backing(struct sgx_encl *encl, unsigned long
> page_index,
> +                               struct sgx_backing *backing)
>  {
>         pgoff_t pcmd_index = PFN_DOWN(encl->size) + 1 + (page_index
> >> 5);
>         struct page *contents;
> @@ -601,6 +607,62 @@ int sgx_encl_get_backing(struct sgx_encl *encl,
> unsigned long page_index,
>         return 0;
>  }
>  
> +/**
> + * sgx_encl_alloc_backing() - allocate a new backing storage page
> + * @encl:      an enclave pointer
> + * @page_index:        enclave page index
> + * @backing:   data for accessing backing storage for the page
> + *
> + * Confirm that the global overcommit limit has not been reached
> before
> + * requesting a new backing storage page for storing the encrypted
> contents
> + * and Paging Crypto MetaData (PCMD) of an enclave page. This is
> called when
> + * there is no existing backing page, just before writing to the
> backing
> + * storage with EWB.
> + *
> + * Return:
> + *   0 on success,
> + *   -errno otherwise.
> + */
> +int sgx_encl_alloc_backing(struct sgx_encl *encl, unsigned long
> page_index,
> +                          struct sgx_backing *backing)
> +{
> +       int ret;
> +
> +       if (sgx_charge_mem())
> +               return -ENOMEM;
> +
> +       ret = sgx_encl_get_backing(encl, page_index, backing);
> +       if (ret)
> +               sgx_uncharge_mem();
> +
> +       return ret;
> +}
> +
> +/**
> + * sgx_encl_lookup_backing() - retrieve an existing backing storage
> page
> + * @encl:      an enclave pointer
> + * @page_index:        enclave page index
> + * @backing:   data for accessing backing storage for the page
> + *
> + * Retrieve a backing page for loading data back into an EPC page
> with ELDU.
> + * This call does not cause a charge to the overcommit limit because
> a page
> + * has already been allocated, but has been swapped out or is in RAM
> + *
> + * It is the caller's responsibility to ensure that it is
> appropriate to
> + * use sgx_encl_lookup_backing() rather than
> sgx_encl_alloc_backing(). If
> + * lookup is not used correctly, this will cause an allocation that
> is
> + * not accounted for.
> + *
> + * Return:
> + *   0 on success,
> + *   -errno otherwise.
> + */
> +int sgx_encl_lookup_backing(struct sgx_encl *encl, unsigned long
> page_index,
> +                           struct sgx_backing *backing)
> +{
> +       return sgx_encl_get_backing(encl, page_index, backing);
> +}
> +
>  /**
>   * sgx_encl_put_backing() - Unpin the backing storage
>   * @backing:   data for accessing backing storage for the page
> @@ -608,9 +670,17 @@ int sgx_encl_get_backing(struct sgx_encl *encl,
> unsigned long page_index,
>   */
>  void sgx_encl_put_backing(struct sgx_backing *backing, bool
> do_write)
>  {
> +       /*
> +        * If the page is being written to by the reclaimer, it is
> +        * still in use and the backing page usage should not be
> +        * uncharged. However, if the page is not being written to,
> +        * it is no longer in use and may be uncharged.
> +        */
>         if (do_write) {
>                 set_page_dirty(backing->pcmd);
>                 set_page_dirty(backing->contents);
> +       } else {
> +               sgx_uncharge_mem();
>         }
>  
>         put_page(backing->pcmd);
> diff --git a/arch/x86/kernel/cpu/sgx/encl.h
> b/arch/x86/kernel/cpu/sgx/encl.h
> index fec43ca65065..8ffb8a83263f 100644
> --- a/arch/x86/kernel/cpu/sgx/encl.h
> +++ b/arch/x86/kernel/cpu/sgx/encl.h
> @@ -105,8 +105,10 @@ int sgx_encl_may_map(struct sgx_encl *encl,
> unsigned long start,
>  
>  void sgx_encl_release(struct kref *ref);
>  int sgx_encl_mm_add(struct sgx_encl *encl, struct mm_struct *mm);
> -int sgx_encl_get_backing(struct sgx_encl *encl, unsigned long
> page_index,
> -                        struct sgx_backing *backing);
> +int sgx_encl_alloc_backing(struct sgx_encl *encl, unsigned long
> page_index,
> +                          struct sgx_backing *backing);
> +int sgx_encl_lookup_backing(struct sgx_encl *encl, unsigned long
> page_index,
> +                           struct sgx_backing *backing);
>  void sgx_encl_put_backing(struct sgx_backing *backing, bool
> do_write);
>  int sgx_encl_test_and_clear_young(struct mm_struct *mm,
>                                   struct sgx_encl_page *page);
> diff --git a/arch/x86/kernel/cpu/sgx/main.c
> b/arch/x86/kernel/cpu/sgx/main.c
> index 261e3702aef9..990f341bbd30 100644
> --- a/arch/x86/kernel/cpu/sgx/main.c
> +++ b/arch/x86/kernel/cpu/sgx/main.c
> @@ -347,8 +347,8 @@ static void sgx_reclaimer_write(struct
> sgx_epc_page *epc_page,
>         encl->secs_child_cnt--;
>  
>         if (!encl->secs_child_cnt && test_bit(SGX_ENCL_INITIALIZED,
> &encl->flags)) {
> -               ret = sgx_encl_get_backing(encl, PFN_DOWN(encl-
> >size),
> -                                          &secs_backing);
> +               ret = sgx_encl_alloc_backing(encl, PFN_DOWN(encl-
> >size),
> +                                            &secs_backing);
>                 if (ret)
>                         goto out;
>  
> @@ -418,7 +418,7 @@ static void sgx_reclaim_pages(void)
>                         goto skip;
>  
>                 page_index = PFN_DOWN(encl_page->desc - encl_page-
> >encl->base);
> -               ret = sgx_encl_get_backing(encl_page->encl,
> page_index, &backing[i]);
> +               ret = sgx_encl_alloc_backing(encl_page->encl,
> page_index, &backing[i]);
>                 if (ret)
>                         goto skip;
>  

Wrappers make sense to do extra checks so:

Reviewed-by: Jarkko Sakkinen <[email protected]>

BR, Jarkko

2022-01-21 22:05:58

by Jarkko Sakkinen

[permalink] [raw]
Subject: Re: [PATCH v3 1/2] x86/sgx: Add accounting for tracking overcommit

On Tue, 2022-01-18 at 09:57 -0800, Kristen Carlson Accardi wrote:
> When the system runs out of enclave memory, SGX can reclaim EPC pages
> by swapping to normal RAM. This normal RAM is allocated via a
> per-enclave shared memory area. The shared memory area is not mapped
> into the enclave or the task mapping it, which makes its memory use
> opaque (including to the OOM killer). Having lots of hard to find
> memory around is problematic, especially when there is no limit.
>
> Introduce a global counter that can be used to limit the number of
> pages
> that enclaves are able to consume for backing storage.  This
> parameter
> is a percentage value that is used in conjunction with the number of
> EPC pages in the system to set a cap on the amount of backing RAM
> that
> can be consumed.
>
> The default for this value is 150, which limits the total number of
> shared memory pages that may be consumed by all enclaves as backing
> pages to 1.5X of EPC pages on the system. For example, on an SGX
> system that has 128MB of EPC, this default would cap the amount of
> normal RAM that SGX consumes for its shared memory areas at 192MB.
> The value of 1.5x the number of EPC pages was chosen because it
> should
> handle the most common case of a few enclaves that don't need much
> overcommit without any impact to user space. In the less common case
> where there are many enclaves, or a few large enclaves which need
> a lot of overcommit due to large EPC memory requirements, the
> reclaimer may fail to allocate a backing page for swapping if the
> limit has been reached. In this case, the page will not be able
> to allocate any new EPC pages. Any ioctl or call to add new EPC
> pages will get -ENOMEM, so for example, new enclaves will fail to
> load, and new EPC pages will not be able to be added.
>
> The SGX overcommit_percent works differently than the core VM
> overcommit
> limit. Enclaves request backing pages one page at a time, and the
> number
> of in use backing pages that are allowed is a global resource that is
> limited for all enclaves.
>
> Introduce a pair of functions which can be used by callers when
> requesting
> backing RAM pages. These functions are responsible for accounting the
> page charges. A request may return an error if the request will cause
> the
> counter to exceed the backing page cap.
>
> Signed-off-by: Kristen Carlson Accardi <[email protected]>
> Tested-by: Jarkko Sakkinen <[email protected]>
> ---
>  arch/x86/kernel/cpu/sgx/main.c | 45
> ++++++++++++++++++++++++++++++++++
>  arch/x86/kernel/cpu/sgx/sgx.h  |  2 ++
>  2 files changed, 47 insertions(+)
>
> diff --git a/arch/x86/kernel/cpu/sgx/main.c
> b/arch/x86/kernel/cpu/sgx/main.c
> index 2857a49f2335..261e3702aef9 100644
> --- a/arch/x86/kernel/cpu/sgx/main.c
> +++ b/arch/x86/kernel/cpu/sgx/main.c
> @@ -43,6 +43,45 @@ static struct sgx_numa_node *sgx_numa_nodes;
>  
>  static LIST_HEAD(sgx_dirty_page_list);
>  
> +/*
> + * Limits the amount of normal RAM that SGX can consume for EPC
> + * overcommit to the total EPC pages * sgx_overcommit_percent / 100
> + */
> +static int sgx_overcommit_percent = 150;
> +
> +/* The number of pages that can be allocated globally for backing
> storage. */
> +static atomic_long_t sgx_nr_available_backing_pages;
> +
> +/**
> + * sgx_charge_mem() - charge for a page used for backing storage
> + *
> + * Backing storage usage is capped by the
> sgx_nr_available_backing_pages.
> + * If the backing storage usage is over the overcommit limit,
> + * return an error.
> + *
> + * Return:
> + * 0:          The page requested does not exceed the limit
> + * -ENOMEM:    The page requested exceeds the overcommit limit
> + */
> +int sgx_charge_mem(void)
> +{
> +       if (!atomic_long_add_unless(&sgx_nr_available_backing_pages,
> -1, 0))
> +               return -ENOMEM;
> +
> +       return 0;
> +}
> +
> +/**
> + * sgx_uncharge_mem() - uncharge a page previously used for backing
> storage
> + *
> + * When backing storage is no longer in use, increment the
> + * sgx_nr_available_backing_pages counter.
> + */
> +void sgx_uncharge_mem(void)
> +{
> +       atomic_long_inc(&sgx_nr_available_backing_pages);
> +}
> +
>  /*
>   * Reset post-kexec EPC pages to the uninitialized state. The pages
> are removed
>   * from the input list, and made available for the page allocator.
> SECS pages
> @@ -783,6 +822,8 @@ static inline u64 __init
> sgx_calc_section_metric(u64 low, u64 high)
>  static bool __init sgx_page_cache_init(void)
>  {
>         u32 eax, ebx, ecx, edx, type;
> +       u64 available_backing_bytes;
> +       u64 total_epc_bytes = 0;
>         u64 pa, size;
>         int nid;
>         int i;
> @@ -830,6 +871,7 @@ static bool __init sgx_page_cache_init(void)
>  
>                 sgx_epc_sections[i].node =  &sgx_numa_nodes[nid];
>                 sgx_numa_nodes[nid].size += size;
> +               total_epc_bytes += size;
>  
>                 sgx_nr_epc_sections++;
>         }
> @@ -839,6 +881,9 @@ static bool __init sgx_page_cache_init(void)
>                 return false;
>         }
>  
> +       available_backing_bytes = total_epc_bytes *
> (sgx_overcommit_percent / 100);
> +       atomic_long_set(&sgx_nr_available_backing_pages,
> available_backing_bytes >> PAGE_SHIFT);
> +
>         return true;
>  }
>  
> diff --git a/arch/x86/kernel/cpu/sgx/sgx.h
> b/arch/x86/kernel/cpu/sgx/sgx.h
> index 0f17def9fe6f..3507a9983fc1 100644
> --- a/arch/x86/kernel/cpu/sgx/sgx.h
> +++ b/arch/x86/kernel/cpu/sgx/sgx.h
> @@ -89,6 +89,8 @@ void sgx_free_epc_page(struct sgx_epc_page *page);
>  void sgx_mark_page_reclaimable(struct sgx_epc_page *page);
>  int sgx_unmark_page_reclaimable(struct sgx_epc_page *page);
>  struct sgx_epc_page *sgx_alloc_epc_page(void *owner, bool reclaim);
> +int sgx_charge_mem(void);
> +void sgx_uncharge_mem(void);
>  
>  #ifdef CONFIG_X86_SGX_KVM
>  int __init sgx_vepc_init(void);

For me this looks cool. I also found out where the charge keyword comes
from while looking at shmem code for doing patches to add the checks that
Dave suggested (shmem_charge(), shmem_uncharge()).

Reviewed-by: Jarkko Sakkinen <[email protected]>

BR, Jarkko