| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 5 Oct 2023 12:24:12 +0000
From: "Huang, Kai" <kai.huang@...el.com>
To: "hpa@...or.com" <hpa@...or.com>,
"linux-sgx@...r.kernel.org" <linux-sgx@...r.kernel.org>,
"x86@...nel.org" <x86@...nel.org>,
"dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
"cgroups@...r.kernel.org" <cgroups@...r.kernel.org>,
"bp@...en8.de" <bp@...en8.de>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"jarkko@...nel.org" <jarkko@...nel.org>,
"tglx@...utronix.de" <tglx@...utronix.de>,
"haitao.huang@...ux.intel.com" <haitao.huang@...ux.intel.com>,
"Mehta, Sohil" <sohil.mehta@...el.com>,
"tj@...nel.org" <tj@...nel.org>,
"mingo@...hat.com" <mingo@...hat.com>
CC: "kristen@...ux.intel.com" <kristen@...ux.intel.com>,
"yangjie@...rosoft.com" <yangjie@...rosoft.com>,
"Li, Zhiquan1" <zhiquan1.li@...el.com>,
"Christopherson,, Sean" <seanjc@...gle.com>,
"mikko.ylinen@...ux.intel.com" <mikko.ylinen@...ux.intel.com>,
"Zhang, Bo" <zhanb@...rosoft.com>,
"anakrish@...rosoft.com" <anakrish@...rosoft.com>
Subject: Re: [PATCH v5 13/18] x86/sgx: Expose sgx_reclaim_pages() for use by
EPC cgroup
On Fri, 2023-09-22 at 20:06 -0700, Haitao Huang wrote:
> From: Sean Christopherson <sean.j.christopherson@...el.com>
>
> Adjust and expose the top-level reclaim function as
> sgx_reclaim_epc_pages() for use by the upcoming EPC cgroup, which will
> initiate reclaim to enforce the max limit.
>
> Make these adjustments to the function signature.
>
> 1) To take a parameter that specifies the number of pages to scan for
> reclaiming. Define a max value of 32, but scan 16 in the case for the
> global reclaimer (ksgxd). The EPC cgroup will use it to specify a
> desired number of pages to be reclaimed up to the max value of 32.
>
> 2) To take a flag to force reclaiming a page regardless of its age. The
> EPC cgroup will use the flag to enforce its limits by draining the
> reclaimable lists before resorting to other measures, e.g. forcefully
> kill enclaves.
>
> 3) Return the number of reclaimed pages. The EPC cgroup will use the
> result to track reclaiming progress and escalate to a more forceful
> reclaiming mode, e.g., calling this function with the flag to ignore age
> of pages.
>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@...el.com>
> Co-developed-by: Kristen Carlson Accardi <kristen@...ux.intel.com>
> Signed-off-by: Kristen Carlson Accardi <kristen@...ux.intel.com>
> Co-developed-by: Haitao Huang <haitao.huang@...ux.intel.com>
> Signed-off-by: Haitao Huang <haitao.huang@...ux.intel.com>
> Cc: Sean Christopherson <seanjc@...gle.com>
> ---
> V4:
> - Combined the 3 patches that made the individual changes to the
> function signature.
> - Removed 'high' limit in commit message.
> ---
> arch/x86/kernel/cpu/sgx/main.c | 31 +++++++++++++++++++++----------
> arch/x86/kernel/cpu/sgx/sgx.h | 1 +
> 2 files changed, 22 insertions(+), 10 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> index 3b875ab4dcd0..4e1a3e038db5 100644
> --- a/arch/x86/kernel/cpu/sgx/main.c
> +++ b/arch/x86/kernel/cpu/sgx/main.c
> @@ -18,6 +18,11 @@
> #include "encl.h"
> #include "encls.h"
>
> +/*
> + * Maximum number of pages to scan for reclaiming.
> + */
> +#define SGX_NR_TO_SCAN_MAX 32
> +
> struct sgx_epc_section sgx_epc_sections[SGX_MAX_EPC_SECTIONS];
> static int sgx_nr_epc_sections;
> static struct task_struct *ksgxd_tsk;
> @@ -279,7 +284,11 @@ static void sgx_reclaimer_write(struct sgx_epc_page *epc_page,
> mutex_unlock(&encl->lock);
> }
>
> -/*
> +/**
> + * sgx_reclaim_epc_pages() - Reclaim EPC pages from the consumers
> + * @nr_to_scan: Number of EPC pages to scan for reclaim
> + * @ignore_age: Reclaim a page even if it is young
> + *
> * Take a fixed number of pages from the head of the active page pool and
> * reclaim them to the enclave's private shmem files. Skip the pages, which have
> * been accessed since the last scan. Move those pages to the tail of active
> @@ -292,15 +301,14 @@ static void sgx_reclaimer_write(struct sgx_epc_page *epc_page,
> * problematic as it would increase the lock contention too much, which would
> * halt forward progress.
> */
> -static void sgx_reclaim_pages(void)
> +size_t sgx_reclaim_epc_pages(size_t nr_to_scan, bool ignore_age)
'size_t' looks odd. Any reason to use it?
Given you only scan 32 at maximum, seems 'int' is good enough?
> {
> - struct sgx_backing backing[SGX_NR_TO_SCAN];
> + struct sgx_backing backing[SGX_NR_TO_SCAN_MAX];
> struct sgx_epc_page *epc_page, *tmp;
> struct sgx_encl_page *encl_page;
> pgoff_t page_index;
> LIST_HEAD(iso);
> - int ret;
> - int i;
> + size_t ret, i;
>
> spin_lock(&sgx_global_lru.lock);
> for (i = 0; i < SGX_NR_TO_SCAN; i++) {
The function comment says
* @nr_to_scan: Number of EPC pages to scan for reclaim
But I don't see it is even used, if my eye isn't deceiving me?
> @@ -326,13 +334,14 @@ static void sgx_reclaim_pages(void)
> spin_unlock(&sgx_global_lru.lock);
>
> if (list_empty(&iso))
> - return;
> + return 0;
>
> i = 0;
> list_for_each_entry_safe(epc_page, tmp, &iso, list) {
> encl_page = epc_page->encl_page;
>
> - if (!sgx_reclaimer_age(epc_page))
> + if (i == SGX_NR_TO_SCAN_MAX ||
i == nr_to_scan?
And should we have a
if (nr_to_scan < SGX_NR_TO_SCAN_MAX)
return 0;
at the very beginning of this function?
> + (!ignore_age && !sgx_reclaimer_age(epc_page)))
> goto skip;
>
> page_index = PFN_DOWN(encl_page->desc - encl_page->encl->base);
> @@ -371,6 +380,8 @@ static void sgx_reclaim_pages(void)
>
> sgx_free_epc_page(epc_page);
> }
> +
> + return i;
> }
>
I found this function a little bit odd, given the mixing of 'nr_to_scan',
SGX_NR_TO_SCAN and SGX_NR_TO_SCAN_MAX.
From the changelog:
1) To take a parameter that specifies the number of pages to scan for
reclaiming. Define a max value of 32, but scan 16 in the case for the
global reclaimer (ksgxd).
It appears we want to make this function to scan @nr_to_scan for cgroup, but
still want to scan a fixed value for ksgxd, which is SGX_NR_TO_SCAN. And
@nr_to_scan can be larger than SGX_NR_TO_SCAN but smaller than
SGX_NR_TO_SCAN_MAX.
Putting behind the mystery of why above is needed, to achieve it, is it more
clear if we do below?
int __sgx_reclaim_epc_pages(int nr_to_scan, bool ignore_age)
{
struct sgx_backing backing[SGX_NR_TO_SCAN_MAX];
...
if (nr_to_scan > SGX_NR_TO_SCAN_MAX)
return 0;
for (i = 0; i < nr_to_scan; i++) {
...
}
return reclaimed;
}
/* This is for ksgxd() */
int sgx_reclaim_epc_page(void)
{
return __sgx_reclaim_epc_pages(SGX_NR_TO_SCAN, false);
}
EPC cgroup calls __sgx_reclaim_epc_pages() directly, or introduce another
wrapper.
Powered by blists - more mailing lists