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

[Joanne Koong <joannelkoong@gmail.com>]

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);
        }

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().

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 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.


Thanks,
Joanne

>
> --
> Cheers,
>
> David / dhildenb
>