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@xxxxxxxxxxxx> 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@xxxxxxxxxxx>
> > Signed-off-by: Rongwei Wang <zigiwang@xxxxxxxxxxx>
> > ---
> >  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;