[REGRESSION][BISECTED][PATCH 0/1] v6.16 panic/hang in zswap

[REGRESSION][BISECTED][PATCH 0/1] v6.16 panic/hang in zswap

[Dan Moulding]

Hello crypto folks,

For some time now I've been battling a system hang that has been
sporadically affecting some of my machines. It seemed to be introduced
sometime during the v6.16 rc releases, but I couldn't quite pin it
down because it was reproducible (though not very easily) in some of
the versions I built, but not in others. The problem seemed to come
and go. At one point I thought I had bisected it to a netfilter fix,
but that ended up being wrong and led nowhere[1], and I was about
ready to give up on it. Then just yesterday one of the machines that
had been sporadically encountering the issue finally produced a panic
instead of just hanging. The panic indicated the problem was in swapd
in the LZ4 compression code (all of my machines that have been
affected use zswap).

Armed with the knowledge that it's a swap problem, I tried to find an
easier and more reliable method to reproduce the problem to try to
better bisect it. I found that I can use the stress-ng[2] tool's page
swapping stressor to immediately and reliably reproduce the
panic. Then, on another affected machine, I ran the page swapping
stressor and also immediately reproduced the hang (so I was fairly
confident that the hang and panic were both caused by the same
regression).

Then I tried bisecting the problem and immediately hit a new snag,
which was that suddenly on a new build of v6.16.0 I couldn't reproduce
the problem, even though I'd reproduced it on that version
before. Eventually I began to suspect that structure layout
randomization was causing the non-reproducibility (and was the reason
my previous attempt to bisect it failed). Using the randstruct.seed
file from one of the known bad kernel builds I had, I was able to
confirm that randstruct does indeed affect the issue. Now with a
"known bad" randstruct.seed I was able to successfully bisect it to
commit 42d9f6c77479 ("crypto: acomp - Move scomp stream allocation
code into acomp"). Instead of just reverting that commit, I tried to
understand why this change would be affected by randstruct, and I
believe I found the problem. Two related structs require the same
ordering of a couple of fields but one of the structs is new and would
be automatically randomized by randstruct (because it only contains
function pointers). I put together the following patch which resolves
the issue. It works by making the two related structs use a shared
struct which can be randomized and still ensure both of the structs
end up with the same layout.

-- Dan

[1] https://lore.kernel.org/regressions/20250731194901.7156-1-dan@danm.net/
[2] https://github.com/ColinIanKing/stress-ng

#regzbot introduced: 42d9f6c77479

Dan Moulding (1):
  crypto: acomp: Use shared struct for context alloc and free ops

 crypto/acompress.c                  |  6 +++---
 crypto/lz4.c                        |  6 ++++--
 include/crypto/internal/acompress.h | 10 +++++++---
 include/crypto/internal/scompress.h |  5 +----
 4 files changed, 15 insertions(+), 12 deletions(-)

-- 
2.49.1

[PATCH 1/1] crypto: acomp: Use shared struct for context alloc and free ops

[Dan Moulding]

In commit 42d9f6c77479 ("crypto: acomp - Move scomp stream allocation
code into acomp"), the crypto_acomp_streams struct was made to rely on
having the alloc_ctx and free_ctx operations defined in the same order
as the scomp_alg struct. But in that same commit, the alloc_ctx and
free_ctx members of scomp_alg may be randomized by structure layout
randomization, since they are contained in a pure ops structure
(containing only function pointers). If the pointers within scomp_alg
are randomized, but those in crypto_acomp_streams are not, then
the order may no longer match. This fixes the problem by defining a
shared structure that both crypto_acomp_streams and scomp_alg share:
acomp_ctx_ops. This new pure ops structure may now be randomized,
while still allowing both crypto_acomp_streams and scomp_alg to have
matching layout.

Signed-off-by: Dan Moulding <dan@danm.net>
Fixes: 42d9f6c77479 ("crypto: acomp - Move scomp stream allocation code into acomp")
---
 crypto/acompress.c                  |  6 +++---
 crypto/lz4.c                        |  6 ++++--
 include/crypto/internal/acompress.h | 10 +++++++---
 include/crypto/internal/scompress.h |  5 +----
 4 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/crypto/acompress.c b/crypto/acompress.c
index be28cbfd22e3..ff910035ee42 100644
--- a/crypto/acompress.c
+++ b/crypto/acompress.c
@@ -375,7 +375,7 @@ static void acomp_stream_workfn(struct work_struct *work)
 		if (ps->ctx)
 			continue;
 
-		ctx = s->alloc_ctx();
+		ctx = s->ctx_ops.alloc_ctx();
 		if (IS_ERR(ctx))
 			break;
 
@@ -398,7 +398,7 @@ void crypto_acomp_free_streams(struct crypto_acomp_streams *s)
 		return;
 
 	cancel_work_sync(&s->stream_work);
-	free_ctx = s->free_ctx;
+	free_ctx = s->ctx_ops.free_ctx;
 
 	for_each_possible_cpu(i) {
 		struct crypto_acomp_stream *ps = per_cpu_ptr(streams, i);
@@ -427,7 +427,7 @@ int crypto_acomp_alloc_streams(struct crypto_acomp_streams *s)
 	if (!streams)
 		return -ENOMEM;
 
-	ctx = s->alloc_ctx();
+	ctx = s->ctx_ops.alloc_ctx();
 	if (IS_ERR(ctx)) {
 		free_percpu(streams);
 		return PTR_ERR(ctx);
diff --git a/crypto/lz4.c b/crypto/lz4.c
index 7a984ae5ae52..c16c0d936708 100644
--- a/crypto/lz4.c
+++ b/crypto/lz4.c
@@ -68,8 +68,10 @@ static int lz4_sdecompress(struct crypto_scomp *tfm, const u8 *src,
 }
 
 static struct scomp_alg scomp = {
-	.alloc_ctx		= lz4_alloc_ctx,
-	.free_ctx		= lz4_free_ctx,
+	.ctx_ops                = {
+		.alloc_ctx	= lz4_alloc_ctx,
+		.free_ctx       = lz4_free_ctx,
+	},
 	.compress		= lz4_scompress,
 	.decompress		= lz4_sdecompress,
 	.base			= {
diff --git a/include/crypto/internal/acompress.h b/include/crypto/internal/acompress.h
index 2d97440028ff..c84a17ac26ca 100644
--- a/include/crypto/internal/acompress.h
+++ b/include/crypto/internal/acompress.h
@@ -55,15 +55,19 @@ struct acomp_alg {
 	};
 };
 
+struct acomp_ctx_ops {
+	void *(*alloc_ctx)(void);
+	void (*free_ctx)(void *);
+};
+
 struct crypto_acomp_stream {
 	spinlock_t lock;
 	void *ctx;
 };
 
 struct crypto_acomp_streams {
-	/* These must come first because of struct scomp_alg. */
-	void *(*alloc_ctx)(void);
-	void (*free_ctx)(void *);
+	/* This must come first because of struct scomp_alg. */
+	struct acomp_ctx_ops ctx_ops;
 
 	struct crypto_acomp_stream __percpu *streams;
 	struct work_struct stream_work;
diff --git a/include/crypto/internal/scompress.h b/include/crypto/internal/scompress.h
index 533d6c16a491..1d807a15aef2 100644
--- a/include/crypto/internal/scompress.h
+++ b/include/crypto/internal/scompress.h
@@ -35,10 +35,7 @@ struct scomp_alg {
 			  void *ctx);
 
 	union {
-		struct {
-			void *(*alloc_ctx)(void);
-			void (*free_ctx)(void *ctx);
-		};
+		struct acomp_ctx_ops ctx_ops;
 		struct crypto_acomp_streams streams;
 	};
 
-- 
2.49.1

[PATCH v2] crypto: acomp: Use shared struct for context alloc and free ops

[Dan Moulding]

In commit 42d9f6c77479 ("crypto: acomp - Move scomp stream allocation
code into acomp"), the crypto_acomp_streams struct was made to rely on
having the alloc_ctx and free_ctx operations defined in the same order
as the scomp_alg struct. But in that same commit, the alloc_ctx and
free_ctx members of scomp_alg may be randomized by structure layout
randomization, since they are contained in a pure ops structure
(containing only function pointers). If the pointers within scomp_alg
are randomized, but those in crypto_acomp_streams are not, then
the order may no longer match. This fixes the problem by defining a
shared structure that both crypto_acomp_streams and scomp_alg share:
acomp_ctx_ops. This new pure ops structure may now be randomized,
while still allowing both crypto_acomp_streams and scomp_alg to have
matching layout.

Signed-off-by: Dan Moulding <dan@danm.net>
Fixes: 42d9f6c77479 ("crypto: acomp - Move scomp stream allocation code into acomp")
---
Changes in v2:
  * Also patch all other crypto algorithms that use struct scomp_alg
    (v1 patch only patched LZ4).
  * Fix whitespace errors.

 crypto/842.c                          |  6 ++++--
 crypto/acompress.c                    |  6 +++---
 crypto/deflate.c                      |  6 ++++--
 crypto/lz4.c                          |  6 ++++--
 crypto/lz4hc.c                        |  6 ++++--
 crypto/lzo-rle.c                      |  6 ++++--
 crypto/lzo.c                          |  6 ++++--
 crypto/zstd.c                         |  6 ++++--
 drivers/crypto/nx/nx-common-powernv.c |  6 ++++--
 drivers/crypto/nx/nx-common-pseries.c |  6 ++++--
 include/crypto/internal/acompress.h   | 10 +++++++---
 include/crypto/internal/scompress.h   |  5 +----
 12 files changed, 47 insertions(+), 28 deletions(-)

diff --git a/crypto/842.c b/crypto/842.c
index 8c257c40e2b9..e0fa2de0cb88 100644
--- a/crypto/842.c
+++ b/crypto/842.c
@@ -54,8 +54,10 @@ static int crypto842_sdecompress(struct crypto_scomp *tfm,
 }
 
 static struct scomp_alg scomp = {
-	.alloc_ctx		= crypto842_alloc_ctx,
-	.free_ctx		= crypto842_free_ctx,
+	.ctx_ops		= {
+		.alloc_ctx	= crypto842_alloc_ctx,
+		.free_ctx	= crypto842_free_ctx,
+	},
 	.compress		= crypto842_scompress,
 	.decompress		= crypto842_sdecompress,
 	.base			= {
diff --git a/crypto/acompress.c b/crypto/acompress.c
index be28cbfd22e3..ff910035ee42 100644
--- a/crypto/acompress.c
+++ b/crypto/acompress.c
@@ -375,7 +375,7 @@ static void acomp_stream_workfn(struct work_struct *work)
 		if (ps->ctx)
 			continue;
 
-		ctx = s->alloc_ctx();
+		ctx = s->ctx_ops.alloc_ctx();
 		if (IS_ERR(ctx))
 			break;
 
@@ -398,7 +398,7 @@ void crypto_acomp_free_streams(struct crypto_acomp_streams *s)
 		return;
 
 	cancel_work_sync(&s->stream_work);
-	free_ctx = s->free_ctx;
+	free_ctx = s->ctx_ops.free_ctx;
 
 	for_each_possible_cpu(i) {
 		struct crypto_acomp_stream *ps = per_cpu_ptr(streams, i);
@@ -427,7 +427,7 @@ int crypto_acomp_alloc_streams(struct crypto_acomp_streams *s)
 	if (!streams)
 		return -ENOMEM;
 
-	ctx = s->alloc_ctx();
+	ctx = s->ctx_ops.alloc_ctx();
 	if (IS_ERR(ctx)) {
 		free_percpu(streams);
 		return PTR_ERR(ctx);
diff --git a/crypto/deflate.c b/crypto/deflate.c
index 21404515dc77..5ea6e857871f 100644
--- a/crypto/deflate.c
+++ b/crypto/deflate.c
@@ -54,8 +54,10 @@ static void deflate_free_stream(void *ctx)
 }
 
 static struct crypto_acomp_streams deflate_streams = {
-	.alloc_ctx = deflate_alloc_stream,
-	.free_ctx = deflate_free_stream,
+	.ctx_ops		= {
+		.alloc_ctx	= deflate_alloc_stream,
+		.free_ctx	= deflate_free_stream,
+	},
 };
 
 static int deflate_compress_one(struct acomp_req *req,
diff --git a/crypto/lz4.c b/crypto/lz4.c
index 7a984ae5ae52..f7eb0702e175 100644
--- a/crypto/lz4.c
+++ b/crypto/lz4.c
@@ -68,8 +68,10 @@ static int lz4_sdecompress(struct crypto_scomp *tfm, const u8 *src,
 }
 
 static struct scomp_alg scomp = {
-	.alloc_ctx		= lz4_alloc_ctx,
-	.free_ctx		= lz4_free_ctx,
+	.ctx_ops		= {
+		.alloc_ctx	= lz4_alloc_ctx,
+		.free_ctx	= lz4_free_ctx,
+	},
 	.compress		= lz4_scompress,
 	.decompress		= lz4_sdecompress,
 	.base			= {
diff --git a/crypto/lz4hc.c b/crypto/lz4hc.c
index 9c61d05b6214..e6ab1f35b6cb 100644
--- a/crypto/lz4hc.c
+++ b/crypto/lz4hc.c
@@ -66,8 +66,10 @@ static int lz4hc_sdecompress(struct crypto_scomp *tfm, const u8 *src,
 }
 
 static struct scomp_alg scomp = {
-	.alloc_ctx		= lz4hc_alloc_ctx,
-	.free_ctx		= lz4hc_free_ctx,
+	.ctx_ops		= {
+		.alloc_ctx	= lz4hc_alloc_ctx,
+		.free_ctx	= lz4hc_free_ctx,
+	},
 	.compress		= lz4hc_scompress,
 	.decompress		= lz4hc_sdecompress,
 	.base			= {
diff --git a/crypto/lzo-rle.c b/crypto/lzo-rle.c
index ba013f2d5090..48352c213937 100644
--- a/crypto/lzo-rle.c
+++ b/crypto/lzo-rle.c
@@ -70,8 +70,10 @@ static int lzorle_sdecompress(struct crypto_scomp *tfm, const u8 *src,
 }
 
 static struct scomp_alg scomp = {
-	.alloc_ctx		= lzorle_alloc_ctx,
-	.free_ctx		= lzorle_free_ctx,
+	.ctx_ops		= {
+		.alloc_ctx	= lzorle_alloc_ctx,
+		.free_ctx	= lzorle_free_ctx,
+	},
 	.compress		= lzorle_scompress,
 	.decompress		= lzorle_sdecompress,
 	.base			= {
diff --git a/crypto/lzo.c b/crypto/lzo.c
index 7867e2c67c4e..9d53aca2491e 100644
--- a/crypto/lzo.c
+++ b/crypto/lzo.c
@@ -70,8 +70,10 @@ static int lzo_sdecompress(struct crypto_scomp *tfm, const u8 *src,
 }
 
 static struct scomp_alg scomp = {
-	.alloc_ctx		= lzo_alloc_ctx,
-	.free_ctx		= lzo_free_ctx,
+	.ctx_ops		= {
+		.alloc_ctx	= lzo_alloc_ctx,
+		.free_ctx	= lzo_free_ctx,
+	},
 	.compress		= lzo_scompress,
 	.decompress		= lzo_sdecompress,
 	.base			= {
diff --git a/crypto/zstd.c b/crypto/zstd.c
index 7570e11b4ee6..057a1f5f93cb 100644
--- a/crypto/zstd.c
+++ b/crypto/zstd.c
@@ -175,8 +175,10 @@ static int zstd_sdecompress(struct crypto_scomp *tfm, const u8 *src,
 }
 
 static struct scomp_alg scomp = {
-	.alloc_ctx		= zstd_alloc_ctx,
-	.free_ctx		= zstd_free_ctx,
+	.ctx_ops		= {
+		.alloc_ctx	= zstd_alloc_ctx,
+		.free_ctx	= zstd_free_ctx,
+	},
 	.compress		= zstd_scompress,
 	.decompress		= zstd_sdecompress,
 	.base			= {
diff --git a/drivers/crypto/nx/nx-common-powernv.c b/drivers/crypto/nx/nx-common-powernv.c
index fd0a98b2fb1b..5f7aff7d43e6 100644
--- a/drivers/crypto/nx/nx-common-powernv.c
+++ b/drivers/crypto/nx/nx-common-powernv.c
@@ -1043,8 +1043,10 @@ static struct scomp_alg nx842_powernv_alg = {
 	.base.cra_priority	= 300,
 	.base.cra_module	= THIS_MODULE,
 
-	.alloc_ctx		= nx842_powernv_crypto_alloc_ctx,
-	.free_ctx		= nx842_crypto_free_ctx,
+	.ctx_ops		= {
+		.alloc_ctx	= nx842_powernv_crypto_alloc_ctx,
+		.free_ctx	= nx842_crypto_free_ctx,
+	},
 	.compress		= nx842_crypto_compress,
 	.decompress		= nx842_crypto_decompress,
 };
diff --git a/drivers/crypto/nx/nx-common-pseries.c b/drivers/crypto/nx/nx-common-pseries.c
index f528e072494a..221dbb9e6b48 100644
--- a/drivers/crypto/nx/nx-common-pseries.c
+++ b/drivers/crypto/nx/nx-common-pseries.c
@@ -1020,8 +1020,10 @@ static struct scomp_alg nx842_pseries_alg = {
 	.base.cra_priority	= 300,
 	.base.cra_module	= THIS_MODULE,
 
-	.alloc_ctx		= nx842_pseries_crypto_alloc_ctx,
-	.free_ctx		= nx842_crypto_free_ctx,
+	.ctx_ops		= {
+		.alloc_ctx	= nx842_pseries_crypto_alloc_ctx,
+		.free_ctx	= nx842_crypto_free_ctx,
+	},
 	.compress		= nx842_crypto_compress,
 	.decompress		= nx842_crypto_decompress,
 };
diff --git a/include/crypto/internal/acompress.h b/include/crypto/internal/acompress.h
index 2d97440028ff..c84a17ac26ca 100644
--- a/include/crypto/internal/acompress.h
+++ b/include/crypto/internal/acompress.h
@@ -55,15 +55,19 @@ struct acomp_alg {
 	};
 };
 
+struct acomp_ctx_ops {
+	void *(*alloc_ctx)(void);
+	void (*free_ctx)(void *);
+};
+
 struct crypto_acomp_stream {
 	spinlock_t lock;
 	void *ctx;
 };
 
 struct crypto_acomp_streams {
-	/* These must come first because of struct scomp_alg. */
-	void *(*alloc_ctx)(void);
-	void (*free_ctx)(void *);
+	/* This must come first because of struct scomp_alg. */
+	struct acomp_ctx_ops ctx_ops;
 
 	struct crypto_acomp_stream __percpu *streams;
 	struct work_struct stream_work;
diff --git a/include/crypto/internal/scompress.h b/include/crypto/internal/scompress.h
index 533d6c16a491..1d807a15aef2 100644
--- a/include/crypto/internal/scompress.h
+++ b/include/crypto/internal/scompress.h
@@ -35,10 +35,7 @@ struct scomp_alg {
 			  void *ctx);
 
 	union {
-		struct {
-			void *(*alloc_ctx)(void);
-			void (*free_ctx)(void *ctx);
-		};
+		struct acomp_ctx_ops ctx_ops;
 		struct crypto_acomp_streams streams;
 	};
 
-- 
2.49.1