Re: [PATCH v9 3/7] x86/sgx: Initial poison handling for dirty and free pages

["Luck, Tony" <tony.luck@intel.com>]

On Fri, Oct 15, 2021 at 11:07:48PM +0000, Sean Christopherson wrote:
> On Mon, Oct 11, 2021, Tony Luck wrote:
> > A memory controller patrol scrubber can report poison in a page
> > that isn't currently being used.
> > 
> > Add "poison" field in the sgx_epc_page that can be set for an
> > sgx_epc_page. Check for it:
> > 1) When sanitizing dirty pages
> > 2) When freeing epc pages
> > 
> > Poison is a new field separated from flags to avoid having to make
> > all updates to flags atomic, or integrate poison state changes into
> > some other locking scheme to protect flags.
> 
> Explain why atomic would be needed.  I lived in this code for a few years and
> still had to look at the source to remember that the reclaimer can set flags
> without taking node->lock.

Will add explanation.

> 
> > In both cases place the poisoned page on a list of poisoned epc pages
> > to make sure it will not be reallocated.
> > 
> > Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
> > Tested-by: Reinette Chatre <reinette.chatre@intel.com>
> > Signed-off-by: Tony Luck <tony.luck@intel.com>
> > ---
> >  arch/x86/kernel/cpu/sgx/main.c | 14 +++++++++++++-
> >  arch/x86/kernel/cpu/sgx/sgx.h  |  3 ++-
> >  2 files changed, 15 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> > index 09fa42690ff2..653bace26100 100644
> > --- a/arch/x86/kernel/cpu/sgx/main.c
> > +++ b/arch/x86/kernel/cpu/sgx/main.c
> > @@ -43,6 +43,7 @@ static nodemask_t sgx_numa_mask;
> >  static struct sgx_numa_node *sgx_numa_nodes;
> >  
> >  static LIST_HEAD(sgx_dirty_page_list);
> > +static LIST_HEAD(sgx_poison_page_list);
> >  
> >  /*
> >   * Reset post-kexec EPC pages to the uninitialized state. The pages are removed
> > @@ -62,6 +63,12 @@ static void __sgx_sanitize_pages(struct list_head *dirty_page_list)
> >  
> >  		page = list_first_entry(dirty_page_list, struct sgx_epc_page, list);
> >  
> > +		if (page->poison) {
> 
> Does this need READ_ONCE (and WRITE_ONCE in the writer) to prevent reloading
> page->poison since the sanitizer doesn't hold node->lock, i.e. page->poison can
> be set any time?  Honest question, I'm terrible with memory ordering rules...
> 

I think it's safe. I set page->poison in arch_memory_failure() while
holding node->lock in kthread context.  So not "at any time".

This particular read is done without holding the lock ... and is thus
racy. But there are a zillion other races early in boot before the EPC
pages get sanitized and moved to the free list. E.g. if an error is
reported before they are added to the sgx_epc_address_space xarray,
then all this code will just ignore the error as "not in Linux
controlled memory".

-Tony