Re: Testing 5.17 bugfix material

[Jarkko Sakkinen <jarkko@kernel.org>]

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