On 9/19/24 12:36 AM, Matthew Wilcox wrote:
> On Wed, Sep 18, 2024 at 09:38:41PM -0600, Jens Axboe wrote:
>> On 9/18/24 9:12 PM, Linus Torvalds wrote:
>>> On Thu, 19 Sept 2024 at 05:03, Linus Torvalds
>>> <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
>>>>
>>>> I think we should just do the simple one-liner of adding a
>>>> "xas_reset()" to after doing xas_split_alloc() (or do it inside the
>>>> xas_split_alloc()).
>>>
>>> .. and obviously that should be actually *verified* to fix the issue
>>> not just with the test-case that Chris and Jens have been using, but
>>> on Christian's real PostgreSQL load.
>>>
>>> Christian?
>>>
>>> Note that the xas_reset() needs to be done after the check for errors
>>> - or like Willy suggested, xas_split_alloc() needs to be re-organized.
>>>
>>> So the simplest fix is probably to just add a
>>>
>>> if (xas_error(&xas))
>>> goto error;
>>> }
>>> + xas_reset(&xas);
>>> xas_lock_irq(&xas);
>>> xas_for_each_conflict(&xas, entry) {
>>> old = entry;
>>>
>>> in __filemap_add_folio() in mm/filemap.c
>>>
>>> (The above is obviously a whitespace-damaged pseudo-patch for the
>>> pre-6758c1128ceb state. I don't actually carry a stable tree around on
>>> my laptop, but I hope it's clear enough what I'm rambling about)
>>
>> I kicked off a quick run with this on 6.9 with my debug patch as well,
>> and it still fails for me... I'll double check everything is sane. For
>> reference, below is the 6.9 filemap patch.
>>
>> diff --git a/mm/filemap.c b/mm/filemap.c
>> index 30de18c4fd28..88093e2b7256 100644
>> --- a/mm/filemap.c
>> +++ b/mm/filemap.c
>> @@ -883,6 +883,7 @@ noinline int __filemap_add_folio(struct address_space *mapping,
>> if (order > folio_order(folio))
>> xas_split_alloc(&xas, xa_load(xas.xa, xas.xa_index),
>> order, gfp);
>> + xas_reset(&xas);
>> xas_lock_irq(&xas);
>> xas_for_each_conflict(&xas, entry) {
>> old = entry;
>
> 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().
To prove this theory, I tried making a minimal version that also
called destroy, but it all ended up less minimal than the code
that's actually in 6.10. I've got a long test going now with
an extra cond_resched() to make the race bigger, and a printk of victory.
It hasn't fired yet, and I need to hop on an airplane, so I'll just leave
it running for now. But long story short, I think we should probably
just tag all of these for stable:
https://lore.kernel.org/all/20240415171857.19244-2-ryncsn@xxxxxxxxx/T/#mdb85922624c39ea7efb775a044af4731890ff776
Also, Willy's proposed changes to xas_split_alloc() seem like a good
idea.
-chris
