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

[Minchan Kim <minchan@kernel.org>]

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
>