Received-SPF: pass (google.com: domain of linux-kernel+bounces-205012-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Precedence: bulk
MIME-Version: 1.0
References: <20240606184818.1566920-1-yosryahmed@google.com>
 <84d78362-e75c-40c8-b6c2-56d5d5292aa7@redhat.com> <CAJD7tkZH9C21nx75W9Erun=oUvmad5ujmDyGYWRRHEwPCCizUw@mail.gmail.com>
In-Reply-To: <CAJD7tkZH9C21nx75W9Erun=oUvmad5ujmDyGYWRRHEwPCCizUw@mail.gmail.com>
From: Barry Song <21cnbao@gmail.com>
Date: Fri, 7 Jun 2024 08:54:56 +1200
Message-ID: <CAGsJ_4zTJgOgkXE=4a=w=ZsuC4WuwoBNHf0v0BwP7tatkkJZqA@mail.gmail.com>
Subject: Re: [PATCH] mm: zswap: add VM_BUG_ON() if large folio swapin is attempted
To: Yosry Ahmed <yosryahmed@google.com>
Cc: David Hildenbrand <david@redhat.com>, Andrew Morton <akpm@linux-foundation.org>, 
	Johannes Weiner <hannes@cmpxchg.org>, Nhat Pham <nphamcs@gmail.com>, 
	Chengming Zhou <chengming.zhou@linux.dev>, Baolin Wang <baolin.wang@linux.alibaba.com>, 
	Chris Li <chrisl@kernel.org>, Ryan Roberts <ryan.roberts@arm.com>, 
	Matthew Wilcox <willy@infradead.org>, linux-mm@kvack.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, Jun 7, 2024 at 8:32=E2=80=AFAM Yosry Ahmed <yosryahmed@google.com> =
wrote:
>
> On Thu, Jun 6, 2024 at 1:22=E2=80=AFPM David Hildenbrand <david@redhat.co=
m> wrote:
> >
> > On 06.06.24 20:48, Yosry Ahmed wrote:
> > > With ongoing work to support large folio swapin, it is important to m=
ake
> > > sure we do not pass large folios to zswap_load() without implementing
> > > proper support.
> > >
> > > For example, if a swapin fault observes that contiguous PTEs are
> > > pointing to contiguous swap entries and tries to swap them in as a la=
rge
> > > folio, swap_read_folio() will pass in a large folio to zswap_load(), =
but
> > > zswap_load() will only effectively load the first page in the folio. =
If
> > > the first page is not in zswap, the folio will be read from disk, eve=
n
> > > though other pages may be in zswap.
> > >
> > > In both cases, this will lead to silent data corruption.
> > >
> > > Proper large folio swapin support needs to go into zswap before zswap
> > > can be enabled in a system that supports large folio swapin.
> > >
> > > Looking at callers of swap_read_folio(), it seems like they are eithe=
r
> > > allocated from __read_swap_cache_async() or do_swap_page() in the
> > > SWP_SYNCHRONOUS_IO path. Both of which allocate order-0 folios, so we
> > > are fine for now.
> > >
> > > Add a VM_BUG_ON() in zswap_load() to make sure that we detect changes=
 in
> > > the order of those allocations without proper handling of zswap.
> > >
> > > Alternatively, swap_read_folio() (or its callers) can be updated to h=
ave
> > > a fallback mechanism that splits large folios or reads subpages
> > > separately. Similar logic may be needed anyway in case part of a larg=
e
> > > folio is already in the swapcache and the rest of it is swapped out.
> > >
> > > Signed-off-by: Yosry Ahmed <yosryahmed@google.com>

Acked-by: Barry Song <baohua@kernel.org>

this has been observed by me[1], that's why you can find the below
code in my swapin patch

+static struct folio *alloc_swap_folio(struct vm_fault *vmf)
+{
+      ...
+      /*
+      * a large folio being swapped-in could be partially in
+      * zswap and partially in swap devices, zswap doesn't
+      * support large folios yet, we might get corrupted
+      * zero-filled data by reading all subpages from swap
+      * devices while some of them are actually in zswap
+      */
+      if (is_zswap_enabled())
+           goto fallback;

[1] https://lore.kernel.org/linux-mm/20240304081348.197341-6-21cnbao@gmail.=
com/

> > > ---
> > >
> > > Sorry for the long CC list, I just found myself repeatedly looking at
> > > new series that add swap support for mTHPs / large folios, making sur=
e
> > > they do not break with zswap or make incorrect assumptions. This debu=
g
> > > check should give us some peace of mind. Hopefully this patch will al=
so
> > > raise awareness among people who are working on this.
> > >
> > > ---
> > >   mm/zswap.c | 3 +++
> > >   1 file changed, 3 insertions(+)
> > >
> > > diff --git a/mm/zswap.c b/mm/zswap.c
> > > index b9b35ef86d9be..6007252429bb2 100644
> > > --- a/mm/zswap.c
> > > +++ b/mm/zswap.c
> > > @@ -1577,6 +1577,9 @@ bool zswap_load(struct folio *folio)
> > >       if (!entry)
> > >               return false;
> > >
> > > +     /* Zswap loads do not handle large folio swapins correctly yet =
*/
> > > +     VM_BUG_ON(folio_test_large(folio));
> > > +
> >
> > There is no way we could have a WARN_ON_ONCE() and recover, right?
>
> Not without making more fundamental changes to the surrounding swap
> code. Currently zswap_load() returns either true (folio was loaded
> from zswap) or false (folio is not in zswap).
>
> To handle this correctly zswap_load() would need to tell
> swap_read_folio() which subpages are in zswap and have been loaded,
> and then swap_read_folio() would need to read the remaining subpages
> from disk. This of course assumes that the caller of swap_read_folio()
> made sure that the entire folio is swapped out and protected against
> races with other swapins.
>
> Also, because swap_read_folio() cannot split the folio itself, other
> swap_read_folio_*() functions that are called from it should be
> updated to handle swapping in tail subpages, which may be questionable
> in its own right.
>
> An alternative would be that zswap_load() (or a separate interface)
> could tell swap_read_folio() that the folio is partially in zswap,
> then we can just bail and tell the caller that it cannot read the
> large folio and that it should be split.
>
> There may be other options as well, but the bottom line is that it is
> possible, but probably not something that we want to do right now.
>
> A stronger protection method would be to introduce a config option or
> boot parameter for large folio swapin, and then make CONFIG_ZSWAP
> depend on it being disabled, or have zswap check it at boot and refuse
> to be enabled if it is on.

Thanks
Barry