Re: [PATCH 2/2] zram: fix uninitialized ZRAM not releasing backing device

[Kairui Song]

On Thu, Dec 5, 2024 at 3:09 PM Sergey Senozhatsky
<senozhatsky@xxxxxxxxxxxx> wrote:
>
> On (24/12/05 02:02), Kairui Song wrote:
> > From: Kairui Song <kasong@xxxxxxxxxxx>
> >
> > Setting backing device is done before ZRAM initialization.
> > If we set the backing device, then remove the ZRAM module without
> > initializing the device, the backing device reference will be leaked
> > and the device will be hold forever.
> >
> > Fix this by always check and release the backing device when resetting
> > or removing ZRAM.
> >
> > Fixes: 013bf95a83ec ("zram: add interface to specif backing device")
> > Reported-by: Desheng Wu <deshengwu@xxxxxxxxxxx>
> > Signed-off-by: Kairui Song <kasong@xxxxxxxxxxx>
> > Cc: stable@xxxxxxxxxxxxxxx
> > ---
> >  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 dd48df5b97c8..dfe9a994e437 100644
> > --- a/drivers/block/zram/zram_drv.c
> > +++ b/drivers/block/zram/zram_drv.c
> > @@ -2335,6 +2335,9 @@ static void zram_reset_device(struct zram *zram)
> >       zram->limit_pages = 0;
> >
> >       if (!init_done(zram)) {
> > +             /* Backing device could be set before ZRAM initialization. */
> > +             reset_bdev(zram);
> > +
> >               up_write(&zram->init_lock);
> >               return;
> >       }
> > --
>
> So here I think we better remove that if entirely and always reset
> the device.  Something like this (untested):
>
> ---
>
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index 0ca6d55c9917..8773b12afc9d 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -1438,12 +1438,16 @@ static void zram_meta_free(struct zram *zram, u64 disksize)
>         size_t num_pages = disksize >> PAGE_SHIFT;
>         size_t index;
>
> +       if (!zram->table)
> +               return;
> +
>         /* Free all pages that are still in this zram device */
>         for (index = 0; index < num_pages; index++)
>                 zram_free_page(zram, index);
>
>         zs_destroy_pool(zram->mem_pool);
>         vfree(zram->table);
> +       zram->table = NULL;
>  }
>
>  static bool zram_meta_alloc(struct zram *zram, u64 disksize)
> @@ -2327,12 +2331,6 @@ static void zram_reset_device(struct zram *zram)
>         down_write(&zram->init_lock);
>
>         zram->limit_pages = 0;
> -
> -       if (!init_done(zram)) {
> -               up_write(&zram->init_lock);
> -               return;
> -       }
> -
>         set_capacity_and_notify(zram->disk, 0);
>         part_stat_set_all(zram->disk->part0, 0);
>
>

Thanks for the suggestion, I've tested it and it works well. Will send
a V2 shortly.