Re: [PATCH v41 12/24] x86/sgx: Add SGX_IOC_ENCLAVE_CREATE

[Dave Hansen <dave.hansen@intel.com>]

Hillf, I noticed that you removed a bunch of folks from cc, including
me.  Was there a reason for that?  I haven't been seeing your feedback
on these patches at all.

On 11/14/20 8:40 PM, Hillf Danton wrote:
> On Fri, 13 Nov 2020 00:01:23 +0200 Jarkko Sakkinen wrote:
>> +long sgx_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
>> +{
>> +	struct sgx_encl *encl = filep->private_data;
>> +	int ret;
>> +
>> +	if (test_and_set_bit(SGX_ENCL_IOCTL, &encl->flags))
>> +		return -EBUSY;
> Looks like encl->ioctl_mutex is needed to exlusively serialize
> concurrent ioctl threads and make encl->flags free of the duty.
> Plus it can cut the confusing EBUSY in userspace as it is not
> a critical path in any form.

I actually think the -EBUSY might be a bit too nice.  This is a *bad*
condition that userspace shouldn't be hitting.

Sean had a great explanation of this in a private mail:

> Reclaiming (i.e. swapping) pages out of an enclave can be done while an enclave
> is being built.  Reclaiming involves ENCLS, which needs to be serialized for a
> given enclave, i.e. reclaiming pages needs to take encl->lock.  To help adjust
> to EPC pressure, reclaim is automatically performed when allocating an EPC page,
> i.e. is triggered by ioctls.  Holding encl->lock for the entire ioctl() will
> thus deadlock if an enclave reclaims from itself.
> 
> There are other ways to solve the deadlock problem, e.g. only reclaim from
> workers and never from process context, but there are other motivations for
> the atomic shenanigans (see below).

> ENCLS instructions must be serialized for a given enclave, but holding
> encl->lock for an entire ioctl() will result in deadlock due to an enclave
> triggering reclaim on itself.
> 
> Building an enclave must also be serialized, i.e. userspace can't queue up
> EADD on multiple threads, because the order in which pages are added to an
> enclave affects the measurement.  In other words, rejecting the ioctl() as
> opposed to waiting on a lock is also desirable.

Sounds like we need should follow up with an add-on patch to get some of
that into a comment.