[PATCH 5.15 0/3] ZRAM not releasing backing device backport

[PATCH 5.15 0/3] ZRAM not releasing backing device backport

[Dominique Martinet]

This is a resend of the patchset discussed here[1] for the 5.15 tree.

[1] https://lore.kernel.org/r/2025011052-backpedal-coat-2fec@gregkh

I've picked the "do not keep dangling zcomp pointer" patch from the
linux-rc tree at the time, so kept Sasha's SOB and added mine on top
-- please let me know if it wasn't appropriate.

I've also prepared the 5.10 patches, but I hadn't realized there were so
many stable deps (8 patchs total!); I'm honestly not sure the problem is
worth the churn but since it's done and tested I'll send the patches if
there is no problem with this 5.15 version.

Thanks!

Dominique Martinet (1):
  zram: check comp is non-NULL before calling comp_destroy

Kairui Song (1):
  zram: fix uninitialized ZRAM not releasing backing device

Sergey Senozhatsky (1):
  drivers/block/zram/zram_drv.c: do not keep dangling zcomp pointer
    after zram reset

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

-- 
2.39.5

[PATCH 5.15 1/3] drivers/block/zram/zram_drv.c: do not keep dangling zcomp pointer after zram reset

[Dominique Martinet]

From: Sergey Senozhatsky <senozhatsky@chromium.org>

[ Upstream commit 6d2453c3dbc5f70eafc1c866289a90a1fc57ce18 ]

We do all reset operations under write lock, so we don't need to save
->disksize and ->comp to stack variables.  Another thing is that ->comp is
freed during zram reset, but comp pointer is not NULL-ed, so zram keeps
the freed pointer value.

Link: https://lkml.kernel.org/r/20220824035100.971816-1-senozhatsky@chromium.org
Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
Cc: Minchan Kim <minchan@kernel.org>
Cc: Nitin Gupta <ngupta@vflare.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Stable-dep-of: 74363ec674cb ("zram: fix uninitialized ZRAM not releasing backing device")
Signed-off-by: Sasha Levin <sashal@kernel.org>
Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com>
---
 drivers/block/zram/zram_drv.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index a9f71b27d235..9eed579d02f0 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -1695,9 +1695,6 @@ static int zram_rw_page(struct block_device *bdev, sector_t sector,
 
 static void zram_reset_device(struct zram *zram)
 {
-	struct zcomp *comp;
-	u64 disksize;
-
 	down_write(&zram->init_lock);
 
 	zram->limit_pages = 0;
@@ -1707,18 +1704,16 @@ static void zram_reset_device(struct zram *zram)
 		return;
 	}
 
-	comp = zram->comp;
-	disksize = zram->disksize;
-	zram->disksize = 0;
-
 	set_capacity_and_notify(zram->disk, 0);
 	part_stat_set_all(zram->disk->part0, 0);
 
 	up_write(&zram->init_lock);
 	/* I/O operation under all of CPU are done so let's free */
-	zram_meta_free(zram, disksize);
+	zram_meta_free(zram, zram->disksize);
+	zram->disksize = 0;
 	memset(&zram->stats, 0, sizeof(zram->stats));
-	zcomp_destroy(comp);
+	zcomp_destroy(zram->comp);
+	zram->comp = NULL;
 	reset_bdev(zram);
 }
 
-- 
2.39.5

[PATCH 5.15 2/3] zram: check comp is non-NULL before calling comp_destroy

[Dominique Martinet]

[ v6.1 stable tree commit 677294e4da96547b9ea2955661a4bbf1d13552a3 ]

This is a pre-requisite for the backport of commit 74363ec674cb ("zram:
fix uninitialized ZRAM not releasing backing device"), which has been
implemented differently in commit 7ac07a26dea7 ("zram: preparation for
multi-zcomp support") upstream.

We only need to ensure that zcomp_destroy is not called with a NULL
comp, so add this check as the other commit cannot be backported easily.

Stable-dep-of: 74363ec674cb ("zram: fix uninitialized ZRAM not releasing backing device")
Link: https://lore.kernel.org/Z3ytcILx4S1v_ueJ@codewreck.org
Suggested-by: Kairui Song <kasong@tencent.com>
Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com>
---
 drivers/block/zram/zram_drv.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 9eed579d02f0..e5626303c8ff 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -1712,7 +1712,8 @@ static void zram_reset_device(struct zram *zram)
 	zram_meta_free(zram, zram->disksize);
 	zram->disksize = 0;
 	memset(&zram->stats, 0, sizeof(zram->stats));
-	zcomp_destroy(zram->comp);
+	if (zram->comp)
+		zcomp_destroy(zram->comp);
 	zram->comp = NULL;
 	reset_bdev(zram);
 }
-- 
2.39.5

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

[Dominique Martinet]

From: Kairui Song <kasong@tencent.com>

[ Upstream commit 74363ec674cb172d8856de25776c8f3103f05e2f ]

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 reset the ZRAM fully on rmmod or reset store.

Link: https://lkml.kernel.org/r/20241209165717.94215-3-ryncsn@gmail.com
Fixes: 013bf95a83ec ("zram: add interface to specif backing device")
Signed-off-by: Kairui Song <kasong@tencent.com>
Reported-by: Desheng Wu <deshengwu@tencent.com>
Suggested-by: Sergey Senozhatsky <senozhatsky@chromium.org>
Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
Cc: <stable@vger.kernel.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com>
---
 drivers/block/zram/zram_drv.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index e5626303c8ff..4e008cd0ef65 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -1150,12 +1150,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)
@@ -1699,11 +1703,6 @@ static void zram_reset_device(struct zram *zram)
 
 	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);
 
-- 
2.39.5

Re: [PATCH 5.15 2/3] zram: check comp is non-NULL before calling comp_destroy

[kernel test robot]

Hi,

Thanks for your patch.

FYI: kernel test robot notices the stable kernel rule is not satisfied.

The check is based on https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html#option-3

Rule: The upstream commit ID must be specified with a separate line above the commit text.
Subject: [PATCH 5.15 2/3] zram: check comp is non-NULL before calling comp_destroy
Link: https://lore.kernel.org/stable/20250110075844.1173719-3-dominique.martinet%40atmark-techno.com

Please ignore this mail if the patch is not relevant for upstream.

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 5.15 1/3] drivers/block/zram/zram_drv.c: do not keep dangling zcomp pointer after zram reset

[Sasha Levin]

[ Sasha's backport helper bot ]

Hi,

The upstream commit SHA1 provided is correct: 6d2453c3dbc5f70eafc1c866289a90a1fc57ce18

WARNING: Author mismatch between patch and upstream commit:
Backport author: Dominique Martinet<dominique.martinet@atmark-techno.com>
Commit author: Sergey Senozhatsky<senozhatsky@chromium.org>


Status in newer kernel trees:
6.12.y | Present (exact SHA1)
6.6.y | Present (exact SHA1)
6.1.y | Present (exact SHA1)
5.15.y | Not found

Note: The patch differs from the upstream commit:
---
1:  6d2453c3dbc5 ! 1:  70169b5d17a5 drivers/block/zram/zram_drv.c: do not keep dangling zcomp pointer after zram reset
    @@ Metadata
      ## Commit message ##
         drivers/block/zram/zram_drv.c: do not keep dangling zcomp pointer after zram reset
     
    +    [ Upstream commit 6d2453c3dbc5f70eafc1c866289a90a1fc57ce18 ]
    +
         We do all reset operations under write lock, so we don't need to save
         ->disksize and ->comp to stack variables.  Another thing is that ->comp is
         freed during zram reset, but comp pointer is not NULL-ed, so zram keeps
    @@ Commit message
         Cc: Minchan Kim <minchan@kernel.org>
         Cc: Nitin Gupta <ngupta@vflare.org>
         Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
    +    Stable-dep-of: 74363ec674cb ("zram: fix uninitialized ZRAM not releasing backing device")
    +    Signed-off-by: Sasha Levin <sashal@kernel.org>
    +    Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com>
     
      ## drivers/block/zram/zram_drv.c ##
     @@ drivers/block/zram/zram_drv.c: static int zram_rw_page(struct block_device *bdev, sector_t sector,
    @@ drivers/block/zram/zram_drv.c: static void zram_reset_device(struct zram *zram)
      	set_capacity_and_notify(zram->disk, 0);
      	part_stat_set_all(zram->disk->part0, 0);
      
    + 	up_write(&zram->init_lock);
      	/* I/O operation under all of CPU are done so let's free */
     -	zram_meta_free(zram, disksize);
     +	zram_meta_free(zram, zram->disksize);
    @@ drivers/block/zram/zram_drv.c: static void zram_reset_device(struct zram *zram)
     +	zcomp_destroy(zram->comp);
     +	zram->comp = NULL;
      	reset_bdev(zram);
    + }
      
    - 	up_write(&zram->init_lock);
---

Results of testing on various branches:

| Branch                    | Patch Apply | Build Test |
|---------------------------|-------------|------------|
| stable/linux-5.15.y       |  Success    |  Success   |

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

[Sasha Levin]

[ Sasha's backport helper bot ]

Hi,

The upstream commit SHA1 provided is correct: 74363ec674cb172d8856de25776c8f3103f05e2f

WARNING: Author mismatch between patch and upstream commit:
Backport author: Dominique Martinet<dominique.martinet@atmark-techno.com>
Commit author: Kairui Song<kasong@tencent.com>


Status in newer kernel trees:
6.12.y | Present (different SHA1: 6fb92e9a52e3)
6.6.y | Present (different SHA1: 0b5b0b65561b)
6.1.y | Present (different SHA1: ac3b5366b9b7)
5.15.y | Not found

Note: The patch differs from the upstream commit:
---
Failed to apply patch cleanly, falling back to interdiff...
---

Results of testing on various branches:

| Branch                    | Patch Apply | Build Test |
|---------------------------|-------------|------------|
| stable/linux-5.15.y       |  Success    |  Success   |

Re: [PATCH 5.15 2/3] zram: check comp is non-NULL before calling comp_destroy

[Sasha Levin]

[ Sasha's backport helper bot ]

Hi,

No upstream commit was identified. Using temporary commit for testing.

Results of testing on various branches:

| Branch                    | Patch Apply | Build Test |
|---------------------------|-------------|------------|
| stable/linux-5.15.y       |  Success    |  Success   |

Re: [PATCH 5.15 0/3] ZRAM not releasing backing device backport

[Greg Kroah-Hartman]

On Fri, Jan 10, 2025 at 04:58:41PM +0900, Dominique Martinet wrote:
> This is a resend of the patchset discussed here[1] for the 5.15 tree.
> 
> [1] https://lore.kernel.org/r/2025011052-backpedal-coat-2fec@gregkh
> 
> I've picked the "do not keep dangling zcomp pointer" patch from the
> linux-rc tree at the time, so kept Sasha's SOB and added mine on top
> -- please let me know if it wasn't appropriate.

It's tricky to know, I dropped it and took what was in Linus's tree as
Sasha didn't actually review this one.

All now queued up, thanks!

greg k-h

Re: [PATCH 5.15 0/3] ZRAM not releasing backing device backport

[Dominique Martinet]

Greg Kroah-Hartman wrote on Sun, Jan 12, 2025 at 11:03:42AM +0100:
> On Fri, Jan 10, 2025 at 04:58:41PM +0900, Dominique Martinet wrote:
> > I've picked the "do not keep dangling zcomp pointer" patch from the
> > linux-rc tree at the time, so kept Sasha's SOB and added mine on top
> > -- please let me know if it wasn't appropriate.
> 
> It's tricky to know, I dropped it and took what was in Linus's tree as
> Sasha didn't actually review this one.

Thanks for saying this; I hadn't actually checked the stable backport
(enough) either so I had another look, and the 6d2453c3dbc5
("drivers/block/zram/zram_drv.c: do not keep dangling zcomp pointer
after zram reset") backport by itself is wrong even if it did
cherry-pick cleanly from master and a quick test appeared to work.
The commit messages says "We do all reset operations under write lock"
but that isn't true without also backporting 6f1637795f28 ("zram: fix
race between zram_reset_device() and disksize_store()"), so with the
current backport we've traded leaking zram->comp behind with a data race
on disksize and comp.

With that extra commit as well, I think we're sane enough, but I'm not
familiar with the zram code so I might have missed another prerequisite.


With that and the previous problems, and given that manipulating
zram devices is a privileged operation (so we're not looking at a must
fix vulnerability), I'm actually rather inclined to just drop all the
zram patches and not backport these to 5.15/5.10 unless someone actually
reports problems around zram reset (or perhaps keep 5.15 but skip 5.10
as you see fit)

(
  I'm not opposed to Kairui or someone else actually do these backport,
  but I'm not confident it's worth the effort and think we're trading a
  known problem (current behaviour) with potential unknown ones if
  we're just cherry-picking an arbitrary subset of patches.
  If someone wants to take over, the commits I had identified (from
  Sasha's initial backport and this mail) for 5.10 were as follow:
3b4f85d02a4b ("loop: let set_capacity_revalidate_and_notify update the bdev size")
5dd55749b79c ("nvme: let set_capacity_revalidate_and_notify update the bdev size")
b200e38c493b ("sd: update the bdev size in sd_revalidate_disk")
449f4ec9892e ("block: remove the update_bdev parameter to set_capacity_revalidate_and_notify")
6e017a3931d7 ("zram: use set_capacity_and_notify")
6f1637795f28 ("zram: fix race between zram_reset_device() and disksize_store()")
6d2453c3dbc5 ("drivers/block/zram/zram_drv.c: do not keep dangling zcomp pointer after zram reset")
677294e4da96 ("zram: check comp is non-NULL before calling comp_destroy")
74363ec674cb ("zram: fix uninitialized ZRAM not releasing backing device")
)


Thank you Greg for the follow-ups, and thank you Kairui for the
suggestions during my earlier bug report.
-- 
Dominique