linux-sgx.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH V3] x86: Remove unnecessary kmap() from sgx_ioc_enclave_init()
@ 2021-02-02 19:47 ira.weiny
  2021-02-02 22:45 ` Jarkko Sakkinen
  0 siblings, 1 reply; 3+ messages in thread
From: ira.weiny @ 2021-02-02 19:47 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: Ira Weiny, Dave Hansen, Sean Christopherson, Jethro Beekman,
	linux-kernel, linux-sgx

From: Ira Weiny <ira.weiny@intel.com>

kmap is inefficient and we are trying to reduce the usage in the kernel.
There is no readily apparent reason why initp_page needs to be allocated
and kmap'ed() but sigstruct needs to be page aligned and token
512 byte aligned.

kmalloc() can give us this alignment but we need to allocate PAGE_SIZE
bytes to do so.  Rather than change this kmap() to kmap_local_page() use
kmalloc() instead.

Remove the alloc_page()/kmap() and replace with kmalloc() to get a
kernel address to use.

In addition add a comment to document the alignment requirements so that
others like myself don't attempt to 'fix' this again.  Finally, add 2
BUILD_BUG_ON's to ensure future changes to sigstruct and token do not go
unnoticed and cause a bug.

Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Sean Christopherson <seanjc@google.com>
Cc: Jethro Beekman <jethro@fortanix.com>
Signed-off-by: Ira Weiny <ira.weiny@intel.com>

---
Changes from v2[1]:
	When allocating a power of 2 size kmalloc() now guarantees the
	alignment of the respective size.  So go back to using kmalloc() but
	with a PAGE_SIZE allocation to get the alignment.  This also follows
	the pattern in sgx_ioc_enclave_create()

Changes from v1[1]:
	Use page_address() instead of kcmalloc() to ensure sigstruct is
	page aligned
	Use BUILD_BUG_ON to ensure token and sigstruct don't collide.

[1] https://lore.kernel.org/lkml/20210129001459.1538805-1-ira.weiny@intel.com/
[2] https://lore.kernel.org/lkml/20210202013725.3514671-1-ira.weiny@intel.com/
---
 arch/x86/kernel/cpu/sgx/ioctl.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
index 90a5caf76939..e0c3301ccd67 100644
--- a/arch/x86/kernel/cpu/sgx/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/ioctl.c
@@ -604,7 +604,6 @@ static long sgx_ioc_enclave_init(struct sgx_encl *encl, void __user *arg)
 {
 	struct sgx_sigstruct *sigstruct;
 	struct sgx_enclave_init init_arg;
-	struct page *initp_page;
 	void *token;
 	int ret;
 
@@ -615,11 +614,16 @@ static long sgx_ioc_enclave_init(struct sgx_encl *encl, void __user *arg)
 	if (copy_from_user(&init_arg, arg, sizeof(init_arg)))
 		return -EFAULT;
 
-	initp_page = alloc_page(GFP_KERNEL);
-	if (!initp_page)
+	/*
+	 * sigstruct must be on a page boundry and token on a 512 byte boundry
+	 * kmalloc() gives us this alignment when allocating PAGE_SIZE bytes
+	 */
+	sigstruct = kmalloc(PAGE_SIZE, GFP_KERNEL);
+	if (!sigstruct)
 		return -ENOMEM;
 
-	sigstruct = kmap(initp_page);
+	BUILD_BUG_ON(sizeof(*sigstruct) > (PAGE_SIZE/2));
+	BUILD_BUG_ON(SGX_LAUNCH_TOKEN_SIZE > (PAGE_SIZE/2));
 	token = (void *)((unsigned long)sigstruct + PAGE_SIZE / 2);
 	memset(token, 0, SGX_LAUNCH_TOKEN_SIZE);
 
@@ -645,8 +649,7 @@ static long sgx_ioc_enclave_init(struct sgx_encl *encl, void __user *arg)
 	ret = sgx_encl_init(encl, sigstruct, token);
 
 out:
-	kunmap(initp_page);
-	__free_page(initp_page);
+	kfree(sigstruct);
 	return ret;
 }
 
-- 
2.28.0.rc0.12.gb6a658bd00c9


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH V3] x86: Remove unnecessary kmap() from sgx_ioc_enclave_init()
  2021-02-02 19:47 [PATCH V3] x86: Remove unnecessary kmap() from sgx_ioc_enclave_init() ira.weiny
@ 2021-02-02 22:45 ` Jarkko Sakkinen
  2021-02-05  5:08   ` Ira Weiny
  0 siblings, 1 reply; 3+ messages in thread
From: Jarkko Sakkinen @ 2021-02-02 22:45 UTC (permalink / raw)
  To: ira.weiny
  Cc: Dave Hansen, Sean Christopherson, Jethro Beekman, linux-kernel,
	linux-sgx

On Tue, Feb 02, 2021 at 11:47:19AM -0800, ira.weiny@intel.com wrote:
> From: Ira Weiny <ira.weiny@intel.com>
> 
> kmap is inefficient and we are trying to reduce the usage in the kernel.
> There is no readily apparent reason why initp_page needs to be allocated
> and kmap'ed() but sigstruct needs to be page aligned and token
> 512 byte aligned.
> 
> kmalloc() can give us this alignment but we need to allocate PAGE_SIZE
> bytes to do so.  Rather than change this kmap() to kmap_local_page() use
> kmalloc() instead.
> 
> Remove the alloc_page()/kmap() and replace with kmalloc() to get a
> kernel address to use.
> 
> In addition add a comment to document the alignment requirements so that
> others like myself don't attempt to 'fix' this again.  Finally, add 2
> BUILD_BUG_ON's to ensure future changes to sigstruct and token do not go
> unnoticed and cause a bug.
> 
> Cc: Dave Hansen <dave.hansen@intel.com>
> Cc: Sean Christopherson <seanjc@google.com>
> Cc: Jethro Beekman <jethro@fortanix.com>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> 
> ---
> Changes from v2[1]:
> 	When allocating a power of 2 size kmalloc() now guarantees the
> 	alignment of the respective size.  So go back to using kmalloc() but
> 	with a PAGE_SIZE allocation to get the alignment.  This also follows
> 	the pattern in sgx_ioc_enclave_create()
> 
> Changes from v1[1]:
> 	Use page_address() instead of kcmalloc() to ensure sigstruct is
> 	page aligned
> 	Use BUILD_BUG_ON to ensure token and sigstruct don't collide.
> 
> [1] https://lore.kernel.org/lkml/20210129001459.1538805-1-ira.weiny@intel.com/
> [2] https://lore.kernel.org/lkml/20210202013725.3514671-1-ira.weiny@intel.com/
> ---
>  arch/x86/kernel/cpu/sgx/ioctl.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
> index 90a5caf76939..e0c3301ccd67 100644
> --- a/arch/x86/kernel/cpu/sgx/ioctl.c
> +++ b/arch/x86/kernel/cpu/sgx/ioctl.c
> @@ -604,7 +604,6 @@ static long sgx_ioc_enclave_init(struct sgx_encl *encl, void __user *arg)
>  {
>  	struct sgx_sigstruct *sigstruct;
>  	struct sgx_enclave_init init_arg;
> -	struct page *initp_page;
>  	void *token;
>  	int ret;
>  
> @@ -615,11 +614,16 @@ static long sgx_ioc_enclave_init(struct sgx_encl *encl, void __user *arg)
>  	if (copy_from_user(&init_arg, arg, sizeof(init_arg)))
>  		return -EFAULT;
>  
> -	initp_page = alloc_page(GFP_KERNEL);
> -	if (!initp_page)
> +	/*
> +	 * sigstruct must be on a page boundry and token on a 512 byte boundry
> +	 * kmalloc() gives us this alignment when allocating PAGE_SIZE bytes
> +	 */
> +	sigstruct = kmalloc(PAGE_SIZE, GFP_KERNEL);
> +	if (!sigstruct)
>  		return -ENOMEM;
>  
> -	sigstruct = kmap(initp_page);
> +	BUILD_BUG_ON(sizeof(*sigstruct) > (PAGE_SIZE/2));
> +	BUILD_BUG_ON(SGX_LAUNCH_TOKEN_SIZE > (PAGE_SIZE/2));

Please remove these.

>  	token = (void *)((unsigned long)sigstruct + PAGE_SIZE / 2);
>  	memset(token, 0, SGX_LAUNCH_TOKEN_SIZE);
>  
> @@ -645,8 +649,7 @@ static long sgx_ioc_enclave_init(struct sgx_encl *encl, void __user *arg)
>  	ret = sgx_encl_init(encl, sigstruct, token);
>  
>  out:
> -	kunmap(initp_page);
> -	__free_page(initp_page);
> +	kfree(sigstruct);
>  	return ret;
>  }
>  
> -- 
> 2.28.0.rc0.12.gb6a658bd00c9
> 
> 

/Jarkko

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH V3] x86: Remove unnecessary kmap() from sgx_ioc_enclave_init()
  2021-02-02 22:45 ` Jarkko Sakkinen
@ 2021-02-05  5:08   ` Ira Weiny
  0 siblings, 0 replies; 3+ messages in thread
From: Ira Weiny @ 2021-02-05  5:08 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: Dave Hansen, Sean Christopherson, Jethro Beekman, linux-kernel,
	linux-sgx

On Wed, Feb 03, 2021 at 12:45:33AM +0200, Jarkko Sakkinen wrote:
> On Tue, Feb 02, 2021 at 11:47:19AM -0800, ira.weiny@intel.com wrote:
> > From: Ira Weiny <ira.weiny@intel.com>
> > 
> > kmap is inefficient and we are trying to reduce the usage in the kernel.
> > There is no readily apparent reason why initp_page needs to be allocated
> > and kmap'ed() but sigstruct needs to be page aligned and token
> > 512 byte aligned.
> > 
> > kmalloc() can give us this alignment but we need to allocate PAGE_SIZE
> > bytes to do so.  Rather than change this kmap() to kmap_local_page() use
> > kmalloc() instead.
> > 
> > Remove the alloc_page()/kmap() and replace with kmalloc() to get a
> > kernel address to use.
> > 
> > In addition add a comment to document the alignment requirements so that
> > others like myself don't attempt to 'fix' this again.  Finally, add 2
> > BUILD_BUG_ON's to ensure future changes to sigstruct and token do not go
> > unnoticed and cause a bug.
> > 
> > Cc: Dave Hansen <dave.hansen@intel.com>
> > Cc: Sean Christopherson <seanjc@google.com>
> > Cc: Jethro Beekman <jethro@fortanix.com>
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > 
> > ---
> > Changes from v2[1]:
> > 	When allocating a power of 2 size kmalloc() now guarantees the
> > 	alignment of the respective size.  So go back to using kmalloc() but
> > 	with a PAGE_SIZE allocation to get the alignment.  This also follows
> > 	the pattern in sgx_ioc_enclave_create()
> > 
> > Changes from v1[1]:
> > 	Use page_address() instead of kcmalloc() to ensure sigstruct is
> > 	page aligned
> > 	Use BUILD_BUG_ON to ensure token and sigstruct don't collide.
> > 
> > [1] https://lore.kernel.org/lkml/20210129001459.1538805-1-ira.weiny@intel.com/
> > [2] https://lore.kernel.org/lkml/20210202013725.3514671-1-ira.weiny@intel.com/
> > ---
> >  arch/x86/kernel/cpu/sgx/ioctl.c | 15 +++++++++------
> >  1 file changed, 9 insertions(+), 6 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
> > index 90a5caf76939..e0c3301ccd67 100644
> > --- a/arch/x86/kernel/cpu/sgx/ioctl.c
> > +++ b/arch/x86/kernel/cpu/sgx/ioctl.c
> > @@ -604,7 +604,6 @@ static long sgx_ioc_enclave_init(struct sgx_encl *encl, void __user *arg)
> >  {
> >  	struct sgx_sigstruct *sigstruct;
> >  	struct sgx_enclave_init init_arg;
> > -	struct page *initp_page;
> >  	void *token;
> >  	int ret;
> >  
> > @@ -615,11 +614,16 @@ static long sgx_ioc_enclave_init(struct sgx_encl *encl, void __user *arg)
> >  	if (copy_from_user(&init_arg, arg, sizeof(init_arg)))
> >  		return -EFAULT;
> >  
> > -	initp_page = alloc_page(GFP_KERNEL);
> > -	if (!initp_page)
> > +	/*
> > +	 * sigstruct must be on a page boundry and token on a 512 byte boundry
> > +	 * kmalloc() gives us this alignment when allocating PAGE_SIZE bytes
> > +	 */
> > +	sigstruct = kmalloc(PAGE_SIZE, GFP_KERNEL);
> > +	if (!sigstruct)
> >  		return -ENOMEM;
> >  
> > -	sigstruct = kmap(initp_page);
> > +	BUILD_BUG_ON(sizeof(*sigstruct) > (PAGE_SIZE/2));
> > +	BUILD_BUG_ON(SGX_LAUNCH_TOKEN_SIZE > (PAGE_SIZE/2));
> 
> Please remove these.

I don't see why these would be a bad thing as they don't have any run time
implications.  But I've removed them for v4 because getting rid of the kmap()
is more important for me right now.

Ira

> 
> >  	token = (void *)((unsigned long)sigstruct + PAGE_SIZE / 2);
> >  	memset(token, 0, SGX_LAUNCH_TOKEN_SIZE);
> >  
> > @@ -645,8 +649,7 @@ static long sgx_ioc_enclave_init(struct sgx_encl *encl, void __user *arg)
> >  	ret = sgx_encl_init(encl, sigstruct, token);
> >  
> >  out:
> > -	kunmap(initp_page);
> > -	__free_page(initp_page);
> > +	kfree(sigstruct);
> >  	return ret;
> >  }
> >  
> > -- 
> > 2.28.0.rc0.12.gb6a658bd00c9
> > 
> > 
> 
> /Jarkko

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-02-05  5:09 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-02 19:47 [PATCH V3] x86: Remove unnecessary kmap() from sgx_ioc_enclave_init() ira.weiny
2021-02-02 22:45 ` Jarkko Sakkinen
2021-02-05  5:08   ` Ira Weiny

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).