[PATCH] statmount: reduce runtime stack usage

[PATCH] statmount: reduce runtime stack usage

[Arnd Bergmann]

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,

Mark the inner function as noinline_for_stack so the second copy is
freed before calling do_statmount() enters filesystem specific code.
The extra copy of the structure is a bit inefficient, but this
system call should not be performance critical.

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

diff --git a/fs/namespace.c b/fs/namespace.c
index d036196f949c..e22fb5c4a9bb 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -4950,7 +4950,8 @@ static inline bool retry_statmount(const long ret, size_t *seq_size)
 	return true;
 }
 
-static int prepare_kstatmount(struct kstatmount *ks, struct mnt_id_req *kreq,
+static int noinline_for_stack
+prepare_kstatmount(struct kstatmount *ks, struct mnt_id_req *kreq,
 			      struct statmount __user *buf, size_t bufsize,
 			      size_t seq_size)
 {
-- 
2.39.2

Re: [PATCH] statmount: reduce runtime stack usage

[Ian Kent]

On 13/12/23 05:48, 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,
>
> Mark the inner function as noinline_for_stack so the second copy is
> freed before calling do_statmount() enters filesystem specific code.
> The extra copy of the structure is a bit inefficient, but this
> system call should not be performance critical.

Are you sure this is not performance sensitive, or is the performance

critical comment not related to the system call being called many times?


It's going to be a while (if ever) before callers change there ways.


Consider what happens when a bunch of mounts are being mounted.


First there are a lot of events and making the getting of mount info.

more efficient means more of those events get processed (itself an issue

that's going to need notification sub-system improvement) resulting in

the system call being called even more.


There are 3 or 4 common programs that monitor the mounts, systemd is

one of those, it usually has 3 processes concurrently listening for

mount table events and every one of these processes grabs the entire

table. Thing is systemd is actually quite good at handling events and

can process a lot of them if they are being occuring.


So this system call will be called a lot.


Ian

>
> Fixes: 49889374ab92 ("statmount: simplify string option retrieval")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>   fs/namespace.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/fs/namespace.c b/fs/namespace.c
> index d036196f949c..e22fb5c4a9bb 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -4950,7 +4950,8 @@ static inline bool retry_statmount(const long ret, size_t *seq_size)
>   	return true;
>   }
>   
> -static int prepare_kstatmount(struct kstatmount *ks, struct mnt_id_req *kreq,
> +static int noinline_for_stack
> +prepare_kstatmount(struct kstatmount *ks, struct mnt_id_req *kreq,
>   			      struct statmount __user *buf, size_t bufsize,
>   			      size_t seq_size)
>   {

Re: [PATCH] statmount: reduce runtime stack usage

[Arnd Bergmann]

On Wed, Dec 13, 2023, at 02:13, Ian Kent wrote:
> On 13/12/23 05:48, 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,
>>
>> Mark the inner function as noinline_for_stack so the second copy is
>> freed before calling do_statmount() enters filesystem specific code.
>> The extra copy of the structure is a bit inefficient, but this
>> system call should not be performance critical.
>
> Are you sure this is not performance sensitive, or is the performance
> critical comment not related to the system call being called many times?
>
>
> It's going to be a while (if ever) before callers change there ways.
>
> Consider what happens when a bunch of mounts are being mounted.
>
>
> First there are a lot of events and making the getting of mount info.
> more efficient means more of those events get processed (itself an issue
> that's going to need notification sub-system improvement) resulting in
> the system call being called even more.

Ok, I'll send a v2 that is more efficent. I expected it to look uglier,
but I don't think it's actually that bad:

--- 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;