Received-SPF: pass (google.com: domain of linux-kernel+bounces-55548-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: <20240206180855.3987204-1-nphamcs@gmail.com> <20240206185145.GA97483@cmpxchg.org>
In-Reply-To: <20240206185145.GA97483@cmpxchg.org>
From: Nhat Pham <nphamcs@gmail.com>
Date: Tue, 6 Feb 2024 11:15:01 -0800
Message-ID: <CAKEwX=Ok0CNjbvdhb6MYHKhgdfxMvOExNd=FtsTdrhfXbgrv7w@mail.gmail.com>
Subject: Re: [PATCH v2] mm/swap_state: update zswap LRU's protection range
 with the folio locked
To: Johannes Weiner <hannes@cmpxchg.org>
Cc: akpm@linux-foundation.org, chengming.zhou@linux.dev, yosryahmed@google.com, 
	linux-mm@kvack.org, kernel-team@meta.com, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Tue, Feb 6, 2024 at 10:51=E2=80=AFAM Johannes Weiner <hannes@cmpxchg.org=
> wrote:
>
> On Tue, Feb 06, 2024 at 10:08:55AM -0800, Nhat Pham wrote:
> > When a folio is swapped in, the protection size of the corresponding
> > zswap LRU is incremented, so that the zswap shrinker is more
> > conservative with its reclaiming action. This field is embedded within
> > the struct lruvec, so updating it requires looking up the folio's memcg
> > and lruvec. However, currently this lookup can happen after the folio i=
s
> > unlocked, for instance if a new folio is allocated, and
> > swap_read_folio() unlocks the folio before returning. In this scenario,
> > there is no stability guarantee for the binding between a folio and its
> > memcg and lruvec:
> >
> > * A folio's memcg and lruvec can be freed between the lookup and the
> >   update, leading to a UAF.
> > * Folio migration can clear the now-unlocked folio's memcg_data, which
> >   directs the zswap LRU protection size update towards the root memcg
> >   instead of the original memcg. This was recently picked up by the
> >   syzbot thanks to a warning in the inlined folio_lruvec() call.
> >
> > Move the zswap LRU protection range update above the swap_read_folio()
> > call, and only when a new page is allocated, to prevent this.
> >
> > Reported-by: syzbot+17a611d10af7d18a7092@syzkaller.appspotmail.com
> > Closes: https://lore.kernel.org/all/000000000000ae47f90610803260@google=
com/
> > Fixes: b5ba474f3f51 ("zswap: shrink zswap pool based on memory pressure=
")
> > Signed-off-by: Nhat Pham <nphamcs@gmail.com>
>
> Looks great, thanks for updating it!
>
> One more thing I just realized:
>
> > ---
> >  mm/swap_state.c | 10 ++++++----
> >  mm/zswap.c      |  1 +
> >  2 files changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/mm/swap_state.c b/mm/swap_state.c
> > index e671266ad772..7255c01a1e4e 100644
> > --- a/mm/swap_state.c
> > +++ b/mm/swap_state.c
> > @@ -680,9 +680,10 @@ struct folio *swap_cluster_readahead(swp_entry_t e=
ntry, gfp_t gfp_mask,
> >       /* The page was likely read above, so no need for plugging here *=
/
> >       folio =3D __read_swap_cache_async(entry, gfp_mask, mpol, ilx,
> >                                       &page_allocated, false);
> > -     if (unlikely(page_allocated))
> > +     if (unlikely(page_allocated)) {
> > +             zswap_folio_swapin(folio);
> >               swap_read_folio(folio, false, NULL);
> > -     zswap_folio_swapin(folio);
> > +     }
> >       return folio;
> >  }
> >
> > @@ -855,9 +856,10 @@ static struct folio *swap_vma_readahead(swp_entry_=
t targ_entry, gfp_t gfp_mask,
> >       /* The folio was likely read above, so no need for plugging here =
*/
> >       folio =3D __read_swap_cache_async(targ_entry, gfp_mask, mpol, tar=
g_ilx,
> >                                       &page_allocated, false);
> > -     if (unlikely(page_allocated))
> > +     if (unlikely(page_allocated)) {
> > +             zswap_folio_swapin(folio);
> >               swap_read_folio(folio, false, NULL);
> > -     zswap_folio_swapin(folio);
> > +     }
> >       return folio;
> >  }
> >
> > diff --git a/mm/zswap.c b/mm/zswap.c
> > index 4aea03285532..8c548f73d52e 100644
> > --- a/mm/zswap.c
> > +++ b/mm/zswap.c
> > @@ -827,6 +827,7 @@ void zswap_folio_swapin(struct folio *folio)
> >       struct lruvec *lruvec;
> >
> >       if (folio) {
> > +             VM_WARN_ON_ONCE(!folio_test_locked(folio));
> >               lruvec =3D folio_lruvec(folio);
> >               atomic_long_inc(&lruvec->zswap_lruvec_state.nr_zswap_prot=
ected);
> >       }
>
> The NULL check is now also no longer necessary.
>
> It used to be called unconditionally, even if
> __read_swap_cache_async() failed and returned NULL.
>
> However, page_allocated =3D=3D true implies success. That newly allocated
> and locked folio is always returned.

Ah yeah, I forgot the context of that :) Just sent a fixlet to do away
with the check. Thanks for picking that out!