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] [day] [month] [year] [list]

Message-ID: <CACePvbWGPApYr7G29FzbmWzRw-BJE39WH7kUHSaHs+Lnw8=-qQ@mail.gmail.com>
Date: Tue, 26 Aug 2025 08:52:35 -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

Hi SeongJae,

I did another pass review on it. This time with the editor so I saw
more source code context and had more feedback.
Mostly just nitpicks. See the detailed comments below.

On Fri, Aug 22, 2025 at 12:08 PM SeongJae Park <sj@...nel.org> wrote:
> @@ -971,8 +975,26 @@ static bool zswap_compress(struct page *page, struct zswap_entry *entry,
>          */
>         comp_ret = crypto_wait_req(crypto_acomp_compress(acomp_ctx->req), &acomp_ctx->wait);
>         dlen = acomp_ctx->req->dlen;
> -       if (comp_ret)
> -               goto unlock;
> +
> +       /*
> +        * If a page cannot be compressed into a size smaller than PAGE_SIZE,
> +        * save the content as is without a compression, to keep the LRU order
> +        * of writebacks.  If writeback is disabled, reject the page since it
> +        * only adds metadata overhead.  swap_writeout() will put the page back
> +        * to the active LRU list in the case.
> +        */
> +       if (comp_ret || !dlen)
> +               dlen = PAGE_SIZE;
> +       if (dlen >= PAGE_SIZE) {

I think these two if can be simplify as:

if (comp_ret || !dlen || dlen >= PAGE_SIZE) {
              dlen = PAGE_SIZE;

then you do the following check.
That way when goto unlock happens, you have dlen == PAGE_SIZE related
to my other feedback in kunmap_local()

> +               if (!mem_cgroup_zswap_writeback_enabled(
> +                                       folio_memcg(page_folio(page)))) {
> +                       comp_ret = comp_ret ? comp_ret : -EINVAL;
> +                       goto unlock;
> +               }
> +               comp_ret = 0;
> +               dlen = PAGE_SIZE;

Delete this line if you use the above suggestion on: dlen = PAGE_SIZE;

> +               dst = kmap_local_page(page);
> +       }
>
>         zpool = pool->zpool;
>         gfp = GFP_NOWAIT | __GFP_NORETRY | __GFP_HIGHMEM | __GFP_MOVABLE;
> @@ -985,6 +1007,8 @@ static bool zswap_compress(struct page *page, struct zswap_entry *entry,
>         entry->length = dlen;
>
>  unlock:
> +       if (dst != acomp_ctx->buffer)
> +               kunmap_local(dst);

I think this has a hidden assumption that kmap_local_page() will
return a different value than acomp_ctx->buffer. That might be true. I
haven't checked the internals. Otherwise you are missing a
kunmap_local(). It also looks a bit strange in the sense that
kumap_local() should be paired with kmap_local_page() in the same
condition. The same condition is not obvious here. How about this to
make it more obvious and get rid of that assumption above:

if (dlen == PAGE_SIZE)
               kunmap_local(dst);

That assumes you also take my suggestion above to assign dlen =
PAGE_SIZE earlier.


>         if (comp_ret == -ENOSPC || alloc_ret == -ENOSPC)
>                 zswap_reject_compress_poor++;
>         else if (comp_ret)
> @@ -1007,6 +1031,14 @@ static bool zswap_decompress(struct zswap_entry *entry, struct folio *folio)
>         acomp_ctx = acomp_ctx_get_cpu_lock(entry->pool);
>         obj = zpool_obj_read_begin(zpool, entry->handle, acomp_ctx->buffer);
>
> +       /* zswap entries of length PAGE_SIZE are not compressed. */
> +       if (entry->length == PAGE_SIZE) {
> +               memcpy_to_folio(folio, 0, obj, entry->length);

The following read_end() followed by acomp unlock() duplicates the
normal decompress ending sequence. It will create complications when
we modify the normal ending sequence in the future, we need to match
it here.How about just goto the ending sequence and share the same
return path as normal:

 +                  goto read_done;

Then insert the read_done label at ending sequence:

        dlen = acomp_ctx->req->dlen;

+ read_done:
        zpool_obj_read_end(zpool, entry->handle, obj);
        acomp_ctx_put_unlock(acomp_ctx);

If you adopt that, you also will need to init the comp_ret to "0"
instead of no init value in the beginning of the function:

        struct crypto_acomp_ctx *acomp_ctx;
-        int decomp_ret, dlen;
+       int decomp_ret = 0, dlen;
        u8 *src, *obj;


> +               zpool_obj_read_end(zpool, entry->handle, obj);
> +               acomp_ctx_put_unlock(acomp_ctx);
> +               return true;

Delete the above 3 lines inside uncompress if case.

Looks good otherwise.

Thanks for the good work.

Chris