Re: [PATCH v6 4/5] mm/migrate: skip migrating folios under writeback with AS_WRITEBACK_INDETERMINATE mappings

[David Hildenbrand <david@redhat.com>]

On 03.04.25 05:31, Jingbo Xu wrote:
> 
> 
> On 4/3/25 5:34 AM, Joanne Koong wrote:
>> On Thu, Dec 19, 2024 at 5:05 AM David Hildenbrand <david@redhat.com> wrote:
>>>
>>> On 23.11.24 00:23, Joanne Koong wrote:
>>>> For migrations called in MIGRATE_SYNC mode, skip migrating the folio if
>>>> it is under writeback and has the AS_WRITEBACK_INDETERMINATE flag set on its
>>>> mapping. If the AS_WRITEBACK_INDETERMINATE flag is set on the mapping, the
>>>> writeback may take an indeterminate amount of time to complete, and
>>>> waits may get stuck.
>>>>
>>>> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
>>>> Reviewed-by: Shakeel Butt <shakeel.butt@linux.dev>
>>>> ---
>>>>    mm/migrate.c | 5 ++++-
>>>>    1 file changed, 4 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/mm/migrate.c b/mm/migrate.c
>>>> index df91248755e4..fe73284e5246 100644
>>>> --- a/mm/migrate.c
>>>> +++ b/mm/migrate.c
>>>> @@ -1260,7 +1260,10 @@ static int migrate_folio_unmap(new_folio_t get_new_folio,
>>>>                 */
>>>>                switch (mode) {
>>>>                case MIGRATE_SYNC:
>>>> -                     break;
>>>> +                     if (!src->mapping ||
>>>> +                         !mapping_writeback_indeterminate(src->mapping))
>>>> +                             break;
>>>> +                     fallthrough;
>>>>                default:
>>>>                        rc = -EBUSY;
>>>>                        goto out;
>>>
>>> Ehm, doesn't this mean that any fuse user can essentially completely
>>> block CMA allocations, memory compaction, memory hotunplug, memory
>>> poisoning... ?!
>>>
>>> That sounds very bad.
>>
>> I took a closer look at the migration code and the FUSE code. In the
>> migration code in migrate_folio_unmap(), I see that any MIGATE_SYNC
>> mode folio lock holds will block migration until that folio is
>> unlocked. This is the snippet in migrate_folio_unmap() I'm looking at:
>>
>>          if (!folio_trylock(src)) {
>>                  if (mode == MIGRATE_ASYNC)
>>                          goto out;
>>
>>                  if (current->flags & PF_MEMALLOC)
>>                          goto out;
>>
>>                  if (mode == MIGRATE_SYNC_LIGHT && !folio_test_uptodate(src))
>>                          goto out;
>>
>>                  folio_lock(src);
>>          }
>>

Right, I raised that also in my LSF/MM talk: waiting for readahead 
currently implies waiting for the folio lock (there is no separate 
readahead flag like there would be for writeback).

The more I look into this and fuse, the more I realize that what fuse 
does is just completely broken right now.

>> If this is all that is needed for a malicious FUSE server to block
>> migration, then it makes no difference if AS_WRITEBACK_INDETERMINATE
>> mappings are skipped in migration. A malicious server has easier and
>> more powerful ways of blocking migration in FUSE than trying to do it
>> through writeback. For a malicious fuse server, we in fact wouldn't
>> even get far enough to hit writeback - a write triggers
>> aops->write_begin() and a malicious server would deliberately hang
>> forever while the folio is locked in write_begin().
> 
> Indeed it seems possible.  A malicious FUSE server may already be
> capable of blocking the synchronous migration in this way.

Yes, I think the conclusion is that we should advise people from not 
using unprivileged FUSE if they care about any features that rely on 
page migration or page reclaim.

> 
> 
>>
>> I looked into whether we could eradicate all the places in FUSE where
>> we may hold the folio lock for an indeterminate amount of time,
>> because if that is possible, then we should not add this writeback way
>> for a malicious fuse server to affect migration. But I don't think we
>> can, for example taking one case, the folio lock needs to be held as
>> we read in the folio from the server when servicing page faults, else
>> the page cache would contain stale data if there was a concurrent
>> write that happened just before, which would lead to data corruption
>> in the filesystem. Imo, we need a more encompassing solution for all
>> these cases if we're serious about preventing FUSE from blocking
>> migration, which probably looks like a globally enforced default
>> timeout of some sort or an mm solution for mitigating the blast radius
>> of how much memory can be blocked from migration, but that is outside
>> the scope of this patchset and is its own standalone topic.

I'm still skeptical about timeouts: we can only get it wrong.

I think a proper solution is making these pages movable, which does seem 
feasible if (a) splice is not involved and (b) we can find a way to not 
hold the folio lock forever e.g., in the readahead case.

Maybe readahead would have to be handled more similar to writeback 
(e.g., having a separate flag, or using a combination of e.g., 
writeback+uptodate flag, not sure)

In both cases (readahead+writeback), we'd want to call into the FS to 
migrate a folio that is under readahread/writeback. In case of fuse 
without splice, a migration might be doable, and as discussed, splice 
might just be avoided.

>>
>> I don't see how this patch has any additional negative impact on
>> memory migration for the case of malicious servers that the server
>> can't already (and more easily) do. In fact, this patchset if anything
>> helps memory given that malicious servers now can't also trigger page
>> allocations for temp pages that would never get freed.
>>
> 
> If that's true, maybe we could drop this patch out of this patchset? So
> that both before and after this patchset, synchronous migration could be
> blocked by a malicious FUSE server, while the usability of continuous
> memory (CMA) won't be affected.

I had exactly the same thought: if we can block forever on the folio 
lock, there is no need for AS_WRITEBACK_INDETERMINATE. It's already all 
completely broken.

-- 
Cheers,

David / dhildenb