Re: [PATCH] zram: don't copy invalid compression algorithms

[Sergey Senozhatsky]

On (09/08/15 17:16), Minchan Kim wrote:
> we should help them to *correct* it rather than keeping such weired
> thing.

A simple quiz

A)
echo zzz > /sys/block/zram0/comp_algorithm > /dev/null
echo 1G > /sys/block/zram0/disksize
-bash: echo: write error: Invalid argument
echo $?
1


B)
echo zzz > /sys/block/zram0/comp_algorithm > /dev/null
echo 1G > /sys/block/zram0/disksize
echo $?
0


which one *DOES* help finding and correcting an error and which one *DOES NOT*?
a million dolla question.

the difference between comp_algorithm store and any other store function - is
that comp_algorithm_store DOES ABSOLUTELY NOTHING. it does not allocate memory,
free memory, register or unregister anything, change backends, etc., etc., etc.
it does none of those. its only purpose is to strcpy() given data. this data
will be used later by a completely different function as a result of additional
actions taken by user space.

Returning back to our quiz. I do suspect that the answer is... "B"!


So, I still NACK the patch. It introduces a goto label, etc. In fact all
we need to do is to move zcomp_available_algorithm() up, before we grab
the ->init_lock. zcomp_available_algorithm() does not depend on anything
that requires a ->init_lock protection.

Next, the patch lacks a reasoning/motivation in its commit message. What
we do in fact here is we introduce compression algorithm fallback to a
previously selected (knowingly supported, which has already passed
zcomp_available_algorithm()) or the `default_compressor'.

Summarizing, it's something like this:

---

From: Sergey SENOZHATSKY <sergey.senozhatsky@xxxxxxxxx>
Subject: [PATCH] zram: introduce comp algorithm fallback functionality

When user supplies an unsupported compression algorithm, keep
a previously selected one (knowingly supported) or the default
one (in case if compression algorithm hasn't been changed before).
---
 drivers/block/zram/zram_drv.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 55e09db..255d68b 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -365,6 +365,9 @@ static ssize_t comp_algorithm_store(struct device *dev,
 	struct zram *zram = dev_to_zram(dev);
 	size_t sz;
 
+	if (!zcomp_available_algorithm(buf))
+		return -EINVAL;
+
 	down_write(&zram->init_lock);
 	if (init_done(zram)) {
 		up_write(&zram->init_lock);
@@ -378,9 +381,6 @@ static ssize_t comp_algorithm_store(struct device *dev,
 	if (sz > 0 && zram->compressor[sz - 1] == '\n')
 		zram->compressor[sz - 1] = 0x00;
 
-	if (!zcomp_available_algorithm(zram->compressor))
-		len = -EINVAL;
-
 	up_write(&zram->init_lock);
 	return len;
 }
-- 
2.5.1.454.g1616360

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/