Re: [PATCH v38 13/24] x86/sgx: Add SGX_IOC_ENCLAVE_ADD_PAGES

[Jarkko Sakkinen]

On Fri, Sep 18, 2020 at 03:20:39PM +0300, Jarkko Sakkinen wrote:
> On Thu, Sep 17, 2020 at 07:09:40PM -0700, Sean Christopherson wrote:
> > On Thu, Sep 17, 2020 at 01:35:10PM -0500, Haitao Huang wrote:
> > > On Thu, 17 Sep 2020 11:02:06 -0500, Jarkko Sakkinen
> > > <jarkko.sakkinen@xxxxxxxxxxxxxxx> wrote:
> > > > 
> > > > Right, I do get the OOM case but wouldn't in that case the reasonable
> > > > thing to do destroy the enclave that is not even running? I mean that
> > > > means that we are globally out of EPC.
> > > > 
> > > 
> > > I would say it could be a policy, but not the only one. If it does not make
> > > much difference to kernel, IMHO we should  not set it in stone now.
> > > Debugging is also huge benefit to me.
> > 
> > Agreed, an EPC cgroup is the proper way to define/enforce what happens when
> > there is EPC pressure.  E.g. if process A is consuming 99% of the EPC, then
> > it doesn't make sense to unconditionally kill enclaves from process B.  If
> > the admin wants to give process A priority, so be it, but such a decision
> > shouldn't be baked into the kernel.
> > 
> > This series obviously doesn't provide an EPC cgroup, but that doesn't mean
> > we can't make decisions that will play nice with a cgroup in the future.
> 
> Here's the core issue why the API "as is used to be" does not work:
> 
> 	if (ret == -EIO) {
> 		mutex_lock(&encl->lock);
> 		sgx_encl_destroy(encl);
> 		mutex_unlock(&encl->lock);
> 	}
> 
> It would be better to instead whitelist *when* the enclave is preserved.
> 
> 	if (ret != -ENOMEM) {
> 		mutex_lock(&encl->lock);
> 		sgx_encl_destroy(encl);
> 		mutex_unlock(&encl->lock);
> 	}
> 
> That is the information we *deterministically* want to know. Otherwise,
> we will live in ultimate chaos.
> 
> Only this way can caller know when there are means to continue, and when
> to quit. I.e. the code is whitelisting wrong way around currently.

Actually since the state cannot corrupt unless EADD or EEXTEND fails it
is fine to have the enclave alive on any other error condition. I think
what I do is that I move the check out sgx_encl_add_page() to the main
ioctl and update the kdoc. It is not too clear on persistence. I'll fix
this.

/Jarkko