| Message ID | 20210708181423.1312359-2-tony.luck@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Basic recovery for machine checks inside SGX | expand |
On Thu, Jul 08, 2021 at 11:14:20AM -0700, Tony Luck wrote: > Memory errors can be reported either synchronously as memory is accessed, > or asynchronously by speculative access or by a memory controller page > scrubber. The life cycle of an EPC page takes it through: > dirty -> free -> in-use -> free. > > Memory errors are reported using physical addresses. It is a simple > matter to find which sgx_epc_page structure maps a given address. > But then recovery code needs to be able to determine the current use of > the page to take the appropriate recovery action. Within the "in-use" > phase different actions are needed based on how the page is used in > the enclave. > > Add new flags bits to describe the phase (with an extra bit for the new > phase of "poisoned"). Drop pages marked as poisoned instead of adding > them to a free list to make sure they are not re-used. > > Add a type field to struct epc_page for how an in-use page has been > allocated. Re-use "enum sgx_page_type" for this type, with a couple > of additions for s/w types. > > Signed-off-by: Tony Luck <tony.luck@intel.com> > --- > arch/x86/include/asm/sgx.h | 6 ++++++ > arch/x86/kernel/cpu/sgx/encl.c | 4 ++-- > arch/x86/kernel/cpu/sgx/ioctl.c | 4 ++-- > arch/x86/kernel/cpu/sgx/main.c | 21 +++++++++++++++++++-- > arch/x86/kernel/cpu/sgx/sgx.h | 14 ++++++++++++-- > arch/x86/kernel/cpu/sgx/virt.c | 2 +- > 6 files changed, 42 insertions(+), 9 deletions(-) > > diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h > index 9c31e0ebc55b..9619a6d77a83 100644 > --- a/arch/x86/include/asm/sgx.h > +++ b/arch/x86/include/asm/sgx.h > @@ -216,6 +216,8 @@ struct sgx_pageinfo { > * %SGX_PAGE_TYPE_REG: a regular page > * %SGX_PAGE_TYPE_VA: a VA page > * %SGX_PAGE_TYPE_TRIM: a page in trimmed state > + * > + * Also used to track current use of &struct sgx_epc_page > */ > enum sgx_page_type { > SGX_PAGE_TYPE_SECS, > @@ -223,6 +225,10 @@ enum sgx_page_type { > SGX_PAGE_TYPE_REG, > SGX_PAGE_TYPE_VA, > SGX_PAGE_TYPE_TRIM, > + > + /* sgx_epc_page.type */ > + SGX_PAGE_TYPE_FREE = 100, > + SGX_PAGE_TYPE_KVM = 101, > }; > > #define SGX_NR_PAGE_TYPES 5 > diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c > index 3be203297988..abf6e1a704c0 100644 > --- a/arch/x86/kernel/cpu/sgx/encl.c > +++ b/arch/x86/kernel/cpu/sgx/encl.c > @@ -72,7 +72,7 @@ static struct sgx_epc_page *sgx_encl_eldu(struct sgx_encl_page *encl_page, > struct sgx_epc_page *epc_page; > int ret; > > - epc_page = sgx_alloc_epc_page(encl_page, false); > + epc_page = sgx_alloc_epc_page(encl_page, SGX_PAGE_TYPE_REG, false); > if (IS_ERR(epc_page)) > return epc_page; > > @@ -679,7 +679,7 @@ struct sgx_epc_page *sgx_alloc_va_page(void) > struct sgx_epc_page *epc_page; > int ret; > > - epc_page = sgx_alloc_epc_page(NULL, true); > + epc_page = sgx_alloc_epc_page(NULL, SGX_PAGE_TYPE_VA, true); > if (IS_ERR(epc_page)) > return ERR_CAST(epc_page); > > diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c > index 83df20e3e633..a74ae00194cc 100644 > --- a/arch/x86/kernel/cpu/sgx/ioctl.c > +++ b/arch/x86/kernel/cpu/sgx/ioctl.c > @@ -83,7 +83,7 @@ static int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs) > > encl->backing = backing; > > - secs_epc = sgx_alloc_epc_page(&encl->secs, true); > + secs_epc = sgx_alloc_epc_page(&encl->secs, SGX_PAGE_TYPE_SECS, true); > if (IS_ERR(secs_epc)) { > ret = PTR_ERR(secs_epc); > goto err_out_backing; > @@ -300,7 +300,7 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long src, > if (IS_ERR(encl_page)) > return PTR_ERR(encl_page); > > - epc_page = sgx_alloc_epc_page(encl_page, true); > + epc_page = sgx_alloc_epc_page(encl_page, SGX_PAGE_TYPE_REG, true); > if (IS_ERR(epc_page)) { > kfree(encl_page); > return PTR_ERR(epc_page); > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c > index 63d3de02bbcc..643df87b3e01 100644 > --- a/arch/x86/kernel/cpu/sgx/main.c > +++ b/arch/x86/kernel/cpu/sgx/main.c > @@ -401,7 +401,12 @@ static void sgx_reclaim_pages(void) > section = &sgx_epc_sections[epc_page->section]; > node = section->node; > > + /* drop poison pages instead of adding to free list */ > + if (epc_page->flags & SGX_EPC_PAGE_POISON) > + continue; > + > spin_lock(&node->lock); > + epc_page->flags = SGX_EPC_PAGE_FREE; > list_add_tail(&epc_page->list, &node->free_page_list); > sgx_nr_free_pages++; > spin_unlock(&node->lock); > @@ -560,6 +565,7 @@ int sgx_unmark_page_reclaimable(struct sgx_epc_page *page) > /** > * sgx_alloc_epc_page() - Allocate an EPC page > * @owner: the owner of the EPC page > + * @type: type of page being allocated > * @reclaim: reclaim pages if necessary > * > * Iterate through EPC sections and borrow a free EPC page to the caller. When a > @@ -574,7 +580,7 @@ int sgx_unmark_page_reclaimable(struct sgx_epc_page *page) > * an EPC page, > * -errno on error > */ > -struct sgx_epc_page *sgx_alloc_epc_page(void *owner, bool reclaim) > +struct sgx_epc_page *sgx_alloc_epc_page(void *owner, enum sgx_page_type type, bool reclaim) > { > struct sgx_epc_page *page; > > @@ -582,6 +588,8 @@ struct sgx_epc_page *sgx_alloc_epc_page(void *owner, bool reclaim) > page = __sgx_alloc_epc_page(); > if (!IS_ERR(page)) { > page->owner = owner; > + page->type = type; > + page->flags = 0; > break; > } > > @@ -616,14 +624,22 @@ struct sgx_epc_page *sgx_alloc_epc_page(void *owner, bool reclaim) > * responsibility to make sure that the page is in uninitialized state. In other > * words, do EREMOVE, EWB or whatever operation is necessary before calling > * this function. > + * > + * Note that if the page has been tagged as poisoned, it is simply > + * dropped on the floor instead of added to the free list to make > + * sure we do not re-use it. > */ > void sgx_free_epc_page(struct sgx_epc_page *page) > { > struct sgx_epc_section *section = &sgx_epc_sections[page->section]; > struct sgx_numa_node *node = section->node; > > + if (page->flags & SGX_EPC_PAGE_POISON) > + return; I tend to think that it would be nice to collect them somewhere instead purposely leaking. E.g. this gives possibility to examine list with debugging tools. /Jarkko
On Fri, Jul 09, 2021 at 09:08:03PM +0300, Jarkko Sakkinen wrote: > On Thu, Jul 08, 2021 at 11:14:20AM -0700, Tony Luck wrote: > > Memory errors can be reported either synchronously as memory is accessed, > > or asynchronously by speculative access or by a memory controller page > > scrubber. The life cycle of an EPC page takes it through: > > dirty -> free -> in-use -> free. > > > > Memory errors are reported using physical addresses. It is a simple > > matter to find which sgx_epc_page structure maps a given address. > > But then recovery code needs to be able to determine the current use of > > the page to take the appropriate recovery action. Within the "in-use" > > phase different actions are needed based on how the page is used in > > the enclave. > > > > Add new flags bits to describe the phase (with an extra bit for the new > > phase of "poisoned"). Drop pages marked as poisoned instead of adding > > them to a free list to make sure they are not re-used. > > > > Add a type field to struct epc_page for how an in-use page has been > > allocated. Re-use "enum sgx_page_type" for this type, with a couple > > of additions for s/w types. > > > > Signed-off-by: Tony Luck <tony.luck@intel.com> > > --- > > arch/x86/include/asm/sgx.h | 6 ++++++ > > arch/x86/kernel/cpu/sgx/encl.c | 4 ++-- > > arch/x86/kernel/cpu/sgx/ioctl.c | 4 ++-- > > arch/x86/kernel/cpu/sgx/main.c | 21 +++++++++++++++++++-- > > arch/x86/kernel/cpu/sgx/sgx.h | 14 ++++++++++++-- > > arch/x86/kernel/cpu/sgx/virt.c | 2 +- > > 6 files changed, 42 insertions(+), 9 deletions(-) > > > > diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h > > index 9c31e0ebc55b..9619a6d77a83 100644 > > --- a/arch/x86/include/asm/sgx.h > > +++ b/arch/x86/include/asm/sgx.h > > @@ -216,6 +216,8 @@ struct sgx_pageinfo { > > * %SGX_PAGE_TYPE_REG: a regular page > > * %SGX_PAGE_TYPE_VA: a VA page > > * %SGX_PAGE_TYPE_TRIM: a page in trimmed state > > + * > > + * Also used to track current use of &struct sgx_epc_page > > */ > > enum sgx_page_type { > > SGX_PAGE_TYPE_SECS, > > @@ -223,6 +225,10 @@ enum sgx_page_type { > > SGX_PAGE_TYPE_REG, > > SGX_PAGE_TYPE_VA, > > SGX_PAGE_TYPE_TRIM, > > + > > + /* sgx_epc_page.type */ > > + SGX_PAGE_TYPE_FREE = 100, > > + SGX_PAGE_TYPE_KVM = 101, > > }; > > > > #define SGX_NR_PAGE_TYPES 5 > > diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c > > index 3be203297988..abf6e1a704c0 100644 > > --- a/arch/x86/kernel/cpu/sgx/encl.c > > +++ b/arch/x86/kernel/cpu/sgx/encl.c > > @@ -72,7 +72,7 @@ static struct sgx_epc_page *sgx_encl_eldu(struct sgx_encl_page *encl_page, > > struct sgx_epc_page *epc_page; > > int ret; > > > > - epc_page = sgx_alloc_epc_page(encl_page, false); > > + epc_page = sgx_alloc_epc_page(encl_page, SGX_PAGE_TYPE_REG, false); > > if (IS_ERR(epc_page)) > > return epc_page; > > > > @@ -679,7 +679,7 @@ struct sgx_epc_page *sgx_alloc_va_page(void) > > struct sgx_epc_page *epc_page; > > int ret; > > > > - epc_page = sgx_alloc_epc_page(NULL, true); > > + epc_page = sgx_alloc_epc_page(NULL, SGX_PAGE_TYPE_VA, true); > > if (IS_ERR(epc_page)) > > return ERR_CAST(epc_page); > > > > diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c > > index 83df20e3e633..a74ae00194cc 100644 > > --- a/arch/x86/kernel/cpu/sgx/ioctl.c > > +++ b/arch/x86/kernel/cpu/sgx/ioctl.c > > @@ -83,7 +83,7 @@ static int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs) > > > > encl->backing = backing; > > > > - secs_epc = sgx_alloc_epc_page(&encl->secs, true); > > + secs_epc = sgx_alloc_epc_page(&encl->secs, SGX_PAGE_TYPE_SECS, true); > > if (IS_ERR(secs_epc)) { > > ret = PTR_ERR(secs_epc); > > goto err_out_backing; > > @@ -300,7 +300,7 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long src, > > if (IS_ERR(encl_page)) > > return PTR_ERR(encl_page); > > > > - epc_page = sgx_alloc_epc_page(encl_page, true); > > + epc_page = sgx_alloc_epc_page(encl_page, SGX_PAGE_TYPE_REG, true); > > if (IS_ERR(epc_page)) { > > kfree(encl_page); > > return PTR_ERR(epc_page); > > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c > > index 63d3de02bbcc..643df87b3e01 100644 > > --- a/arch/x86/kernel/cpu/sgx/main.c > > +++ b/arch/x86/kernel/cpu/sgx/main.c > > @@ -401,7 +401,12 @@ static void sgx_reclaim_pages(void) > > section = &sgx_epc_sections[epc_page->section]; > > node = section->node; > > > > + /* drop poison pages instead of adding to free list */ > > + if (epc_page->flags & SGX_EPC_PAGE_POISON) > > + continue; > > + > > spin_lock(&node->lock); > > + epc_page->flags = SGX_EPC_PAGE_FREE; > > list_add_tail(&epc_page->list, &node->free_page_list); > > sgx_nr_free_pages++; > > spin_unlock(&node->lock); > > @@ -560,6 +565,7 @@ int sgx_unmark_page_reclaimable(struct sgx_epc_page *page) > > /** > > * sgx_alloc_epc_page() - Allocate an EPC page > > * @owner: the owner of the EPC page > > + * @type: type of page being allocated > > * @reclaim: reclaim pages if necessary > > * > > * Iterate through EPC sections and borrow a free EPC page to the caller. When a > > @@ -574,7 +580,7 @@ int sgx_unmark_page_reclaimable(struct sgx_epc_page *page) > > * an EPC page, > > * -errno on error > > */ > > -struct sgx_epc_page *sgx_alloc_epc_page(void *owner, bool reclaim) > > +struct sgx_epc_page *sgx_alloc_epc_page(void *owner, enum sgx_page_type type, bool reclaim) > > { > > struct sgx_epc_page *page; > > > > @@ -582,6 +588,8 @@ struct sgx_epc_page *sgx_alloc_epc_page(void *owner, bool reclaim) > > page = __sgx_alloc_epc_page(); > > if (!IS_ERR(page)) { > > page->owner = owner; > > + page->type = type; > > + page->flags = 0; > > break; > > } > > > > @@ -616,14 +624,22 @@ struct sgx_epc_page *sgx_alloc_epc_page(void *owner, bool reclaim) > > * responsibility to make sure that the page is in uninitialized state. In other > > * words, do EREMOVE, EWB or whatever operation is necessary before calling > > * this function. > > + * > > + * Note that if the page has been tagged as poisoned, it is simply > > + * dropped on the floor instead of added to the free list to make > > + * sure we do not re-use it. > > */ > > void sgx_free_epc_page(struct sgx_epc_page *page) > > { > > struct sgx_epc_section *section = &sgx_epc_sections[page->section]; > > struct sgx_numa_node *node = section->node; > > > > + if (page->flags & SGX_EPC_PAGE_POISON) > > + return; > > I tend to think that it would be nice to collect them somewhere instead > purposely leaking. E.g. this gives possibility to examine list with > debugging tools. I'm not also sure why free and dirty pages need to be tagged. Why a poison flag is enough? This could be better explained in the commit message.
Hi Tony, On 7/8/2021 11:14 AM, Tony Luck wrote: > > Add a type field to struct epc_page for how an in-use page has been > allocated. Re-use "enum sgx_page_type" for this type, with a couple > of additions for s/w types. Tracking the enclave page type is a useful addition that will also help the SGX2 support where some instructions (ENCLS[EMODPR]) are only allowed on pages with particular type. Could this tracking be done at the enclave page (struct sgx_encl_page) instead? The enclave page's EPC page information is not available when the page is in swap and it would be useful to know the page type without loading the page from swap. The information would continue to be accessible from struct epc_page via the owner pointer that may make some of the changes easier since it would not be needed to pass the page type around so much and thus possibly address the SECS page issue that Sean pointed out in https://lore.kernel.org/lkml/YO3FuBupQTKYaKBf@google.com/ > diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h > index 4628acec0009..e43d3c27eb96 100644 > --- a/arch/x86/kernel/cpu/sgx/sgx.h > +++ b/arch/x86/kernel/cpu/sgx/sgx.h > @@ -26,9 +26,19 @@ > /* Pages, which are being tracked by the page reclaimer. */ > #define SGX_EPC_PAGE_RECLAIMER_TRACKED BIT(0) > > +/* Pages, on the "sgx_dirty_page_list" */ > +#define SGX_EPC_PAGE_DIRTY BIT(1) > + > +/* Pages, on one of the node free lists */ > +#define SGX_EPC_PAGE_FREE BIT(2) > + > +/* Pages, with h/w poison errors */ > +#define SGX_EPC_PAGE_POISON BIT(3) > + > struct sgx_epc_page { > unsigned int section; > - unsigned int flags; > + u16 flags; > + u16 type; Could this be "enum sgx_page_type type" ? Reinette
> Could this tracking be done at the enclave page (struct sgx_encl_page) > instead? In principle yes. Though Sean has some issues with me tracking types at all. > The enclave page's EPC page information is not available when > the page is in swap and it would be useful to know the page type without > loading the page from swap. The information would continue to be > accessible from struct epc_page via the owner pointer that may make some > of the changes easier since it would not be needed to pass the page type > around so much and thus possibly address the SECS page issue that Sean > pointed out in > https://lore.kernel.org/lkml/YO3FuBupQTKYaKBf@google.com/ I think I noticed that the "owner" pointer in sgx_encl_page doesn't point back to the epc_page for all types of SGX pages. So some additional changes would be needed. I'm not at all sure why this is different (or what use the non-REG pages use "owner" for. >> struct sgx_epc_page { >> unsigned int section; >> - unsigned int flags; >> + u16 flags; >> + u16 type; > > Could this be "enum sgx_page_type type" ? Maybe. I thought I needed extra types (like FREE and DIRTY). But Sean pointed out how to avoid some of them. -Tony
Hi Tony, On 7/14/2021 1:59 PM, Luck, Tony wrote: >> Could this tracking be done at the enclave page (struct sgx_encl_page) >> instead? > > In principle yes. Though Sean has some issues with me tracking types > at all. For the SGX2 work knowing the page types are useful. Some instructions only work on certain page types and knowing beforehand whether an instruction could work helps to avoid dealing with the errors when it does not work. >> The enclave page's EPC page information is not available when >> the page is in swap and it would be useful to know the page type without >> loading the page from swap. The information would continue to be >> accessible from struct epc_page via the owner pointer that may make some >> of the changes easier since it would not be needed to pass the page type >> around so much and thus possibly address the SECS page issue that Sean >> pointed out in >> https://lore.kernel.org/lkml/YO3FuBupQTKYaKBf@google.com/ > > I think I noticed that the "owner" pointer in sgx_encl_page doesn't point > back to the epc_page for all types of SGX pages. So some additional > changes would be needed. I'm not at all sure why this is different (or > what use the non-REG pages use "owner" for. This may be VA pages? struct sgx_va_page also contains a pointer to an EPC page. I did not consider that for this case. Perhaps these could be identified uniquely. Reinette
On Wed, Jul 14, 2021, Reinette Chatre wrote: > Hi Tony, > > On 7/14/2021 1:59 PM, Luck, Tony wrote: > > > Could this tracking be done at the enclave page (struct sgx_encl_page) > > > instead? > > > > In principle yes. Though Sean has some issues with me tracking types > > at all. I've no objection to tracking the type for SGX2, my argument in the context of #MC support is that there should be no need to track the type. Either the #MC is recoverable or it isn't, and the enclave is toast regardless of what type of page hit the #MC. There might be a need to identify track vEPC pages, e.g. to avoid the retpoline associated with a virtual function table, but IMO that would be better done as a new flag instead of overloading the page type. E.g. a page can be both a vEPC page and an SECS/REG/VA page depending on its use in the guest. > For the SGX2 work knowing the page types are useful. Some instructions only > work on certain page types and knowing beforehand whether an instruction > could work helps to avoid dealing with the errors when it does not work. Yes, but the SGX2 use case is specific to "native" enclaves, i.e. it can and should be limited to sgx_encl_page, as opposed to being shoved into sgx_epc_page. > > > The enclave page's EPC page information is not available when > > > the page is in swap and it would be useful to know the page type without > > > loading the page from swap. The information would continue to be > > > accessible from struct epc_page via the owner pointer that may make some > > > of the changes easier since it would not be needed to pass the page type > > > around so much and thus possibly address the SECS page issue that Sean > > > pointed out in > > > https://lore.kernel.org/lkml/YO3FuBupQTKYaKBf@google.com/ > > > > I think I noticed that the "owner" pointer in sgx_encl_page doesn't point > > back to the epc_page for all types of SGX pages. So some additional > > changes would be needed. I'm not at all sure why this is different (or > > what use the non-REG pages use "owner" for. > > This may be VA pages? struct sgx_va_page also contains a pointer to an EPC > page. I did not consider that for this case. Perhaps these could be > identified uniquely. The "owner" is currently only used for reclaim. IIRC, the proposed EPC cgroup also used "owner" to enable forced "reclaim", i.e. reclaiming EPC by nuking the owning entity, e.g. tearing down a virtual EPC section. And I believe the cgroup also used the aforementioned vEPC flag to invoke the correct EPC OOM reaper.
> I've no objection to tracking the type for SGX2, my argument in the context of > #MC support is that there should be no need to track the type. Either the #MC > is recoverable or it isn't, and the enclave is toast regardless of what type of > page hit the #MC. I'll separate the "phase" from the "type". Here phase is used for the life-cycle of EPC pages: DIRTY -> FREE -> IN-USE -> DIRTY Errors can be reported by memory controller page scrubbers for pages that are not "IN-USE" ... and the recovery action is just to make sure that they are never allocated. When a page is IN-USE ... it has a "type". I currently only have a way to inject errors into SGX_PAGE_TYPE_REG pages. That means initial recovery code is going to focus on those since that is all I can test. But I'll try not to special case them as far as possible. -Tony
On Wed, Jul 14, 2021, Luck, Tony wrote: > > I've no objection to tracking the type for SGX2, my argument in the context of > > #MC support is that there should be no need to track the type. Either the #MC > > is recoverable or it isn't, and the enclave is toast regardless of what type of > > page hit the #MC. > > I'll separate the "phase" from the "type". > > Here phase is used for the life-cycle of EPC pages: > > DIRTY -> FREE -> IN-USE -> DIRTY Not that it affects anything, but that's not quite true. In hardware, pages are either FREE or IN-USE, there is no concept of DIRTY. DIRTY is the kernel's arbitrary description of a page that has not been sanitized and so is considered to be in an unknown state, i.e. the kernel doesn't know if it's FREE or IN-USE. Once a page is sanitized (during boot), its state is known and the page is never put back on the so called dirty list, i.e. the software flow is: DIRTY -> FREE -> IN-USE -> FREE > Errors can be reported by memory controller page scrubbers for pages that are > not "IN-USE" ... and the recovery action is just to make sure that they are > never allocated. > > When a page is IN-USE ... it has a "type". I currently only have a way to > inject errors into SGX_PAGE_TYPE_REG pages. That means initial recovery code > is going to focus on those since that is all I can test. But I'll try not to > special case them as far as possible. Inability to test expected behavior doesn't mean we shouldn't implement towards the expected behavior, i.e. someone somewhere must know how SECS and VA pages behave in response to a memory error.
diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h index 9c31e0ebc55b..9619a6d77a83 100644 --- a/arch/x86/include/asm/sgx.h +++ b/arch/x86/include/asm/sgx.h @@ -216,6 +216,8 @@ struct sgx_pageinfo { * %SGX_PAGE_TYPE_REG: a regular page * %SGX_PAGE_TYPE_VA: a VA page * %SGX_PAGE_TYPE_TRIM: a page in trimmed state + * + * Also used to track current use of &struct sgx_epc_page */ enum sgx_page_type { SGX_PAGE_TYPE_SECS, @@ -223,6 +225,10 @@ enum sgx_page_type { SGX_PAGE_TYPE_REG, SGX_PAGE_TYPE_VA, SGX_PAGE_TYPE_TRIM, + + /* sgx_epc_page.type */ + SGX_PAGE_TYPE_FREE = 100, + SGX_PAGE_TYPE_KVM = 101, }; #define SGX_NR_PAGE_TYPES 5 diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c index 3be203297988..abf6e1a704c0 100644 --- a/arch/x86/kernel/cpu/sgx/encl.c +++ b/arch/x86/kernel/cpu/sgx/encl.c @@ -72,7 +72,7 @@ static struct sgx_epc_page *sgx_encl_eldu(struct sgx_encl_page *encl_page, struct sgx_epc_page *epc_page; int ret; - epc_page = sgx_alloc_epc_page(encl_page, false); + epc_page = sgx_alloc_epc_page(encl_page, SGX_PAGE_TYPE_REG, false); if (IS_ERR(epc_page)) return epc_page; @@ -679,7 +679,7 @@ struct sgx_epc_page *sgx_alloc_va_page(void) struct sgx_epc_page *epc_page; int ret; - epc_page = sgx_alloc_epc_page(NULL, true); + epc_page = sgx_alloc_epc_page(NULL, SGX_PAGE_TYPE_VA, true); if (IS_ERR(epc_page)) return ERR_CAST(epc_page); diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c index 83df20e3e633..a74ae00194cc 100644 --- a/arch/x86/kernel/cpu/sgx/ioctl.c +++ b/arch/x86/kernel/cpu/sgx/ioctl.c @@ -83,7 +83,7 @@ static int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs) encl->backing = backing; - secs_epc = sgx_alloc_epc_page(&encl->secs, true); + secs_epc = sgx_alloc_epc_page(&encl->secs, SGX_PAGE_TYPE_SECS, true); if (IS_ERR(secs_epc)) { ret = PTR_ERR(secs_epc); goto err_out_backing; @@ -300,7 +300,7 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long src, if (IS_ERR(encl_page)) return PTR_ERR(encl_page); - epc_page = sgx_alloc_epc_page(encl_page, true); + epc_page = sgx_alloc_epc_page(encl_page, SGX_PAGE_TYPE_REG, true); if (IS_ERR(epc_page)) { kfree(encl_page); return PTR_ERR(epc_page); diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c index 63d3de02bbcc..643df87b3e01 100644 --- a/arch/x86/kernel/cpu/sgx/main.c +++ b/arch/x86/kernel/cpu/sgx/main.c @@ -401,7 +401,12 @@ static void sgx_reclaim_pages(void) section = &sgx_epc_sections[epc_page->section]; node = section->node; + /* drop poison pages instead of adding to free list */ + if (epc_page->flags & SGX_EPC_PAGE_POISON) + continue; + spin_lock(&node->lock); + epc_page->flags = SGX_EPC_PAGE_FREE; list_add_tail(&epc_page->list, &node->free_page_list); sgx_nr_free_pages++; spin_unlock(&node->lock); @@ -560,6 +565,7 @@ int sgx_unmark_page_reclaimable(struct sgx_epc_page *page) /** * sgx_alloc_epc_page() - Allocate an EPC page * @owner: the owner of the EPC page + * @type: type of page being allocated * @reclaim: reclaim pages if necessary * * Iterate through EPC sections and borrow a free EPC page to the caller. When a @@ -574,7 +580,7 @@ int sgx_unmark_page_reclaimable(struct sgx_epc_page *page) * an EPC page, * -errno on error */ -struct sgx_epc_page *sgx_alloc_epc_page(void *owner, bool reclaim) +struct sgx_epc_page *sgx_alloc_epc_page(void *owner, enum sgx_page_type type, bool reclaim) { struct sgx_epc_page *page; @@ -582,6 +588,8 @@ struct sgx_epc_page *sgx_alloc_epc_page(void *owner, bool reclaim) page = __sgx_alloc_epc_page(); if (!IS_ERR(page)) { page->owner = owner; + page->type = type; + page->flags = 0; break; } @@ -616,14 +624,22 @@ struct sgx_epc_page *sgx_alloc_epc_page(void *owner, bool reclaim) * responsibility to make sure that the page is in uninitialized state. In other * words, do EREMOVE, EWB or whatever operation is necessary before calling * this function. + * + * Note that if the page has been tagged as poisoned, it is simply + * dropped on the floor instead of added to the free list to make + * sure we do not re-use it. */ void sgx_free_epc_page(struct sgx_epc_page *page) { struct sgx_epc_section *section = &sgx_epc_sections[page->section]; struct sgx_numa_node *node = section->node; + if (page->flags & SGX_EPC_PAGE_POISON) + return; + spin_lock(&node->lock); + page->flags = SGX_EPC_PAGE_FREE; list_add_tail(&page->list, &node->free_page_list); sgx_nr_free_pages++; @@ -651,7 +667,8 @@ static bool __init sgx_setup_epc_section(u64 phys_addr, u64 size, for (i = 0; i < nr_pages; i++) { section->pages[i].section = index; - section->pages[i].flags = 0; + section->pages[i].flags = SGX_EPC_PAGE_DIRTY; + section->pages[i].type = SGX_PAGE_TYPE_FREE; section->pages[i].owner = NULL; list_add_tail(§ion->pages[i].list, &sgx_dirty_page_list); } diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h index 4628acec0009..e43d3c27eb96 100644 --- a/arch/x86/kernel/cpu/sgx/sgx.h +++ b/arch/x86/kernel/cpu/sgx/sgx.h @@ -26,9 +26,19 @@ /* Pages, which are being tracked by the page reclaimer. */ #define SGX_EPC_PAGE_RECLAIMER_TRACKED BIT(0) +/* Pages, on the "sgx_dirty_page_list" */ +#define SGX_EPC_PAGE_DIRTY BIT(1) + +/* Pages, on one of the node free lists */ +#define SGX_EPC_PAGE_FREE BIT(2) + +/* Pages, with h/w poison errors */ +#define SGX_EPC_PAGE_POISON BIT(3) + struct sgx_epc_page { unsigned int section; - unsigned int flags; + u16 flags; + u16 type; struct sgx_encl_page *owner; struct list_head list; }; @@ -82,7 +92,7 @@ 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); +struct sgx_epc_page *sgx_alloc_epc_page(void *owner, enum sgx_page_type type, bool reclaim); #ifdef CONFIG_X86_SGX_KVM int __init sgx_vepc_init(void); diff --git a/arch/x86/kernel/cpu/sgx/virt.c b/arch/x86/kernel/cpu/sgx/virt.c index 64511c4a5200..044dd92ebd63 100644 --- a/arch/x86/kernel/cpu/sgx/virt.c +++ b/arch/x86/kernel/cpu/sgx/virt.c @@ -46,7 +46,7 @@ static int __sgx_vepc_fault(struct sgx_vepc *vepc, if (epc_page) return 0; - epc_page = sgx_alloc_epc_page(vepc, false); + epc_page = sgx_alloc_epc_page(vepc, SGX_PAGE_TYPE_KVM, false); if (IS_ERR(epc_page)) return PTR_ERR(epc_page);
Memory errors can be reported either synchronously as memory is accessed, or asynchronously by speculative access or by a memory controller page scrubber. The life cycle of an EPC page takes it through: dirty -> free -> in-use -> free. Memory errors are reported using physical addresses. It is a simple matter to find which sgx_epc_page structure maps a given address. But then recovery code needs to be able to determine the current use of the page to take the appropriate recovery action. Within the "in-use" phase different actions are needed based on how the page is used in the enclave. Add new flags bits to describe the phase (with an extra bit for the new phase of "poisoned"). Drop pages marked as poisoned instead of adding them to a free list to make sure they are not re-used. Add a type field to struct epc_page for how an in-use page has been allocated. Re-use "enum sgx_page_type" for this type, with a couple of additions for s/w types. Signed-off-by: Tony Luck <tony.luck@intel.com> --- arch/x86/include/asm/sgx.h | 6 ++++++ arch/x86/kernel/cpu/sgx/encl.c | 4 ++-- arch/x86/kernel/cpu/sgx/ioctl.c | 4 ++-- arch/x86/kernel/cpu/sgx/main.c | 21 +++++++++++++++++++-- arch/x86/kernel/cpu/sgx/sgx.h | 14 ++++++++++++-- arch/x86/kernel/cpu/sgx/virt.c | 2 +- 6 files changed, 42 insertions(+), 9 deletions(-)