| Message ID | 20220720182120.1160956-1-kristen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/sgx: Improve comments for sgx_encl_lookup/alloc_backing() | expand |
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 <kristen@linux.intel.com> 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 <kristen@linux.intel.com> > The rename is unnecessary. Well, it was done to address some reviewer confusion: https://lore.kernel.org/all/YtUs3MKLzFg+rqEV@zn.tnic/ 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 <kristen@linux.intel.com> > > The rename is unnecessary. > > Well, it was done to address some reviewer confusion: > > https://lore.kernel.org/all/YtUs3MKLzFg+rqEV@zn.tnic/ > > 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/YtUs3MKLzFg+rqEV@zn.tnic/ 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 <kristen@linux.intel.com> > > > The rename is unnecessary. > > > > Well, it was done to address some reviewer confusion: > > > > https://lore.kernel.org/all/YtUs3MKLzFg+rqEV@zn.tnic/ > > > > 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/YtUs3MKLzFg+rqEV@zn.tnic/ Actually, just adding this to the current patch would be fine (no need to split necessariy). BR, Jarkko
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);
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 <kristen@linux.intel.com> --- arch/x86/kernel/cpu/sgx/encl.c | 21 ++++++++++++++------- arch/x86/kernel/cpu/sgx/encl.h | 2 -- 2 files changed, 14 insertions(+), 9 deletions(-)