[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.