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

From: Dave Hansen <dave.hansen@intel.com>
To: Hillf Danton <hdanton@sina.com>, Jarkko Sakkinen <jarkko@kernel.org>
Cc: linux-kernel@vger.kernel.org, Jarkko Sakkinen <jarkko@kernel.org>,
	Jethro Beekman <jethro@fortanix.com>,
	akpm@linux-foundation.org, andriy.shevchenko@linux.intel.com,
	asapek@google.com, bp@alien8.de, cedric.xing@intel.com,
	chenalexchen@google.com, conradparker@google.com,
	cyhanish@google.com, dave.hansen@intel.com,
	haitao.huang@intel.com, kai.huang@intel.com, kai.svahn@intel.com,
	kmoy@google.com, ludloff@google.com, luto@kernel.org,
	nhorman@redhat.com, npmccallum@redhat.com, puiterwijk@redhat.com,
	rientjes@google.com, tglx@linutronix.de, yaozhangx@google.com,
	mikko.ylinen@intel.com
Subject: Re: [PATCH v41 12/24] x86/sgx: Add SGX_IOC_ENCLAVE_CREATE
Date: Mon, 16 Nov 2020 09:54:47 -0800	[thread overview]
Message-ID: <15c3349c-44dd-7057-395c-8fd8c674e87d@intel.com> (raw)
In-Reply-To: <20201115044044.11040-1-hdanton@sina.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.