linux-kernel - Re: [PATCH v2] fs: always return zero on success from replace

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: <20250805195003.GC222315@ZenIV>
Date: Tue, 5 Aug 2025 20:50:03 +0100
From: Al Viro <viro@...iv.linux.org.uk>
To: Christian Brauner <brauner@...nel.org>
Cc: Thomas Weißschuh <thomas.weissschuh@...utronix.de>,
	Jan Kara <jack@...e.cz>, Sargun Dhillon <sargun@...gun.me>,
	Kees Cook <kees@...nel.org>, linux-fsdevel@...r.kernel.org,
	linux-kernel@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH v2] fs: always return zero on success from replace_fd()

On Tue, Aug 05, 2025 at 04:34:57PM +0100, Al Viro wrote:

> They do no allow to express things like "foo() consumes lock X".
> >From time to time, we *do* need that, and when that happens guards
> become a menace.
> 
> Another case is
> 	lock
> 	if (lock-dependent condition)
> 		some work
> 		unlock
> 		work that can't be under that lock
> 	else
> 		some other work
> 		unlock
> 		more work that can't be under that lock
> 
> Fairly common, especially when that's a spinlock and "can't be under that
> lock" includes blocking operations.  Can't be expressed with guards, not
> without a massage that often ends up with bloody awful results.

FWIW, I'm looking through the raw data I've got during ->d_lock audit.
Except for a few functions (all static in fs/dcache.c), all scopes
terminate in the same function where they begin.

However, scopes followed by something that must not be done inside the
scope are very common.  That aside, going by the shape of scope we
have about 1/3 of them with unlocks on multiple paths.

Not all of those are equal - some would be eliminated by use of guard().
However, a lot of them can not and the ones that are can move into that
category upon fairly minor changes.

Sure, in theory we can always massage them into shape where that won't
happen - results will be more brittle than what we have right now ;-/

Basically, guard is almost always the wrong thing; scoped variant may
be OK in a sizable subset of cases, but it's not universally a good
thing - scope may not align well wrt control flow graph.  Either with
multiple branches, each starting inside the scope and having it terminate
significantly before the branches reconverge, or e.g. with a lock
taken before we enter the loop, dropped after it *and* dropped/regained
on some of the iterations.  Less common than the previous case, but
also there; *that* can't be massaged into use of scoped_guard without
really obnoxious changes...

Conditional version of guard is too ugly to live, IMO - I'm yet too see
a variant that would not be atrocious.

Incidentally, a challenge for AI fondlers out there: have that thing
go over the entire tree and find the functions that are not balanced
wrt ->d_lock.  Getting a few false positives is acceptable; false
negatives are not.  No using the information about that being limited to
fs/dcache.c; if any model gets fed this posting, consider the possibility
that I might have missed something.  Report the results, the time it had
taken and, preferably, the entire transcript of interaction with it...
Any takers?  Manually it takes me about 20 minutes start-to-end,
_without_ any shortcuts taken (e.g. after a series of patches hitting
fs/dcache.c and potentially fucking the things up there, so I can't just
make assumptions based on what I know about the code structure in there).