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

Minchan Kim
2015-09-08T13:32:54Z

On Tue, Sep 08, 2015 at 06:54:28PM +0900, Sergey Senozhatsky wrote:
> 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.

Wrong quiz.
For the helping finding, user should do this.

echo zzz > /sys/block/zram0/comp_algorithm
echo $?

If he want to not show error message, he should do.
echo zzz > /sys/block/zram/comp_algorithm 2> /dev/null
echo $?

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

You are saying implementation of kernel, not interface itself.
Nothing different. It's a interface to say something from the user
to the kenrel. If user passes wrong input, normally, kernel should return
a error and user should check it.
It's a general rule for the programming for several decades.

>
> 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@gmail.com>
> 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;

If you ignore tailling space, your change would make a bug.
If you fix it, it looks good to me.
I hope Luis can send it with his SOB and indication of
your credit about checking with avoiding unnecessary locking if you don't mind.

Thanks, Guys!

> +
> 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
>