diff mbox series

[v2] statmount: reduce runtime stack usage

Message ID 20231213090015.518044-1-arnd@kernel.org
State New
Headers
Series [v2] statmount: reduce runtime stack usage |

Commit Message

Arnd Bergmann Dec. 13, 2023, 9 a.m. UTC 
  From: Arnd Bergmann <arnd@arndb.de>

prepare_kstatmount() constructs a copy of 'struct kstatmount' on the stack
and copies it into the local variable on the stack of its caller. Because
of the size of this structure, this ends up overflowing the limit for
a single function's stack frame when prepare_kstatmount() gets inlined
and both copies are on the same frame without the compiler being able
to collapse them into one:

fs/namespace.c:4995:1: error: stack frame size (1536) exceeds limit (1024) in '__se_sys_statmount' [-Werror,-Wframe-larger-than]
 4995 | SYSCALL_DEFINE4(statmount, const struct mnt_id_req __user *, req,

Replace the assignment with an in-place memset() plus assignment that
should always be more efficient for both stack usage and runtime cost.

Fixes: 49889374ab92 ("statmount: simplify string option retrieval")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 fs/namespace.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

Comments

Christian Brauner Dec. 13, 2023, 10:15 a.m. UTC | #1 
On Wed, Dec 13, 2023 at 10:00:03AM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> prepare_kstatmount() constructs a copy of 'struct kstatmount' on the stack
> and copies it into the local variable on the stack of its caller. Because
> of the size of this structure, this ends up overflowing the limit for
> a single function's stack frame when prepare_kstatmount() gets inlined
> and both copies are on the same frame without the compiler being able
> to collapse them into one:
> 
> fs/namespace.c:4995:1: error: stack frame size (1536) exceeds limit (1024) in '__se_sys_statmount' [-Werror,-Wframe-larger-than]
>  4995 | SYSCALL_DEFINE4(statmount, const struct mnt_id_req __user *, req,
> 
> Replace the assignment with an in-place memset() plus assignment that
> should always be more efficient for both stack usage and runtime cost.
> 
> Fixes: 49889374ab92 ("statmount: simplify string option retrieval")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---

I folded this patch and placed a link here. Thanks!
Ian Kent Dec. 14, 2023, 12:11 a.m. UTC | #2 
On 13/12/23 17:00, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
>
> prepare_kstatmount() constructs a copy of 'struct kstatmount' on the stack
> and copies it into the local variable on the stack of its caller. Because
> of the size of this structure, this ends up overflowing the limit for
> a single function's stack frame when prepare_kstatmount() gets inlined
> and both copies are on the same frame without the compiler being able
> to collapse them into one:
>
> fs/namespace.c:4995:1: error: stack frame size (1536) exceeds limit (1024) in '__se_sys_statmount' [-Werror,-Wframe-larger-than]
>   4995 | SYSCALL_DEFINE4(statmount, const struct mnt_id_req __user *, req,
>
> Replace the assignment with an in-place memset() plus assignment that
> should always be more efficient for both stack usage and runtime cost.

Cunning plan, to use the work efficient instead of inefficient, ;(

But, TBH, the libc integration seems complex but I also feel there's

no choice and this looks fine too.


>
> Fixes: 49889374ab92 ("statmount: simplify string option retrieval")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>   fs/namespace.c | 15 ++++++---------
>   1 file changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/fs/namespace.c b/fs/namespace.c
> index d036196f949c..159f1df379fc 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -4957,15 +4957,12 @@ static int prepare_kstatmount(struct kstatmount *ks, struct mnt_id_req *kreq,
>   	if (!access_ok(buf, bufsize))
>   		return -EFAULT;
>   
> -	*ks = (struct kstatmount){
> -		.mask		= kreq->param,
> -		.buf		= buf,
> -		.bufsize	= bufsize,
> -		.seq = {
> -			.size	= seq_size,
> -			.buf	= kvmalloc(seq_size, GFP_KERNEL_ACCOUNT),
> -		},
> -	};
> +	memset(ks, 0, sizeof(*ks));
> +	ks->mask = kreq->param;
> +	ks->buf = buf;
> +	ks->bufsize = bufsize;
> +	ks->seq.size = seq_size;
> +	ks->seq.buf = kvmalloc(seq_size, GFP_KERNEL_ACCOUNT);
>   	if (!ks->seq.buf)
>   		return -ENOMEM;
>   	return 0;

This looks much better than what it replaces IMHO.


Reviewed-by: Ian Kent <raven@themaw.net>


Ian
diff mbox series

Patch

diff --git a/fs/namespace.c b/fs/namespace.c
index d036196f949c..159f1df379fc 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -4957,15 +4957,12 @@  static int prepare_kstatmount(struct kstatmount *ks, struct mnt_id_req *kreq,
 	if (!access_ok(buf, bufsize))
 		return -EFAULT;
 
-	*ks = (struct kstatmount){
-		.mask		= kreq->param,
-		.buf		= buf,
-		.bufsize	= bufsize,
-		.seq = {
-			.size	= seq_size,
-			.buf	= kvmalloc(seq_size, GFP_KERNEL_ACCOUNT),
-		},
-	};
+	memset(ks, 0, sizeof(*ks));
+	ks->mask = kreq->param;
+	ks->buf = buf;
+	ks->bufsize = bufsize;
+	ks->seq.size = seq_size;
+	ks->seq.buf = kvmalloc(seq_size, GFP_KERNEL_ACCOUNT);
 	if (!ks->seq.buf)
 		return -ENOMEM;
 	return 0;