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

[Minchan Kim <minchan@kernel.org>]

Hey Sergey,

On Tue, Sep 08, 2015 at 08:56:35AM +0900, Sergey Senozhatsky wrote:
> On (09/07/15 21:48), Luis Henriques wrote:
> > Validate the new compression algorithm before copying it into the zram
> > 'compressor' field, keeping the old one if it's invalid.
> > 
> 
> NACK.
> 
> This is intentional. We haven't returned 'invalid compression algorithm'
> error from comp_algorithm_store() historically, so someone's script can
> simply ignore it. However, the script will fail to init the device and
> user will be able to figure out the root cause, because zram will report
> to syslog an actually requested alg name.
> 
> Example
> 
> [ 1669.473296] zram: Cannot initialise llzo compressing backend

I don't understand your concern. To me, this patch makes sense to me.
Could you explain your point clearly, again?

Thanks.
> 
> 
> 	-ss
> 
> > The error path code is also slightly refactored.
> > 
> > Signed-off-by: Luis Henriques <luis.henriques@canonical.com>
> > ---
> >  drivers/block/zram/zram_drv.c | 13 ++++++++-----
> >  1 file changed, 8 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> > index 9c01f5bfa33f..33551ec9e7f5 100644
> > --- a/drivers/block/zram/zram_drv.c
> > +++ b/drivers/block/zram/zram_drv.c
> > @@ -367,10 +367,15 @@ static ssize_t comp_algorithm_store(struct device *dev,
> >  
> >  	down_write(&zram->init_lock);
> >  	if (init_done(zram)) {
> > -		up_write(&zram->init_lock);
> >  		pr_info("Can't change algorithm for initialized device\n");
> > -		return -EBUSY;
> > +		len = -EBUSY;
> > +		goto out;
> > +	}
> > +	if (!zcomp_available_algorithm(buf)) {
> > +		len = -EINVAL;
> > +		goto out;
> >  	}
> > +
> >  	strlcpy(zram->compressor, buf, sizeof(zram->compressor));
> >  
> >  	/* ignore trailing newline */
> > @@ -378,9 +383,7 @@ 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;
> > -
> > +out:
> >  	up_write(&zram->init_lock);
> >  	return len;
> >  }
> >