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

On Fri, Jul 24, 2026 at 1:17 PM Sergey Senozhatsky
<[email protected]> 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?