linux-kernel - Re: [PATCH] mm: zswap: increase reject_compress_poor but not reject_compress

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: <Zc8bjZFZAZneObQG@google.com>
Date: Fri, 16 Feb 2024 08:23:41 +0000
From: Yosry Ahmed <yosryahmed@...gle.com>
To: Barry Song <21cnbao@...il.com>
Cc: akpm@...ux-foundation.org, hannes@...xchg.org, linux-mm@...ck.org, 
	linux-kernel@...r.kernel.org, Barry Song <v-songbaohua@...o.com>, 
	Chengming Zhou <zhouchengming@...edance.com>, Nhat Pham <nphamcs@...il.com>, 
	Sergey Senozhatsky <senozhatsky@...omium.org>
Subject: Re: [PATCH] mm: zswap: increase reject_compress_poor but not
 reject_compress_fail if compression returns ENOSPC

On Fri, Feb 16, 2024 at 04:05:39PM +1300, Barry Song wrote:
> From: Barry Song <v-songbaohua@...o.com>
> 
> My commit fc8580edbaa6 ("mm: zsmalloc: return -ENOSPC rather than -EINVAL
> in zs_malloc while size is too large") wanted to depend on zs_malloc's
> returned ENOSPC to distinguish the case that compressed data is larger
> than the original data from normal compression cases. The commit, for
> sure, was correct and worked as expected but the code wouldn't run to
> there after commit 744e1885922a ("crypto: scomp - fix req->dst buffer
> overflow") as Chengming's this patch makes zswap_store() goto out
> immediately after the special compression case happens. So there is
> no chance to execute zs_malloc() now. We need to fix the count right
> after compressions return ENOSPC.
> 
> Fixes: fc8580edbaa6 ("mm: zsmalloc: return -ENOSPC rather than -EINVAL in zs_malloc while size is too large")

I don't see how this is a fix for that commit. Commit fc8580edbaa6 made
sure zsmalloc returns a correct errno when the compressed size is too
large. The fact that zswap stores were failing before calling into
zsmalloc and not reporting the error correctly in debug counters is not
that commits fault.

I think the proper fixes should be 744e1885922a if it introduced the
first scenario where -ENOSPC can be returned from scomp without handling
it properly in zswap. If -ENOSPC was a possible return value before
that, then it should be cb61dad80fdc ("zswap: export compression failure
stats"), where the counter was introduced.

> Cc: Chengming Zhou <zhouchengming@...edance.com>
> Cc: Nhat Pham <nphamcs@...il.com>
> Cc: Sergey Senozhatsky <senozhatsky@...omium.org>
> Signed-off-by: Barry Song <v-songbaohua@...o.com>
> ---
>  mm/zswap.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/zswap.c b/mm/zswap.c
> index 6319d2281020..9a21dbe8c056 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -1627,7 +1627,10 @@ bool zswap_store(struct folio *folio)
>  	dlen = acomp_ctx->req->dlen;
>  
>  	if (ret) {
> -		zswap_reject_compress_fail++;
> +		if (ret == -ENOSPC)
> +			zswap_reject_compress_poor++;
> +		else
> +			zswap_reject_compress_fail++;

With this diff, we have four locations in zswap_store() where we
increment zswap_reject_compress_{poor/fail}.

How about the following instead?A

diff --git a/mm/zswap.c b/mm/zswap.c
index 62fe307521c93..3a7e8ba7f6116 100644
--- a/mm/zswap.c
+++ b/mm/zswap.c
@@ -1059,24 +1059,16 @@ static bool zswap_compress(struct folio *folio, struct zswap_entry *entry)
 	 */
 	ret = crypto_wait_req(crypto_acomp_compress(acomp_ctx->req), &acomp_ctx->wait);
 	dlen = acomp_ctx->req->dlen;
-	if (ret) {
-		zswap_reject_compress_fail++;
+	if (ret)
 		goto unlock;
-	}

 	zpool = zswap_find_zpool(entry);
 	gfp = __GFP_NORETRY | __GFP_NOWARN | __GFP_KSWAPD_RECLAIM;
 	if (zpool_malloc_support_movable(zpool))
 		gfp |= __GFP_HIGHMEM | __GFP_MOVABLE;
 	ret = zpool_malloc(zpool, dlen, gfp, &handle);
-	if (ret == -ENOSPC) {
-		zswap_reject_compress_poor++;
-		goto unlock;
-	}
-	if (ret) {
-		zswap_reject_alloc_fail++;
+	if (ret)
 		goto unlock;
-	}

 	buf = zpool_map_handle(zpool, handle, ZPOOL_MM_WO);
 	memcpy(buf, dst, dlen);
@@ -1086,6 +1078,10 @@ static bool zswap_compress(struct folio *folio, struct zswap_entry *entry)
 	entry->length = dlen;

 unlock:
+	if (ret == -ENOSPC)
+		zswap_reject_compress_poor++;
+	else if (ret)
+		zswap_reject_alloc_fail++;
 	mutex_unlock(&acomp_ctx->mutex);
 	return ret == 0;
 }

>  		goto put_dstmem;
>  	}
>  
> -- 
> 2.34.1
>