linux-kernel - Re: [intel-sgx-kernel-dev] [PATCH RFC v3 07/12] intel_sgx: driver for Intel Software Guard Extensions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 14 Nov 2017 21:33:34 +0200
From:   Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>
To:     Dave Hansen <dave.hansen@...el.com>
Cc:     intel-sgx-kernel-dev@...ts.01.org,
        platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [intel-sgx-kernel-dev] [PATCH RFC v3 07/12] intel_sgx: driver
 for Intel Software Guard Extensions

On Tue, Nov 07, 2017 at 11:05:08AM -0800, Dave Hansen wrote:
> On 11/07/2017 10:47 AM, Jarkko Sakkinen wrote:
> > On Mon, Nov 06, 2017 at 07:54:00AM -0800, Dave Hansen wrote:
> >> On 10/10/2017 07:32 AM, Jarkko Sakkinen wrote:
> >>> +static LIST_HEAD(sgx_free_list);
> >>> +static DEFINE_SPINLOCK(sgx_free_list_lock);
> >>
> >> Is this a global list?  Will this be a scalability problem on larger
> >> systems?
> > 
> > It will be need to be refined for NUMA.
> > 
> > In addition, per-CPU caches would probably make sense.
> > 
> > For simplicity, I would keep it as it is up until the driver is in the
> > mainline.
> 
> FWIW, I don't think we should merge things that aren't performant.
> Global locks like this are just intolerable.  You can add this as a
> later patch, but please don't merge stuff like this.

The back pointer to struct sgx_encl_page from struct sgx_epc_page is
useless. I've had this in backlog already long time ago but had forgot
it as I've been mostly working with the launch infrastructure lately.

Your comment worked as kind of a reminder of that. Thank you.

Once that field is removed the whole struct is useless and EPC bank
converges to an array. With an array the driver should be able reserve
pages without a global lock.

I've started writing a patch to make all this happen and it is
progressing really well. I'm planning to include this change to v6.
As it simplifies code I'm going to squash it as part of the initial
driver patch.

How does this sound?

/Jarkko