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 10:23:30 +0200
From:   Mateusz Guzik <mjguzik@...il.com>
To:     Christian Brauner <brauner@...nel.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>,
        Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [PATCH] fs: use __fput_sync in close(2)

On 8/8/23, Christian Brauner <brauner@...nel.org> wrote:
> It adds two new exports of filp_close_sync() and close_fd_sync() without
> any users. That's not something we do and we also shouldn't encourage
> random drivers to switch to sync behavior.
>

They don't need to be exported for the patch to work.

> That rseq thing is completely orthogonal and maybe that needs to be
> fixed and you can go and convince the glibc people to do it.
>

This is not a glibc problem, but rseq problem -- it should not be
lumped into any case which uses task_work_add.

> And making filp_close() fully sync again is also really not great.

The patch is not doing it.

> Simplicity wins out and if all codepaths share the same behavior we're
> better off then having parts use task work and other parts just not.
>

The difference is not particularly complicated.

> Yes, we just did re-added the f_pos optimization because it may have had
> an impact. And that makes more sense because that was something we had
> changed just a few days/weeks before.
>

I don't think perf tax on something becomes more sensible the longer
it is there.

> But this is over 10 year old behavior and this micro benchmarking isn't
> a strong selling point imho. We could make close(2) go sync, sure. But
> I'm skeptical even about this without real-world data or from a proper
> testsuite.
>

I responded to this in my mail to Eric.

> (There's also a stray sysctl_fput_sync there which is scary to think that
> we'd ever allow a sysctl that allows userspace to control how we close
> fds.)
>

This is a leftover from my tests -- I added a runtime switch so can I
flip back and forth, most definitely not something I would expect to
be shipped.

-- 
Mateusz Guzik <mjguzik gmail.com>