| 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
| ||
|
Message-Id: <20250827161832.164192-1-sj@kernel.org>
Date: Wed, 27 Aug 2025 09:18:32 -0700
From: SeongJae Park <sj@...nel.org>
To: Chris Li <chrisl@...nel.org>
Cc: SeongJae Park <sj@...nel.org>,
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 Tue, 26 Aug 2025 08:52:35 -0700 Chris Li <chrisl@...nel.org> wrote:
> 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.
Thank you for your review!
>
> 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;
Thank you for nice suggestion!
>
> > + 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.
Good point, I agree.
> How about this to
> make it more obvious and get rid of that assumption above:
>
> if (dlen = PAGE_SIZE)
> kunmap_local(dst);
However, if the execution reached here because compression failed and writeback
was disabled, kmap_local_page() wouldn't be called, so we could try to unmap
unmapped memory.
What do you think about adding another bool vairable for recording if
kunmap_local() need to be executed? For example,
"""
--- 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;
acomp_ctx = acomp_ctx_get_cpu_lock(pool);
dst = acomp_ctx->buffer;
@@ -994,6 +995,7 @@ static bool zswap_compress(struct page *page, struct zswap_entry *entry,
comp_ret = 0;
dlen = PAGE_SIZE;
dst = kmap_local_page(page);
+ dst_need_unmap = true;
}
zpool = pool->zpool;
@@ -1007,7 +1009,7 @@ static bool zswap_compress(struct page *page, struct zswap_entry *entry,
entry->length = dlen;
unlock:
- if (dst != acomp_ctx->buffer)
+ if (dst_need_unmap)
kunmap_local(dst);
if (comp_ret == -ENOSPC || alloc_ret == -ENOSPC)
zswap_reject_compress_poor++;
"""
>
> 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);
I agree your concern and this looks good to me :)
>
> 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;
We may also need to initialize 'dlen' as PAGE_SIZE ?
>
>
> > + 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.
Thank you for your kind review and nice suggestions! Since the change is
simple, I will post a fixup patch as reply to this, for adopting your
suggestions with my additional changes (adding dst_need_unmap bool variable on
zswap_compress(), and initializing dlen on zswap_decompress()) if you have no
objection or different suggestions for the my addition of the changes. Please
let me know if you have any concern or other suggestions for my suggested
additional changes.
Thanks,
SJ
>
> Chris
>
Powered by blists - more mailing lists