linux-kernel - Re: fallocate INSERT_RANGE/COLLAPSE_RANGE is completely broken [PATCH]

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: <20160329060410.GA2383@kmo-pixel>
Date:	Mon, 28 Mar 2016 22:04:10 -0800
From:	Kent Overstreet <kent.overstreet@...il.com>
To:	Dave Chinner <david@...morbit.com>
Cc:	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	Al Viro <viro@...iv.linux.org.uk>,
	Rik van Riel <riel@...hat.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Theodore Ts'o <tytso@....edu>
Subject: Re: fallocate INSERT_RANGE/COLLAPSE_RANGE is completely broken
 [PATCH]

On Tue, Mar 29, 2016 at 04:15:58PM +1100, Dave Chinner wrote:
> On Mon, Mar 28, 2016 at 08:25:46PM -0800, Kent Overstreet wrote:
> > Bit of previous discussion:
> > http://thread.gmane.org/gmane.linux.file-systems/101201/
> > 
> > The underlying issue is that we have no mechanism for invalidating a range of
> > the pagecache and then _keeping it invalidated_ while we Do Stuff. 
> > 
> > The fallocate INSERT_RANGE/COLLAPSE_RANGE situation seems likely to be worse
> > than I initially thought. I've been digging into this in the course of bcachefs
> > testing - I was hitting assertions that meant state hanging off the page cache
> > (in this case, allocation information, i.e. whether we needed to reserve space
> > on write) was inconsistent with the btree in writepages().
> > 
> > Well, bcachefs isn't the only filesystem that hangs additional state off the
> > pagecache, and the situation today is that an unpriviliged user can cause
> > inconsistencies there by just doing buffered reads concurrently with
> > INSERT_RANGE/COLLAPSE_RANGE. I highly highly doubt this is an issue of just
> > "oops, you corrupted your file because you were doing stupid stuff" - who knows
> > what internal invariants are getting broken here, and I don't particularly care
> > to find out.
> 
> I'd like to see a test case for this. Concurrent IO and/or page
> faults should not run at the same as fallocate on XFS. Hence I'd
> like to see the test cases that demonstrate buffered reads are
> causing corruption during insert/collapse range operations. We use
> the same locking strategy for fallocate as we use for truncate and
> all the other internal extent manipulation operations, so if there's
> something wrong, we need to fix it.

It's entirely possible I'm wrong about XFS - your fault path locking looked
correct, and I did see you had extra locking in your buffered read path but I
thought it was a different lock. I'll recheck later, but for the moment I'm just
going to assume I misspoke (and tbh always found xfs's locking to be quite
rigorous).

ext4 uses the generic code in all the places you're hooking into though -
.fault, .read_iter, etc.

The scheme I've got in this patch should perform quite a bit better than what
you're doing - only locking in the slow cache miss path, vs. every time you
touch the page cache.