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

Minchan Kim
2015-09-08T01:14:04Z

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;
> > }
> >