linux-kernel - Re: [PATCH v2] vfs: shave work on failed file open

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]

Message-ID: <20230929-kerzen-fachjargon-ca17177e9eeb@brauner>
Date:   Fri, 29 Sep 2023 11:20:48 +0200
From:   Christian Brauner <brauner@...nel.org>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Jann Horn <jannh@...gle.com>, Mateusz Guzik <mjguzik@...il.com>,
        viro@...iv.linux.org.uk, linux-kernel@...r.kernel.org,
        linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH v2] vfs: shave work on failed file open

> But yes, that protection would be broken by SLAB_TYPESAFE_BY_RCU,
> since then the "f_count is zero" is no longer a final thing.

I've tried coming up with a patch that is simple enough so the pattern
is easy to follow and then converting all places to rely on a pattern
that combine lookup_fd_rcu() or similar with get_file_rcu(). The obvious
thing is that we'll force a few places to now always acquire a reference
when they don't really need one right now and that already may cause
performance issues.

We also can't fully get rid of plain get_file_rcu() uses itself because
of users such as mm->exe_file. They don't go from one of the rcu fdtable
lookup helpers to the struct file obviously. They rcu replace the file
pointer in their struct ofc so we could change get_file_rcu() to take a
struct file __rcu **f and then comparing that the passed in pointer
hasn't changed before we managed to do atomic_long_inc_not_zero(). Which
afaict should work for such cases.

But overall we would introduce a fairly big and at the same time subtle
semantic change. The idea is pretty neat and it was fun to do but I'm
just not convinced we should do it given how ubiquitous struct file is
used and now to make the semanics even more special by allowing
refcounts.

I've kept your original release_empty_file() proposal in vfs.misc which
I think is a really nice change.

Let me know if you all passionately disagree. ;)