| Message ID | 20240705074524.443713-2-dmitrii.kuvaiskii@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/sgx: Fix two data races in EAUG/EREMOVE flows | expand |
On Fri, 05 Jul 2024 02:45:22 -0500, Dmitrii Kuvaiskii <dmitrii.kuvaiskii@intel.com> wrote: > SGX_ENCL_PAGE_BEING_RECLAIMED flag is set when the enclave page is being > reclaimed (moved to the backing store). This flag however has two > logical meanings: > > 1. Don't attempt to load the enclave page (the page is busy). > 2. Don't attempt to remove the PCMD page corresponding to this enclave > page (the PCMD page is busy). > > To reflect these two meanings, split SGX_ENCL_PAGE_BEING_RECLAIMED into > two flags: SGX_ENCL_PAGE_BUSY and SGX_ENCL_PAGE_PCMD_BUSY. Currently, > both flags are set only when the enclave page is being reclaimed. A > future commit will introduce a new case when the enclave page is being > removed; this new case will set only the SGX_ENCL_PAGE_BUSY flag. > LGTM. Reviewed-by: Haitao Huang <haitao.huang@linux.intel.com> Thanks Haitao
On Fri Jul 5, 2024 at 10:45 AM EEST, Dmitrii Kuvaiskii wrote: > SGX_ENCL_PAGE_BEING_RECLAIMED flag is set when the enclave page is being > reclaimed (moved to the backing store). This flag however has two > logical meanings: ~~~~~~~~ side-effects Missing the actor. "The page reclaimer thread sets SGX_ENC_PAGE_BEING_RECLAIMED flag..." It is not just randomly "set". > > 1. Don't attempt to load the enclave page (the page is busy). Please point out where in the source code. > 2. Don't attempt to remove the PCMD page corresponding to this enclave > page (the PCMD page is busy). Please point out where in the source code. These would be great reference when looking back. > > To reflect these two meanings, split SGX_ENCL_PAGE_BEING_RECLAIMED into I don't care about meanings. Only who does and what. BR, Jarkko
On Fri Jul 5, 2024 at 10:45 AM EEST, Dmitrii Kuvaiskii wrote: > +/* > + * 'desc' bit indicating that PCMD page associated with the enclave page is > + * busy (e.g. because the enclave page is being reclaimed). > + */ > +#define SGX_ENCL_PAGE_PCMD_BUSY BIT(3) What are other situations when this flag is set than being reclaimed? The comment says that it is only one use for this flag. BR, Jarkko
On 5/07/2024 7:45 pm, Dmitrii Kuvaiskii wrote: > SGX_ENCL_PAGE_BEING_RECLAIMED flag is set when the enclave page is being > reclaimed (moved to the backing store). This flag however has two > logical meanings: > > 1. Don't attempt to load the enclave page (the page is busy). > 2. Don't attempt to remove the PCMD page corresponding to this enclave > page (the PCMD page is busy). Nit: I think the SGX_ENCL_PAGE_PCMD_BUSY isn't that accurate, because obviously the actual backing page is busy (i.e., cannot be truncated) too. So the current SGX_ENCL_PAGE_BEING_RECLAIMED is also fine to me. But truncating the actual backing page can also be determined by SGX_ENCL_PAGE_PCMD_BUSY I suppose (if needed in the future), so this looks fine to me too. > > To reflect these two meanings, split SGX_ENCL_PAGE_BEING_RECLAIMED into > two flags: SGX_ENCL_PAGE_BUSY and SGX_ENCL_PAGE_PCMD_BUSY. Currently, > both flags are set only when the enclave page is being reclaimed. A > future commit will introduce a new case when the enclave page is being > removed; this new case will set only the SGX_ENCL_PAGE_BUSY flag. As replied to the last patch, IIUC EREMOVE ioctl doesn't seem to be the only case where the BUSY needs to be marked, so let's just say something general but not mention the removal case specifically. Anyway, feel free to add: Acked-by: Kai Huang <kai.huang@intel.com>
On Wed, Jul 17, 2024 at 01:36:08PM +0300, Jarkko Sakkinen wrote: > On Fri Jul 5, 2024 at 10:45 AM EEST, Dmitrii Kuvaiskii wrote: > > SGX_ENCL_PAGE_BEING_RECLAIMED flag is set when the enclave page is being > > reclaimed (moved to the backing store). This flag however has two > > logical meanings: > ~~~~~~~~ > side-effects Could you clarify the required action here? Do you expect me to replace "This flag however has two logical meanings" with "This flag however has two logical side-effects"? The suggested word doesn't seem to apply nicely to this case. In my text, I have the following two sentences: "Don't attempt to load the enclave page" and "Don't attempt to remove the PCMD page ...". I don't think it's proper English to say that "Don't attempt ..." is a side effect. Or do you want me to also modify the two sentences in the list? By the way, this text is a rephrasing of Dave Hansen's comment: https://lore.kernel.org/all/1d405428-3847-4862-b146-dd57711c881e@intel.com/ > > To reflect these two meanings, split SGX_ENCL_PAGE_BEING_RECLAIMED into > > I don't care about meanings. Only who does and what. Could you clarify the required action here? What would be a better rephrasing? Aren't we supposed to clarify the rationale behind the code changes in the commit message? -- Dmitrii Kuvaiskii
On Wed, Jul 17, 2024 at 01:37:39PM +0300, Jarkko Sakkinen wrote: > On Fri Jul 5, 2024 at 10:45 AM EEST, Dmitrii Kuvaiskii wrote: > > +/* > > + * 'desc' bit indicating that PCMD page associated with the enclave page is > > + * busy (e.g. because the enclave page is being reclaimed). > > + */ > > +#define SGX_ENCL_PAGE_PCMD_BUSY BIT(3) > > What are other situations when this flag is set than being > reclaimed? The comment says that it is only one use for this > flag. Yes, your understanding is correct, currently there is only one situation. Do you want me to modify the comment somehow? -- Dmitrii Kuvaiskii
On Mon Aug 12, 2024 at 11:12 AM EEST, Dmitrii Kuvaiskii wrote: > On Wed, Jul 17, 2024 at 01:36:08PM +0300, Jarkko Sakkinen wrote: > > On Fri Jul 5, 2024 at 10:45 AM EEST, Dmitrii Kuvaiskii wrote: > > > SGX_ENCL_PAGE_BEING_RECLAIMED flag is set when the enclave page is being > > > reclaimed (moved to the backing store). This flag however has two > > > logical meanings: > > ~~~~~~~~ > > side-effects > > Could you clarify the required action here? Do you expect me to replace > "This flag however has two logical meanings" with "This flag however has > two logical side-effects"? The suggested word doesn't seem to apply nicely > to this case. In my text, I have the following two sentences: "Don't > attempt to load the enclave page" and "Don't attempt to remove the PCMD > page ...". I don't think it's proper English to say that "Don't attempt > ..." is a side effect. Or do you want me to also modify the two sentences > in the list? I agree with you here, you can ignore this comment. BR, Jarkko
On Mon Aug 12, 2024 at 11:16 AM EEST, Dmitrii Kuvaiskii wrote: > On Wed, Jul 17, 2024 at 01:37:39PM +0300, Jarkko Sakkinen wrote: > > On Fri Jul 5, 2024 at 10:45 AM EEST, Dmitrii Kuvaiskii wrote: > > > +/* > > > + * 'desc' bit indicating that PCMD page associated with the enclave page is > > > + * busy (e.g. because the enclave page is being reclaimed). > > > + */ > > > +#define SGX_ENCL_PAGE_PCMD_BUSY BIT(3) > > > > What are other situations when this flag is set than being > > reclaimed? The comment says that it is only one use for this > > flag. > > Yes, your understanding is correct, currently there is only one situation. > > Do you want me to modify the comment somehow? Yes, just s/e.g.// > > -- > Dmitrii Kuvaiskii BR, Jarkko
diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c index 279148e72459..c0a3c00284c8 100644 --- a/arch/x86/kernel/cpu/sgx/encl.c +++ b/arch/x86/kernel/cpu/sgx/encl.c @@ -46,10 +46,10 @@ static int sgx_encl_lookup_backing(struct sgx_encl *encl, unsigned long page_ind * a check if an enclave page sharing the PCMD page is in the process of being * reclaimed. * - * The reclaimer sets the SGX_ENCL_PAGE_BEING_RECLAIMED flag when it - * intends to reclaim that enclave page - it means that the PCMD page - * associated with that enclave page is about to get some data and thus - * even if the PCMD page is empty, it should not be truncated. + * The reclaimer sets the SGX_ENCL_PAGE_PCMD_BUSY flag when it intends to + * reclaim that enclave page - it means that the PCMD page associated with that + * enclave page is about to get some data and thus even if the PCMD page is + * empty, it should not be truncated. * * Context: Enclave mutex (&sgx_encl->lock) must be held. * Return: 1 if the reclaimer is about to write to the PCMD page @@ -77,8 +77,7 @@ static int reclaimer_writing_to_pcmd(struct sgx_encl *encl, * Stop when reaching the SECS page - it does not * have a page_array entry and its reclaim is * started and completed with enclave mutex held so - * it does not use the SGX_ENCL_PAGE_BEING_RECLAIMED - * flag. + * it does not use the SGX_ENCL_PAGE_PCMD_BUSY flag. */ if (addr == encl->base + encl->size) break; @@ -91,8 +90,7 @@ static int reclaimer_writing_to_pcmd(struct sgx_encl *encl, * VA page slot ID uses same bit as the flag so it is important * to ensure that the page is not already in backing store. */ - if (entry->epc_page && - (entry->desc & SGX_ENCL_PAGE_BEING_RECLAIMED)) { + if (entry->epc_page && (entry->desc & SGX_ENCL_PAGE_PCMD_BUSY)) { reclaimed = 1; break; } @@ -257,7 +255,7 @@ static struct sgx_encl_page *__sgx_encl_load_page(struct sgx_encl *encl, /* Entry successfully located. */ if (entry->epc_page) { - if (entry->desc & SGX_ENCL_PAGE_BEING_RECLAIMED) + if (entry->desc & SGX_ENCL_PAGE_BUSY) return ERR_PTR(-EBUSY); return entry; diff --git a/arch/x86/kernel/cpu/sgx/encl.h b/arch/x86/kernel/cpu/sgx/encl.h index f94ff14c9486..11b09899cd92 100644 --- a/arch/x86/kernel/cpu/sgx/encl.h +++ b/arch/x86/kernel/cpu/sgx/encl.h @@ -22,8 +22,14 @@ /* 'desc' bits holding the offset in the VA (version array) page. */ #define SGX_ENCL_PAGE_VA_OFFSET_MASK GENMASK_ULL(11, 3) -/* 'desc' bit marking that the page is being reclaimed. */ -#define SGX_ENCL_PAGE_BEING_RECLAIMED BIT(3) +/* 'desc' bit indicating that the page is busy (e.g. being reclaimed). */ +#define SGX_ENCL_PAGE_BUSY BIT(2) + +/* + * 'desc' bit indicating that PCMD page associated with the enclave page is + * busy (e.g. because the enclave page is being reclaimed). + */ +#define SGX_ENCL_PAGE_PCMD_BUSY BIT(3) struct sgx_encl_page { unsigned long desc; diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c index 166692f2d501..e94b09c43673 100644 --- a/arch/x86/kernel/cpu/sgx/main.c +++ b/arch/x86/kernel/cpu/sgx/main.c @@ -204,7 +204,7 @@ static void sgx_encl_ewb(struct sgx_epc_page *epc_page, void *va_slot; int ret; - encl_page->desc &= ~SGX_ENCL_PAGE_BEING_RECLAIMED; + encl_page->desc &= ~(SGX_ENCL_PAGE_BUSY | SGX_ENCL_PAGE_PCMD_BUSY); va_page = list_first_entry(&encl->va_pages, struct sgx_va_page, list); @@ -340,7 +340,7 @@ static void sgx_reclaim_pages(void) goto skip; } - encl_page->desc |= SGX_ENCL_PAGE_BEING_RECLAIMED; + encl_page->desc |= SGX_ENCL_PAGE_BUSY | SGX_ENCL_PAGE_PCMD_BUSY; mutex_unlock(&encl_page->encl->lock); continue;
SGX_ENCL_PAGE_BEING_RECLAIMED flag is set when the enclave page is being reclaimed (moved to the backing store). This flag however has two logical meanings: 1. Don't attempt to load the enclave page (the page is busy). 2. Don't attempt to remove the PCMD page corresponding to this enclave page (the PCMD page is busy). To reflect these two meanings, split SGX_ENCL_PAGE_BEING_RECLAIMED into two flags: SGX_ENCL_PAGE_BUSY and SGX_ENCL_PAGE_PCMD_BUSY. Currently, both flags are set only when the enclave page is being reclaimed. A future commit will introduce a new case when the enclave page is being removed; this new case will set only the SGX_ENCL_PAGE_BUSY flag. Cc: stable@vger.kernel.org Signed-off-by: Dmitrii Kuvaiskii <dmitrii.kuvaiskii@intel.com> --- arch/x86/kernel/cpu/sgx/encl.c | 16 +++++++--------- arch/x86/kernel/cpu/sgx/encl.h | 10 ++++++++-- arch/x86/kernel/cpu/sgx/main.c | 4 ++-- 3 files changed, 17 insertions(+), 13 deletions(-)