Testing 5.17 bugfix material

Testing 5.17 bugfix material

[Dave Hansen]

Hi Everyone,

There are a few SGX fixes that have showed up in the last week or so,
mostly around RAS and fixing the backing storage issues.  Could folks
please give this branch a good thrashing?

> https://git.kernel.org/pub/scm/linux/kernel/git/daveh/devel.git/log/?h=x86/sgx

I'm planning to send this bunch up to Linus after 5.17-rc1 comes out.

Kristen, I really dug into the changelogs of your two patches to make it
more clear that they are bugfix and stable@ material.  I'd appreciate
some additional eyeballs there.

Re: Testing 5.17 bugfix material

[Haitao Huang]

Hi Dave,

On Fri, 21 Jan 2022 13:57:50 -0600, Dave Hansen <dave.hansen@intel.com>
wrote:

> Hi Everyone,
>
> There are a few SGX fixes that have showed up in the last week or so,
> mostly around RAS and fixing the backing storage issues.  Could folks
> please give this branch a good thrashing?
>
>> https://git.kernel.org/pub/scm/linux/kernel/git/daveh/devel.git/log/?h=x86/sgx
>
> I'm planning to send this bunch up to Linus after 5.17-rc1 comes out.
>
> Kristen, I really dug into the changelogs of your two patches to make it
> more clear that they are bugfix and stable@ material.  I'd appreciate
> some additional eyeballs there.

When testing this with a large enclave loaded by Intel runtime, we saw it  
hang
on EADD ioctl and the ioctl never returns.

Also noticed the selftest fails on oversub but the return errno is EINTR  
not
ENOMEM as expected.

When user space register signal handler with SA_RESTART flag, EINTR would
be an auto restart of ioctl. So that might be what's going on with Intel
runtime test. And I suspect following code may cause infinite restart of  
the
ioctl:

struct sgx_epc_page *sgx_alloc_epc_page(void *owner, bool reclaim)
{
       struct sgx_epc_page *page;

       for ( ; ; ) {
           page = __sgx_alloc_epc_page();
           if (!IS_ERR(page)) {
               page->owner = owner;
               break;
           }

           if (list_empty(&sgx_active_page_list))<-- should also check
sgx_nr_available_backing_pages?
               return ERR_PTR(-ENOMEM);

           if (!reclaim) {
               page = ERR_PTR(-EBUSY);
               break;
           }

           if (signal_pending(current)) {
               page = ERR_PTR(-ERESTARTSYS);<---- this is the only exit of
the for loop when backing store limit reaches.
               break;
           }

           sgx_reclaim_pages(); <---- no checking for backing store fail due
to limit here.
           cond_resched();
       }

Thanks
Haitao

Re: Testing 5.17 bugfix material

[Jarkko Sakkinen]

On Sat, Jan 22, 2022 at 01:15:01PM -0600, Haitao Huang wrote:
> Hi Dave,
> 
> On Fri, 21 Jan 2022 13:57:50 -0600, Dave Hansen <dave.hansen@intel.com>
> wrote:
> 
> > Hi Everyone,
> > 
> > There are a few SGX fixes that have showed up in the last week or so,
> > mostly around RAS and fixing the backing storage issues.  Could folks
> > please give this branch a good thrashing?
> > 
> > > https://git.kernel.org/pub/scm/linux/kernel/git/daveh/devel.git/log/?h=x86/sgx
> > 
> > I'm planning to send this bunch up to Linus after 5.17-rc1 comes out.
> > 
> > Kristen, I really dug into the changelogs of your two patches to make it
> > more clear that they are bugfix and stable@ material.  I'd appreciate
> > some additional eyeballs there.
> 
> When testing this with a large enclave loaded by Intel runtime, we saw it
> hang
> on EADD ioctl and the ioctl never returns.
> 
> Also noticed the selftest fails on oversub but the return errno is EINTR not
> ENOMEM as expected.
> 
> When user space register signal handler with SA_RESTART flag, EINTR would
> be an auto restart of ioctl. So that might be what's going on with Intel
> runtime test. And I suspect following code may cause infinite restart of the
> ioctl:
> 
> struct sgx_epc_page *sgx_alloc_epc_page(void *owner, bool reclaim)
> {
>       struct sgx_epc_page *page;
> 
>       for ( ; ; ) {
>           page = __sgx_alloc_epc_page();
>           if (!IS_ERR(page)) {
>               page->owner = owner;
>               break;
>           }
> 
>           if (list_empty(&sgx_active_page_list))<-- should also check
> sgx_nr_available_backing_pages?
>               return ERR_PTR(-ENOMEM);
> 
>           if (!reclaim) {
>               page = ERR_PTR(-EBUSY);
>               break;
>           }
> 
>           if (signal_pending(current)) {
>               page = ERR_PTR(-ERESTARTSYS);<---- this is the only exit of
> the for loop when backing store limit reaches.
>               break;
>           }
> 
>           sgx_reclaim_pages(); <---- no checking for backing store fail due
> to limit here.
>           cond_resched();
>       }
> 
> Thanks
> Haitao

Haitao, this does not match what I see in tip/x86/sgx:

struct sgx_epc_page *sgx_alloc_epc_page(void *owner, bool reclaim)
{
	struct sgx_epc_page *page;

	for ( ; ; ) {
		page = __sgx_alloc_epc_page();
		if (!IS_ERR(page)) {
			page->owner = owner;
			break;
		}

		if (list_empty(&sgx_active_page_list))
			return ERR_PTR(-ENOMEM);

		if (!reclaim) {
			page = ERR_PTR(-EBUSY);
			break;
		}

		if (signal_pending(current)) {
			page = ERR_PTR(-ERESTARTSYS);
			break;
		}

		sgx_reclaim_pages();
		cond_resched();
	}

	if (sgx_should_reclaim(SGX_NR_LOW_PAGES))
		wake_up(&ksgxd_waitq);

	return page;
}

Just in case I also checked tip/master, and the result is the same.

/Jarkko

Re: Testing 5.17 bugfix material

[Jarkko Sakkinen]

On Mon, Jan 24, 2022 at 07:42:08PM +0200, Jarkko Sakkinen wrote:
> On Sat, Jan 22, 2022 at 01:15:01PM -0600, Haitao Huang wrote:
> > Hi Dave,
> > 
> > On Fri, 21 Jan 2022 13:57:50 -0600, Dave Hansen <dave.hansen@intel.com>
> > wrote:
> > 
> > > Hi Everyone,
> > > 
> > > There are a few SGX fixes that have showed up in the last week or so,
> > > mostly around RAS and fixing the backing storage issues.  Could folks
> > > please give this branch a good thrashing?
> > > 
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/daveh/devel.git/log/?h=x86/sgx
> > > 
> > > I'm planning to send this bunch up to Linus after 5.17-rc1 comes out.
> > > 
> > > Kristen, I really dug into the changelogs of your two patches to make it
> > > more clear that they are bugfix and stable@ material.  I'd appreciate
> > > some additional eyeballs there.
> > 
> > When testing this with a large enclave loaded by Intel runtime, we saw it
> > hang
> > on EADD ioctl and the ioctl never returns.
> > 
> > Also noticed the selftest fails on oversub but the return errno is EINTR not
> > ENOMEM as expected.
> > 
> > When user space register signal handler with SA_RESTART flag, EINTR would
> > be an auto restart of ioctl. So that might be what's going on with Intel
> > runtime test. And I suspect following code may cause infinite restart of the
> > ioctl:
> > 
> > struct sgx_epc_page *sgx_alloc_epc_page(void *owner, bool reclaim)
> > {
> >       struct sgx_epc_page *page;
> > 
> >       for ( ; ; ) {
> >           page = __sgx_alloc_epc_page();
> >           if (!IS_ERR(page)) {
> >               page->owner = owner;
> >               break;
> >           }
> > 
> >           if (list_empty(&sgx_active_page_list))<-- should also check
> > sgx_nr_available_backing_pages?
> >               return ERR_PTR(-ENOMEM);

I don't think so but why do you think it should?

> > 
> >           if (!reclaim) {
> >               page = ERR_PTR(-EBUSY);
> >               break;
> >           }
> > 
> >           if (signal_pending(current)) {
> >               page = ERR_PTR(-ERESTARTSYS);<---- this is the only exit of
> > the for loop when backing store limit reaches.
> >               break;
> >           }
> > 
> >           sgx_reclaim_pages(); <---- no checking for backing store fail due
> > to limit here.

Initially I'd think you're making here the right conclusion. Given the
limit sgx_reclaim_pages() should return e.g. boolean or int to tell that
we are out of backing storage.

Kristen?

> >           cond_resched();
> >       }
> > 
> > Thanks
> > Haitao

The reason was that it was not x86/sgx branch of tip but this:

https://git.kernel.org/pub/scm/linux/kernel/git/daveh/devel.git/log/?h=x86/sgx

Sorry for initial reject.

BR, Jarkko

Re: Testing 5.17 bugfix material

[Haitao Huang]

On Mon, 24 Jan 2022 11:55:10 -0600, Jarkko Sakkinen <jarkko@kernel.org>
wrote:

> On Mon, Jan 24, 2022 at 07:42:08PM +0200, Jarkko Sakkinen wrote:
>> On Sat, Jan 22, 2022 at 01:15:01PM -0600, Haitao Huang wrote:
>> > Hi Dave,
>> >
>> > On Fri, 21 Jan 2022 13:57:50 -0600, Dave Hansen  
>> <dave.hansen@intel.com>
>> > wrote:
>> >
>> > > Hi Everyone,
>> > >
>> > > There are a few SGX fixes that have showed up in the last week or  
>> so,
>> > > mostly around RAS and fixing the backing storage issues.  Could  
>> folks
>> > > please give this branch a good thrashing?
>> > >
>> > > >  
>> https://git.kernel.org/pub/scm/linux/kernel/git/daveh/devel.git/log/?h=x86/sgx
>> > >
>> > > I'm planning to send this bunch up to Linus after 5.17-rc1 comes  
>> out.
>> > >
>> > > Kristen, I really dug into the changelogs of your two patches to  
>> make it
>> > > more clear that they are bugfix and stable@ material.  I'd  
>> appreciate
>> > > some additional eyeballs there.
>> >
>> > When testing this with a large enclave loaded by Intel runtime, we  
>> saw it
>> > hang
>> > on EADD ioctl and the ioctl never returns.
>> >
>> > Also noticed the selftest fails on oversub but the return errno is  
>> EINTR not
>> > ENOMEM as expected.
>> >
>> > When user space register signal handler with SA_RESTART flag, EINTR  
>> would
>> > be an auto restart of ioctl. So that might be what's going on with  
>> Intel
>> > runtime test. And I suspect following code may cause infinite restart  
>> of the
>> > ioctl:
>> >
>> > struct sgx_epc_page *sgx_alloc_epc_page(void *owner, bool reclaim)
>> > {
>> >       struct sgx_epc_page *page;
>> >
>> >       for ( ; ; ) {
>> >           page = __sgx_alloc_epc_page();
>> >           if (!IS_ERR(page)) {
>> >               page->owner = owner;
>> >               break;
>> >           }
>> >
>> >           if (list_empty(&sgx_active_page_list))<-- should also check
>> > sgx_nr_available_backing_pages?
>> >               return ERR_PTR(-ENOMEM);
>
> I don't think so but why do you think it should?

I was thinking no point to continue if
(atomic_long_read(&sgx_nr_available_backing_pages)==0).
We just failed on finding available EPC above in __sgx_alloc_epc_page. So  
if we can't swap, then nothing we can do, right?

BR
Haitao

Re: Testing 5.17 bugfix material

[Jarkko Sakkinen]

On Fri, Jan 21, 2022 at 11:57:50AM -0800, Dave Hansen wrote:
> Hi Everyone,
> 
> There are a few SGX fixes that have showed up in the last week or so,
> mostly around RAS and fixing the backing storage issues.  Could folks
                ~~~

Remote ATtestation?

> please give this branch a good thrashing?
> 
> > https://git.kernel.org/pub/scm/linux/kernel/git/daveh/devel.git/log/?h=x86/sgx
> 
> I'm planning to send this bunch up to Linus after 5.17-rc1 comes out.
> 
> Kristen, I really dug into the changelogs of your two patches to make it
> more clear that they are bugfix and stable@ material.  I'd appreciate
> some additional eyeballs there.

Thanks for the heads up, this actually fits perfectly my next weeks other
goals.

I'm implementing ECDSA attestation for Enarx next week (i.e. using AESMD).
I was planning to use this branch in any case.

I can combine that with enclaves that trigger backing storage (it can
host arbitarily sized WebAssembly programs). For those changes I'll do
testing both my own hardware (i5-9600KF) and also Icelake Server
(Platinum 8352Y).

If anything comes up, I'll post a fix directly, or if I get stuck I'll
rise up the issues here.

BR, Jarkko

Re: Testing 5.17 bugfix material

[Accardi, Kristen C]

On Fri, 2022-01-21 at 11:57 -0800, Dave Hansen wrote:
> Hi Everyone,
> 
> There are a few SGX fixes that have showed up in the last week or so,
> mostly around RAS and fixing the backing storage issues.  Could folks
> please give this branch a good thrashing?
> 
> > https://git.kernel.org/pub/scm/linux/kernel/git/daveh/devel.git/log/?h=x86/sgx
> 
> I'm planning to send this bunch up to Linus after 5.17-rc1 comes out.
> 
> Kristen, I really dug into the changelogs of your two patches to make
> it
> more clear that they are bugfix and stable@ material.  I'd appreciate
> some additional eyeballs there.

There's a bug in the calculation for the available backing bytes,
pointed out by Haitao and team. Here's a fix applied to your tree.

From 2ebcf0e70b1235224410e08c983e357d5ac3c435 Mon Sep 17 00:00:00 2001
From: Kristen Carlson Accardi <kristen@linux.intel.com>
Date: Mon, 24 Jan 2022 09:28:56 -0800
Subject: [PATCH] x86/sgx: fixup for available backing pages calculation

Remove improper parentheses from calculation for available backing
bytes. Without this fix, the result will be incorrect due to
rounding.

Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
---
 arch/x86/kernel/cpu/sgx/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
index 7ed6a1b10c21..10a6af89bf64 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -922,7 +922,7 @@ static bool __init sgx_page_cache_init(void)
 		return false;
 	}
 
-	available_backing_bytes = total_epc_bytes * (sgx_overcommit_percent / 100);
+	available_backing_bytes = total_epc_bytes * sgx_overcommit_percent / 100;
 	atomic_long_set(&sgx_nr_available_backing_pages, available_backing_bytes >> PAGE_SHIFT);
 
 	return true;
-- 
2.20.1

Re: Testing 5.17 bugfix material

[Jarkko Sakkinen]

On Mon, Jan 24, 2022 at 05:44:43PM +0000, Accardi, Kristen C wrote:
> On Fri, 2022-01-21 at 11:57 -0800, Dave Hansen wrote:
> > Hi Everyone,
> > 
> > There are a few SGX fixes that have showed up in the last week or so,
> > mostly around RAS and fixing the backing storage issues.  Could folks
> > please give this branch a good thrashing?
> > 
> > > https://git.kernel.org/pub/scm/linux/kernel/git/daveh/devel.git/log/?h=x86/sgx
> > 
> > I'm planning to send this bunch up to Linus after 5.17-rc1 comes out.
> > 
> > Kristen, I really dug into the changelogs of your two patches to make
> > it
> > more clear that they are bugfix and stable@ material.  I'd appreciate
> > some additional eyeballs there.
> 
> There's a bug in the calculation for the available backing bytes,
> pointed out by Haitao and team. Here's a fix applied to your tree.
> 
> From 2ebcf0e70b1235224410e08c983e357d5ac3c435 Mon Sep 17 00:00:00 2001
> From: Kristen Carlson Accardi <kristen@linux.intel.com>
> Date: Mon, 24 Jan 2022 09:28:56 -0800
> Subject: [PATCH] x86/sgx: fixup for available backing pages calculation
> 
> Remove improper parentheses from calculation for available backing
> bytes. Without this fix, the result will be incorrect due to
> rounding.
> 
> Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
> ---
>  arch/x86/kernel/cpu/sgx/main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> index 7ed6a1b10c21..10a6af89bf64 100644
> --- a/arch/x86/kernel/cpu/sgx/main.c
> +++ b/arch/x86/kernel/cpu/sgx/main.c
> @@ -922,7 +922,7 @@ static bool __init sgx_page_cache_init(void)
>  		return false;
>  	}
>  
> -	available_backing_bytes = total_epc_bytes * (sgx_overcommit_percent / 100);
> +	available_backing_bytes = total_epc_bytes * sgx_overcommit_percent / 100;
>  	atomic_long_set(&sgx_nr_available_backing_pages, available_backing_bytes >> PAGE_SHIFT);
>  
>  	return true;
> -- 
> 2.20.1
> 

So what about sgx_reclaim_pages() having no return value that Haitao
pointed out?

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

/Jarkko

Re: Testing 5.17 bugfix material

[Accardi, Kristen C]

On Mon, 2022-01-24 at 19:56 +0200, Jarkko Sakkinen wrote:
> On Mon, Jan 24, 2022 at 05:44:43PM +0000, Accardi, Kristen C wrote:
> > On Fri, 2022-01-21 at 11:57 -0800, Dave Hansen wrote:
> > > Hi Everyone,
> > > 
> > > There are a few SGX fixes that have showed up in the last week or
> > > so,
> > > mostly around RAS and fixing the backing storage issues.  Could
> > > folks
> > > please give this branch a good thrashing?
> > > 
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/daveh/devel.git/log/?h=x86/sgx
> > > 
> > > I'm planning to send this bunch up to Linus after 5.17-rc1 comes
> > > out.
> > > 
> > > Kristen, I really dug into the changelogs of your two patches to
> > > make
> > > it
> > > more clear that they are bugfix and stable@ material.  I'd
> > > appreciate
> > > some additional eyeballs there.
> > 
> > There's a bug in the calculation for the available backing bytes,
> > pointed out by Haitao and team. Here's a fix applied to your tree.
> > 
> > From 2ebcf0e70b1235224410e08c983e357d5ac3c435 Mon Sep 17 00:00:00
> > 2001
> > From: Kristen Carlson Accardi <kristen@linux.intel.com>
> > Date: Mon, 24 Jan 2022 09:28:56 -0800
> > Subject: [PATCH] x86/sgx: fixup for available backing pages
> > calculation
> > 
> > Remove improper parentheses from calculation for available backing
> > bytes. Without this fix, the result will be incorrect due to
> > rounding.
> > 
> > Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
> > ---
> >  arch/x86/kernel/cpu/sgx/main.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/kernel/cpu/sgx/main.c
> > b/arch/x86/kernel/cpu/sgx/main.c
> > index 7ed6a1b10c21..10a6af89bf64 100644
> > --- a/arch/x86/kernel/cpu/sgx/main.c
> > +++ b/arch/x86/kernel/cpu/sgx/main.c
> > @@ -922,7 +922,7 @@ static bool __init sgx_page_cache_init(void)
> >  		return false;
> >  	}
> >  
> > -	available_backing_bytes = total_epc_bytes *
> > (sgx_overcommit_percent / 100);
> > +	available_backing_bytes = total_epc_bytes *
> > sgx_overcommit_percent / 100;
> >  	atomic_long_set(&sgx_nr_available_backing_pages,
> > available_backing_bytes >> PAGE_SHIFT);
> >  
> >  	return true;
> > -- 
> > 2.20.1
> > 
> 
> So what about sgx_reclaim_pages() having no return value that Haitao
> pointed out?

I'm taking a look at that, and testing, and will send a separate fix.

Re: Testing 5.17 bugfix material

[Jarkko Sakkinen]

On Mon, Jan 24, 2022 at 08:59:14PM +0000, Accardi, Kristen C wrote:
> On Mon, 2022-01-24 at 19:56 +0200, Jarkko Sakkinen wrote:
> > On Mon, Jan 24, 2022 at 05:44:43PM +0000, Accardi, Kristen C wrote:
> > > On Fri, 2022-01-21 at 11:57 -0800, Dave Hansen wrote:
> > > > Hi Everyone,
> > > > 
> > > > There are a few SGX fixes that have showed up in the last week or
> > > > so,
> > > > mostly around RAS and fixing the backing storage issues.  Could
> > > > folks
> > > > please give this branch a good thrashing?
> > > > 
> > > > > https://git.kernel.org/pub/scm/linux/kernel/git/daveh/devel.git/log/?h=x86/sgx
> > > > 
> > > > I'm planning to send this bunch up to Linus after 5.17-rc1 comes
> > > > out.
> > > > 
> > > > Kristen, I really dug into the changelogs of your two patches to
> > > > make
> > > > it
> > > > more clear that they are bugfix and stable@ material.  I'd
> > > > appreciate
> > > > some additional eyeballs there.
> > > 
> > > There's a bug in the calculation for the available backing bytes,
> > > pointed out by Haitao and team. Here's a fix applied to your tree.
> > > 
> > > From 2ebcf0e70b1235224410e08c983e357d5ac3c435 Mon Sep 17 00:00:00
> > > 2001
> > > From: Kristen Carlson Accardi <kristen@linux.intel.com>
> > > Date: Mon, 24 Jan 2022 09:28:56 -0800
> > > Subject: [PATCH] x86/sgx: fixup for available backing pages
> > > calculation
> > > 
> > > Remove improper parentheses from calculation for available backing
> > > bytes. Without this fix, the result will be incorrect due to
> > > rounding.
> > > 
> > > Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
> > > ---
> > >  arch/x86/kernel/cpu/sgx/main.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/arch/x86/kernel/cpu/sgx/main.c
> > > b/arch/x86/kernel/cpu/sgx/main.c
> > > index 7ed6a1b10c21..10a6af89bf64 100644
> > > --- a/arch/x86/kernel/cpu/sgx/main.c
> > > +++ b/arch/x86/kernel/cpu/sgx/main.c
> > > @@ -922,7 +922,7 @@ static bool __init sgx_page_cache_init(void)
> > >  		return false;
> > >  	}
> > >  
> > > -	available_backing_bytes = total_epc_bytes *
> > > (sgx_overcommit_percent / 100);
> > > +	available_backing_bytes = total_epc_bytes *
> > > sgx_overcommit_percent / 100;
> > >  	atomic_long_set(&sgx_nr_available_backing_pages,
> > > available_backing_bytes >> PAGE_SHIFT);
> > >  
> > >  	return true;
> > > -- 
> > > 2.20.1
> > > 
> > 
> > So what about sgx_reclaim_pages() having no return value that Haitao
> > pointed out?
> 
> I'm taking a look at that, and testing, and will send a separate fix.

NP, take your time, just remarked that it looks legit :-)

/Jarkko

Re: Testing 5.17 bugfix material

[Jarkko Sakkinen]

On Fri, Jan 21, 2022 at 11:57:50AM -0800, Dave Hansen wrote:
> Hi Everyone,
> 
> There are a few SGX fixes that have showed up in the last week or so,
> mostly around RAS and fixing the backing storage issues.  Could folks
> please give this branch a good thrashing?
> 
> > https://git.kernel.org/pub/scm/linux/kernel/git/daveh/devel.git/log/?h=x86/sgx
> 
> I'm planning to send this bunch up to Linus after 5.17-rc1 comes out.
> 
> Kristen, I really dug into the changelogs of your two patches to make it
> more clear that they are bugfix and stable@ material.  I'd appreciate
> some additional eyeballs there.

Please drop my shmem patches as it crashes kselftest. I'll send two replacements
soonish (it was easier to debug that way and makes sense for future
bijections etc.).

/Jarkko