linux-kernel - Re: [PATCH v6 06/11] 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-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20171130173200.GA11190@linux.intel.com>
Date:   Thu, 30 Nov 2017 09:32:01 -0800
From:   Sean Christopherson <sean.j.christopherson@...el.com>
To:     Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>,
        platform-driver-x86@...r.kernel.org, x86@...nel.org
Cc:     linux-kernel@...r.kernel.org, Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>,
        Darren Hart <dvhart@...radead.org>,
        Andy Shevchenko <andy@...radead.org>
Subject: Re: [PATCH v6 06/11] intel_sgx: driver for Intel Software Guard
 Extensions

On Sat, Nov 25, 2017 at 09:29:24PM +0200, Jarkko Sakkinen wrote:
> +static void *sgx_try_alloc_page(void)
> +{
> +       struct sgx_epc_bank *bank;
> +       void *page = NULL;
> +       int i;
> +
> +       for (i = 0; i < sgx_nr_epc_banks; i++) {
> +               bank = &sgx_epc_banks[i];
> +
> +               down_write(&bank->lock);

Is a R/W semaphore actually preferable to a spinlock?  Concurrent
free calls don't seem that interesting/beneficial because freeing
an enclave's pages isn't multiplexed across multiple CPUs, unlike
the allocation of EPC pages.

As a whole, I'm not a fan of packing the EPC page pointers into an
array rather than encapsulating them in a struct+list.  The primary
benefit I see for the array approach is that it saves ~8 bytes per
free EPC page, but at a cost of increased memory usage for in-use
pages and severely restricting the ability to enhance/modify how
EPC pages are tracked, reclaimed, etc...

The main issue is that the array approach relies on the caller to
handle reclaim.  This effectively makes it impossible to reclaim
pages from multiple processes, requires other consumers e.g. KVM
to implement their own reclaim logic and kthread, and prevents
cgroup accounting because the cgroup can't initiate reclaim.

> +
> +               if (atomic_read(&bank->free_cnt))
> +                       page = bank->pages[atomic_dec_return(&bank->free_cnt)];
> +
> +               up_write(&bank->lock);
> +
> +               if (page)
> +                       break;
> +       }
> +
> +       if (page)
> +               atomic_dec(&sgx_nr_free_pages);
> +
> +       return page;
> +}