diff mbox series

[v5] x86: Remove unnecessary kmap() from sgx_ioc_enclave_init()

Message ID 20210306002058.303796-1-ira.weiny@intel.com (mailing list archive)
State New, archived
Headers show
Series [v5] x86: Remove unnecessary kmap() from sgx_ioc_enclave_init() | expand

Commit Message

Ira Weiny March 6, 2021, 12:20 a.m. UTC
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(PAGE_SIZE, ...)
to get a page aligned 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.

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

---
Changes from v4[4]:
	Add Ack and Reviews
	Send to the correct maintainers

Changes from v3[3]:
	Remove BUILD_BUG_ONs

Changes from v2[2]:
        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/
[3] https://lore.kernel.org/lkml/20210205050850.GC5033@iweiny-DESK2.sc.intel.com/#t
[4] https://lore.kernel.org/lkml/YCBY02iEKLVyj7Ix@kernel.org/

---
 arch/x86/kernel/cpu/sgx/ioctl.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

Comments

Ira Weiny March 17, 2021, 8:06 a.m. UTC | #1
On Fri, Mar 05, 2021 at 04:20:58PM -0800, 'Ira Weiny' 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.

Friendly ping, maybe I missed a response?
Ira

> 
> 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(PAGE_SIZE, ...)
> to get a page aligned 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.
> 
> Cc: Sean Christopherson <seanjc@google.com>
> Cc: Jethro Beekman <jethro@fortanix.com>
> Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
> Acked-by: Dave Hansen <dave.hansen@intel.com>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> 
> ---
> Changes from v4[4]:
> 	Add Ack and Reviews
> 	Send to the correct maintainers
> 
> Changes from v3[3]:
> 	Remove BUILD_BUG_ONs
> 
> Changes from v2[2]:
>         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/
> [3] https://lore.kernel.org/lkml/20210205050850.GC5033@iweiny-DESK2.sc.intel.com/#t
> [4] https://lore.kernel.org/lkml/YCBY02iEKLVyj7Ix@kernel.org/
> 
> ---
>  arch/x86/kernel/cpu/sgx/ioctl.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
> index 90a5caf76939..38e540de5e2a 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,14 @@ 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);
>  	token = (void *)((unsigned long)sigstruct + PAGE_SIZE / 2);
>  	memset(token, 0, SGX_LAUNCH_TOKEN_SIZE);
>  
> @@ -645,8 +647,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
>
Borislav Petkov March 24, 2021, 9:47 a.m. UTC | #2
On Fri, Mar 05, 2021 at 04:20:58PM -0800, ira.weiny@intel.com wrote:

> Subject: Re: [PATCH v5] x86: Remove unnecessary kmap() from  sgx_ioc_enclave_init()

The tip tree preferred format for patch subject prefixes is
'subsys/component:', e.g. 'x86/apic:', 'x86/mm/fault:', 'sched/fair:',
'genirq/core:'. Please do not use file names or complete file paths as
prefix. 'git log path/to/file' should give you a reasonable hint in most
cases.

> From: Ira Weiny <ira.weiny@intel.com>
> 
> kmap is inefficient and we are trying to reduce the usage in the kernel.

Please use passive voice in your commit message: no "we" or "I", etc,
and describe your changes in imperative mood.

Also, pls read section "2) Describe your changes" in
Documentation/process/submitting-patches.rst for more details.

Bottom line is: personal pronouns are ambiguous in text, especially with
so many parties/companies/etc developing the kernel so let's avoid them
please.

> @@ -615,11 +614,14 @@ 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

Please integrate scripts/checkpatch.pl into your patch creation
workflow. Some of the warnings/errors *actually* make sense.

WARNING: 'boundry' may be misspelled - perhaps 'boundary'?
#90: FILE: arch/x86/kernel/cpu/sgx/ioctl.c:618:
+        * sigstruct must be on a page boundry and token on a 512 byte boundry
                                       ^^^^^^^

WARNING: 'boundry' may be misspelled - perhaps 'boundary'?
#90: FILE: arch/x86/kernel/cpu/sgx/ioctl.c:618:
+        * sigstruct must be on a page boundry and token on a 512 byte boundry
                                                                       ^^^^^^^

Also, do you see how other comments in this file are proper sentences?
Please formulate yours this way too.

The change itself looks ok.

Thx.
Ira Weiny March 24, 2021, 6:24 p.m. UTC | #3
On Wed, Mar 24, 2021 at 10:47:20AM +0100, Borislav Petkov wrote:
> On Fri, Mar 05, 2021 at 04:20:58PM -0800, ira.weiny@intel.com wrote:
> 
> > Subject: Re: [PATCH v5] x86: Remove unnecessary kmap() from  sgx_ioc_enclave_init()
> 
> The tip tree preferred format for patch subject prefixes is
> 'subsys/component:', e.g. 'x86/apic:', 'x86/mm/fault:', 'sched/fair:',
> 'genirq/core:'. Please do not use file names or complete file paths as
> prefix. 'git log path/to/file' should give you a reasonable hint in most
> cases.

Fixed.

> 
> > From: Ira Weiny <ira.weiny@intel.com>
> > 
> > kmap is inefficient and we are trying to reduce the usage in the kernel.
> 
> Please use passive voice in your commit message: no "we" or "I", etc,
> and describe your changes in imperative mood.
> 
> Also, pls read section "2) Describe your changes" in
> Documentation/process/submitting-patches.rst for more details.
> 
> Bottom line is: personal pronouns are ambiguous in text, especially with
> so many parties/companies/etc developing the kernel so let's avoid them
> please.

Fixed.

> 
> > @@ -615,11 +614,14 @@ 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
> 
> Please integrate scripts/checkpatch.pl into your patch creation
> workflow. Some of the warnings/errors *actually* make sense.
> 
> WARNING: 'boundry' may be misspelled - perhaps 'boundary'?
> #90: FILE: arch/x86/kernel/cpu/sgx/ioctl.c:618:
> +        * sigstruct must be on a page boundry and token on a 512 byte boundry
>                                        ^^^^^^^
> 
> WARNING: 'boundry' may be misspelled - perhaps 'boundary'?
> #90: FILE: arch/x86/kernel/cpu/sgx/ioctl.c:618:
> +        * sigstruct must be on a page boundry and token on a 512 byte boundry
>                                                                        ^^^^^^^
> 
> Also, do you see how other comments in this file are proper sentences?
> Please formulate yours this way too.

Thanks fixed.

> 
> The change itself looks ok.

Thanks, new version 5 sent.

Ira

> 
> Thx.
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette
diff mbox series

Patch

diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
index 90a5caf76939..38e540de5e2a 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,14 @@  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);
 	token = (void *)((unsigned long)sigstruct + PAGE_SIZE / 2);
 	memset(token, 0, SGX_LAUNCH_TOKEN_SIZE);
 
@@ -645,8 +647,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;
 }