Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
Feedback-ID: i31e841b0:Fastmail
Message-ID: <6608665d-a255-7b1d-653f-1d13d2b3fa2e@themaw.net>
Date:   Thu, 14 Dec 2023 08:11:32 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Subject: Re: [PATCH] [v2] statmount: reduce runtime stack usage
To:     Arnd Bergmann <arnd@kernel.org>,
        Alexander Viro <viro@zeniv.linux.org.uk>,
        Christian Brauner <brauner@kernel.org>
Cc:     Arnd Bergmann <arnd@arndb.de>, Jan Kara <jack@suse.cz>,
        Miklos Szeredi <mszeredi@redhat.com>,
        "Seth Forshee (DigitalOcean)" <sforshee@kernel.org>,
        Dave Chinner <dchinner@redhat.com>,
        Amir Goldstein <amir73il@gmail.com>,
        linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20231213090015.518044-1-arnd@kernel.org>
Content-Language: en-US
From:   Ian Kent <raven@themaw.net>
In-Reply-To: <20231213090015.518044-1-arnd@kernel.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


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