Re: [PATCH] x86: Remove unnecessary kmap() from sgx_ioc_enclave_init()
On Tue, Feb 02, 2021 at 10:55:36AM -0800, Dave Hansen wrote: > On 2/1/21 5:37 PM, ira.we...@intel.com wrote: > > kmap is inefficient and we are trying to reduce the usage in the kernel. > > There is no readily apparent reason why the initp_page page needs to be > > allocated and kmap'ed() but sigstruct needs to be page aligned and token > > 512 byte aligned. > > Hi Ira, > > It's a *relatively* recent guaranteed, but: > > https://www.kernel.org/doc/Documentation/core-api/memory-allocation.rst > > says: > > > The address of a chunk allocated with `kmalloc` is aligned to at least > > ARCH_KMALLOC_MINALIGN bytes. For sizes which are a power of two, the > > alignment is also guaranteed to be at least the respective size. > > So, if you allocate a page with kmalloc(), you get an aligned page. Yay! And this what we do sgx_ioc_enclave_create() anyway, as I stated in my earlier response. Better to use the same pattern everywhere consitently when it makes sense. /Jarkko
Re: [PATCH] x86: Remove unnecessary kmap() from sgx_ioc_enclave_init()
On 2/1/21 5:37 PM, ira.we...@intel.com wrote: > kmap is inefficient and we are trying to reduce the usage in the kernel. > There is no readily apparent reason why the initp_page page needs to be > allocated and kmap'ed() but sigstruct needs to be page aligned and token > 512 byte aligned. Hi Ira, It's a *relatively* recent guaranteed, but: https://www.kernel.org/doc/Documentation/core-api/memory-allocation.rst says: > The address of a chunk allocated with `kmalloc` is aligned to at least > ARCH_KMALLOC_MINALIGN bytes. For sizes which are a power of two, the > alignment is also guaranteed to be at least the respective size. So, if you allocate a page with kmalloc(), you get an aligned page. Yay!
Re: [PATCH] x86: Remove unnecessary kmap() from sgx_ioc_enclave_init()
On Mon, Feb 01, 2021 at 08:48:12AM +, Christoph Hellwig wrote: > On Fri, Jan 29, 2021 at 09:37:30AM -0800, Sean Christopherson wrote: > > On Thu, Jan 28, 2021, ira.we...@intel.com wrote: > > > From: Ira Weiny > > > > > > There is no reason to alloc a page and kmap it to store this temporary > > > data from the user. > > > > Actually, there is, it's just poorly documented. The sigstruct needs to be > > page aligned, and the token needs to be 512-byte aligned. kmcalloc doesn't > > guarantee alignment. IIRC things will work until slub_debug is enabled, at > > which point the natural alignment behavior goes out the window. > > Well, there still is absolutely no need for the kmap as you can use > page_address for a GFP_KERNEL allocation. Yeah, we do that in sgx_ioc_enclave_create already based on feedback: secs = kmalloc(PAGE_SIZE, GFP_KERNEL); if (!secs) return -ENOMEM; The kmap() in sgx_ioc_enclave_init() is an unfortunate miss. Let's just follow the pre-existing pattern. /Jarkko
[PATCH] x86: Remove unnecessary kmap() from sgx_ioc_enclave_init()
From: Ira Weiny kmap is inefficient and we are trying to reduce the usage in the kernel. There is no readily apparent reason why the initp_page page needs to be allocated and kmap'ed() but sigstruct needs to be page aligned and token 512 byte aligned. In this case page_address() can be used instead of kmap_local_page() as a much more efficient way to use the address because the page is allocated GFP_KERNEL. Remove the kmap and replace with page_address() to get a kernel address for the alloc'ed page. In addition add a comment regarding the alignment requirements as well as 2 BUILD_BUG_ON's to ensure future changes to sigstruct and token do not go unnoticed and cause a bug. Cc: Sean Christopherson , Cc: Jethro Beekman , Signed-off-by: Ira Weiny --- 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.we...@intel.com/ --- arch/x86/kernel/cpu/sgx/ioctl.c | 10 -- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c index 90a5caf76939..678b02d67c3c 100644 --- a/arch/x86/kernel/cpu/sgx/ioctl.c +++ b/arch/x86/kernel/cpu/sgx/ioctl.c @@ -615,11 +615,18 @@ static long sgx_ioc_enclave_init(struct sgx_encl *encl, void __user *arg) if (copy_from_user(_arg, arg, sizeof(init_arg))) return -EFAULT; + /* +* sigstruct must be on a page boundry and token on a 512 byte boundry +* so use alloc_page/page_address instead of a kmalloc(). +*/ initp_page = alloc_page(GFP_KERNEL); if (!initp_page) return -ENOMEM; - sigstruct = kmap(initp_page); + sigstruct = page_address(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,7 +652,6 @@ 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); return ret; } -- 2.28.0.rc0.12.gb6a658bd00c9
Re: [PATCH] x86: Remove unnecessary kmap() from sgx_ioc_enclave_init()
On Fri, Jan 29, 2021 at 09:37:30AM -0800, Sean Christopherson wrote: > On Thu, Jan 28, 2021, ira.we...@intel.com wrote: > > From: Ira Weiny > > > > There is no reason to alloc a page and kmap it to store this temporary > > data from the user. > > Actually, there is, it's just poorly documented. The sigstruct needs to be > page aligned, and the token needs to be 512-byte aligned. kmcalloc doesn't > guarantee alignment. IIRC things will work until slub_debug is enabled, at > which point the natural alignment behavior goes out the window. Well, there still is absolutely no need for the kmap as you can use page_address for a GFP_KERNEL allocation.
Re: [PATCH] x86: Remove unnecessary kmap() from sgx_ioc_enclave_init()
On Thu, Jan 28, 2021, ira.we...@intel.com wrote: > From: Ira Weiny > > There is no reason to alloc a page and kmap it to store this temporary > data from the user. Actually, there is, it's just poorly documented. The sigstruct needs to be page aligned, and the token needs to be 512-byte aligned. kmcalloc doesn't guarantee alignment. IIRC things will work until slub_debug is enabled, at which point the natural alignment behavior goes out the window. > This is especially true when we are trying to > remove kmap usages. Also placing the token pointer 1/2 way into the > page is fragile. > > Replace this allocation with two kzalloc()'s which also removes the need > for the memset(). > > Signed-off-by: Ira Weiny > --- > arch/x86/kernel/cpu/sgx/ioctl.c | 18 ++ > 1 file changed, 10 insertions(+), 8 deletions(-) > > diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c > index 90a5caf76939..9c9019760585 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,13 +614,15 @@ static long sgx_ioc_enclave_init(struct sgx_encl *encl, > void __user *arg) > if (copy_from_user(_arg, arg, sizeof(init_arg))) > return -EFAULT; > > - initp_page = alloc_page(GFP_KERNEL); > - if (!initp_page) > + sigstruct = kzalloc(sizeof(*sigstruct), 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); > + token = kzalloc(SGX_LAUNCH_TOKEN_SIZE, GFP_KERNEL); > + if (!token) { > + ret = -ENOMEM; > + goto free_sigstruct; > + } > > if (copy_from_user(sigstruct, (void __user *)init_arg.sigstruct, > sizeof(*sigstruct))) { > @@ -645,8 +646,9 @@ 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(token); > +free_sigstruct: > + kfree(sigstruct); > return ret; > } > > -- > 2.28.0.rc0.12.gb6a658bd00c9 >
[PATCH] x86: Remove unnecessary kmap() from sgx_ioc_enclave_init()
From: Ira Weiny There is no reason to alloc a page and kmap it to store this temporary data from the user. This is especially true when we are trying to remove kmap usages. Also placing the token pointer 1/2 way into the page is fragile. Replace this allocation with two kzalloc()'s which also removes the need for the memset(). Signed-off-by: Ira Weiny --- arch/x86/kernel/cpu/sgx/ioctl.c | 18 ++ 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c index 90a5caf76939..9c9019760585 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,13 +614,15 @@ static long sgx_ioc_enclave_init(struct sgx_encl *encl, void __user *arg) if (copy_from_user(_arg, arg, sizeof(init_arg))) return -EFAULT; - initp_page = alloc_page(GFP_KERNEL); - if (!initp_page) + sigstruct = kzalloc(sizeof(*sigstruct), 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); + token = kzalloc(SGX_LAUNCH_TOKEN_SIZE, GFP_KERNEL); + if (!token) { + ret = -ENOMEM; + goto free_sigstruct; + } if (copy_from_user(sigstruct, (void __user *)init_arg.sigstruct, sizeof(*sigstruct))) { @@ -645,8 +646,9 @@ 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(token); +free_sigstruct: + kfree(sigstruct); return ret; } -- 2.28.0.rc0.12.gb6a658bd00c9