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

On (26/07/28 00:50), haoqin huang wrote:
> On Fri, Jul 24, 2026 at 1:28 PM Sergey Senozhatsky
> <[email protected]> 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.