Received-SPF: pass (google.com: domain of linux-kernel+bounces-110833-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240321-zswap-fill-v1-1-b6180dbf7c27@kernel.org> <CAJD7tkY8os3yvYLiotaiRuYa1jdEGiPHQsZEU6E52zRBQ34kQQ@mail.gmail.com>
In-Reply-To: <CAJD7tkY8os3yvYLiotaiRuYa1jdEGiPHQsZEU6E52zRBQ34kQQ@mail.gmail.com>
From: Chris Li <chrisl@kernel.org>
Date: Thu, 21 Mar 2024 17:41:23 -0700
Message-ID: <CANeU7QmHs-8K7-6yoJS0ccfvP2SMNsSoYNZ8CnFep8isS8h=JQ@mail.gmail.com>
Subject: Re: [PATCH] zswap: initialize entry->pool on same filled entry
To: Yosry Ahmed <yosryahmed@google.com>
Cc: Andrew Morton <akpm@linux-foundation.org>, linux-kernel@vger.kernel.org, 
	linux-mm@kvack.org, Nhat Pham <nphamcs@gmail.com>, 
	Johannes Weiner <hannes@cmpxchg.org>, Chengming Zhou <zhouchengming@bytedance.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Mar 21, 2024 at 4:56=E2=80=AFPM Yosry Ahmed <yosryahmed@google.com>=
 wrote:
>
> On Thu, Mar 21, 2024 at 4:53=E2=80=AFPM Chris Li <chrisl@kernel.org> wrot=
e:
> >
> > Current zswap will leave the entry->pool uninitialized if
> > the page is same  filled. The entry->pool pointer can
> > contain data written by previous usage.
> >
> > Initialize entry->pool to zero for the same filled zswap entry.
> >
> > Signed-off-by: Chris Li <chrisl@kernel.org>
> > ---
> > Per Yosry's suggestion to split out this clean up
> > from the zxwap rb tree to xarray patch.
> >
> > https://lore.kernel.org/all/ZemDuW25YxjqAjm-@google.com/
> > ---
> >  mm/zswap.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/mm/zswap.c b/mm/zswap.c
> > index b31c977f53e9..f04a75a36236 100644
> > --- a/mm/zswap.c
> > +++ b/mm/zswap.c
> > @@ -1527,6 +1527,7 @@ bool zswap_store(struct folio *folio)
> >                         kunmap_local(src);
> >                         entry->length =3D 0;
> >                         entry->value =3D value;
> > +                       entry->pool =3D 0;
>
> This should be NULL.
>
> That being said, I am working on a series that should make non-filled
> entries not use a zswap_entry at all. So I think this cleanup is
> unnecessary, especially that it is documented in the definition of
> struct zswap_entry that entry->pool is invalid for same-filled
> entries.

It does not really hurt to initialize it. It is obviously correct if
we initialize it as well. One thing to consider is that, this pointer
can contain user space data if the page previously was map to user
space. Kdump typically doesn't save user space data. This
uninitialized value might let kdump contain user space data.

Chris

>
> >                         atomic_inc(&zswap_same_filled_pages);
> >                         goto insert_entry;
> >                 }
> >
> > ---
> > base-commit: a824831a082f1d8f9b51a4c0598e633d38555fcf
> > change-id: 20240315-zswap-fill-f65f44574760
> >
> > Best regards,
> > --
> > Chris Li <chrisl@kernel.org>
> >