Re: [PATCH] [v2] statmount: reduce runtime stack usage

[Ian Kent]

On 13/12/23 17:00, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@xxxxxxxx>
>
> 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@xxxxxxxx>
> ---
>   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@xxxxxxxxxx>


Ian