| Message ID | 20241204180224.31069-3-ryncsn@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | zram: fix backing device setup issue | expand |
On (24/12/05 02:02), Kairui Song wrote: > From: Kairui Song <kasong@tencent.com> > > 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@tencent.com> > Signed-off-by: Kairui Song <kasong@tencent.com> > Cc: stable@vger.kernel.org > --- > 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);
On Thu, Dec 5, 2024 at 3:09 PM Sergey Senozhatsky <senozhatsky@chromium.org> wrote: > > On (24/12/05 02:02), Kairui Song wrote: > > From: Kairui Song <kasong@tencent.com> > > > > 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@tencent.com> > > Signed-off-by: Kairui Song <kasong@tencent.com> > > Cc: stable@vger.kernel.org > > --- > > 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.
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; }