[<prev] [next>] [thread-next>] [day] [month] [year] [list]
Message-ID: <15c3349c-44dd-7057-395c-8fd8c674e87d@intel.com>
Date: Mon, 16 Nov 2020 09:54:47 -0800
From: Dave Hansen <dave.hansen@...el.com>
To: Hillf Danton <hdanton@...a.com>,
Jarkko Sakkinen <jarkko@...nel.org>
Cc: linux-kernel@...r.kernel.org, Jarkko Sakkinen <jarkko@...nel.org>,
Jethro Beekman <jethro@...tanix.com>,
akpm@...ux-foundation.org, andriy.shevchenko@...ux.intel.com,
asapek@...gle.com, bp@...en8.de, cedric.xing@...el.com,
chenalexchen@...gle.com, conradparker@...gle.com,
cyhanish@...gle.com, dave.hansen@...el.com, haitao.huang@...el.com,
kai.huang@...el.com, kai.svahn@...el.com, kmoy@...gle.com,
ludloff@...gle.com, luto@...nel.org, nhorman@...hat.com,
npmccallum@...hat.com, puiterwijk@...hat.com, rientjes@...gle.com,
tglx@...utronix.de, yaozhangx@...gle.com, mikko.ylinen@...el.com
Subject: Re: [PATCH v41 12/24] x86/sgx: Add SGX_IOC_ENCLAVE_CREATE
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.
Powered by blists - more mailing lists