[PATCH 0/2] zram: fix backing device setup issue

[PATCH 0/2] zram: fix backing device setup issue

[Kairui Song]

From: Kairui Song <kasong@tencent.com>

This series fixes two bugs of backing device setting:

- ZRAM should reject using a zero sized (or the uninitialized ZRAM
  device itself) as the backing device.
- Fix backing device leaking when removing a uninitialized ZRAM
  device.

Kairui Song (2):
  zram: refuse to use zero sized block device as backing device
  zram: fix uninitialized ZRAM not releasing backing device

 drivers/block/zram/zram_drv.c | 9 +++++++++
 1 file changed, 9 insertions(+)

-- 
2.47.0

[PATCH 1/2] zram: refuse to use zero sized block device as backing device

[Kairui Song]

From: Kairui Song <kasong@tencent.com>

Setting a zero sized block device as backing device is pointless, and
one can easily create a recursive loop by setting the uninitialized
ZRAM device itself as its own backing device by (zram0 is uninitialized):

    echo /dev/zram0 > /sys/block/zram0/backing_dev

It's definitely a wrong config, and the module will pin itself,
kernel should refuse doing so in the first place.

By refusing to use zero sized device we avoided misuse cases
including this one above.

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 | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 0ca6d55c9917..dd48df5b97c8 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -614,6 +614,12 @@ static ssize_t backing_dev_store(struct device *dev,
 	}
 
 	nr_pages = i_size_read(inode) >> PAGE_SHIFT;
+	/* Refuse to use zero sized device (also prevents self reference) */
+	if (!nr_pages) {
+		err = -EINVAL;
+		goto out;
+	}
+
 	bitmap_sz = BITS_TO_LONGS(nr_pages) * sizeof(long);
 	bitmap = kvzalloc(bitmap_sz, GFP_KERNEL);
 	if (!bitmap) {
-- 
2.47.0

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

[Kairui Song]

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;
 	}
-- 
2.47.0

Re: [PATCH 1/2] zram: refuse to use zero sized block device as backing device

[Sergey Senozhatsky]

On (24/12/05 02:02), Kairui Song wrote:
> From: Kairui Song <kasong@tencent.com>
> 
> Setting a zero sized block device as backing device is pointless, and
> one can easily create a recursive loop by setting the uninitialized
> ZRAM device itself as its own backing device by (zram0 is uninitialized):
> 
>     echo /dev/zram0 > /sys/block/zram0/backing_dev
> 
> It's definitely a wrong config, and the module will pin itself,
> kernel should refuse doing so in the first place.
> 
> By refusing to use zero sized device we avoided misuse cases
> including this one above.
> 
> 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

Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>

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

[Sergey Senozhatsky]

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);
 

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@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.