Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 28 Jul 2021 23:32:42 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Dave Hansen <dave.hansen@intel.com>
Cc:     "Luck, Tony" <tony.luck@intel.com>,
        Jarkko Sakkinen <jarkko@kernel.org>,
        "x86@kernel.org" <x86@kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v3 1/7] x86/sgx: Provide indication of life-cycle of EPC
 pages
Message-ID: <YQHpGq0GyAsYpE+D@google.com>
References: <20210719182009.1409895-1-tony.luck@intel.com>
 <20210728204653.1509010-1-tony.luck@intel.com>
 <20210728204653.1509010-2-tony.luck@intel.com>
 <17054ca5-0ef7-4b28-ab26-b1b96aa7403f@intel.com>
 <f2685d7c8dc14792a4e0f9807f742ea6@intel.com>
 <fd8f8e79-f63f-7d6f-277e-1ad08ab7b6b8@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <fd8f8e79-f63f-7d6f-277e-1ad08ab7b6b8@intel.com>
Precedence: bulk

On Wed, Jul 28, 2021, Dave Hansen wrote:
> On 7/28/21 3:57 PM, Luck, Tony wrote:
> >> Wouldn't it be safer to do something like:
> >>
> >> 	page->owner = owner ? owner : (void *)-1;
> >>
> >> -1 is non-NULL, but also invalid, which makes it harder for us to poke
> >> ourselves in the eye.
> > Does Linux have some #define INVALID_POINTER thing that
> > provides a guaranteed bad (e.g. non-canonical) value?
> > 
> > (void *)-1 seems hacky.
> 
> ERR_PTR(-SOMETHING) wouldn't be too bad.  I guess it could even be:
> 
> 	page->owner = ERR_PTR(SGX_EPC_PAGE_VA);
> 
> and then:
> 
> #define SGX_EPC_PAGE_VA 0xffff...something...greppable
> 
> I *thought* we had a file full of these magic values, but maybe I'm
> misremembering the uapi magic header.

Rather than use a magic const, just pass in the actual va_page.  The only reason
NULL is passed is that prior to virtual EPC, there were only enclave pages and
VA pages, and assiging a non-NULL pointer to sgx_epc_page.owner, which is a
struct sgx_encl_page, was gross.  Virtual EPC sets owner somewhat prematurely;
it's needed iff an EPC cgroup is added, to support OOM EPC killing (and a pointer
to va_page is also needed in this case).

sgx_epc_page.owner can even be converted to 'void *' without additional changes
since all consumers capture it in a local sgx_encl_page variable.


diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
index 001808e3901c..f9da8fe4dd6b 100644
--- a/arch/x86/kernel/cpu/sgx/encl.c
+++ b/arch/x86/kernel/cpu/sgx/encl.c
@@ -674,12 +674,12 @@ int sgx_encl_test_and_clear_young(struct mm_struct *mm,
  *   a VA page,
  *   -errno otherwise
  */
-struct sgx_epc_page *sgx_alloc_va_page(void)
+struct sgx_epc_page *sgx_alloc_va_page(struct sgx_va_page *va_page)
 {
        struct sgx_epc_page *epc_page;
        int ret;

-       epc_page = sgx_alloc_epc_page(NULL, true);
+       epc_page = sgx_alloc_epc_page(va_page, true);
        if (IS_ERR(epc_page))
                return ERR_CAST(epc_page);

diff --git a/arch/x86/kernel/cpu/sgx/encl.h b/arch/x86/kernel/cpu/sgx/encl.h
index fec43ca65065..3d12dbeae14a 100644
--- a/arch/x86/kernel/cpu/sgx/encl.h
+++ b/arch/x86/kernel/cpu/sgx/encl.h
@@ -111,7 +111,7 @@ void sgx_encl_put_backing(struct sgx_backing *backing, bool do_write);
 int sgx_encl_test_and_clear_young(struct mm_struct *mm,
                                  struct sgx_encl_page *page);

-struct sgx_epc_page *sgx_alloc_va_page(void);
+struct sgx_epc_page *sgx_alloc_va_page(struct sgx_va_page *va_page);
 unsigned int sgx_alloc_va_slot(struct sgx_va_page *va_page);
 void sgx_free_va_slot(struct sgx_va_page *va_page, unsigned int offset);
 bool sgx_va_page_full(struct sgx_va_page *va_page);
diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
index 83df20e3e633..655ce0bb069d 100644
--- a/arch/x86/kernel/cpu/sgx/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/ioctl.c
@@ -30,7 +30,7 @@ static struct sgx_va_page *sgx_encl_grow(struct sgx_encl *encl)
                if (!va_page)
                        return ERR_PTR(-ENOMEM);

-               va_page->epc_page = sgx_alloc_va_page();
+               va_page->epc_page = sgx_alloc_va_page(va_page);
                if (IS_ERR(va_page->epc_page)) {
                        err = ERR_CAST(va_page->epc_page);
                        kfree(va_page);
diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h
index 4628acec0009..4e1a410b8a62 100644
--- a/arch/x86/kernel/cpu/sgx/sgx.h
+++ b/arch/x86/kernel/cpu/sgx/sgx.h
@@ -29,7 +29,7 @@
 struct sgx_epc_page {
        unsigned int section;
        unsigned int flags;
-       struct sgx_encl_page *owner;
+       void *owner;
        struct list_head list;
 };