Re: Known and unfixed active data loss bug in MM + XFS with large folios since Dec 2021 (any kernel from 6.1 upwards)

[Matthew Wilcox <willy@infradead.org>]

On Fri, Sep 20, 2024 at 03:54:55PM +0200, Chris Mason wrote:
> On 9/19/24 12:36 AM, Matthew Wilcox wrote:
> > My brain is still mushy, but I think there is still a problem (both with
> > the simple fix for 6.9 and indeed with 6.10).
> > 
> > For splitting a folio, we have the folio locked, so we know it's not
> > going anywhere.  The tree may get rearranged around it while we don't
> > have the xa_lock, but we're somewhat protected.
> > 
> > In this case we're splitting something that was, at one point, a shadow
> > entry.  There's no struct there to lock.  So I think we can have a
> > situation where we replicate 'old' (in 6.10) or xa_load() (in 6.9)
> > into the nodes we allocate in xas_split_alloc().  In 6.10, that's at
> > least guaranteed to be a shadow entry, but in 6.9, it might already be a
> > folio by this point because we've raced with something else also doing a
> > split.
> > 
> > Probably xas_split_alloc() needs to just do the alloc, like the name
> > says, and drop the 'entry' argument.  ICBW, but I think it explains
> > what you're seeing?  Maybe it doesn't?
> 
> Jens and I went through a lot of iterations making the repro more
> reliable, and we were able to pretty consistently show a UAF with
> the debug code that Willy suggested:
> 
> XA_NODE_BUG_ON(xas->xa_alloc, memchr_inv(&xas->xa_alloc->slots, 0, sizeof(void *) * XA_CHUNK_SIZE));
> 
> But, I didn't really catch what Willy was saying about xas_split_alloc()
> until this morning.
> 
> xas_split_alloc() does the allocation and also shoves an entry into some of
> the slots.  When the tree changes, the entry we've stored is wildly 
> wrong, but xas_reset() doesn't undo any of that.  So when we actually
> use the xas->xa_alloc nodes we've setup, they are pointing to the
> wrong things.
> 
> Which is probably why the commits in 6.10 added this:
> 
> /* entry may have changed before we re-acquire the lock */
> if (alloced_order && (old != alloced_shadow || order != alloced_order)) {
> 	xas_destroy(&xas);
>         alloced_order = 0;
> }
> 
> The only way to undo the work done by xas_split_alloc() is to call
> xas_destroy().

I hadn't fully understood this until today.  Here's what the code in 6.9
did (grossly simplified):

        do {
                unsigned int order = xa_get_order(xas.xa, xas.xa_index);
                if (order > folio_order(folio))
                        xas_split_alloc(&xas, xa_load(xas.xa, xas.xa_index),
                                        order, gfp);
                xas_lock_irq(&xas);
                if (old) {
                        order = xa_get_order(xas.xa, xas.xa_index);
                        if (order > folio_order(folio)) {
                                xas_split(&xas, old, order);
                        }
                }
                xas_store(&xas, folio);
                xas_unlock_irq(&xas);
        } while (xas_nomem(&xas, gfp));

The intent was that xas_store() would use the node allocated by
xas_nomem() and xas_split() would use the nodes allocated by
xas_split_alloc().  That doesn't end up happening if the split already
happened before getting the lock.  So if we were looking for a minimal
fix for pre-6.10, calling xas_destroy if we don't call xas_split()
would fix the problem.  But I think we're better off backporting the
6.10 patches.

For 6.12, I'm going to put this in -next:

http://git.infradead.org/?p=users/willy/xarray.git;a=commitdiff;h=6684aba0780da9f505c202f27e68ee6d18c0aa66

and then send it to Linus in a couple of weeks as an "obviously correct"
bit of hardening.  We really should have called xas_reset() before
retaking the lock.

Beyond that, I really want to revisit how, when and what we split.
A few months ago we came to the realisation that splitting order-9
folios to 512 order-0 folios was just legacy thinking.  What each user
really wants is to specify a precise page and say "I want this page to
end up in a folio that is of order N" (where N is smaller than the order
of the folio that it's currently in).  That is, if we truncate a file
which is currently a multiple of 2MB in size to one which has a tail of,
say, 13377ea bytes, we'd want to create a 1MB folio which we leave at
the end of the file, then a 512kB folio which we free, then a 256kB
folio which we keep, a 128kB folio which we discard, a 64kB folio which
we discard, ...

So we need to do that first, then all this code becomes way easier and
xas_split_alloc() no longer needs to fill in the node at the wrong time.