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(-)
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); }
[ 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 Martinetdominique.martinet@atmark-techno.com Commit author: Sergey Senozhatskysenozhatsky@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 |
[ 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); }
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#opti...
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%4...
Please ignore this mail if the patch is not relevant for upstream.
[ 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 |
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);
[ 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 Martinetdominique.martinet@atmark-techno.com Commit author: Kairui Songkasong@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 |
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
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.
linux-stable-mirror@lists.linaro.org
-
Dominique Martinet -
Greg Kroah-Hartman -
kernel test robot -
Sasha Levin