linux-kernel - Re: [PATCH v5] mm/zswap: store <PAGE_SIZE compression failed page as-is

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: <CACePvbXnaWZh61aH=BHoGDqbKvBSE52Me+PpE-WMXcGpRy0FFw@mail.gmail.com>
Date: Wed, 27 Aug 2025 14:16:37 -0700
From: Chris Li <chrisl@...nel.org>
To: SeongJae Park <sj@...nel.org>
Cc: Andrew Morton <akpm@...ux-foundation.org>, Chengming Zhou <chengming.zhou@...ux.dev>, 
	Herbert Xu <herbert@...dor.apana.org.au>, Johannes Weiner <hannes@...xchg.org>, 
	Nhat Pham <nphamcs@...il.com>, Yosry Ahmed <yosry.ahmed@...ux.dev>, kernel-team@...a.com, 
	linux-kernel@...r.kernel.org, linux-mm@...ck.org, 
	Takero Funaki <flintglass@...il.com>, David Hildenbrand <david@...hat.com>, Baoquan He <bhe@...hat.com>, 
	Barry Song <baohua@...nel.org>, Kairui Song <kasong@...cent.com>
Subject: Re: [PATCH v5] mm/zswap: store <PAGE_SIZE compression failed page as-is

On Wed, Aug 27, 2025 at 10:45 AM SeongJae Park <sj@...nel.org> wrote:
> > > >
> > > > On Fri, Aug 22, 2025 at 12:08 PM SeongJae Park <sj@...nel.org> wrote:
> [...]
> > > """
> > > --- a/mm/zswap.c
> > > +++ b/mm/zswap.c
> > > @@ -952,6 +952,7 @@ static bool zswap_compress(struct page *page, struct zswap_entry *entry,
> > >         struct zpool *zpool;
> > >         gfp_t gfp;
> > >         u8 *dst;
> > > +       bool dst_need_unmap = false;
> >
> > A bit nitpicky. That variable name is too long as a C local variable.
> > We want local auto variables to be short and sweet. That is why you
> > have "dst" rather than  "u8 *destination_compressed_buffer;"
> > The local variable name is too long and it can hurt the reading as well.
> > Can we have something shorter? e.g. "mapped" or "has_map"
>
> I agree your points, and thank you for suggestions.  I will take "mapped".

Thank you.

> > > We may also need to initialize 'dlen' as PAGE_SIZE ?
> >
> > If there is a code path can lead to dlen use not initialized value? If
> > not then we don't have to assign it.
>
> The success return path of zswap_decompress() checks dlen together with
> decomp_ret as below.  So I think we need to initialize dlen, too.  Please let
> me know if I'm missing something.

I normally don't worry about those, the compiler will complain if I
get it wrong. The compiler might have false positives, but should not
have false negatives because the compiler can see all the incoming
edges of the basic block. It is a trivial graph reachability problem
if we allow false positives reports.

Anyway, I just opened the editor to check again. Yes, if we goto the
read_done, the if condition using dlen can introduce a new incoming
edge that has len uninitialized value to be used. Yes, need to
initialize dlen == PAGE_SIZE or you initialize it at the memcpy of
page.

> I will post the fixup by tomorrow morning (Pacific time) unless I
> hear other opinions or find my mistakes on the above plan by tonight.

You are too humble, that is the normal reviewing process. You can take
your time.

Chris