linux-kernel - Re: [PATCH] statmount: reduce runtime stack usage

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <833f0dd5-ded8-4925-9c3c-639728000598@app.fastmail.com>
Date:   Wed, 13 Dec 2023 08:40:01 +0100
From:   "Arnd Bergmann" <arnd@...db.de>
To:     "Ian Kent" <raven@...maw.net>, "Arnd Bergmann" <arnd@...nel.org>,
        "Alexander Viro" <viro@...iv.linux.org.uk>,
        "Christian Brauner" <brauner@...nel.org>
Cc:     "Jan Kara" <jack@...e.cz>, "Miklos Szeredi" <mszeredi@...hat.com>,
        "Seth Forshee (DigitalOcean)" <sforshee@...nel.org>,
        "Dave Chinner" <dchinner@...hat.com>,
        "Amir Goldstein" <amir73il@...il.com>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] statmount: reduce runtime stack usage

On Wed, Dec 13, 2023, at 02:13, Ian Kent wrote:
> On 13/12/23 05:48, Arnd Bergmann wrote:
>> From: Arnd Bergmann <arnd@...db.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;