[PATCH 1/3] zram: fix zstd dict use-after-free on per-CPU error path

[PATCH 1/3] zram: fix zstd dict use-after-free on per-CPU error path

[Haoqin Huang]

From: Haoqin Huang <haoqinhuang@tencent.com>

zstd_setup_params() creates global cdict and ddict stored in
params->drv_data, shared across all per-CPU contexts. When a
per-CPU zstd_create() failed, its error path called
zstd_release_params() which freed those shared objects while
other per-CPU contexts might already hold references to them.

Remove the premature zstd_release_params() from the per-CPU
error path, the global cdict/ddict are properly released later
by zstd_release_params(), called from zcomp_init()'s cleanup
or from zcomp_destroy().

Fixes: 6a559ecd6e7e ("zram: add dictionary support to zstd backend")
Signed-off-by: Haoqin Huang <haoqinhuang@tencent.com>
Reviewed-by: Rongwei Wang <zigiwang@tencent.com>
---
 drivers/block/zram/backend_zstd.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/block/zram/backend_zstd.c b/drivers/block/zram/backend_zstd.c
index d00b548056dc..2584f47c9b3c 100644
--- a/drivers/block/zram/backend_zstd.c
+++ b/drivers/block/zram/backend_zstd.c
@@ -161,7 +161,6 @@ static int zstd_create(struct zcomp_params *params, struct zcomp_ctx *ctx)
 	return 0;
 
 error:
-	zstd_release_params(params);
 	zstd_destroy(ctx);
 	return -EINVAL;
 }
-- 
2.43.7

[PATCH 2/3] zram: add per-backend capability flags and validate parameters early

[Haoqin Huang]

From: Haoqin Huang <haoqinhuang@tencent.com>

Writing dict or level parameters for algorithms that don't support
them was silently accepted but had no effect.  Out-of-range levels
were silently clamped by the underlying library.  Dict read failures
always lost the real error from kernel_read_file_from_path().

Add caps, level_min and level_max to zcomp_ops and validate
user-supplied parameters in algorithm_params_store() before storing,
giving immediate error feedback. Also fix comp_params_store() to
read the new dict into a temporary buffer before resetting old
parameters, making the update atomic.

Signed-off-by: Haoqin Huang <haoqinhuang@tencent.com>
Reviewed-by: Rongwei Wang <zigiwang@tencent.com>
---
 drivers/block/zram/backend_842.c     |  1 +
 drivers/block/zram/backend_deflate.c |  3 +++
 drivers/block/zram/backend_lz4.c     |  3 +++
 drivers/block/zram/backend_lz4hc.c   |  3 +++
 drivers/block/zram/backend_lzo.c     |  1 +
 drivers/block/zram/backend_lzorle.c  |  1 +
 drivers/block/zram/backend_zstd.c    |  3 +++
 drivers/block/zram/zcomp.c           | 23 ++++++++++++++++++++
 drivers/block/zram/zcomp.h           |  8 +++++++
 drivers/block/zram/zram_drv.c        | 32 +++++++++++++++++++++-------
 include/linux/zstd_lib.h             |  1 +
 lib/zstd/compress/clevels.h          |  1 -
 12 files changed, 71 insertions(+), 9 deletions(-)

diff --git a/drivers/block/zram/backend_842.c b/drivers/block/zram/backend_842.c
index 10d9d5c60f53..d796ebda1fa0 100644
--- a/drivers/block/zram/backend_842.c
+++ b/drivers/block/zram/backend_842.c
@@ -57,5 +57,6 @@ const struct zcomp_ops backend_842 = {
 	.destroy_ctx	= destroy_842,
 	.setup_params	= setup_params_842,
 	.release_params	= release_params_842,
+	.caps		= 0,
 	.name		= "842",
 };
diff --git a/drivers/block/zram/backend_deflate.c b/drivers/block/zram/backend_deflate.c
index f92a52a720d1..cedc3daad33a 100644
--- a/drivers/block/zram/backend_deflate.c
+++ b/drivers/block/zram/backend_deflate.c
@@ -144,5 +144,8 @@ const struct zcomp_ops backend_deflate = {
 	.destroy_ctx	= deflate_destroy,
 	.setup_params	= deflate_setup_params,
 	.release_params	= deflate_release_params,
+	.caps		= ZCOMP_CAP_LEVEL,
+	.level_min	= Z_DEFAULT_COMPRESSION,
+	.level_max	= Z_BEST_COMPRESSION,
 	.name		= "deflate",
 };
diff --git a/drivers/block/zram/backend_lz4.c b/drivers/block/zram/backend_lz4.c
index c449d511ba86..bd1e5ca4d134 100644
--- a/drivers/block/zram/backend_lz4.c
+++ b/drivers/block/zram/backend_lz4.c
@@ -146,5 +146,8 @@ const struct zcomp_ops backend_lz4 = {
 	.destroy_ctx	= lz4_destroy,
 	.setup_params	= lz4_setup_params,
 	.release_params	= lz4_release_params,
+	.caps		= ZCOMP_CAP_DICT | ZCOMP_CAP_LEVEL,
+	.level_min	= LZ4_ACCELERATION_DEFAULT,
+	.level_max	= 65535,
 	.name		= "lz4",
 };
diff --git a/drivers/block/zram/backend_lz4hc.c b/drivers/block/zram/backend_lz4hc.c
index f6a336acfe20..0e0d7c68a7d4 100644
--- a/drivers/block/zram/backend_lz4hc.c
+++ b/drivers/block/zram/backend_lz4hc.c
@@ -124,5 +124,8 @@ const struct zcomp_ops backend_lz4hc = {
 	.destroy_ctx	= lz4hc_destroy,
 	.setup_params	= lz4hc_setup_params,
 	.release_params	= lz4hc_release_params,
+	.caps		= ZCOMP_CAP_DICT | ZCOMP_CAP_LEVEL,
+	.level_min	= LZ4HC_MIN_CLEVEL,
+	.level_max	= LZ4HC_MAX_CLEVEL,
 	.name		= "lz4hc",
 };
diff --git a/drivers/block/zram/backend_lzo.c b/drivers/block/zram/backend_lzo.c
index 4c906beaae6b..965f007e2ca8 100644
--- a/drivers/block/zram/backend_lzo.c
+++ b/drivers/block/zram/backend_lzo.c
@@ -55,5 +55,6 @@ const struct zcomp_ops backend_lzo = {
 	.destroy_ctx	= lzo_destroy,
 	.setup_params	= lzo_setup_params,
 	.release_params	= lzo_release_params,
+	.caps		= 0,
 	.name		= "lzo",
 };
diff --git a/drivers/block/zram/backend_lzorle.c b/drivers/block/zram/backend_lzorle.c
index 10640c96cbfc..757b4598be03 100644
--- a/drivers/block/zram/backend_lzorle.c
+++ b/drivers/block/zram/backend_lzorle.c
@@ -55,5 +55,6 @@ const struct zcomp_ops backend_lzorle = {
 	.destroy_ctx	= lzorle_destroy,
 	.setup_params	= lzorle_setup_params,
 	.release_params	= lzorle_release_params,
+	.caps		= 0,
 	.name		= "lzo-rle",
 };
diff --git a/drivers/block/zram/backend_zstd.c b/drivers/block/zram/backend_zstd.c
index 2584f47c9b3c..0fbd2460883a 100644
--- a/drivers/block/zram/backend_zstd.c
+++ b/drivers/block/zram/backend_zstd.c
@@ -212,5 +212,8 @@ const struct zcomp_ops backend_zstd = {
 	.destroy_ctx	= zstd_destroy,
 	.setup_params	= zstd_setup_params,
 	.release_params	= zstd_release_params,
+	.caps		= ZCOMP_CAP_DICT | ZCOMP_CAP_LEVEL,
+	.level_min	= (int)-ZSTD_TARGETLENGTH_MAX,
+	.level_max	= ZSTD_MAX_CLEVEL,
 	.name		= "zstd",
 };
diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
index 974c4691887e..15de28b50d42 100644
--- a/drivers/block/zram/zcomp.c
+++ b/drivers/block/zram/zcomp.c
@@ -9,6 +9,9 @@
 #include <linux/cpuhotplug.h>
 #include <linux/vmalloc.h>
 #include <linux/sysfs.h>
+#include <linux/lz4.h>
+#include <linux/zlib.h>
+#include <linux/zstd.h>
 
 #include "zcomp.h"
 
@@ -94,6 +97,26 @@ const char *zcomp_lookup_backend_name(const char *comp)
 	return NULL;
 }
 
+unsigned int zcomp_get_caps(const char *comp)
+{
+	const struct zcomp_ops *backend = lookup_backend_ops(comp);
+
+	return backend ? backend->caps : 0;
+}
+
+int zcomp_validate_level(const char *comp, s32 level)
+{
+	const struct zcomp_ops *backend = lookup_backend_ops(comp);
+
+	if (!backend)
+		return -EINVAL;
+	if (!(backend->caps & ZCOMP_CAP_LEVEL))
+		return -EOPNOTSUPP;
+	if (level < backend->level_min || level > backend->level_max)
+		return -EINVAL;
+	return 0;
+}
+
 /* show available compressors */
 ssize_t zcomp_available_show(const char *comp, char *buf, ssize_t at)
 {
diff --git a/drivers/block/zram/zcomp.h b/drivers/block/zram/zcomp.h
index 81a0f3f6ff48..16f812a94c67 100644
--- a/drivers/block/zram/zcomp.h
+++ b/drivers/block/zram/zcomp.h
@@ -7,6 +7,9 @@
 
 #define ZCOMP_PARAM_NOT_SET	INT_MIN
 
+#define ZCOMP_CAP_DICT		BIT(0)	/* dictionary support */
+#define ZCOMP_CAP_LEVEL		BIT(1)	/* adjustable compression level */
+
 struct deflate_params {
 	s32 winbits;
 };
@@ -66,6 +69,9 @@ struct zcomp_ops {
 	int (*setup_params)(struct zcomp_params *params);
 	void (*release_params)(struct zcomp_params *params);
 
+	unsigned int caps;
+	s32 level_min;
+	s32 level_max;
 	const char *name;
 };
 
@@ -81,6 +87,8 @@ int zcomp_cpu_up_prepare(unsigned int cpu, struct hlist_node *node);
 int zcomp_cpu_dead(unsigned int cpu, struct hlist_node *node);
 ssize_t zcomp_available_show(const char *comp, char *buf, ssize_t at);
 const char *zcomp_lookup_backend_name(const char *comp);
+unsigned int zcomp_get_caps(const char *comp);
+int zcomp_validate_level(const char *comp, s32 level);
 
 struct zcomp *zcomp_create(const char *alg, struct zcomp_params *params);
 void zcomp_destroy(struct zcomp *comp);
diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index ace65c586072..a667d0672720 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -1699,21 +1699,24 @@ static int comp_params_store(struct zram *zram, u32 prio, s32 level,
 			     const char *dict_path,
 			     struct deflate_params *deflate_params)
 {
+	void *new_dict = NULL;
 	ssize_t sz = 0;
 
-	comp_params_reset(zram, prio);
-
 	if (dict_path) {
-		sz = kernel_read_file_from_path(dict_path, 0,
-						&zram->params[prio].dict,
-						INT_MAX,
-						NULL,
-						READING_POLICY);
-		if (sz < 0)
+		sz = kernel_read_file_from_path(dict_path, 0, &new_dict,
+						INT_MAX, NULL, READING_POLICY);
+		if (sz < 0) {
+			vfree(new_dict);
+			return sz;
+		} else if (sz == 0) {
+			vfree(new_dict);
 			return -EINVAL;
+		}
 	}
 
+	comp_params_reset(zram, prio);
 	zram->params[prio].dict_sz = sz;
+	zram->params[prio].dict = new_dict;
 	zram->params[prio].level = level;
 	zram->params[prio].deflate.winbits = deflate_params->winbits;
 	return 0;
@@ -1794,6 +1797,19 @@ static ssize_t algorithm_params_store(struct device *dev,
 			return -EINVAL;
 	}
 
+	if (zram->comp_algs[prio]) {
+		unsigned int caps = zcomp_get_caps(zram->comp_algs[prio]);
+
+		if (dict_path && !(caps & ZCOMP_CAP_DICT))
+			return -EOPNOTSUPP;
+
+		if (level != ZCOMP_PARAM_NOT_SET) {
+			ret = zcomp_validate_level(zram->comp_algs[prio], level);
+			if (ret)
+				return ret;
+		}
+	}
+
 	ret = comp_params_store(zram, prio, level, dict_path, &deflate_params);
 	return ret ? ret : len;
 }
diff --git a/include/linux/zstd_lib.h b/include/linux/zstd_lib.h
index e295d4125dde..f4b26844e53a 100644
--- a/include/linux/zstd_lib.h
+++ b/include/linux/zstd_lib.h
@@ -1241,6 +1241,7 @@ ZSTDLIB_API size_t ZSTD_sizeof_DDict(const ZSTD_DDict* ddict);
 #define ZSTD_SEARCHLOG_MIN        1
 #define ZSTD_MINMATCH_MAX         7   /* only for ZSTD_fast, other strategies are limited to 6 */
 #define ZSTD_MINMATCH_MIN         3   /* only for ZSTD_btopt+, faster strategies are limited to 4 */
+#define ZSTD_MAX_CLEVEL		22
 #define ZSTD_TARGETLENGTH_MAX    ZSTD_BLOCKSIZE_MAX
 #define ZSTD_TARGETLENGTH_MIN     0   /* note : comparing this constant to an unsigned results in a tautological test */
 #define ZSTD_STRATEGY_MIN        ZSTD_fast
diff --git a/lib/zstd/compress/clevels.h b/lib/zstd/compress/clevels.h
index 6ab8be6532ef..db3b275b502e 100644
--- a/lib/zstd/compress/clevels.h
+++ b/lib/zstd/compress/clevels.h
@@ -17,7 +17,6 @@
 
 /*-=====  Pre-defined compression levels  =====-*/
 
-#define ZSTD_MAX_CLEVEL     22
 
 __attribute__((__unused__))
 
-- 
2.43.7

[PATCH 3/3] zram: reset per-priority params when changing algorithm before init

[Haoqin Huang]

From: Haoqin Huang <haoqinhuang@tencent.com>

Parameters validated against one algorithm may be invalid for another
(e.g. lz4 accepts level=65535 but zstd does not).  Although algorithm
changes are blocked after disksize is set, they are allowed before
device initialization.  Reset per-priority params on algorithm change
so that stale parameters do not silently carry over.

Signed-off-by: Haoqin Huang <haoqinhuang@tencent.com>
Reviewed-by: Rongwei Wang <zigiwang@tencent.com>
---
 drivers/block/zram/zram_drv.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index a667d0672720..6b47586e8718 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -57,6 +57,7 @@ static size_t huge_class_size;
 static const struct block_device_operations zram_devops;
 
 static void slot_free(struct zram *zram, u32 index);
+static void comp_params_reset(struct zram *zram, u32 prio);
 #define slot_dep_map(zram, index) (&(zram)->table[(index)].dep_map)
 
 static void slot_lock_init(struct zram *zram, u32 index)
@@ -1681,6 +1682,7 @@ static int __comp_algorithm_store(struct zram *zram, u32 prio, const char *buf)
 	}
 
 	comp_algorithm_set(zram, prio, alg);
+	comp_params_reset(zram, prio);
 	return 0;
 }
 
-- 
2.43.7

Re: [PATCH 1/3] zram: fix zstd dict use-after-free on per-CPU error path

[Sergey Senozhatsky]

On (26/06/27 15:02), Haoqin Huang wrote:
> zstd_setup_params() creates global cdict and ddict stored in
> params->drv_data, shared across all per-CPU contexts. When a
> per-CPU zstd_create() failed, its error path called
> zstd_release_params() which freed those shared objects while
> other per-CPU contexts might already hold references to them.
> 
> Remove the premature zstd_release_params() from the per-CPU
> error path, the global cdict/ddict are properly released later
> by zstd_release_params(), called from zcomp_init()'s cleanup
> or from zcomp_destroy().
> 
> Fixes: 6a559ecd6e7e ("zram: add dictionary support to zstd backend")
> Signed-off-by: Haoqin Huang <haoqinhuang@tencent.com>
> Reviewed-by: Rongwei Wang <zigiwang@tencent.com>

Apologies for the delay, I'm catching up on emails and will
look into it in the coming days.

Re: [PATCH 1/3] zram: fix zstd dict use-after-free on per-CPU error path

[Sergey Senozhatsky]

On (26/06/27 15:02), Haoqin Huang wrote:
> zstd_setup_params() creates global cdict and ddict stored in
> params->drv_data, shared across all per-CPU contexts. When a
> per-CPU zstd_create() failed, its error path called
> zstd_release_params() which freed those shared objects while
> other per-CPU contexts might already hold references to them.

zstd_release_params() sets ->drv_data to NULL so we can free params
only once.  In addition, "while other per-CPU contexts might
already hold references to them" -- other CPUs cannot do anything
with those params, the device is not setup and we cannot handle any
IO requests, right?  There is no double-free nor UAF there as far
as I can tell.

> Remove the premature zstd_release_params() from the per-CPU
> error path, the global cdict/ddict are properly released later
> by zstd_release_params(), called from zcomp_init()'s cleanup
> or from zcomp_destroy().
> 
> Fixes: 6a559ecd6e7e ("zram: add dictionary support to zstd backend")

What exactly does this fix?

Re: [PATCH 3/3] zram: reset per-priority params when changing algorithm before init

[Sergey Senozhatsky]

On (26/06/27 15:02), Haoqin Huang wrote:
> Parameters validated against one algorithm may be invalid for another
> (e.g. lz4 accepts level=65535 but zstd does not).  Although algorithm
> changes are blocked after disksize is set, they are allowed before
> device initialization.  Reset per-priority params on algorithm change
> so that stale parameters do not silently carry over.
> 
> Signed-off-by: Haoqin Huang <haoqinhuang@tencent.com>
> Reviewed-by: Rongwei Wang <zigiwang@tencent.com>

I suppose we can do that.  99% (if not 100%) of zram setup routines
are completely automatic (systemd, mmd, .etc) but why not.

Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>

Re: [PATCH 2/3] zram: add per-backend capability flags and validate parameters early

[Sergey Senozhatsky]

On (26/06/27 15:02), Haoqin Huang wrote:
> Writing dict or level parameters for algorithms that don't support
> them was silently accepted but had no effect.  Out-of-range levels
> were silently clamped by the underlying library.  Dict read failures
> always lost the real error from kernel_read_file_from_path().
> 
> Add caps, level_min and level_max to zcomp_ops and validate
> user-supplied parameters in algorithm_params_store() before storing,
> giving immediate error feedback. Also fix comp_params_store() to
> read the new dict into a temporary buffer before resetting old
> parameters, making the update atomic.

I probably would prefer not to add this.  Again, zram setup is almost
always automated, you figure out what you need to put into your init
script once and you never touch it again.

The 0 i_size for CD-dict is something that simply should not happen.
If you insist on handling that then we can replace "sz < 0" with "sz <= 0",
but that 0 len dictionary case is something purely theoretical.

Re: [PATCH 1/3] zram: fix zstd dict use-after-free on per-CPU error path

[haoqin huang]

On Fri, Jul 24, 2026 at 1:17 PM Sergey Senozhatsky
<senozhatsky@chromium.org> wrote:
>
> On (26/06/27 15:02), Haoqin Huang wrote:
> > zstd_setup_params() creates global cdict and ddict stored in
> > params->drv_data, shared across all per-CPU contexts. When a
> > per-CPU zstd_create() failed, its error path called
> > zstd_release_params() which freed those shared objects while
> > other per-CPU contexts might already hold references to them.
>
> zstd_release_params() sets ->drv_data to NULL so we can free params
> only once.  In addition, "while other per-CPU contexts might
> already hold references to them" -- other CPUs cannot do anything
> with those params, the device is not setup and we cannot handle any
> IO requests, right?  There is no double-free nor UAF there as far
> as I can tell.
>

You're right.  On the init failure path, drv_data=NULL prevents a
double-free, no IO is ever submitted, and cpuhp_rollback_install()
properly tears down the per-CPU contexts that succeeded (which only
calls zstd_destroy(), never touching drv_data). So the system doesn't
crash or leak -- it just means the global cdict/ddict get released by
the per-CPU error path rather than by zcomp_init()'s cleanup label.
The original commit message was poorly worded, I'll fix it.

> > Remove the premature zstd_release_params() from the per-CPU
> > error path, the global cdict/ddict are properly released later
> > by zstd_release_params(), called from zcomp_init()'s cleanup
> > or from zcomp_destroy().
> >
> > Fixes: 6a559ecd6e7e ("zram: add dictionary support to zstd backend")
>
> What exactly does this fix?

It's more of a correctness / ownership fix than a crash fix.

The per-CPU zstd_create() should clean up only its own context
(zstd_destroy). The global cdict/ddict in params->drv_data are
created by zstd_setup_params() and are conceptually owned by the
compression lifecycle (zcomp_init / zcomp_destroy), not by individual
per-CPU callbacks.  Having a per-CPU error path call
zstd_release_params() is a layering violation -- it works today only
because the NULL guard happens to mask the double-release.

The one scenario where this could matter is CPU hotplug after the
device is already live: if a hot-added CPU's zstd_create() fails, the
old code would free the globally-shared cdict/ddict while worker
threads on other CPUs are actively using them. That's admittedly a
low-probability corner case, but fixing the ownership makes it go
away for free.

I'll update the commit message to reflect this more accurately.
Does that address your concern?

Re: [PATCH 2/3] zram: add per-backend capability flags and validate parameters early

[haoqin huang]

On Fri, Jul 24, 2026 at 1:28 PM Sergey Senozhatsky
<senozhatsky@chromium.org> wrote:
>
> On (26/06/27 15:02), Haoqin Huang wrote:
> > Writing dict or level parameters for algorithms that don't support
> > them was silently accepted but had no effect.  Out-of-range levels
> > were silently clamped by the underlying library.  Dict read failures
> > always lost the real error from kernel_read_file_from_path().
> >
> > Add caps, level_min and level_max to zcomp_ops and validate
> > user-supplied parameters in algorithm_params_store() before storing,
> > giving immediate error feedback. Also fix comp_params_store() to
> > read the new dict into a temporary buffer before resetting old
> > parameters, making the update atomic.
>
> I probably would prefer not to add this.  Again, zram setup is almost
> always automated, you figure out what you need to put into your init
> script once and you never touch it again.
>

That's a fair point. The sysfs validation is admittedly defensive.

My thinking was just that without any feedback, a misconfiguration can
be hard to notice. For instance:

 algo=lzo dict=/data/dict    -> setup_params() silently discards it
 algo=deflate dict=/data/dict -> same, silently ignored
 algo=zstd level=999         -> silently clamped to 22 by the library

All of these succeed without a peep -- not a crash, but also no
indication that anything was wrong.

> The 0 i_size for CD-dict is something that simply should not happen.
> If you insist on handling that then we can replace "sz < 0" with "sz <= 0",
> but that 0 len dictionary case is something purely theoretical.

That's fine, I'll use sz <= 0 in v2.

Anyway, let me split the original patch so the non-controversial part
stands on its own:

  [2/3] zram: make dict update in comp_params_store() atomic

    Move comp_params_reset() after kernel_read_file_from_path(), so an
    I/O error doesn't leave the old dict freed with nothing to replace
    it.  Also preserve the actual error code.

  [3/3] zram: add per-backend caps and validate parameters early

    Optional -- happy to drop if you still feel it's unnecessary.

Re: [PATCH 2/3] zram: add per-backend capability flags and validate parameters early

[Sergey Senozhatsky]

On (26/07/28 00:50), haoqin huang wrote:
> On Fri, Jul 24, 2026 at 1:28 PM Sergey Senozhatsky
> <senozhatsky@chromium.org> wrote:
> >
> > On (26/06/27 15:02), Haoqin Huang wrote:
> > > Writing dict or level parameters for algorithms that don't support
> > > them was silently accepted but had no effect.  Out-of-range levels
> > > were silently clamped by the underlying library.  Dict read failures
> > > always lost the real error from kernel_read_file_from_path().
> > >
> > > Add caps, level_min and level_max to zcomp_ops and validate
> > > user-supplied parameters in algorithm_params_store() before storing,
> > > giving immediate error feedback. Also fix comp_params_store() to
> > > read the new dict into a temporary buffer before resetting old
> > > parameters, making the update atomic.
> >
> > I probably would prefer not to add this.  Again, zram setup is almost
> > always automated, you figure out what you need to put into your init
> > script once and you never touch it again.
> >
> 
> That's a fair point. The sysfs validation is admittedly defensive.
> 
> My thinking was just that without any feedback, a misconfiguration can
> be hard to notice. For instance:
> 
>  algo=lzo dict=/data/dict    -> setup_params() silently discards it
>  algo=deflate dict=/data/dict -> same, silently ignored
>  algo=zstd level=999         -> silently clamped to 22 by the library
> 
> All of these succeed without a peep -- not a crash, but also no
> indication that anything was wrong.

We were thinking about moving from zcomp to crypto API acomp.  So I'm
not sure if we want to invest a lot of time into zcomp.  If you want
to have it then I'd ask to split caps and kernel_read_file_from_path()
zero i_size check into separate patches.  We also probably want some
pr_err() on failed validation, because just returning -EINVAL doesn't
explain the error and doesn't help the (supposedly) clueless user
who attempts to configure zram by throwing random numbers at it.

> > The 0 i_size for CD-dict is something that simply should not happen.
> > If you insist on handling that then we can replace "sz < 0" with "sz <= 0",
> > but that 0 len dictionary case is something purely theoretical.
> 
> That's fine, I'll use sz <= 0 in v2.
> 
> Anyway, let me split the original patch so the non-controversial part
> stands on its own:
> 
>   [2/3] zram: make dict update in comp_params_store() atomic
> 
>     Move comp_params_reset() after kernel_read_file_from_path(), so an
>     I/O error doesn't leave the old dict freed with nothing to replace
>     it.  Also preserve the actual error code.
> 
>   [3/3] zram: add per-backend caps and validate parameters early
> 
>     Optional -- happy to drop if you still feel it's unnecessary.

Sounds good.

Re: [PATCH 1/3] zram: fix zstd dict use-after-free on per-CPU error path

[Sergey Senozhatsky]

On (26/07/28 00:06), haoqin huang wrote:
> > > zstd_setup_params() creates global cdict and ddict stored in
> > > params->drv_data, shared across all per-CPU contexts. When a
> > > per-CPU zstd_create() failed, its error path called
> > > zstd_release_params() which freed those shared objects while
> > > other per-CPU contexts might already hold references to them.
> >
> > zstd_release_params() sets ->drv_data to NULL so we can free params
> > only once.  In addition, "while other per-CPU contexts might
> > already hold references to them" -- other CPUs cannot do anything
> > with those params, the device is not setup and we cannot handle any
> > IO requests, right?  There is no double-free nor UAF there as far
> > as I can tell.
> >
> 
> You're right.  On the init failure path, drv_data=NULL prevents a
> double-free, no IO is ever submitted, and cpuhp_rollback_install()
> properly tears down the per-CPU contexts that succeeded (which only
> calls zstd_destroy(), never touching drv_data). So the system doesn't
> crash or leak -- it just means the global cdict/ddict get released by
> the per-CPU error path rather than by zcomp_init()'s cleanup label.
> The original commit message was poorly worded, I'll fix it.
> 
> > > Remove the premature zstd_release_params() from the per-CPU
> > > error path, the global cdict/ddict are properly released later
> > > by zstd_release_params(), called from zcomp_init()'s cleanup
> > > or from zcomp_destroy().
> > >
> > > Fixes: 6a559ecd6e7e ("zram: add dictionary support to zstd backend")
> >
> > What exactly does this fix?
> 
> It's more of a correctness / ownership fix than a crash fix.

OK, fair. Agreed, let's rewrite commit message and then we are good
to go.

Re: [PATCH 2/3] zram: add per-backend capability flags and validate parameters early

[Sergey Senozhatsky]

On (26/06/27 15:02), Haoqin Huang wrote:
[..]

> diff --git a/include/linux/zstd_lib.h b/include/linux/zstd_lib.h
> index e295d4125dde..f4b26844e53a 100644
> --- a/include/linux/zstd_lib.h
> +++ b/include/linux/zstd_lib.h
> @@ -1241,6 +1241,7 @@ ZSTDLIB_API size_t ZSTD_sizeof_DDict(const ZSTD_DDict* ddict);
>  #define ZSTD_SEARCHLOG_MIN        1
>  #define ZSTD_MINMATCH_MAX         7   /* only for ZSTD_fast, other strategies are limited to 6 */
>  #define ZSTD_MINMATCH_MIN         3   /* only for ZSTD_btopt+, faster strategies are limited to 4 */
> +#define ZSTD_MAX_CLEVEL		22
>  #define ZSTD_TARGETLENGTH_MAX    ZSTD_BLOCKSIZE_MAX
>  #define ZSTD_TARGETLENGTH_MIN     0   /* note : comparing this constant to an unsigned results in a tautological test */
>  #define ZSTD_STRATEGY_MIN        ZSTD_fast
> diff --git a/lib/zstd/compress/clevels.h b/lib/zstd/compress/clevels.h
> index 6ab8be6532ef..db3b275b502e 100644
> --- a/lib/zstd/compress/clevels.h
> +++ b/lib/zstd/compress/clevels.h
> @@ -17,7 +17,6 @@
>  
>  /*-=====  Pre-defined compression levels  =====-*/
>  
> -#define ZSTD_MAX_CLEVEL     22
>  
>  __attribute__((__unused__))

I think the zstd part wants to be a separate patch as well.

Re: [PATCH 2/3] zram: add per-backend capability flags and validate parameters early

[haoqin huang]

On Tue, Jul 28, 2026 at 9:19 AM Sergey Senozhatsky
<senozhatsky@chromium.org> wrote:
>
> On (26/07/28 00:50), haoqin huang wrote:
> > On Fri, Jul 24, 2026 at 1:28 PM Sergey Senozhatsky
> > <senozhatsky@chromium.org> wrote:
> > >
> > > On (26/06/27 15:02), Haoqin Huang wrote:
> > > > Writing dict or level parameters for algorithms that don't support
> > > > them was silently accepted but had no effect.  Out-of-range levels
> > > > were silently clamped by the underlying library.  Dict read failures
> > > > always lost the real error from kernel_read_file_from_path().
> > > >
> > > > Add caps, level_min and level_max to zcomp_ops and validate
> > > > user-supplied parameters in algorithm_params_store() before storing,
> > > > giving immediate error feedback. Also fix comp_params_store() to
> > > > read the new dict into a temporary buffer before resetting old
> > > > parameters, making the update atomic.
> > >
> > > I probably would prefer not to add this.  Again, zram setup is almost
> > > always automated, you figure out what you need to put into your init
> > > script once and you never touch it again.
> > >
> >
> > That's a fair point. The sysfs validation is admittedly defensive.
> >
> > My thinking was just that without any feedback, a misconfiguration can
> > be hard to notice. For instance:
> >
> >  algo=lzo dict=/data/dict    -> setup_params() silently discards it
> >  algo=deflate dict=/data/dict -> same, silently ignored
> >  algo=zstd level=999         -> silently clamped to 22 by the library
> >
> > All of these succeed without a peep -- not a crash, but also no
> > indication that anything was wrong.
>
> We were thinking about moving from zcomp to crypto API acomp.  So I'm
> not sure if we want to invest a lot of time into zcomp.  If you want
> to have it then I'd ask to split caps and kernel_read_file_from_path()
> zero i_size check into separate patches.  We also probably want some
> pr_err() on failed validation, because just returning -EINVAL doesn't
> explain the error and doesn't help the (supposedly) clueless user
> who attempts to configure zram by throwing random numbers at it.
>

Sounds good.  I'll add pr_err() on validation failures in the caps
patch and post v2 with the split series.

> > > The 0 i_size for CD-dict is something that simply should not happen.
> > > If you insist on handling that then we can replace "sz < 0" with "sz <= 0",
> > > but that 0 len dictionary case is something purely theoretical.
> >
> > That's fine, I'll use sz <= 0 in v2.
> >
> > Anyway, let me split the original patch so the non-controversial part
> > stands on its own:
> >
> >   [2/3] zram: make dict update in comp_params_store() atomic
> >
> >     Move comp_params_reset() after kernel_read_file_from_path(), so an
> >     I/O error doesn't leave the old dict freed with nothing to replace
> >     it.  Also preserve the actual error code.
> >
> >   [3/3] zram: add per-backend caps and validate parameters early
> >
> >     Optional -- happy to drop if you still feel it's unnecessary.
>
> Sounds good.

Re: [PATCH 2/3] zram: add per-backend capability flags and validate parameters early

[haoqin huang]

On Tue, Jul 28, 2026 at 9:27 AM Sergey Senozhatsky
<senozhatsky@chromium.org> wrote:
>
> On (26/06/27 15:02), Haoqin Huang wrote:
> [..]
>
> > diff --git a/include/linux/zstd_lib.h b/include/linux/zstd_lib.h
> > index e295d4125dde..f4b26844e53a 100644
> > --- a/include/linux/zstd_lib.h
> > +++ b/include/linux/zstd_lib.h
> > @@ -1241,6 +1241,7 @@ ZSTDLIB_API size_t ZSTD_sizeof_DDict(const ZSTD_DDict* ddict);
> >  #define ZSTD_SEARCHLOG_MIN        1
> >  #define ZSTD_MINMATCH_MAX         7   /* only for ZSTD_fast, other strategies are limited to 6 */
> >  #define ZSTD_MINMATCH_MIN         3   /* only for ZSTD_btopt+, faster strategies are limited to 4 */
> > +#define ZSTD_MAX_CLEVEL              22
> >  #define ZSTD_TARGETLENGTH_MAX    ZSTD_BLOCKSIZE_MAX
> >  #define ZSTD_TARGETLENGTH_MIN     0   /* note : comparing this constant to an unsigned results in a tautological test */
> >  #define ZSTD_STRATEGY_MIN        ZSTD_fast
> > diff --git a/lib/zstd/compress/clevels.h b/lib/zstd/compress/clevels.h
> > index 6ab8be6532ef..db3b275b502e 100644
> > --- a/lib/zstd/compress/clevels.h
> > +++ b/lib/zstd/compress/clevels.h
> > @@ -17,7 +17,6 @@
> >
> >  /*-=====  Pre-defined compression levels  =====-*/
> >
> > -#define ZSTD_MAX_CLEVEL     22
> >
> >  __attribute__((__unused__))
>
> I think the zstd part wants to be a separate patch as well.

Agreed, I've split it into a standalone patch in v2:
  [3/5] zstd: move ZSTD_MAX_CLEVEL to zstd_lib.h

[PATCH v2 1/5] zram: fix early release of global cdict/ddict in per-CPU error path

[Haoqin Huang]

From: Haoqin Huang <haoqinhuang@tencent.com>

zstd_setup_params() creates global cdict and ddict stored in
params->drv_data, shared across all per-CPU contexts. The per-CPU
zstd_create() error path called zstd_release_params(), which freed
those globally-shared objects. While the drv_data=NULL guard in
zstd_release_params() prevents a double-free on the init failure
path, this is still a layering violation: a per-CPU callback should
only clean up its own context, not release resources owned by the
compression lifecycle (zcomp_init / zcomp_destroy).

Fix by removing zstd_release_params() from the per-CPU error path
and replacing it with only zstd_destroy(), which properly cleans
up the per-CPU context without touching the global params->drv_data.

Fixes: 6a559ecd6e7e ("zram: add dictionary support to zstd backend")
Signed-off-by: Haoqin Huang <haoqinhuang@tencent.com>
Signed-off-by: Rongwei Wang <zigiwang@tencent.com>
---
 drivers/block/zram/backend_zstd.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/block/zram/backend_zstd.c b/drivers/block/zram/backend_zstd.c
index d00b548056dc..2584f47c9b3c 100644
--- a/drivers/block/zram/backend_zstd.c
+++ b/drivers/block/zram/backend_zstd.c
@@ -161,7 +161,6 @@ static int zstd_create(struct zcomp_params *params, struct zcomp_ctx *ctx)
 	return 0;
 
 error:
-	zstd_release_params(params);
 	zstd_destroy(ctx);
 	return -EINVAL;
 }
-- 
2.43.7

[PATCH v2 2/5] zram: make dict update in comp_params_store() atomic

[Haoqin Huang]

From: Haoqin Huang <haoqinhuang@tencent.com>

comp_params_store() resets old parameters before reading a new dict,
so if kernel_read_file_from_path() fails the params are left broken
and the actual error is swallowed. Fix by reading into a temporary
buffer first, swapping only on success. Use sz <= 0 to also reject
zero-size dicts.

Signed-off-by: Haoqin Huang <haoqinhuang@tencent.com>
Signed-off-by: Rongwei Wang <zigiwang@tencent.com>
---
 drivers/block/zram/zram_drv.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index ace65c586072..9ea7ba9d1ed0 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -1699,21 +1699,23 @@ static int comp_params_store(struct zram *zram, u32 prio, s32 level,
 			     const char *dict_path,
 			     struct deflate_params *deflate_params)
 {
+	void *new_dict = NULL;
 	ssize_t sz = 0;
 
-	comp_params_reset(zram, prio);
-
 	if (dict_path) {
-		sz = kernel_read_file_from_path(dict_path, 0,
-						&zram->params[prio].dict,
-						INT_MAX,
-						NULL,
-						READING_POLICY);
-		if (sz < 0)
-			return -EINVAL;
+		sz = kernel_read_file_from_path(dict_path, 0, &new_dict,
+						INT_MAX, NULL, READING_POLICY);
+		if (sz <= 0) {
+			vfree(new_dict);
+			if (sz == 0)
+				return -EINVAL;
+			return sz;
+		}
 	}
 
+	comp_params_reset(zram, prio);
 	zram->params[prio].dict_sz = sz;
+	zram->params[prio].dict = new_dict;
 	zram->params[prio].level = level;
 	zram->params[prio].deflate.winbits = deflate_params->winbits;
 	return 0;
-- 
2.43.7

[PATCH v2 3/5] zstd: move ZSTD_MAX_CLEVEL to zstd_lib.h

[Haoqin Huang]

From: Haoqin Huang <haoqinhuang@tencent.com>

Move ZSTD_MAX_CLEVEL from lib/zstd/compress/clevels.h to
include/linux/zstd_lib.h so that external users (e.g. the zram
zstd backend) can reference it alongside ZSTD_TARGETLENGTH_MAX
to define a valid level range.

Signed-off-by: Haoqin Huang <haoqinhuang@tencent.com>
Signed-off-by: Rongwei Wang <zigiwang@tencent.com>
---
 include/linux/zstd_lib.h    | 1 +
 lib/zstd/compress/clevels.h | 2 --
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/include/linux/zstd_lib.h b/include/linux/zstd_lib.h
index e295d4125dde..f4b26844e53a 100644
--- a/include/linux/zstd_lib.h
+++ b/include/linux/zstd_lib.h
@@ -1241,6 +1241,7 @@ ZSTDLIB_API size_t ZSTD_sizeof_DDict(const ZSTD_DDict* ddict);
 #define ZSTD_SEARCHLOG_MIN        1
 #define ZSTD_MINMATCH_MAX         7   /* only for ZSTD_fast, other strategies are limited to 6 */
 #define ZSTD_MINMATCH_MIN         3   /* only for ZSTD_btopt+, faster strategies are limited to 4 */
+#define ZSTD_MAX_CLEVEL		22
 #define ZSTD_TARGETLENGTH_MAX    ZSTD_BLOCKSIZE_MAX
 #define ZSTD_TARGETLENGTH_MIN     0   /* note : comparing this constant to an unsigned results in a tautological test */
 #define ZSTD_STRATEGY_MIN        ZSTD_fast
diff --git a/lib/zstd/compress/clevels.h b/lib/zstd/compress/clevels.h
index 6ab8be6532ef..06565e064456 100644
--- a/lib/zstd/compress/clevels.h
+++ b/lib/zstd/compress/clevels.h
@@ -17,8 +17,6 @@
 
 /*-=====  Pre-defined compression levels  =====-*/
 
-#define ZSTD_MAX_CLEVEL     22
-
 __attribute__((__unused__))
 
 static const ZSTD_compressionParameters ZSTD_defaultCParameters[4][ZSTD_MAX_CLEVEL+1] = {
-- 
2.43.7

[PATCH v2 4/5] zram: add per-backend caps and validate parameters early

[Haoqin Huang]

From: Haoqin Huang <haoqinhuang@tencent.com>

Writing dict or out-of-range level for backends that don't support
them is currently silently accepted but has no effect. For example,
"algo=lzo dict=/data/dict" succeeds but lzo_setup_params() ignores
the dict entirely.

Add caps, level_min and level_max to zcomp_ops and validate
user-supplied parameters in algorithm_params_store() before storing,
printing an error on failure so the user knows what went wrong.

Signed-off-by: Haoqin Huang <haoqinhuang@tencent.com>
Signed-off-by: Rongwei Wang <zigiwang@tencent.com>
---
 drivers/block/zram/backend_842.c     |  1 +
 drivers/block/zram/backend_deflate.c |  3 +++
 drivers/block/zram/backend_lz4.c     |  3 +++
 drivers/block/zram/backend_lz4hc.c   |  3 +++
 drivers/block/zram/backend_lzo.c     |  1 +
 drivers/block/zram/backend_lzorle.c  |  1 +
 drivers/block/zram/backend_zstd.c    |  3 +++
 drivers/block/zram/zcomp.c           | 23 +++++++++++++++++++++++
 drivers/block/zram/zcomp.h           |  8 ++++++++
 drivers/block/zram/zram_drv.c        | 19 +++++++++++++++++++
 10 files changed, 65 insertions(+)

diff --git a/drivers/block/zram/backend_842.c b/drivers/block/zram/backend_842.c
index 10d9d5c60f53..d796ebda1fa0 100644
--- a/drivers/block/zram/backend_842.c
+++ b/drivers/block/zram/backend_842.c
@@ -57,5 +57,6 @@ const struct zcomp_ops backend_842 = {
 	.destroy_ctx	= destroy_842,
 	.setup_params	= setup_params_842,
 	.release_params	= release_params_842,
+	.caps		= 0,
 	.name		= "842",
 };
diff --git a/drivers/block/zram/backend_deflate.c b/drivers/block/zram/backend_deflate.c
index f92a52a720d1..cedc3daad33a 100644
--- a/drivers/block/zram/backend_deflate.c
+++ b/drivers/block/zram/backend_deflate.c
@@ -144,5 +144,8 @@ const struct zcomp_ops backend_deflate = {
 	.destroy_ctx	= deflate_destroy,
 	.setup_params	= deflate_setup_params,
 	.release_params	= deflate_release_params,
+	.caps		= ZCOMP_CAP_LEVEL,
+	.level_min	= Z_DEFAULT_COMPRESSION,
+	.level_max	= Z_BEST_COMPRESSION,
 	.name		= "deflate",
 };
diff --git a/drivers/block/zram/backend_lz4.c b/drivers/block/zram/backend_lz4.c
index c449d511ba86..bd1e5ca4d134 100644
--- a/drivers/block/zram/backend_lz4.c
+++ b/drivers/block/zram/backend_lz4.c
@@ -146,5 +146,8 @@ const struct zcomp_ops backend_lz4 = {
 	.destroy_ctx	= lz4_destroy,
 	.setup_params	= lz4_setup_params,
 	.release_params	= lz4_release_params,
+	.caps		= ZCOMP_CAP_DICT | ZCOMP_CAP_LEVEL,
+	.level_min	= LZ4_ACCELERATION_DEFAULT,
+	.level_max	= 65535,
 	.name		= "lz4",
 };
diff --git a/drivers/block/zram/backend_lz4hc.c b/drivers/block/zram/backend_lz4hc.c
index f6a336acfe20..0e0d7c68a7d4 100644
--- a/drivers/block/zram/backend_lz4hc.c
+++ b/drivers/block/zram/backend_lz4hc.c
@@ -124,5 +124,8 @@ const struct zcomp_ops backend_lz4hc = {
 	.destroy_ctx	= lz4hc_destroy,
 	.setup_params	= lz4hc_setup_params,
 	.release_params	= lz4hc_release_params,
+	.caps		= ZCOMP_CAP_DICT | ZCOMP_CAP_LEVEL,
+	.level_min	= LZ4HC_MIN_CLEVEL,
+	.level_max	= LZ4HC_MAX_CLEVEL,
 	.name		= "lz4hc",
 };
diff --git a/drivers/block/zram/backend_lzo.c b/drivers/block/zram/backend_lzo.c
index 4c906beaae6b..965f007e2ca8 100644
--- a/drivers/block/zram/backend_lzo.c
+++ b/drivers/block/zram/backend_lzo.c
@@ -55,5 +55,6 @@ const struct zcomp_ops backend_lzo = {
 	.destroy_ctx	= lzo_destroy,
 	.setup_params	= lzo_setup_params,
 	.release_params	= lzo_release_params,
+	.caps		= 0,
 	.name		= "lzo",
 };
diff --git a/drivers/block/zram/backend_lzorle.c b/drivers/block/zram/backend_lzorle.c
index 10640c96cbfc..757b4598be03 100644
--- a/drivers/block/zram/backend_lzorle.c
+++ b/drivers/block/zram/backend_lzorle.c
@@ -55,5 +55,6 @@ const struct zcomp_ops backend_lzorle = {
 	.destroy_ctx	= lzorle_destroy,
 	.setup_params	= lzorle_setup_params,
 	.release_params	= lzorle_release_params,
+	.caps		= 0,
 	.name		= "lzo-rle",
 };
diff --git a/drivers/block/zram/backend_zstd.c b/drivers/block/zram/backend_zstd.c
index 2584f47c9b3c..0fbd2460883a 100644
--- a/drivers/block/zram/backend_zstd.c
+++ b/drivers/block/zram/backend_zstd.c
@@ -212,5 +212,8 @@ const struct zcomp_ops backend_zstd = {
 	.destroy_ctx	= zstd_destroy,
 	.setup_params	= zstd_setup_params,
 	.release_params	= zstd_release_params,
+	.caps		= ZCOMP_CAP_DICT | ZCOMP_CAP_LEVEL,
+	.level_min	= (int)-ZSTD_TARGETLENGTH_MAX,
+	.level_max	= ZSTD_MAX_CLEVEL,
 	.name		= "zstd",
 };
diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
index 974c4691887e..15de28b50d42 100644
--- a/drivers/block/zram/zcomp.c
+++ b/drivers/block/zram/zcomp.c
@@ -9,6 +9,9 @@
 #include <linux/cpuhotplug.h>
 #include <linux/vmalloc.h>
 #include <linux/sysfs.h>
+#include <linux/lz4.h>
+#include <linux/zlib.h>
+#include <linux/zstd.h>
 
 #include "zcomp.h"
 
@@ -94,6 +97,26 @@ const char *zcomp_lookup_backend_name(const char *comp)
 	return NULL;
 }
 
+unsigned int zcomp_get_caps(const char *comp)
+{
+	const struct zcomp_ops *backend = lookup_backend_ops(comp);
+
+	return backend ? backend->caps : 0;
+}
+
+int zcomp_validate_level(const char *comp, s32 level)
+{
+	const struct zcomp_ops *backend = lookup_backend_ops(comp);
+
+	if (!backend)
+		return -EINVAL;
+	if (!(backend->caps & ZCOMP_CAP_LEVEL))
+		return -EOPNOTSUPP;
+	if (level < backend->level_min || level > backend->level_max)
+		return -EINVAL;
+	return 0;
+}
+
 /* show available compressors */
 ssize_t zcomp_available_show(const char *comp, char *buf, ssize_t at)
 {
diff --git a/drivers/block/zram/zcomp.h b/drivers/block/zram/zcomp.h
index 81a0f3f6ff48..16f812a94c67 100644
--- a/drivers/block/zram/zcomp.h
+++ b/drivers/block/zram/zcomp.h
@@ -7,6 +7,9 @@
 
 #define ZCOMP_PARAM_NOT_SET	INT_MIN
 
+#define ZCOMP_CAP_DICT		BIT(0)	/* dictionary support */
+#define ZCOMP_CAP_LEVEL		BIT(1)	/* adjustable compression level */
+
 struct deflate_params {
 	s32 winbits;
 };
@@ -66,6 +69,9 @@ struct zcomp_ops {
 	int (*setup_params)(struct zcomp_params *params);
 	void (*release_params)(struct zcomp_params *params);
 
+	unsigned int caps;
+	s32 level_min;
+	s32 level_max;
 	const char *name;
 };
 
@@ -81,6 +87,8 @@ int zcomp_cpu_up_prepare(unsigned int cpu, struct hlist_node *node);
 int zcomp_cpu_dead(unsigned int cpu, struct hlist_node *node);
 ssize_t zcomp_available_show(const char *comp, char *buf, ssize_t at);
 const char *zcomp_lookup_backend_name(const char *comp);
+unsigned int zcomp_get_caps(const char *comp);
+int zcomp_validate_level(const char *comp, s32 level);
 
 struct zcomp *zcomp_create(const char *alg, struct zcomp_params *params);
 void zcomp_destroy(struct zcomp *comp);
diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 9ea7ba9d1ed0..d86f4eb06d58 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -1796,6 +1796,25 @@ static ssize_t algorithm_params_store(struct device *dev,
 			return -EINVAL;
 	}
 
+	if (zram->comp_algs[prio]) {
+		unsigned int caps = zcomp_get_caps(zram->comp_algs[prio]);
+
+		if (dict_path && !(caps & ZCOMP_CAP_DICT)) {
+			pr_err("zram: %s does not support dictionary\n",
+			       zram->comp_algs[prio]);
+			return -EOPNOTSUPP;
+		}
+
+		if (level != ZCOMP_PARAM_NOT_SET) {
+			ret = zcomp_validate_level(zram->comp_algs[prio], level);
+			if (ret) {
+				pr_err("zram: invalid level for %s\n",
+				       zram->comp_algs[prio]);
+				return ret;
+			}
+		}
+	}
+
 	ret = comp_params_store(zram, prio, level, dict_path, &deflate_params);
 	return ret ? ret : len;
 }
-- 
2.43.7

[PATCH v2 5/5] zram: reset per-priority params when changing algorithm before init

[Haoqin Huang]

From: Haoqin Huang <haoqinhuang@tencent.com>

Parameters validated against one algorithm may be invalid for another
(e.g. lz4 accepts level=65535 but zstd does not).  Although algorithm
changes are blocked after disksize is set, they are allowed before
device initialization.  Reset per-priority params on algorithm change
so that stale parameters do not silently carry over.

Signed-off-by: Haoqin Huang <haoqinhuang@tencent.com>
Signed-off-by: Rongwei Wang <zigiwang@tencent.com>
Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
---
 drivers/block/zram/zram_drv.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index d86f4eb06d58..da2921082712 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -1661,6 +1661,8 @@ static void comp_algorithm_set(struct zram *zram, u32 prio, const char *alg)
 	zram->comp_algs[prio] = alg;
 }
 
+static void comp_params_reset(struct zram *zram, u32 prio);
+
 static int __comp_algorithm_store(struct zram *zram, u32 prio, const char *buf)
 {
 	const char *alg;
@@ -1681,6 +1683,7 @@ static int __comp_algorithm_store(struct zram *zram, u32 prio, const char *buf)
 	}
 
 	comp_algorithm_set(zram, prio, alg);
+	comp_params_reset(zram, prio);
 	return 0;
 }
 
-- 
2.43.7

Re: [PATCH v2 1/5] zram: fix early release of global cdict/ddict in per-CPU error path

[Andrew Morton]

On Tue, 28 Jul 2026 17:29:31 +0800 Haoqin Huang <haoqinhuang7@gmail.com> wrote:

> zstd_setup_params() creates global cdict and ddict stored in
> params->drv_data, shared across all per-CPU contexts. The per-CPU
> zstd_create() error path called zstd_release_params(), which freed
> those globally-shared objects. While the drv_data=NULL guard in
> zstd_release_params() prevents a double-free on the init failure
> path, this is still a layering violation: a per-CPU callback should
> only clean up its own context, not release resources owned by the
> compression lifecycle (zcomp_init / zcomp_destroy).
> 
> Fix by removing zstd_release_params() from the per-CPU error path
> and replacing it with only zstd_destroy(), which properly cleans
> up the per-CPU context without touching the global params->drv_data.

Thanks.  A [0/N] cover letter would be appropriate.


Do any of these changes have userspace-visible runtime effects?  If so,
please changelog these in full detail.  If not, a statement (in the
[0/N]!) telling us this would be helpful.


AI review might have found a few things, about half of them
pre-existing (zram/zcomp):

	https://sashiko.dev/#/patchset/20260728092935.31139-1-haoqinhuang7@gmail.com

Re: [PATCH v2 1/5] zram: fix early release of global cdict/ddict in per-CPU error path

[Sergey Senozhatsky]

On (26/07/28 17:29), Haoqin Huang wrote:
> zstd_setup_params() creates global cdict and ddict stored in
> params->drv_data, shared across all per-CPU contexts. The per-CPU
> zstd_create() error path called zstd_release_params(), which freed
> those globally-shared objects. While the drv_data=NULL guard in
> zstd_release_params() prevents a double-free on the init failure
> path, this is still a layering violation: a per-CPU callback should
> only clean up its own context, not release resources owned by the
> compression lifecycle (zcomp_init / zcomp_destroy).
> 
> Fix by removing zstd_release_params() from the per-CPU error path
> and replacing it with only zstd_destroy(), which properly cleans
> up the per-CPU context without touching the global params->drv_data.
> 
> Fixes: 6a559ecd6e7e ("zram: add dictionary support to zstd backend")

As we agreed earlier, this patch doesn't fix any known issues per se.
Let's not send a false signal to stable folks that this patch is
fixing a bug, let's not add Fixes: where they don't belong.

Re: [PATCH v2 4/5] zram: add per-backend caps and validate parameters early

[Sergey Senozhatsky]

On (26/07/28 17:29), Haoqin Huang wrote:
[..]
> +unsigned int zcomp_get_caps(const char *comp)
> +{
> +	const struct zcomp_ops *backend = lookup_backend_ops(comp);
> +
> +	return backend ? backend->caps : 0;
> +}
> +
> +int zcomp_validate_level(const char *comp, s32 level)
> +{
> +	const struct zcomp_ops *backend = lookup_backend_ops(comp);
> +
> +	if (!backend)
> +		return -EINVAL;
> +	if (!(backend->caps & ZCOMP_CAP_LEVEL))
> +		return -EOPNOTSUPP;
> +	if (level < backend->level_min || level > backend->level_max)
> +		return -EINVAL;
> +	return 0;
> +}

[..]

> +unsigned int zcomp_get_caps(const char *comp);
> +int zcomp_validate_level(const char *comp, s32 level);

[..]

> @@ -1796,6 +1796,25 @@ static ssize_t algorithm_params_store(struct device *dev,
>  			return -EINVAL;
>  	}
>  
> +	if (zram->comp_algs[prio]) {
> +		unsigned int caps = zcomp_get_caps(zram->comp_algs[prio]);
> +
> +		if (dict_path && !(caps & ZCOMP_CAP_DICT)) {
> +			pr_err("zram: %s does not support dictionary\n",
> +			       zram->comp_algs[prio]);
> +			return -EOPNOTSUPP;
> +		}
> +
> +		if (level != ZCOMP_PARAM_NOT_SET) {
> +			ret = zcomp_validate_level(zram->comp_algs[prio], level);
> +			if (ret) {
> +				pr_err("zram: invalid level for %s\n",
> +				       zram->comp_algs[prio]);
> +				return ret;
> +			}
> +		}
> +	}

So I wonder if instead of introducing 2 new zcomp functions (zcomp_get_caps()
and zcomp_validate_level()) and still basically open-coding params verification
in zram, maybe we we can just have one
	int zcomp_validate_params(comp, level, dict_path)
and handle all the validation in zcomp internally.  So that in
algorithm_params_store() it will be just

	ret = zcomp_validate_params(zram->comp_algs[prio], level, dict_path);
	if (ret)
		return ret;

Re: [PATCH v2 2/5] zram: make dict update in comp_params_store() atomic

[Sergey Senozhatsky]

On (26/07/28 17:29), Haoqin Huang wrote:
> comp_params_store() resets old parameters before reading a new dict,
> so if kernel_read_file_from_path() fails the params are left broken
> and the actual error is swallowed. Fix by reading into a temporary
> buffer first, swapping only on success. Use sz <= 0 to also reject
> zero-size dicts.
> 
> Signed-off-by: Haoqin Huang <haoqinhuang@tencent.com>
> Signed-off-by: Rongwei Wang <zigiwang@tencent.com>
> ---
>  drivers/block/zram/zram_drv.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index ace65c586072..9ea7ba9d1ed0 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -1699,21 +1699,23 @@ static int comp_params_store(struct zram *zram, u32 prio, s32 level,
>  			     const char *dict_path,
>  			     struct deflate_params *deflate_params)
>  {
> +	void *new_dict = NULL;
>  	ssize_t sz = 0;
>  
> -	comp_params_reset(zram, prio);

I don't see why is that a problem.  All you wanted to do here is to
handle zero i_size.  Why do we need dict setting to be atomic?

>  	if (dict_path) {
> -		sz = kernel_read_file_from_path(dict_path, 0,
> -						&zram->params[prio].dict,
> -						INT_MAX,
> -						NULL,
> -						READING_POLICY);
> -		if (sz < 0)
> -			return -EINVAL;
> +		sz = kernel_read_file_from_path(dict_path, 0, &new_dict,
> +						INT_MAX, NULL, READING_POLICY);
> +		if (sz <= 0) {
> +			vfree(new_dict);
> +			if (sz == 0)
> +				return -EINVAL;
> +			return sz;
> +		}
>  	}
>  
> +	comp_params_reset(zram, prio);
>  	zram->params[prio].dict_sz = sz;
> +	zram->params[prio].dict = new_dict;
>  	zram->params[prio].level = level;
>  	zram->params[prio].deflate.winbits = deflate_params->winbits;

Re: [PATCH v2 1/5] zram: fix early release of global cdict/ddict in per-CPU error path

[Sergey Senozhatsky]

On (26/07/28 17:29), Haoqin Huang wrote:
> Subject: [PATCH v2 1/5] zram: fix early release of global cdict/ddict in per-CPU error path

Let's rename it to something like
	"zram: do not release zstd params in per-CPU error path"

Re: [PATCH v2 1/5] zram: fix early release of global cdict/ddict in per-CPU error path

[Sergey Senozhatsky]

On (26/07/28 17:34), Andrew Morton wrote:
> AI review might have found a few things, about half of them
> pre-existing (zram/zcomp):
> 
> 	https://sashiko.dev/#/patchset/20260728092935.31139-1-haoqinhuang7@gmail.com

I'll take a look at those pre-existing issues.

Re: [PATCH v2 3/5] zstd: move ZSTD_MAX_CLEVEL to zstd_lib.h

[Sergey Senozhatsky]

On (26/07/28 17:29), Haoqin Huang wrote:
[..]
> diff --git a/include/linux/zstd_lib.h b/include/linux/zstd_lib.h
> index e295d4125dde..f4b26844e53a 100644
> --- a/include/linux/zstd_lib.h
> +++ b/include/linux/zstd_lib.h
> @@ -1241,6 +1241,7 @@ ZSTDLIB_API size_t ZSTD_sizeof_DDict(const ZSTD_DDict* ddict);
>  #define ZSTD_SEARCHLOG_MIN        1
>  #define ZSTD_MINMATCH_MAX         7   /* only for ZSTD_fast, other strategies are limited to 6 */
>  #define ZSTD_MINMATCH_MIN         3   /* only for ZSTD_btopt+, faster strategies are limited to 4 */
> +#define ZSTD_MAX_CLEVEL		22
>  #define ZSTD_TARGETLENGTH_MAX    ZSTD_BLOCKSIZE_MAX
>  #define ZSTD_TARGETLENGTH_MIN     0   /* note : comparing this constant to an unsigned results in a tautological test */
>  #define ZSTD_STRATEGY_MIN        ZSTD_fast
> diff --git a/lib/zstd/compress/clevels.h b/lib/zstd/compress/clevels.h
> index 6ab8be6532ef..06565e064456 100644
> --- a/lib/zstd/compress/clevels.h
> +++ b/lib/zstd/compress/clevels.h
> @@ -17,8 +17,6 @@
>  
>  /*-=====  Pre-defined compression levels  =====-*/
>  
> -#define ZSTD_MAX_CLEVEL     22
> -
>  __attribute__((__unused__))

Sashiko made a good point.  Can we use zstd_max_clevel() instead?

Re: [PATCH v2 1/5] zram: fix early release of global cdict/ddict in per-CPU error path

[haoqin huang]

On Wed, Jul 29, 2026 at 8:34 AM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Tue, 28 Jul 2026 17:29:31 +0800 Haoqin Huang <haoqinhuang7@gmail.com> wrote:
>
> > zstd_setup_params() creates global cdict and ddict stored in
> > params->drv_data, shared across all per-CPU contexts. The per-CPU
> > zstd_create() error path called zstd_release_params(), which freed
> > those globally-shared objects. While the drv_data=NULL guard in
> > zstd_release_params() prevents a double-free on the init failure
> > path, this is still a layering violation: a per-CPU callback should
> > only clean up its own context, not release resources owned by the
> > compression lifecycle (zcomp_init / zcomp_destroy).
> >
> > Fix by removing zstd_release_params() from the per-CPU error path
> > and replacing it with only zstd_destroy(), which properly cleans
> > up the per-CPU context without touching the global params->drv_data.
>
> Thanks.  A [0/N] cover letter would be appropriate.
>
>
> Do any of these changes have userspace-visible runtime effects?  If so,
> please changelog these in full detail.  If not, a statement (in the
> [0/N]!) telling us this would be helpful.
>

That's fair, these are fairly small changes so I skipped the cover
letter.  I'll add one with a changelog in v3.

The changes don't have any userspace-visible runtime effects.

>
> AI review might have found a few things, about half of them
> pre-existing (zram/zcomp):
>
>         https://sashiko.dev/#/patchset/20260728092935.31139-1-haoqinhuang7@gmail.com
>
>

I took a look at the AI review and there are a few things I can clean
up in the next version — I'll fold those into v3 as well.

Thanks.

Re: [PATCH v2 1/5] zram: fix early release of global cdict/ddict in per-CPU error path

[haoqin huang]

On Wed, Jul 29, 2026 at 10:20 AM Sergey Senozhatsky
<senozhatsky@chromium.org> wrote:
>
> On (26/07/28 17:29), Haoqin Huang wrote:
> > zstd_setup_params() creates global cdict and ddict stored in
> > params->drv_data, shared across all per-CPU contexts. The per-CPU
> > zstd_create() error path called zstd_release_params(), which freed
> > those globally-shared objects. While the drv_data=NULL guard in
> > zstd_release_params() prevents a double-free on the init failure
> > path, this is still a layering violation: a per-CPU callback should
> > only clean up its own context, not release resources owned by the
> > compression lifecycle (zcomp_init / zcomp_destroy).
> >
> > Fix by removing zstd_release_params() from the per-CPU error path
> > and replacing it with only zstd_destroy(), which properly cleans
> > up the per-CPU context without touching the global params->drv_data.
> >
> > Fixes: 6a559ecd6e7e ("zram: add dictionary support to zstd backend")
>
> As we agreed earlier, this patch doesn't fix any known issues per se.
> Let's not send a false signal to stable folks that this patch is
> fixing a bug, let's not add Fixes: where they don't belong.

Agreed, I'll drop the Fixes: tag and rename it to "zram: do not
release zstd params in per-CPU error path" in v3, which I'll send
out later.

Thanks.

Re: [PATCH v2 4/5] zram: add per-backend caps and validate parameters early

[haoqin huang]

On Wed, Jul 29, 2026 at 10:26 AM Sergey Senozhatsky
<senozhatsky@chromium.org> wrote:
>
> On (26/07/28 17:29), Haoqin Huang wrote:
> [..]
> > +unsigned int zcomp_get_caps(const char *comp)
> > +{
> > +     const struct zcomp_ops *backend = lookup_backend_ops(comp);
> > +
> > +     return backend ? backend->caps : 0;
> > +}
> > +
> > +int zcomp_validate_level(const char *comp, s32 level)
> > +{
> > +     const struct zcomp_ops *backend = lookup_backend_ops(comp);
> > +
> > +     if (!backend)
> > +             return -EINVAL;
> > +     if (!(backend->caps & ZCOMP_CAP_LEVEL))
> > +             return -EOPNOTSUPP;
> > +     if (level < backend->level_min || level > backend->level_max)
> > +             return -EINVAL;
> > +     return 0;
> > +}
>
> [..]
>
> > +unsigned int zcomp_get_caps(const char *comp);
> > +int zcomp_validate_level(const char *comp, s32 level);
>
> [..]
>
> > @@ -1796,6 +1796,25 @@ static ssize_t algorithm_params_store(struct device *dev,
> >                       return -EINVAL;
> >       }
> >
> > +     if (zram->comp_algs[prio]) {
> > +             unsigned int caps = zcomp_get_caps(zram->comp_algs[prio]);
> > +
> > +             if (dict_path && !(caps & ZCOMP_CAP_DICT)) {
> > +                     pr_err("zram: %s does not support dictionary\n",
> > +                            zram->comp_algs[prio]);
> > +                     return -EOPNOTSUPP;
> > +             }
> > +
> > +             if (level != ZCOMP_PARAM_NOT_SET) {
> > +                     ret = zcomp_validate_level(zram->comp_algs[prio], level);
> > +                     if (ret) {
> > +                             pr_err("zram: invalid level for %s\n",
> > +                                    zram->comp_algs[prio]);
> > +                             return ret;
> > +                     }
> > +             }
> > +     }
>
> So I wonder if instead of introducing 2 new zcomp functions (zcomp_get_caps()
> and zcomp_validate_level()) and still basically open-coding params verification
> in zram, maybe we we can just have one
>         int zcomp_validate_params(comp, level, dict_path)
> and handle all the validation in zcomp internally.  So that in
> algorithm_params_store() it will be just
>
>         ret = zcomp_validate_params(zram->comp_algs[prio], level, dict_path);
>         if (ret)
>                 return ret;

Sounds good, I'll merge them into a single zcomp_validate_params() in v3.

Re: [PATCH v2 2/5] zram: make dict update in comp_params_store() atomic

[haoqin huang]

On Wed, Jul 29, 2026 at 10:30 AM Sergey Senozhatsky
<senozhatsky@chromium.org> wrote:
>
> On (26/07/28 17:29), Haoqin Huang wrote:
> > comp_params_store() resets old parameters before reading a new dict,
> > so if kernel_read_file_from_path() fails the params are left broken
> > and the actual error is swallowed. Fix by reading into a temporary
> > buffer first, swapping only on success. Use sz <= 0 to also reject
> > zero-size dicts.
> >
> > Signed-off-by: Haoqin Huang <haoqinhuang@tencent.com>
> > Signed-off-by: Rongwei Wang <zigiwang@tencent.com>
> > ---
> >  drivers/block/zram/zram_drv.c | 20 +++++++++++---------
> >  1 file changed, 11 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> > index ace65c586072..9ea7ba9d1ed0 100644
> > --- a/drivers/block/zram/zram_drv.c
> > +++ b/drivers/block/zram/zram_drv.c
> > @@ -1699,21 +1699,23 @@ static int comp_params_store(struct zram *zram, u32 prio, s32 level,
> >                            const char *dict_path,
> >                            struct deflate_params *deflate_params)
> >  {
> > +     void *new_dict = NULL;
> >       ssize_t sz = 0;
> >
> > -     comp_params_reset(zram, prio);
>
> I don't see why is that a problem.  All you wanted to do here is to
> handle zero i_size.  Why do we need dict setting to be atomic?
>

comp_params_reset() calls vfree() on the old dict and resets level/
winbits to NOT_SET before reading the new dict.  If
kernel_read_file_from_path() then fails for any reason, not just
zero-size, but also ENOENT, ENOMEM, etc. the old dict is already
freed and unrecoverable, and the params are left in a broken,
half-reset state.

The "atomic" in the subject is about all-or-nothing semantics: don't
destroy valid state until the replacement is confirmed good.  Reading
into a temp buffer first, then swapping only on success, is the natural
way to do that.  The sz <= 0 check falls out naturally.

That said, if you prefer a more minimal fix, I can drop the temp-buffer
approach and just change sz < 0 to sz <= 0.  But since reading into
a temp buffer first protects against all failure paths, not just zero-size,
it seemed worth doing in one step.

> >       if (dict_path) {
> > -             sz = kernel_read_file_from_path(dict_path, 0,
> > -                                             &zram->params[prio].dict,
> > -                                             INT_MAX,
> > -                                             NULL,
> > -                                             READING_POLICY);
> > -             if (sz < 0)
> > -                     return -EINVAL;
> > +             sz = kernel_read_file_from_path(dict_path, 0, &new_dict,
> > +                                             INT_MAX, NULL, READING_POLICY);
> > +             if (sz <= 0) {
> > +                     vfree(new_dict);
> > +                     if (sz == 0)
> > +                             return -EINVAL;
> > +                     return sz;
> > +             }
> >       }
> >
> > +     comp_params_reset(zram, prio);
> >       zram->params[prio].dict_sz = sz;
> > +     zram->params[prio].dict = new_dict;
> >       zram->params[prio].level = level;
> >       zram->params[prio].deflate.winbits = deflate_params->winbits;

Re: [PATCH v2 3/5] zstd: move ZSTD_MAX_CLEVEL to zstd_lib.h

[haoqin huang]

On Wed, Jul 29, 2026 at 11:03 AM Sergey Senozhatsky
<senozhatsky@chromium.org> wrote:
>
> On (26/07/28 17:29), Haoqin Huang wrote:
> [..]
> > diff --git a/include/linux/zstd_lib.h b/include/linux/zstd_lib.h
> > index e295d4125dde..f4b26844e53a 100644
> > --- a/include/linux/zstd_lib.h
> > +++ b/include/linux/zstd_lib.h
> > @@ -1241,6 +1241,7 @@ ZSTDLIB_API size_t ZSTD_sizeof_DDict(const ZSTD_DDict* ddict);
> >  #define ZSTD_SEARCHLOG_MIN        1
> >  #define ZSTD_MINMATCH_MAX         7   /* only for ZSTD_fast, other strategies are limited to 6 */
> >  #define ZSTD_MINMATCH_MIN         3   /* only for ZSTD_btopt+, faster strategies are limited to 4 */
> > +#define ZSTD_MAX_CLEVEL              22
> >  #define ZSTD_TARGETLENGTH_MAX    ZSTD_BLOCKSIZE_MAX
> >  #define ZSTD_TARGETLENGTH_MIN     0   /* note : comparing this constant to an unsigned results in a tautological test */
> >  #define ZSTD_STRATEGY_MIN        ZSTD_fast
> > diff --git a/lib/zstd/compress/clevels.h b/lib/zstd/compress/clevels.h
> > index 6ab8be6532ef..06565e064456 100644
> > --- a/lib/zstd/compress/clevels.h
> > +++ b/lib/zstd/compress/clevels.h
> > @@ -17,8 +17,6 @@
> >
> >  /*-=====  Pre-defined compression levels  =====-*/
> >
> > -#define ZSTD_MAX_CLEVEL     22
> > -
> >  __attribute__((__unused__))
>
> Sashiko made a good point.  Can we use zstd_max_clevel() instead?

Good point, I'll drop this patch and use zstd_max_clevel() instead
in v3.

Since it's a runtime function and can't be used for static struct
initialization, I plan to set backend_zstd's level_max to -1 as a
sentinel value, and query the actual maximum in
zcomp_validate_params():

    s32 max = backend->level_max;
    if (max < 0)
        max = zstd_max_clevel();

Do you think this approach is feasible?

Re: [PATCH v2 2/5] zram: make dict update in comp_params_store() atomic

[Sergey Senozhatsky]

On (26/07/29 12:06), haoqin huang wrote:
> > On (26/07/28 17:29), Haoqin Huang wrote:
[..]
> > > @@ -1699,21 +1699,23 @@ static int comp_params_store(struct zram *zram, u32 prio, s32 level,
> > >                            const char *dict_path,
> > >                            struct deflate_params *deflate_params)
> > >  {
> > > +     void *new_dict = NULL;
> > >       ssize_t sz = 0;
> > >
> > > -     comp_params_reset(zram, prio);
> >
> > I don't see why is that a problem.  All you wanted to do here is to
> > handle zero i_size.  Why do we need dict setting to be atomic?
> >
> 
> comp_params_reset() calls vfree() on the old dict and resets level/
> winbits to NOT_SET before reading the new dict

But what is the scenario here?  Who would have several dicts?

  echo "dict=/etc/dict.foo prio=1" > algorithm_params

and if that fails then

  echo "dict=/etc/dict.bar prio=1" > algorithm_params

I don't think this is something that we need to consider.  What am I
missing?

[..]
> The "atomic" in the subject is about all-or-nothing semantics: don't
> destroy valid state until the replacement is confirmed good.

I don't think that "valid state configuration replacement" ever happens.

Re: [PATCH v2 3/5] zstd: move ZSTD_MAX_CLEVEL to zstd_lib.h

[Sergey Senozhatsky]

On (26/07/29 12:16), haoqin huang wrote:
> > On (26/07/28 17:29), Haoqin Huang wrote:
> > [..]
> > >  #define ZSTD_MINMATCH_MAX         7   /* only for ZSTD_fast, other strategies are limited to 6 */
> > >  #define ZSTD_MINMATCH_MIN         3   /* only for ZSTD_btopt+, faster strategies are limited to 4 */
> > > +#define ZSTD_MAX_CLEVEL              22
> > >  #define ZSTD_TARGETLENGTH_MAX    ZSTD_BLOCKSIZE_MAX
> > >  #define ZSTD_TARGETLENGTH_MIN     0   /* note : comparing this constant to an unsigned results in a tautological test */
> > >  #define ZSTD_STRATEGY_MIN        ZSTD_fast
> > > diff --git a/lib/zstd/compress/clevels.h b/lib/zstd/compress/clevels.h
> > > index 6ab8be6532ef..06565e064456 100644
> > > --- a/lib/zstd/compress/clevels.h
> > > +++ b/lib/zstd/compress/clevels.h
> > > @@ -17,8 +17,6 @@
> > >
> > >  /*-=====  Pre-defined compression levels  =====-*/
> > >
> > > -#define ZSTD_MAX_CLEVEL     22
> > > -
> > >  __attribute__((__unused__))
> >
> > Sashiko made a good point.  Can we use zstd_max_clevel() instead?
> 
> Good point, I'll drop this patch and use zstd_max_clevel() instead
> in v3.
> 
> Since it's a runtime function and can't be used for static struct
> initialization, I plan to set backend_zstd's level_max to -1 as a
> sentinel value, and query the actual maximum in
> zcomp_validate_params():
> 
>     s32 max = backend->level_max;
>     if (max < 0)
>         max = zstd_max_clevel();
> 
> Do you think this approach is feasible?

Hmm, no, that doesn't look good. zcomp should not include
zstd.h or any other libs directly.  Should params validation
be a per-backend callback then?

Re: [PATCH v2 2/5] zram: make dict update in comp_params_store() atomic

[haoqin huang]

On Wed, Jul 29, 2026 at 12:16 PM Sergey Senozhatsky
<senozhatsky@chromium.org> wrote:
>
> On (26/07/29 12:06), haoqin huang wrote:
> > > On (26/07/28 17:29), Haoqin Huang wrote:
> [..]
> > > > @@ -1699,21 +1699,23 @@ static int comp_params_store(struct zram *zram, u32 prio, s32 level,
> > > >                            const char *dict_path,
> > > >                            struct deflate_params *deflate_params)
> > > >  {
> > > > +     void *new_dict = NULL;
> > > >       ssize_t sz = 0;
> > > >
> > > > -     comp_params_reset(zram, prio);
> > >
> > > I don't see why is that a problem.  All you wanted to do here is to
> > > handle zero i_size.  Why do we need dict setting to be atomic?
> > >
> >
> > comp_params_reset() calls vfree() on the old dict and resets level/
> > winbits to NOT_SET before reading the new dict
>
> But what is the scenario here?  Who would have several dicts?
>
>   echo "dict=/etc/dict.foo prio=1" > algorithm_params
>
> and if that fails then
>
>   echo "dict=/etc/dict.bar prio=1" > algorithm_params
>
> I don't think this is something that we need to consider.  What am I
> missing?
>
> [..]
> > The "atomic" in the subject is about all-or-nothing semantics: don't
> > destroy valid state until the replacement is confirmed good.
>
> I don't think that "valid state configuration replacement" ever happens.

You're right, the scenario I had in mind was a bit far-fetched.  It requires
loading dict_a first, then trying to replace it with dict_b before init, with
the second read failing:

    echo "algo=zstd dict=/path/dict_a" > algorithm_params
    echo "algo=zstd dict=/path/dict_b" > algorithm_params  # fails

comp_params_reset() would free dict_a before dict_b fails to load. But in
practice params are set once before init and never change.

I'll simplify this to just the sz <= 0 change in v3.

Re: [PATCH v2 3/5] zstd: move ZSTD_MAX_CLEVEL to zstd_lib.h

[haoqin huang]

On Wed, Jul 29, 2026 at 12:23 PM Sergey Senozhatsky
<senozhatsky@chromium.org> wrote:
>
> On (26/07/29 12:16), haoqin huang wrote:
> > > On (26/07/28 17:29), Haoqin Huang wrote:
> > > [..]
> > > >  #define ZSTD_MINMATCH_MAX         7   /* only for ZSTD_fast, other strategies are limited to 6 */
> > > >  #define ZSTD_MINMATCH_MIN         3   /* only for ZSTD_btopt+, faster strategies are limited to 4 */
> > > > +#define ZSTD_MAX_CLEVEL              22
> > > >  #define ZSTD_TARGETLENGTH_MAX    ZSTD_BLOCKSIZE_MAX
> > > >  #define ZSTD_TARGETLENGTH_MIN     0   /* note : comparing this constant to an unsigned results in a tautological test */
> > > >  #define ZSTD_STRATEGY_MIN        ZSTD_fast
> > > > diff --git a/lib/zstd/compress/clevels.h b/lib/zstd/compress/clevels.h
> > > > index 6ab8be6532ef..06565e064456 100644
> > > > --- a/lib/zstd/compress/clevels.h
> > > > +++ b/lib/zstd/compress/clevels.h
> > > > @@ -17,8 +17,6 @@
> > > >
> > > >  /*-=====  Pre-defined compression levels  =====-*/
> > > >
> > > > -#define ZSTD_MAX_CLEVEL     22
> > > > -
> > > >  __attribute__((__unused__))
> > >
> > > Sashiko made a good point.  Can we use zstd_max_clevel() instead?
> >
> > Good point, I'll drop this patch and use zstd_max_clevel() instead
> > in v3.
> >
> > Since it's a runtime function and can't be used for static struct
> > initialization, I plan to set backend_zstd's level_max to -1 as a
> > sentinel value, and query the actual maximum in
> > zcomp_validate_params():
> >
> >     s32 max = backend->level_max;
> >     if (max < 0)
> >         max = zstd_max_clevel();
> >
> > Do you think this approach is feasible?
>
> Hmm, no, that doesn't look good. zcomp should not include
> zstd.h or any other libs directly.  Should params validation
> be a per-backend callback then?

Good point, zcomp.c shouldn't include library headers.  A per-backend
callback would be cleaner.

I'll add an optional validate_params to zcomp_ops: the zstd backend
implements it using zstd_max_clevel() internally, while lzo/deflate
and others just rely on the static caps check (no callback needed).
zcomp.c stays free of any library headers.

I'll send this in v3.

Re: [PATCH v2 3/5] zstd: move ZSTD_MAX_CLEVEL to zstd_lib.h

[Sergey Senozhatsky]

On (26/07/29 12:32), haoqin huang wrote:
> > > > > -#define ZSTD_MAX_CLEVEL     22
> > > > > -
> > > > >  __attribute__((__unused__))
> > > >
> > > > Sashiko made a good point.  Can we use zstd_max_clevel() instead?
> > >
> > > Good point, I'll drop this patch and use zstd_max_clevel() instead
> > > in v3.
> > >
> > > Since it's a runtime function and can't be used for static struct
> > > initialization, I plan to set backend_zstd's level_max to -1 as a
> > > sentinel value, and query the actual maximum in
> > > zcomp_validate_params():
> > >
> > >     s32 max = backend->level_max;
> > >     if (max < 0)
> > >         max = zstd_max_clevel();
> > >
> > > Do you think this approach is feasible?
> >
> > Hmm, no, that doesn't look good. zcomp should not include
> > zstd.h or any other libs directly.  Should params validation
> > be a per-backend callback then?
> 
> Good point, zcomp.c shouldn't include library headers.  A per-backend
> callback would be cleaner.
> 
> I'll add an optional validate_params to zcomp_ops: the zstd backend
> implements it using zstd_max_clevel() internally, while lzo/deflate
> and others just rely on the static caps check (no callback needed).
> zcomp.c stays free of any library headers.

I'm actually leaning towards validation in .setup_params() now.
It's not immediate but, first, it doesn't matter that much, we still
don't create zram with invalid params configuration and, second, it
sort of makes sense to do validation in .setup_params().  We already
started doing that for deflate winbits (a patch from earlier today).

Can you please add your validation to per-backend .setup_params()?