[PATCH] zram: protect recomp_algorithm_show() with ->init_lock

[PATCH] zram: protect recomp_algorithm_show() with ->init_lock

[Sergey Senozhatsky]

sysfs handlers should be called under ->init_lock and are not
supposed to unlock it until return, otherwise e.g. a concurrent
reset() can occur.  There is one handler that breaks that rule:
recomp_algorithm_show().

Move ->init_lock handling outside of __comp_algorithm_show()
(also drop it and call zcomp_available_show() directly) so that
the entire recomp_algorithm_show() loop is protected by the
lock, as opposed to protecting individual iterations.

Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
Reported-by: Seyediman Seyedarab <imandevel@gmail.com>
---
 drivers/block/zram/zram_drv.c | 23 ++++++++---------------
 1 file changed, 8 insertions(+), 15 deletions(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 8acad3cc6e6e..9ac271b82780 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -1225,18 +1225,6 @@ static void comp_algorithm_set(struct zram *zram, u32 prio, const char *alg)
 	zram->comp_algs[prio] = alg;
 }
 
-static ssize_t __comp_algorithm_show(struct zram *zram, u32 prio,
-				     char *buf, ssize_t at)
-{
-	ssize_t sz;
-
-	down_read(&zram->init_lock);
-	sz = zcomp_available_show(zram->comp_algs[prio], buf, at);
-	up_read(&zram->init_lock);
-
-	return sz;
-}
-
 static int __comp_algorithm_store(struct zram *zram, u32 prio, const char *buf)
 {
 	char *compressor;
@@ -1387,8 +1375,12 @@ static ssize_t comp_algorithm_show(struct device *dev,
 				   char *buf)
 {
 	struct zram *zram = dev_to_zram(dev);
+	ssize_t sz;
 
-	return __comp_algorithm_show(zram, ZRAM_PRIMARY_COMP, buf, 0);
+	down_read(&zram->init_lock);
+	sz = zcomp_available_show(zram->comp_algs[ZRAM_PRIMARY_COMP], buf, 0);
+	up_read(&zram->init_lock);
+	return sz;
 }
 
 static ssize_t comp_algorithm_store(struct device *dev,
@@ -1412,14 +1404,15 @@ static ssize_t recomp_algorithm_show(struct device *dev,
 	ssize_t sz = 0;
 	u32 prio;
 
+	down_read(&zram->init_lock);
 	for (prio = ZRAM_SECONDARY_COMP; prio < ZRAM_MAX_COMPS; prio++) {
 		if (!zram->comp_algs[prio])
 			continue;
 
 		sz += sysfs_emit_at(buf, sz, "#%d: ", prio);
-		sz += __comp_algorithm_show(zram, prio, buf, sz);
+		sz += zcomp_available_show(zram->comp_algs[prio], buf, sz);
 	}
-
+	up_read(&zram->init_lock);
 	return sz;
 }
 
-- 
2.50.1.565.gc32cd1483b-goog

Re: [PATCH] zram: protect recomp_algorithm_show() with ->init_lock

[Andrew Morton]

On Tue,  5 Aug 2025 19:19:29 +0900 Sergey Senozhatsky <senozhatsky@chromium.org> wrote:

> sysfs handlers should be called under ->init_lock and are not
> supposed to unlock it until return, otherwise e.g. a concurrent
> reset() can occur.  There is one handler that breaks that rule:
> recomp_algorithm_show().
> 
> Move ->init_lock handling outside of __comp_algorithm_show()
> (also drop it and call zcomp_available_show() directly) so that
> the entire recomp_algorithm_show() loop is protected by the
> lock, as opposed to protecting individual iterations.

As always, I'm wondering "does -stable need this".  But without knowing
the runtime effects of the bug, I cannot know.

Providing this info in the changelog would answer this for everyone, please.

> Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> Reported-by: Seyediman Seyedarab <imandevel@gmail.com>

And including a Closes: for Seyediman's report (if it's publicly
linkable) would be great too, thanks.

I'm guessing that a Fixes: isn't appropriate here because the
bug has been there since day 1.

Re: [PATCH] zram: protect recomp_algorithm_show() with ->init_lock

[Sergey Senozhatsky]

On (25/08/05 15:03), Andrew Morton wrote:
> On Tue,  5 Aug 2025 19:19:29 +0900 Sergey Senozhatsky <senozhatsky@chromium.org> wrote:
> 
> > sysfs handlers should be called under ->init_lock and are not
> > supposed to unlock it until return, otherwise e.g. a concurrent
> > reset() can occur.  There is one handler that breaks that rule:
> > recomp_algorithm_show().
> > 
> > Move ->init_lock handling outside of __comp_algorithm_show()
> > (also drop it and call zcomp_available_show() directly) so that
> > the entire recomp_algorithm_show() loop is protected by the
> > lock, as opposed to protecting individual iterations.
> 
> As always, I'm wondering "does -stable need this".  But without knowing
> the runtime effects of the bug, I cannot know.
> 
> Providing this info in the changelog would answer this for everyone, please.

Sure, Andrew.

The patch does not need to go to -stable, as it does not fix any
runtime errors (at least I can't think of any).  It makes
recomp_algorithm_show() "atomic" w.r.t. zram reset() (just like
the rest of zram sysfs show() handlers), that's a pretty minor
change.

> > Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> > Reported-by: Seyediman Seyedarab <imandevel@gmail.com>
> 
> And including a Closes: for Seyediman's report (if it's publicly
> linkable) would be great too, thanks.
> 
> I'm guessing that a Fixes: isn't appropriate here because the
> bug has been there since day 1.

Yes, also there isn't really a public bug report there, I just noticed
that while looking at some things that Seyediman was looking at.  So I
wanted to give Seyediman some credit.  I suppose I probably should have
added
	Suggested-by: Seyediman Seyedarab <imandevel@gmail.com>

instead.  Should I send a v2?

Re: [PATCH] zram: protect recomp_algorithm_show() with ->init_lock

[Andrew Morton]

On Wed, 6 Aug 2025 11:58:49 +0900 Sergey Senozhatsky <senozhatsky@chromium.org> wrote:

> > > Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> > > Reported-by: Seyediman Seyedarab <imandevel@gmail.com>
> > 
> > And including a Closes: for Seyediman's report (if it's publicly
> > linkable) would be great too, thanks.
> > 
> > I'm guessing that a Fixes: isn't appropriate here because the
> > bug has been there since day 1.
> 
> Yes, also there isn't really a public bug report there, I just noticed
> that while looking at some things that Seyediman was looking at.  So I
> wanted to give Seyediman some credit.  I suppose I probably should have
> added
> 	Suggested-by: Seyediman Seyedarab <imandevel@gmail.com>
> 
> instead.

Great, thanks.

> Should I send a v2?

Nope, I updated the changelog locally.