Modify the comments for sgx_encl_lookup_backing() and for
sgx_encl_alloc_backing() to indicate that they take a reference
which must be dropped with a call to sgx_encl_put_backing().
Make sgx_encl_lookup_backing() static for now, and change the
name of sgx_encl_get_backing() to __sgx_encl_get_backing() to
make it more clear that sgx_encl_get_backing() is an internal
function.
Signed-off-by: Kristen Carlson Accardi <[email protected]>
---
arch/x86/kernel/cpu/sgx/encl.c | 21 ++++++++++++++-------
arch/x86/kernel/cpu/sgx/encl.h | 2 --
2 files changed, 14 insertions(+), 9 deletions(-)
diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
index 19876ebfb504..325c2d59e6b4 100644
--- a/arch/x86/kernel/cpu/sgx/encl.c
+++ b/arch/x86/kernel/cpu/sgx/encl.c
@@ -12,6 +12,9 @@
#include "encls.h"
#include "sgx.h"
+static int sgx_encl_lookup_backing(struct sgx_encl *encl, unsigned long page_index,
+ struct sgx_backing *backing);
+
#define PCMDS_PER_PAGE (PAGE_SIZE / sizeof(struct sgx_pcmd))
/*
* 32 PCMD entries share a PCMD page. PCMD_FIRST_MASK is used to
@@ -706,7 +709,7 @@ static struct page *sgx_encl_get_backing_page(struct sgx_encl *encl,
}
/**
- * sgx_encl_get_backing() - Pin the backing storage
+ * __sgx_encl_get_backing() - Pin the backing storage
* @encl: an enclave pointer
* @page_index: enclave page index
* @backing: data for accessing backing storage for the page
@@ -718,7 +721,7 @@ static struct page *sgx_encl_get_backing_page(struct sgx_encl *encl,
* 0 on success,
* -errno otherwise.
*/
-static int sgx_encl_get_backing(struct sgx_encl *encl, unsigned long page_index,
+static int __sgx_encl_get_backing(struct sgx_encl *encl, unsigned long page_index,
struct sgx_backing *backing)
{
pgoff_t page_pcmd_off = sgx_encl_get_backing_page_pcmd_offset(encl, page_index);
@@ -794,7 +797,7 @@ static struct mem_cgroup *sgx_encl_get_mem_cgroup(struct sgx_encl *encl)
}
/**
- * sgx_encl_alloc_backing() - allocate a new backing storage page
+ * sgx_encl_alloc_backing() - create a new backing storage page
* @encl: an enclave pointer
* @page_index: enclave page index
* @backing: data for accessing backing storage for the page
@@ -802,7 +805,9 @@ static struct mem_cgroup *sgx_encl_get_mem_cgroup(struct sgx_encl *encl)
* When called from ksgxd, sets the active memcg from one of the
* mms in the enclave's mm_list prior to any backing page allocation,
* in order to ensure that shmem page allocations are charged to the
- * enclave.
+ * enclave. Create a backing page for loading data back into an EPC page with
+ * ELDU. This function takes a reference on a new backing page which
+ * must be dropped with a corresponding call to sgx_encl_put_backing().
*
* Return:
* 0 on success,
@@ -815,7 +820,7 @@ int sgx_encl_alloc_backing(struct sgx_encl *encl, unsigned long page_index,
struct mem_cgroup *memcg = set_active_memcg(encl_memcg);
int ret;
- ret = sgx_encl_get_backing(encl, page_index, backing);
+ ret = __sgx_encl_get_backing(encl, page_index, backing);
set_active_memcg(memcg);
mem_cgroup_put(encl_memcg);
@@ -833,15 +838,17 @@ int sgx_encl_alloc_backing(struct sgx_encl *encl, unsigned long page_index,
* It is the caller's responsibility to ensure that it is appropriate to use
* sgx_encl_lookup_backing() rather than sgx_encl_alloc_backing(). If lookup is
* not used correctly, this will cause an allocation which is not accounted for.
+ * This function takes a reference on an existing backing page which must be
+ * dropped with a corresponding call to sgx_encl_put_backing().
*
* Return:
* 0 on success,
* -errno otherwise.
*/
-int sgx_encl_lookup_backing(struct sgx_encl *encl, unsigned long page_index,
+static int sgx_encl_lookup_backing(struct sgx_encl *encl, unsigned long page_index,
struct sgx_backing *backing)
{
- return sgx_encl_get_backing(encl, page_index, backing);
+ return __sgx_encl_get_backing(encl, page_index, backing);
}
/**
diff --git a/arch/x86/kernel/cpu/sgx/encl.h b/arch/x86/kernel/cpu/sgx/encl.h
index 332ef3568267..d731ef53f815 100644
--- a/arch/x86/kernel/cpu/sgx/encl.h
+++ b/arch/x86/kernel/cpu/sgx/encl.h
@@ -106,8 +106,6 @@ int sgx_encl_may_map(struct sgx_encl *encl, unsigned long start,
bool current_is_ksgxd(void);
void sgx_encl_release(struct kref *ref);
int sgx_encl_mm_add(struct sgx_encl *encl, struct mm_struct *mm);
-int sgx_encl_lookup_backing(struct sgx_encl *encl, unsigned long page_index,
- struct sgx_backing *backing);
int sgx_encl_alloc_backing(struct sgx_encl *encl, unsigned long page_index,
struct sgx_backing *backing);
void sgx_encl_put_backing(struct sgx_backing *backing);
--
2.36.1
On Wed, Jul 20, 2022 at 11:21:19AM -0700, Kristen Carlson Accardi wrote:
> Modify the comments for sgx_encl_lookup_backing() and for
> sgx_encl_alloc_backing() to indicate that they take a reference
> which must be dropped with a call to sgx_encl_put_backing().
> Make sgx_encl_lookup_backing() static for now, and change the
> name of sgx_encl_get_backing() to __sgx_encl_get_backing() to
> make it more clear that sgx_encl_get_backing() is an internal
> function.
>
> Signed-off-by: Kristen Carlson Accardi <[email protected]>
The rename is unnecessary.
> ---
> arch/x86/kernel/cpu/sgx/encl.c | 21 ++++++++++++++-------
> arch/x86/kernel/cpu/sgx/encl.h | 2 --
> 2 files changed, 14 insertions(+), 9 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
> index 19876ebfb504..325c2d59e6b4 100644
> --- a/arch/x86/kernel/cpu/sgx/encl.c
> +++ b/arch/x86/kernel/cpu/sgx/encl.c
> @@ -12,6 +12,9 @@
> #include "encls.h"
> #include "sgx.h"
>
> +static int sgx_encl_lookup_backing(struct sgx_encl *encl, unsigned long page_index,
> + struct sgx_backing *backing);
> +
> #define PCMDS_PER_PAGE (PAGE_SIZE / sizeof(struct sgx_pcmd))
> /*
> * 32 PCMD entries share a PCMD page. PCMD_FIRST_MASK is used to
> @@ -706,7 +709,7 @@ static struct page *sgx_encl_get_backing_page(struct sgx_encl *encl,
> }
>
> /**
> - * sgx_encl_get_backing() - Pin the backing storage
> + * __sgx_encl_get_backing() - Pin the backing storage
> * @encl: an enclave pointer
> * @page_index: enclave page index
> * @backing: data for accessing backing storage for the page
> @@ -718,7 +721,7 @@ static struct page *sgx_encl_get_backing_page(struct sgx_encl *encl,
> * 0 on success,
> * -errno otherwise.
> */
> -static int sgx_encl_get_backing(struct sgx_encl *encl, unsigned long page_index,
> +static int __sgx_encl_get_backing(struct sgx_encl *encl, unsigned long page_index,
> struct sgx_backing *backing)
> {
> pgoff_t page_pcmd_off = sgx_encl_get_backing_page_pcmd_offset(encl, page_index);
> @@ -794,7 +797,7 @@ static struct mem_cgroup *sgx_encl_get_mem_cgroup(struct sgx_encl *encl)
> }
>
> /**
> - * sgx_encl_alloc_backing() - allocate a new backing storage page
> + * sgx_encl_alloc_backing() - create a new backing storage page
> * @encl: an enclave pointer
> * @page_index: enclave page index
> * @backing: data for accessing backing storage for the page
> @@ -802,7 +805,9 @@ static struct mem_cgroup *sgx_encl_get_mem_cgroup(struct sgx_encl *encl)
> * When called from ksgxd, sets the active memcg from one of the
> * mms in the enclave's mm_list prior to any backing page allocation,
> * in order to ensure that shmem page allocations are charged to the
> - * enclave.
> + * enclave. Create a backing page for loading data back into an EPC page with
> + * ELDU. This function takes a reference on a new backing page which
> + * must be dropped with a corresponding call to sgx_encl_put_backing().
> *
> * Return:
> * 0 on success,
> @@ -815,7 +820,7 @@ int sgx_encl_alloc_backing(struct sgx_encl *encl, unsigned long page_index,
> struct mem_cgroup *memcg = set_active_memcg(encl_memcg);
> int ret;
>
> - ret = sgx_encl_get_backing(encl, page_index, backing);
> + ret = __sgx_encl_get_backing(encl, page_index, backing);
>
> set_active_memcg(memcg);
> mem_cgroup_put(encl_memcg);
> @@ -833,15 +838,17 @@ int sgx_encl_alloc_backing(struct sgx_encl *encl, unsigned long page_index,
> * It is the caller's responsibility to ensure that it is appropriate to use
> * sgx_encl_lookup_backing() rather than sgx_encl_alloc_backing(). If lookup is
> * not used correctly, this will cause an allocation which is not accounted for.
> + * This function takes a reference on an existing backing page which must be
> + * dropped with a corresponding call to sgx_encl_put_backing().
> *
> * Return:
> * 0 on success,
> * -errno otherwise.
> */
> -int sgx_encl_lookup_backing(struct sgx_encl *encl, unsigned long page_index,
> +static int sgx_encl_lookup_backing(struct sgx_encl *encl, unsigned long page_index,
> struct sgx_backing *backing)
> {
> - return sgx_encl_get_backing(encl, page_index, backing);
> + return __sgx_encl_get_backing(encl, page_index, backing);
> }
>
> /**
> diff --git a/arch/x86/kernel/cpu/sgx/encl.h b/arch/x86/kernel/cpu/sgx/encl.h
> index 332ef3568267..d731ef53f815 100644
> --- a/arch/x86/kernel/cpu/sgx/encl.h
> +++ b/arch/x86/kernel/cpu/sgx/encl.h
> @@ -106,8 +106,6 @@ int sgx_encl_may_map(struct sgx_encl *encl, unsigned long start,
> bool current_is_ksgxd(void);
> void sgx_encl_release(struct kref *ref);
> int sgx_encl_mm_add(struct sgx_encl *encl, struct mm_struct *mm);
> -int sgx_encl_lookup_backing(struct sgx_encl *encl, unsigned long page_index,
> - struct sgx_backing *backing);
> int sgx_encl_alloc_backing(struct sgx_encl *encl, unsigned long page_index,
> struct sgx_backing *backing);
> void sgx_encl_put_backing(struct sgx_backing *backing);
> --
> 2.36.1
>
BR, JArkko
On 7/28/22 00:58, Jarkko Sakkinen wrote:
> On Wed, Jul 20, 2022 at 11:21:19AM -0700, Kristen Carlson Accardi wrote:
>> Modify the comments for sgx_encl_lookup_backing() and for
>> sgx_encl_alloc_backing() to indicate that they take a reference
>> which must be dropped with a call to sgx_encl_put_backing().
>> Make sgx_encl_lookup_backing() static for now, and change the
>> name of sgx_encl_get_backing() to __sgx_encl_get_backing() to
>> make it more clear that sgx_encl_get_backing() is an internal
>> function.
>>
>> Signed-off-by: Kristen Carlson Accardi <[email protected]>
> The rename is unnecessary.
Well, it was done to address some reviewer confusion:
https://lore.kernel.org/all/[email protected]/
I wouldn't call it unnecessary. Heck, I'd argue that the one of the
main reasons that this code leaked memory in the first place was the
naming and lack of comments.
On Thu, Jul 28, 2022 at 06:47:01AM -0700, Dave Hansen wrote:
> On 7/28/22 00:58, Jarkko Sakkinen wrote:
> > On Wed, Jul 20, 2022 at 11:21:19AM -0700, Kristen Carlson Accardi wrote:
> >> Modify the comments for sgx_encl_lookup_backing() and for
> >> sgx_encl_alloc_backing() to indicate that they take a reference
> >> which must be dropped with a call to sgx_encl_put_backing().
> >> Make sgx_encl_lookup_backing() static for now, and change the
> >> name of sgx_encl_get_backing() to __sgx_encl_get_backing() to
> >> make it more clear that sgx_encl_get_backing() is an internal
> >> function.
> >>
> >> Signed-off-by: Kristen Carlson Accardi <[email protected]>
> > The rename is unnecessary.
>
> Well, it was done to address some reviewer confusion:
>
> https://lore.kernel.org/all/[email protected]/
>
> I wouldn't call it unnecessary. Heck, I'd argue that the one of the
> main reasons that this code leaked memory in the first place was the
> naming and lack of comments.
Maybe the rename should be its own patch with this tag in the commit:
Link: https://lore.kernel.org/all/[email protected]/
BR, Jarkko
On Mon, Aug 01, 2022 at 07:40:42PM +0300, Jarkko Sakkinen wrote:
> On Thu, Jul 28, 2022 at 06:47:01AM -0700, Dave Hansen wrote:
> > On 7/28/22 00:58, Jarkko Sakkinen wrote:
> > > On Wed, Jul 20, 2022 at 11:21:19AM -0700, Kristen Carlson Accardi wrote:
> > >> Modify the comments for sgx_encl_lookup_backing() and for
> > >> sgx_encl_alloc_backing() to indicate that they take a reference
> > >> which must be dropped with a call to sgx_encl_put_backing().
> > >> Make sgx_encl_lookup_backing() static for now, and change the
> > >> name of sgx_encl_get_backing() to __sgx_encl_get_backing() to
> > >> make it more clear that sgx_encl_get_backing() is an internal
> > >> function.
> > >>
> > >> Signed-off-by: Kristen Carlson Accardi <[email protected]>
> > > The rename is unnecessary.
> >
> > Well, it was done to address some reviewer confusion:
> >
> > https://lore.kernel.org/all/[email protected]/
> >
> > I wouldn't call it unnecessary. Heck, I'd argue that the one of the
> > main reasons that this code leaked memory in the first place was the
> > naming and lack of comments.
>
> Maybe the rename should be its own patch with this tag in the commit:
>
> Link: https://lore.kernel.org/all/[email protected]/
Actually, just adding this to the current patch would be fine
(no need to split necessariy).
BR, Jarkko