[v3,2/5] x86/sgx: Use sgx_free_epc_page() in sgx_reclaim_pages()
diff mbox series

Message ID 20210303150323.433207-3-jarkko@kernel.org
State New, archived
Headers show
Series
  • [v3,1/5] x86/sgx: Fix a resource leak in sgx_init()
Related show

Commit Message

Jarkko Sakkinen March 3, 2021, 3:03 p.m. UTC
Replace the ad-hoc code with a sgx_free_epc_page(), in order to make sure
that all the relevant checks and book keeping is done, while freeing a
borrowed EPC page.

Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
 arch/x86/kernel/cpu/sgx/main.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

Comments

Dave Hansen March 3, 2021, 4:59 p.m. UTC | #1
On 3/3/21 7:03 AM, Jarkko Sakkinen wrote:
> diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> index 52d070fb4c9a..ed99c60024dc 100644
> --- a/arch/x86/kernel/cpu/sgx/main.c
> +++ b/arch/x86/kernel/cpu/sgx/main.c
> @@ -305,7 +305,6 @@ static void sgx_reclaim_pages(void)
>  {
>  	struct sgx_epc_page *chunk[SGX_NR_TO_SCAN];
>  	struct sgx_backing backing[SGX_NR_TO_SCAN];
> -	struct sgx_epc_section *section;
>  	struct sgx_encl_page *encl_page;
>  	struct sgx_epc_page *epc_page;
>  	pgoff_t page_index;
> @@ -378,11 +377,7 @@ static void sgx_reclaim_pages(void)
>  		kref_put(&encl_page->encl->refcount, sgx_encl_release);
>  		epc_page->flags &= ~SGX_EPC_PAGE_RECLAIMER_TRACKED;
>  
> -		section = &sgx_epc_sections[epc_page->section];
> -		spin_lock(&section->lock);
> -		list_add_tail(&epc_page->list, &section->page_list);
> -		section->free_cnt++;
> -		spin_unlock(&section->lock);
> +		sgx_free_epc_page(epc_page);
>  	}
>  }

In current upstream (3fb6d0e00e), sgx_free_epc_page() calls __eremove().
 This code does not call __eremove().  That seems to be changing
behavior where none was intended.

Was this, perhaps, based on top of Kai's series that changes the
behavior of sgx_free_epc_page()?
Jarkko Sakkinen March 10, 2021, 3:11 p.m. UTC | #2
On Wed, Mar 03, 2021 at 08:59:17AM -0800, Dave Hansen wrote:
> On 3/3/21 7:03 AM, Jarkko Sakkinen wrote:
> > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> > index 52d070fb4c9a..ed99c60024dc 100644
> > --- a/arch/x86/kernel/cpu/sgx/main.c
> > +++ b/arch/x86/kernel/cpu/sgx/main.c
> > @@ -305,7 +305,6 @@ static void sgx_reclaim_pages(void)
> >  {
> >  	struct sgx_epc_page *chunk[SGX_NR_TO_SCAN];
> >  	struct sgx_backing backing[SGX_NR_TO_SCAN];
> > -	struct sgx_epc_section *section;
> >  	struct sgx_encl_page *encl_page;
> >  	struct sgx_epc_page *epc_page;
> >  	pgoff_t page_index;
> > @@ -378,11 +377,7 @@ static void sgx_reclaim_pages(void)
> >  		kref_put(&encl_page->encl->refcount, sgx_encl_release);
> >  		epc_page->flags &= ~SGX_EPC_PAGE_RECLAIMER_TRACKED;
> >  
> > -		section = &sgx_epc_sections[epc_page->section];
> > -		spin_lock(&section->lock);
> > -		list_add_tail(&epc_page->list, &section->page_list);
> > -		section->free_cnt++;
> > -		spin_unlock(&section->lock);
> > +		sgx_free_epc_page(epc_page);
> >  	}
> >  }
> 
> In current upstream (3fb6d0e00e), sgx_free_epc_page() calls __eremove().
>  This code does not call __eremove().  That seems to be changing
> behavior where none was intended.

EREMOVE does not matter here, as it doesn't in almost all most of the sites
where sgx_free_epc_page() is used in the driver. It does nothing to an
uninitialized pages.

The two patches that I posted originally for Kai's series took EREMOVE out
of sgx_free_epc_page() and put an explicit EREMOVE where it is actually
needed, but for reasons unknown to me, that change is gone.

Replacing the ad-hoc code with sgx_free_epc_page() is absolutely the right
action to take because it follows the pattern how sgx_free_epc_page() is
used in the driver.

For reference:

https://lore.kernel.org/linux-sgx/20210113233541.17669-1-jarkko@kernel.org/

> Was this, perhaps, based on top of Kai's series that changes the
> behavior of sgx_free_epc_page()?

I did not refer to that patch series.

/Jarkko
Dave Hansen March 10, 2021, 3:55 p.m. UTC | #3
On 3/10/21 7:11 AM, Jarkko Sakkinen wrote:
>>> -		section = &sgx_epc_sections[epc_page->section];
>>> -		spin_lock(&section->lock);
>>> -		list_add_tail(&epc_page->list, &section->page_list);
>>> -		section->free_cnt++;
>>> -		spin_unlock(&section->lock);
>>> +		sgx_free_epc_page(epc_page);
>>>  	}
>>>  }
>> In current upstream (3fb6d0e00e), sgx_free_epc_page() calls __eremove().
>>  This code does not call __eremove().  That seems to be changing
>> behavior where none was intended.
> EREMOVE does not matter here, as it doesn't in almost all most of the sites
> where sgx_free_epc_page() is used in the driver. It does nothing to an
> uninitialized pages.
> 
> The two patches that I posted originally for Kai's series took EREMOVE out
> of sgx_free_epc_page() and put an explicit EREMOVE where it is actually
> needed, but for reasons unknown to me, that change is gone.
> 
> Replacing the ad-hoc code with sgx_free_epc_page() is absolutely the right
> action to take because it follows the pattern how sgx_free_epc_page() is
> used in the driver.

That sounds generally fine.  But, this is a functional change.  Where
there are functional changes, I always hope to see some mention of the
change in the changelog.

Could you add some of this to the next changelog, please?
Kai Huang March 10, 2021, 8:36 p.m. UTC | #4
On Wed, 2021-03-10 at 17:11 +0200, Jarkko Sakkinen wrote:
> On Wed, Mar 03, 2021 at 08:59:17AM -0800, Dave Hansen wrote:
> > On 3/3/21 7:03 AM, Jarkko Sakkinen wrote:
> > > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> > > index 52d070fb4c9a..ed99c60024dc 100644
> > > --- a/arch/x86/kernel/cpu/sgx/main.c
> > > +++ b/arch/x86/kernel/cpu/sgx/main.c
> > > @@ -305,7 +305,6 @@ static void sgx_reclaim_pages(void)
> > >  {
> > >  	struct sgx_epc_page *chunk[SGX_NR_TO_SCAN];
> > >  	struct sgx_backing backing[SGX_NR_TO_SCAN];
> > > -	struct sgx_epc_section *section;
> > >  	struct sgx_encl_page *encl_page;
> > >  	struct sgx_epc_page *epc_page;
> > >  	pgoff_t page_index;
> > > @@ -378,11 +377,7 @@ static void sgx_reclaim_pages(void)
> > >  		kref_put(&encl_page->encl->refcount, sgx_encl_release);
> > >  		epc_page->flags &= ~SGX_EPC_PAGE_RECLAIMER_TRACKED;
> > >  
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > -		section = &sgx_epc_sections[epc_page->section];
> > > -		spin_lock(&section->lock);
> > > -		list_add_tail(&epc_page->list, &section->page_list);
> > > -		section->free_cnt++;
> > > -		spin_unlock(&section->lock);
> > > +		sgx_free_epc_page(epc_page);
> > >  	}
> > >  }
> > 
> > In current upstream (3fb6d0e00e), sgx_free_epc_page() calls __eremove().
> >  This code does not call __eremove().  That seems to be changing
> > behavior where none was intended.
> 
> EREMOVE does not matter here, as it doesn't in almost all most of the sites
> where sgx_free_epc_page() is used in the driver. It does nothing to an
> uninitialized pages.

Right. EREMOVE on uninitialized pages does nothing, so a more reasonable way is to
just NOT call EREMOVE (your original code), since it is absolutely unnecessary.

I don't see ANY reason we should call EREMOVE here. 

Actually w/o my patch to split EREMOVE out of sgx_free_epc_page(), it then makes
perfect sense to have new sgx_free_epc_page() here.

> 
> The two patches that I posted originally for Kai's series took EREMOVE out
> of sgx_free_epc_page() and put an explicit EREMOVE where it is actually
> needed, but for reasons unknown to me, that change is gone.
> 

It's not gone. It goes into a new sgx_encl_free_epc_page(), which is exactly the same
as current sgx_free_epc_page() which as EREMOVE, instead of putting EREMOVE into a
dedicated sgx_reset_epc_page(), as you did in your series:

https://lore.kernel.org/linux-sgx/20210113233541.17669-1-jarkko@kernel.org/

However, your change has side effort: it always put page back into free pool, even
EREMOVE fails. To make your change w/o having any functional change, it has to be:

	if(!sgx_reset_epc_page())
		sgx_free_epc_page();

And for this, Dave raised one concern we should add a WARN() to let user know EPC
page is leaked, and reboot is requied to get them back.

However with sgx_reset_epc_page(), there's no place to add such WARN(), and
implementing original sgx_free_epc_page() as sgx_encl_free_epc_page() looks very
reasonable to me:

https://www.spinics.net/lists/linux-sgx/msg04631.html


> Replacing the ad-hoc code with sgx_free_epc_page() is absolutely the right
> action to take because it follows the pattern how sgx_free_epc_page() is
> used in the driver.
> 
> For reference:
> 
> https://lore.kernel.org/linux-sgx/20210113233541.17669-1-jarkko@kernel.org/
> 
> > Was this, perhaps, based on top of Kai's series that changes the
> > behavior of sgx_free_epc_page()?
> 
> I did not refer to that patch series.
> 
> /Jarkko
Jarkko Sakkinen March 10, 2021, 9:56 p.m. UTC | #5
On Wed, Mar 10, 2021 at 07:55:35AM -0800, Dave Hansen wrote:
> On 3/10/21 7:11 AM, Jarkko Sakkinen wrote:
> >>> -		section = &sgx_epc_sections[epc_page->section];
> >>> -		spin_lock(&section->lock);
> >>> -		list_add_tail(&epc_page->list, &section->page_list);
> >>> -		section->free_cnt++;
> >>> -		spin_unlock(&section->lock);
> >>> +		sgx_free_epc_page(epc_page);
> >>>  	}
> >>>  }
> >> In current upstream (3fb6d0e00e), sgx_free_epc_page() calls __eremove().
> >>  This code does not call __eremove().  That seems to be changing
> >> behavior where none was intended.
> > EREMOVE does not matter here, as it doesn't in almost all most of the sites
> > where sgx_free_epc_page() is used in the driver. It does nothing to an
> > uninitialized pages.
> > 
> > The two patches that I posted originally for Kai's series took EREMOVE out
> > of sgx_free_epc_page() and put an explicit EREMOVE where it is actually
> > needed, but for reasons unknown to me, that change is gone.
> > 
> > Replacing the ad-hoc code with sgx_free_epc_page() is absolutely the right
> > action to take because it follows the pattern how sgx_free_epc_page() is
> > used in the driver.
> 
> That sounds generally fine.  But, this is a functional change.  Where
> there are functional changes, I always hope to see some mention of the
> change in the changelog.
> 
> Could you add some of this to the next changelog, please?

This appears for the first time in this patch set version, which means that
there is no patch changelog for this.

Maybe a better idea would be to explain the functional change in the commit
message (which of course implies also entry to the patch change log)?

/Jarkko
Jarkko Sakkinen March 10, 2021, 10:10 p.m. UTC | #6
On Thu, Mar 11, 2021 at 09:36:15AM +1300, Kai Huang wrote:
> On Wed, 2021-03-10 at 17:11 +0200, Jarkko Sakkinen wrote:
> > On Wed, Mar 03, 2021 at 08:59:17AM -0800, Dave Hansen wrote:
> > > On 3/3/21 7:03 AM, Jarkko Sakkinen wrote:
> > > > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> > > > index 52d070fb4c9a..ed99c60024dc 100644
> > > > --- a/arch/x86/kernel/cpu/sgx/main.c
> > > > +++ b/arch/x86/kernel/cpu/sgx/main.c
> > > > @@ -305,7 +305,6 @@ static void sgx_reclaim_pages(void)
> > > >  {
> > > >  	struct sgx_epc_page *chunk[SGX_NR_TO_SCAN];
> > > >  	struct sgx_backing backing[SGX_NR_TO_SCAN];
> > > > -	struct sgx_epc_section *section;
> > > >  	struct sgx_encl_page *encl_page;
> > > >  	struct sgx_epc_page *epc_page;
> > > >  	pgoff_t page_index;
> > > > @@ -378,11 +377,7 @@ static void sgx_reclaim_pages(void)
> > > >  		kref_put(&encl_page->encl->refcount, sgx_encl_release);
> > > >  		epc_page->flags &= ~SGX_EPC_PAGE_RECLAIMER_TRACKED;
> > > >  
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > -		section = &sgx_epc_sections[epc_page->section];
> > > > -		spin_lock(&section->lock);
> > > > -		list_add_tail(&epc_page->list, &section->page_list);
> > > > -		section->free_cnt++;
> > > > -		spin_unlock(&section->lock);
> > > > +		sgx_free_epc_page(epc_page);
> > > >  	}
> > > >  }
> > > 
> > > In current upstream (3fb6d0e00e), sgx_free_epc_page() calls __eremove().
> > >  This code does not call __eremove().  That seems to be changing
> > > behavior where none was intended.
> > 
> > EREMOVE does not matter here, as it doesn't in almost all most of the sites
> > where sgx_free_epc_page() is used in the driver. It does nothing to an
> > uninitialized pages.
> 
> Right. EREMOVE on uninitialized pages does nothing, so a more reasonable way is to
> just NOT call EREMOVE (your original code), since it is absolutely unnecessary.
> 
> I don't see ANY reason we should call EREMOVE here. 
> 
> Actually w/o my patch to split EREMOVE out of sgx_free_epc_page(), it then makes
> perfect sense to have new sgx_free_epc_page() here.
> 
> > 
> > The two patches that I posted originally for Kai's series took EREMOVE out
> > of sgx_free_epc_page() and put an explicit EREMOVE where it is actually
> > needed, but for reasons unknown to me, that change is gone.
> > 
> 
> It's not gone. It goes into a new sgx_encl_free_epc_page(), which is exactly the same
> as current sgx_free_epc_page() which as EREMOVE, instead of putting EREMOVE into a
> dedicated sgx_reset_epc_page(), as you did in your series:
> 
> https://lore.kernel.org/linux-sgx/20210113233541.17669-1-jarkko@kernel.org/
> 
> However, your change has side effort: it always put page back into free pool, even
> EREMOVE fails. To make your change w/o having any functional change, it has to be:
> 
> 	if(!sgx_reset_epc_page())
> 		sgx_free_epc_page();

OK, great, your patch set uses the wrapper only in the necessary call
sites. Sorry, I overlooked this part.

Anyway, it knowingly does that. I considered either as equally harmful
side-ffects when I implemented. Either can only trigger, when there is a
bug in the kernel code.

It *could* do what that snippet suggest but it's like "out of the frying pan,
into the fire" kind of change.

Since NUMA patch set anyway requires to have a global dirty list, I think
the better way to deal with this, would be to declare a new global in the
patch under discussion:

static struct list_head sgx_dirty_list;

And sgx_encl_free_epc_page() could simply put the pages in this list. In
some cases you could possibly even reset the system state using kexec for
debugging purposes, so it could potentially bring a tiny bit of value.

I can rebase then my NUMA patches on top of SGX specific KVM patches, once
Boris have applied them.

> And for this, Dave raised one concern we should add a WARN() to let user know EPC
> page is leaked, and reboot is requied to get them back.
> 
> However with sgx_reset_epc_page(), there's no place to add such WARN(), and
> implementing original sgx_free_epc_page() as sgx_encl_free_epc_page() looks very
> reasonable to me:
> 
> https://www.spinics.net/lists/linux-sgx/msg04631.html

/Jarkko
Jarkko Sakkinen March 10, 2021, 10:12 p.m. UTC | #7
On Thu, Mar 11, 2021 at 12:10:56AM +0200, Jarkko Sakkinen wrote:
> On Thu, Mar 11, 2021 at 09:36:15AM +1300, Kai Huang wrote:
> > On Wed, 2021-03-10 at 17:11 +0200, Jarkko Sakkinen wrote:
> > > On Wed, Mar 03, 2021 at 08:59:17AM -0800, Dave Hansen wrote:
> > > > On 3/3/21 7:03 AM, Jarkko Sakkinen wrote:
> > > > > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> > > > > index 52d070fb4c9a..ed99c60024dc 100644
> > > > > --- a/arch/x86/kernel/cpu/sgx/main.c
> > > > > +++ b/arch/x86/kernel/cpu/sgx/main.c
> > > > > @@ -305,7 +305,6 @@ static void sgx_reclaim_pages(void)
> > > > >  {
> > > > >  	struct sgx_epc_page *chunk[SGX_NR_TO_SCAN];
> > > > >  	struct sgx_backing backing[SGX_NR_TO_SCAN];
> > > > > -	struct sgx_epc_section *section;
> > > > >  	struct sgx_encl_page *encl_page;
> > > > >  	struct sgx_epc_page *epc_page;
> > > > >  	pgoff_t page_index;
> > > > > @@ -378,11 +377,7 @@ static void sgx_reclaim_pages(void)
> > > > >  		kref_put(&encl_page->encl->refcount, sgx_encl_release);
> > > > >  		epc_page->flags &= ~SGX_EPC_PAGE_RECLAIMER_TRACKED;
> > > > >  
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > -		section = &sgx_epc_sections[epc_page->section];
> > > > > -		spin_lock(&section->lock);
> > > > > -		list_add_tail(&epc_page->list, &section->page_list);
> > > > > -		section->free_cnt++;
> > > > > -		spin_unlock(&section->lock);
> > > > > +		sgx_free_epc_page(epc_page);
> > > > >  	}
> > > > >  }
> > > > 
> > > > In current upstream (3fb6d0e00e), sgx_free_epc_page() calls __eremove().
> > > >  This code does not call __eremove().  That seems to be changing
> > > > behavior where none was intended.
> > > 
> > > EREMOVE does not matter here, as it doesn't in almost all most of the sites
> > > where sgx_free_epc_page() is used in the driver. It does nothing to an
> > > uninitialized pages.
> > 
> > Right. EREMOVE on uninitialized pages does nothing, so a more reasonable way is to
> > just NOT call EREMOVE (your original code), since it is absolutely unnecessary.
> > 
> > I don't see ANY reason we should call EREMOVE here. 
> > 
> > Actually w/o my patch to split EREMOVE out of sgx_free_epc_page(), it then makes
> > perfect sense to have new sgx_free_epc_page() here.
> > 
> > > 
> > > The two patches that I posted originally for Kai's series took EREMOVE out
> > > of sgx_free_epc_page() and put an explicit EREMOVE where it is actually
> > > needed, but for reasons unknown to me, that change is gone.
> > > 
> > 
> > It's not gone. It goes into a new sgx_encl_free_epc_page(), which is exactly the same
> > as current sgx_free_epc_page() which as EREMOVE, instead of putting EREMOVE into a
> > dedicated sgx_reset_epc_page(), as you did in your series:
> > 
> > https://lore.kernel.org/linux-sgx/20210113233541.17669-1-jarkko@kernel.org/
> > 
> > However, your change has side effort: it always put page back into free pool, even
> > EREMOVE fails. To make your change w/o having any functional change, it has to be:
> > 
> > 	if(!sgx_reset_epc_page())
> > 		sgx_free_epc_page();
> 
> OK, great, your patch set uses the wrapper only in the necessary call
> sites. Sorry, I overlooked this part.
> 
> Anyway, it knowingly does that. I considered either as equally harmful
> side-ffects when I implemented. Either can only trigger, when there is a
> bug in the kernel code.
> 
> It *could* do what that snippet suggest but it's like "out of the frying pan,
> into the fire" kind of change.
> 
> Since NUMA patch set anyway requires to have a global dirty list, I think
> the better way to deal with this, would be to declare a new global in the
> patch under discussion:
> 
> static struct list_head sgx_dirty_list;

sgx_dirty_page_list

> 
> And sgx_encl_free_epc_page() could simply put the pages in this list. In
> some cases you could possibly even reset the system state using kexec for
> debugging purposes, so it could potentially bring a tiny bit of value.
> 
> I can rebase then my NUMA patches on top of SGX specific KVM patches, once
> Boris have applied them.
> 
> > And for this, Dave raised one concern we should add a WARN() to let user know EPC
> > page is leaked, and reboot is requied to get them back.
> > 
> > However with sgx_reset_epc_page(), there's no place to add such WARN(), and
> > implementing original sgx_free_epc_page() as sgx_encl_free_epc_page() looks very
> > reasonable to me:
> > 
> > https://www.spinics.net/lists/linux-sgx/msg04631.html
> 
> /Jarkko


/Jarkko
Jarkko Sakkinen March 10, 2021, 10:35 p.m. UTC | #8
On Thu, Mar 11, 2021 at 12:12:17AM +0200, Jarkko Sakkinen wrote:
> On Thu, Mar 11, 2021 at 12:10:56AM +0200, Jarkko Sakkinen wrote:
> > On Thu, Mar 11, 2021 at 09:36:15AM +1300, Kai Huang wrote:
> > > On Wed, 2021-03-10 at 17:11 +0200, Jarkko Sakkinen wrote:
> > > > On Wed, Mar 03, 2021 at 08:59:17AM -0800, Dave Hansen wrote:
> > > > > On 3/3/21 7:03 AM, Jarkko Sakkinen wrote:
> > > > > > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> > > > > > index 52d070fb4c9a..ed99c60024dc 100644
> > > > > > --- a/arch/x86/kernel/cpu/sgx/main.c
> > > > > > +++ b/arch/x86/kernel/cpu/sgx/main.c
> > > > > > @@ -305,7 +305,6 @@ static void sgx_reclaim_pages(void)
> > > > > >  {
> > > > > >  	struct sgx_epc_page *chunk[SGX_NR_TO_SCAN];
> > > > > >  	struct sgx_backing backing[SGX_NR_TO_SCAN];
> > > > > > -	struct sgx_epc_section *section;
> > > > > >  	struct sgx_encl_page *encl_page;
> > > > > >  	struct sgx_epc_page *epc_page;
> > > > > >  	pgoff_t page_index;
> > > > > > @@ -378,11 +377,7 @@ static void sgx_reclaim_pages(void)
> > > > > >  		kref_put(&encl_page->encl->refcount, sgx_encl_release);
> > > > > >  		epc_page->flags &= ~SGX_EPC_PAGE_RECLAIMER_TRACKED;
> > > > > >  
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > -		section = &sgx_epc_sections[epc_page->section];
> > > > > > -		spin_lock(&section->lock);
> > > > > > -		list_add_tail(&epc_page->list, &section->page_list);
> > > > > > -		section->free_cnt++;
> > > > > > -		spin_unlock(&section->lock);
> > > > > > +		sgx_free_epc_page(epc_page);
> > > > > >  	}
> > > > > >  }
> > > > > 
> > > > > In current upstream (3fb6d0e00e), sgx_free_epc_page() calls __eremove().
> > > > >  This code does not call __eremove().  That seems to be changing
> > > > > behavior where none was intended.
> > > > 
> > > > EREMOVE does not matter here, as it doesn't in almost all most of the sites
> > > > where sgx_free_epc_page() is used in the driver. It does nothing to an
> > > > uninitialized pages.
> > > 
> > > Right. EREMOVE on uninitialized pages does nothing, so a more reasonable way is to
> > > just NOT call EREMOVE (your original code), since it is absolutely unnecessary.
> > > 
> > > I don't see ANY reason we should call EREMOVE here. 
> > > 
> > > Actually w/o my patch to split EREMOVE out of sgx_free_epc_page(), it then makes
> > > perfect sense to have new sgx_free_epc_page() here.
> > > 
> > > > 
> > > > The two patches that I posted originally for Kai's series took EREMOVE out
> > > > of sgx_free_epc_page() and put an explicit EREMOVE where it is actually
> > > > needed, but for reasons unknown to me, that change is gone.
> > > > 
> > > 
> > > It's not gone. It goes into a new sgx_encl_free_epc_page(), which is exactly the same
> > > as current sgx_free_epc_page() which as EREMOVE, instead of putting EREMOVE into a
> > > dedicated sgx_reset_epc_page(), as you did in your series:
> > > 
> > > https://lore.kernel.org/linux-sgx/20210113233541.17669-1-jarkko@kernel.org/
> > > 
> > > However, your change has side effort: it always put page back into free pool, even
> > > EREMOVE fails. To make your change w/o having any functional change, it has to be:
> > > 
> > > 	if(!sgx_reset_epc_page())
> > > 		sgx_free_epc_page();
> > 
> > OK, great, your patch set uses the wrapper only in the necessary call
> > sites. Sorry, I overlooked this part.
> > 
> > Anyway, it knowingly does that. I considered either as equally harmful
> > side-ffects when I implemented. Either can only trigger, when there is a
> > bug in the kernel code.
> > 
> > It *could* do what that snippet suggest but it's like "out of the frying pan,
> > into the fire" kind of change.
> > 
> > Since NUMA patch set anyway requires to have a global dirty list, I think
> > the better way to deal with this, would be to declare a new global in the
> > patch under discussion:
> > 
> > static struct list_head sgx_dirty_list;
> 
> sgx_dirty_page_list

Actually, I think it is good as it is now. Please do nothing :-)

Acked-by: Jarkko Sakkinen <jarkko@kernel.org>

I can continue from that and improve the fallback further. Not perfect, but
good enough.

/Jarkko
Kai Huang March 10, 2021, 10:43 p.m. UTC | #9
On Thu, 2021-03-11 at 00:35 +0200, Jarkko Sakkinen wrote:
> On Thu, Mar 11, 2021 at 12:12:17AM +0200, Jarkko Sakkinen wrote:
> > On Thu, Mar 11, 2021 at 12:10:56AM +0200, Jarkko Sakkinen wrote:
> > > On Thu, Mar 11, 2021 at 09:36:15AM +1300, Kai Huang wrote:
> > > > On Wed, 2021-03-10 at 17:11 +0200, Jarkko Sakkinen wrote:
> > > > > On Wed, Mar 03, 2021 at 08:59:17AM -0800, Dave Hansen wrote:
> > > > > > On 3/3/21 7:03 AM, Jarkko Sakkinen wrote:
> > > > > > > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> > > > > > > index 52d070fb4c9a..ed99c60024dc 100644
> > > > > > > --- a/arch/x86/kernel/cpu/sgx/main.c
> > > > > > > +++ b/arch/x86/kernel/cpu/sgx/main.c
> > > > > > > @@ -305,7 +305,6 @@ static void sgx_reclaim_pages(void)
> > > > > > >  {
> > > > > > >  	struct sgx_epc_page *chunk[SGX_NR_TO_SCAN];
> > > > > > >  	struct sgx_backing backing[SGX_NR_TO_SCAN];
> > > > > > > -	struct sgx_epc_section *section;
> > > > > > >  	struct sgx_encl_page *encl_page;
> > > > > > >  	struct sgx_epc_page *epc_page;
> > > > > > >  	pgoff_t page_index;
> > > > > > > @@ -378,11 +377,7 @@ static void sgx_reclaim_pages(void)
> > > > > > >  		kref_put(&encl_page->encl->refcount, sgx_encl_release);
> > > > > > >  		epc_page->flags &= ~SGX_EPC_PAGE_RECLAIMER_TRACKED;
> > > > > > >  
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > -		section = &sgx_epc_sections[epc_page->section];
> > > > > > > -		spin_lock(&section->lock);
> > > > > > > -		list_add_tail(&epc_page->list, &section->page_list);
> > > > > > > -		section->free_cnt++;
> > > > > > > -		spin_unlock(&section->lock);
> > > > > > > +		sgx_free_epc_page(epc_page);
> > > > > > >  	}
> > > > > > >  }
> > > > > > 
> > > > > > In current upstream (3fb6d0e00e), sgx_free_epc_page() calls __eremove().
> > > > > >  This code does not call __eremove().  That seems to be changing
> > > > > > behavior where none was intended.
> > > > > 
> > > > > EREMOVE does not matter here, as it doesn't in almost all most of the sites
> > > > > where sgx_free_epc_page() is used in the driver. It does nothing to an
> > > > > uninitialized pages.
> > > > 
> > > > Right. EREMOVE on uninitialized pages does nothing, so a more reasonable way is to
> > > > just NOT call EREMOVE (your original code), since it is absolutely unnecessary.
> > > > 
> > > > I don't see ANY reason we should call EREMOVE here. 
> > > > 
> > > > Actually w/o my patch to split EREMOVE out of sgx_free_epc_page(), it then makes
> > > > perfect sense to have new sgx_free_epc_page() here.
> > > > 
> > > > > 
> > > > > The two patches that I posted originally for Kai's series took EREMOVE out
> > > > > of sgx_free_epc_page() and put an explicit EREMOVE where it is actually
> > > > > needed, but for reasons unknown to me, that change is gone.
> > > > > 
> > > > 
> > > > It's not gone. It goes into a new sgx_encl_free_epc_page(), which is exactly the same
> > > > as current sgx_free_epc_page() which as EREMOVE, instead of putting EREMOVE into a
> > > > dedicated sgx_reset_epc_page(), as you did in your series:
> > > > 
> > > > https://lore.kernel.org/linux-sgx/20210113233541.17669-1-jarkko@kernel.org/
> > > > 
> > > > However, your change has side effort: it always put page back into free pool, even
> > > > EREMOVE fails. To make your change w/o having any functional change, it has to be:
> > > > 
> > > > 	if(!sgx_reset_epc_page())
> > > > 		sgx_free_epc_page();
> > > 
> > > OK, great, your patch set uses the wrapper only in the necessary call
> > > sites. Sorry, I overlooked this part.
> > > 
> > > Anyway, it knowingly does that. I considered either as equally harmful
> > > side-ffects when I implemented. Either can only trigger, when there is a
> > > bug in the kernel code.
> > > 
> > > It *could* do what that snippet suggest but it's like "out of the frying pan,
> > > into the fire" kind of change.
> > > 
> > > Since NUMA patch set anyway requires to have a global dirty list, I think
> > > the better way to deal with this, would be to declare a new global in the
> > > patch under discussion:
> > > 
> > > static struct list_head sgx_dirty_list;
> > 
> > sgx_dirty_page_list
> 
> Actually, I think it is good as it is now. Please do nothing :-)
> 
> Acked-by: Jarkko Sakkinen <jarkko@kernel.org>
> 
> I can continue from that and improve the fallback further. Not perfect, but
> good enough.

Great. Thank you Jarkko.

I'll add your Acked-by and repost it since I also made a mistake in copy-paste:)
Kai Huang March 10, 2021, 10:52 p.m. UTC | #10
On Thu, 2021-03-11 at 11:43 +1300, Kai Huang wrote:
> On Thu, 2021-03-11 at 00:35 +0200, Jarkko Sakkinen wrote:
> > On Thu, Mar 11, 2021 at 12:12:17AM +0200, Jarkko Sakkinen wrote:
> > > On Thu, Mar 11, 2021 at 12:10:56AM +0200, Jarkko Sakkinen wrote:
> > > > On Thu, Mar 11, 2021 at 09:36:15AM +1300, Kai Huang wrote:
> > > > > On Wed, 2021-03-10 at 17:11 +0200, Jarkko Sakkinen wrote:
> > > > > > On Wed, Mar 03, 2021 at 08:59:17AM -0800, Dave Hansen wrote:
> > > > > > > On 3/3/21 7:03 AM, Jarkko Sakkinen wrote:
> > > > > > > > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> > > > > > > > index 52d070fb4c9a..ed99c60024dc 100644
> > > > > > > > --- a/arch/x86/kernel/cpu/sgx/main.c
> > > > > > > > +++ b/arch/x86/kernel/cpu/sgx/main.c
> > > > > > > > @@ -305,7 +305,6 @@ static void sgx_reclaim_pages(void)
> > > > > > > >  {
> > > > > > > >  	struct sgx_epc_page *chunk[SGX_NR_TO_SCAN];
> > > > > > > >  	struct sgx_backing backing[SGX_NR_TO_SCAN];
> > > > > > > > -	struct sgx_epc_section *section;
> > > > > > > >  	struct sgx_encl_page *encl_page;
> > > > > > > >  	struct sgx_epc_page *epc_page;
> > > > > > > >  	pgoff_t page_index;
> > > > > > > > @@ -378,11 +377,7 @@ static void sgx_reclaim_pages(void)
> > > > > > > >  		kref_put(&encl_page->encl->refcount, sgx_encl_release);
> > > > > > > >  		epc_page->flags &= ~SGX_EPC_PAGE_RECLAIMER_TRACKED;
> > > > > > > >  
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > -		section = &sgx_epc_sections[epc_page->section];
> > > > > > > > -		spin_lock(&section->lock);
> > > > > > > > -		list_add_tail(&epc_page->list, &section->page_list);
> > > > > > > > -		section->free_cnt++;
> > > > > > > > -		spin_unlock(&section->lock);
> > > > > > > > +		sgx_free_epc_page(epc_page);
> > > > > > > >  	}
> > > > > > > >  }
> > > > > > > 
> > > > > > > In current upstream (3fb6d0e00e), sgx_free_epc_page() calls __eremove().
> > > > > > >  This code does not call __eremove().  That seems to be changing
> > > > > > > behavior where none was intended.
> > > > > > 
> > > > > > EREMOVE does not matter here, as it doesn't in almost all most of the sites
> > > > > > where sgx_free_epc_page() is used in the driver. It does nothing to an
> > > > > > uninitialized pages.
> > > > > 
> > > > > Right. EREMOVE on uninitialized pages does nothing, so a more reasonable way is to
> > > > > just NOT call EREMOVE (your original code), since it is absolutely unnecessary.
> > > > > 
> > > > > I don't see ANY reason we should call EREMOVE here. 
> > > > > 
> > > > > Actually w/o my patch to split EREMOVE out of sgx_free_epc_page(), it then makes
> > > > > perfect sense to have new sgx_free_epc_page() here.
> > > > > 
> > > > > > 
> > > > > > The two patches that I posted originally for Kai's series took EREMOVE out
> > > > > > of sgx_free_epc_page() and put an explicit EREMOVE where it is actually
> > > > > > needed, but for reasons unknown to me, that change is gone.
> > > > > > 
> > > > > 
> > > > > It's not gone. It goes into a new sgx_encl_free_epc_page(), which is exactly the same
> > > > > as current sgx_free_epc_page() which as EREMOVE, instead of putting EREMOVE into a
> > > > > dedicated sgx_reset_epc_page(), as you did in your series:
> > > > > 
> > > > > https://lore.kernel.org/linux-sgx/20210113233541.17669-1-jarkko@kernel.org/
> > > > > 
> > > > > However, your change has side effort: it always put page back into free pool, even
> > > > > EREMOVE fails. To make your change w/o having any functional change, it has to be:
> > > > > 
> > > > > 	if(!sgx_reset_epc_page())
> > > > > 		sgx_free_epc_page();
> > > > 
> > > > OK, great, your patch set uses the wrapper only in the necessary call
> > > > sites. Sorry, I overlooked this part.
> > > > 
> > > > Anyway, it knowingly does that. I considered either as equally harmful
> > > > side-ffects when I implemented. Either can only trigger, when there is a
> > > > bug in the kernel code.
> > > > 
> > > > It *could* do what that snippet suggest but it's like "out of the frying pan,
> > > > into the fire" kind of change.
> > > > 
> > > > Since NUMA patch set anyway requires to have a global dirty list, I think
> > > > the better way to deal with this, would be to declare a new global in the
> > > > patch under discussion:
> > > > 
> > > > static struct list_head sgx_dirty_list;
> > > 
> > > sgx_dirty_page_list
> > 
> > Actually, I think it is good as it is now. Please do nothing :-)
> > 
> > Acked-by: Jarkko Sakkinen <jarkko@kernel.org>
> > 
> > I can continue from that and improve the fallback further. Not perfect, but
> > good enough.
> 
> Great. Thank you Jarkko.
> 
> I'll add your Acked-by and repost it since I also made a mistake in copy-paste:)
> 

Hmm.. This patch was originally from you, so it has From you, and has your SoB. It
also has Co-developed-by me, but does it still require Acked-by from you?

Anyway I have added it to my local. Let me know if I should remove it.

Patch
diff mbox series

diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
index 52d070fb4c9a..ed99c60024dc 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -305,7 +305,6 @@  static void sgx_reclaim_pages(void)
 {
 	struct sgx_epc_page *chunk[SGX_NR_TO_SCAN];
 	struct sgx_backing backing[SGX_NR_TO_SCAN];
-	struct sgx_epc_section *section;
 	struct sgx_encl_page *encl_page;
 	struct sgx_epc_page *epc_page;
 	pgoff_t page_index;
@@ -378,11 +377,7 @@  static void sgx_reclaim_pages(void)
 		kref_put(&encl_page->encl->refcount, sgx_encl_release);
 		epc_page->flags &= ~SGX_EPC_PAGE_RECLAIMER_TRACKED;
 
-		section = &sgx_epc_sections[epc_page->section];
-		spin_lock(&section->lock);
-		list_add_tail(&epc_page->list, &section->page_list);
-		section->free_cnt++;
-		spin_unlock(&section->lock);
+		sgx_free_epc_page(epc_page);
 	}
 }