linux-kernel - Re: [RFC 1/4] mm/zswap: skip swapcache for swapping in zswap pages

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAKEwX=PVbiEd9_4NAvLrHG9dr3k-25c4AB1iEagi4ZBcYRX24w@mail.gmail.com>
Date: Fri, 25 Oct 2024 11:19:15 -0700
From: Nhat Pham <nphamcs@...il.com>
To: Yosry Ahmed <yosryahmed@...gle.com>
Cc: Usama Arif <usamaarif642@...il.com>, akpm@...ux-foundation.org, linux-mm@...ck.org, 
	hannes@...xchg.org, david@...hat.com, willy@...radead.org, 
	kanchana.p.sridhar@...el.com, chengming.zhou@...ux.dev, ryan.roberts@....com, 
	ying.huang@...el.com, 21cnbao@...il.com, riel@...riel.com, 
	shakeel.butt@...ux.dev, kernel-team@...a.com, linux-kernel@...r.kernel.org, 
	linux-doc@...r.kernel.org, Kairui Song <kasong@...cent.com>, 
	Kairui Song <ryncsn@...il.com>
Subject: Re: [RFC 1/4] mm/zswap: skip swapcache for swapping in zswap pages

On Tue, Oct 22, 2024 at 5:46 PM Yosry Ahmed <yosryahmed@...gle.com> wrote:
>
> [..]
> > >> @@ -1576,6 +1576,52 @@ bool zswap_store(struct folio *folio)
> > >>         return ret;
> > >>  }
> > >>
> > >> +static bool swp_offset_in_zswap(unsigned int type, pgoff_t offset)
> > >> +{
> > >> +       return (offset >> SWAP_ADDRESS_SPACE_SHIFT) <  nr_zswap_trees[type];
> > >
> > > I am not sure I understand what we are looking for here. When does
> > > this return false? Aren't the zswap trees always allocated during
> > > swapon?
> > >
> >
> > Hi Yosry,
> >
> > Thanks for the review!
> >
> > It becomes useful in patch 3 when trying to determine if a large folio can be allocated.
> >
> > For e.g. if the swap entry is the last entry of the last tree, and 1M folios are enabled
> > (nr_pages = 256), then the while loop in zswap_present_test will try to access a tree
> > that doesn't exist from the 2nd 4K page onwards if we dont have this check in
> > zswap_present_test.
>
> Doesn't swap_pte_batch() make sure that the range of swap entries
> passed here all corresponds to existing swap entries, and those
> entries should always have a corresponding zswap tree? How can the
> passed in range contain an entry that is not in any zswap tree?
>
> I feel like I am missing something.
>
> >
> > >> +}
> > >> +
> > >> +/* Returns true if the entire folio is in zswap */
> > >
> > > There isn't really a folio at this point, maybe "Returns true if the
> > > entire range is in zswap"?
> >
> > Will change, Thanks!
> >
> > >
> > > Also, this is racy because an exclusive load, invalidation, or
> > > writeback can cause an entry to be removed from zswap. Under what
> > > conditions is this safe? The caller can probably guarantee we don't
> > > race against invalidation, but can we guarantee that concurrent
> > > exclusive loads or writebacks don't happen?
> > >
> > > If the answer is yes, this needs to be properly documented.
> >
> > swapcache_prepare should stop things from becoming racy.
> >
> > lets say trying to swapin a mTHP of size 32 pages:
> > - T1 is doing do_swap_page, T2 is doing zswap_writeback.
> > - T1 - Check if the entire 32 pages is in zswap, swapcache_prepare(entry, nr_pages) in do_swap_page is not yet called.
> > - T2 - zswap_writeback_entry starts and lets say writes page 2 to swap. it calls __read_swap_cache_async -> swapcache_prepare increments swap_map count, writes page to swap.
>
> Can the folio be dropped from the swapcache at this point (e.g. by
> reclaim)? If yes, it seems like swapcache_prepare() can succeed and
> try to read it from zswap.
>

I think you're onto something.

Can this happen: say T2 writebacks a couple of tail pages, but not all
of them, then drops everything from swap cache. Then T1 can definitely
proceed. It would then check again in zswap_load(), which returns
false (thanks to zswap_present()) test.

All fine and good so far, but then in swap_read_folio(), it would try
to fall back to reading the entire large folio from swapfile, which
will contain bogus data in pages that have not been written back by
T2.

I think the problem is swap_read_folio() assumes it always succeeds,
and a precondition for successful reading is the large folio must have
no mixed backing state for its subpages, which we fail to guarantee
before entering swap_read_folio(). This can lead to memory corruption.

Buuut, I think all we need to do is just check the backing state again
after T1's swapcache_prepare() call. At this point, we are guaranteed
to have a stable backing state. If it fails here, then we can just
exit and fall back to individual page swapping in.