linux-kernel - Re: [PATCH] fs: use __fput

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] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 8 Aug 2023 19:10:56 +0200
From:   Mateusz Guzik <mjguzik@...il.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     "Eric W. Biederman" <ebiederm@...ssion.com>,
        viro@...iv.linux.org.uk, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org, oleg@...hat.com,
        Matthew Wilcox <willy@...radead.org>,
        Christian Brauner <brauner@...nel.org>
Subject: Re: [PATCH] fs: use __fput_sync in close(2)

On 8/8/23, Linus Torvalds <torvalds@...ux-foundation.org> wrote:
> That said, I detest Mateusz' patch. I hate these kinds of "do
> different things based on flags" interfaces. Particularly when it
> spreads out like this.

I agree it is kind of crap, I needed something to test the impact.

> But even if we want to do this - and I have absolutely no objections
> to it conceptually as per above - we need to be a lot more surgical
> about it, and not pass stupid flags around.
>
> Here's a TOTALLY UNTESTED(!) patch that I think effectively does what
> Mateusz wants done, but does it all within just fs/open.c and only for
> the obvious context of the close() system call itself.
>
> All it needs is to just split out the "flush" part from filp_close(),
> and we already had all the other infrastructure for this operation.
>

Few hours ago I sent another version which very closely resembles what
you did :)
2 main differences:
- i somehow missed close_fd_get_file so I hacked my own based on close_fd
- you need to whack the kthread assert in __fput_sync

> Mateusz, two questions:
>
>  (a) does this patch work for you?
>

Well I'm not *testing* patch right now, but it does work for me in the
sense of taking care of the problem (modulo whatever fixups,
definitely the assert thing)

>  (b) do you have numbers for this all?
>

I'm offended you ask, it's all in my opening e-mail.

Copy pasting again from the commit message:
[orig]
fs: use __fput_sync in close(2)

close(2) is a special close which guarantees shallow kernel stack,
making delegation to task_work machinery unnecessary. Said delegation is
problematic as it involves atomic ops and interrupt masking trips, none
of which are cheap on x86-64. Forcing close(2) to do it looks like an
oversight in the original work.

Moreover presence of CONFIG_RSEQ adds an additional overhead as fput()
-> task_work_add(..., TWA_RESUME) -> set_notify_resume() makes the
thread returning to userspace land in resume_user_mode_work(), where
rseq_handle_notify_resume takes a SMAP round-trip if rseq is enabled for
the thread (and it is by default with contemporary glibc).

Sample result when benchmarking open1_processes -t 1 from will-it-scale
(that's a open + close loop) + tmpfs on /tmp, running on the Sapphire
Rapid CPU (ops/s):
stock+RSEQ:	1329857
stock-RSEQ:	1421667	(+7%)
patched:	1523521 (+14.5% / +7%) (with / without rseq)

Patched result is the same as it dodges rseq.
[/orig]

Since modulo fixups your patch clearly elides all of this, I don't
think there is a need to re-bench (but I can do it if you ship me a
version you consider committable just to be on the safe side)

-- 
Mateusz Guzik <mjguzik gmail.com>