Message-ID: <20150525142149.GD14922@blaptop>
Date:	Mon, 25 May 2015 23:21:49 +0900
From:	Minchan Kim <minchan@...nel.org>
To:	Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
Cc:	Marcin Jabrzyk <m.jabrzyk@...sung.com>, ngupta@...are.org,
	linux-kernel@...r.kernel.org, linux-mm@...ck.org,
	kyungmin.park@...sung.com
Subject: Re: [PATCH] zram: check compressor name before setting it

On Mon, May 25, 2015 at 03:18:38PM +0900, Sergey Senozhatsky wrote:
> On (05/22/15 15:26), Marcin Jabrzyk wrote:
> > >> From the other hand, the only valid values that can be written are
> > >>in 'comp_algorithm'.
> > >>So when writing other one, returning -EINVAL seems to be reasonable.
> > >>The user would get immediately information that he can't do that,
> > >>now the information can be very deferred in time.
> > >
> > >it's not.
> > >the error message appears in syslog right before we return -EINVAL
> > >back to user.
> > 
> > Yes I've read up the code more detailed and I saw that error message
> > just before returning to user with error value.
> > 
> > But this happens when 'disksize' is wirtten, not when 'comp_algorithm'.
> > I understood, the error message in dmesg is clear there is no such
> > algorithm.
> > 
> > But this is not an immediate error, when setting the 'comp_algorithm',
> > where we already know that it's wrong, not existing etc.
> > Anything after that moment would be wrong and would not work at all.
> > 
> > From what I saw 'comp_algorithm_store' is the only *_store in zram that
> > believes user that he writes proper value and just makes strlcpy.
> > 
> > So what I've ing mind is to provide direct feedback, you have
> > written wrong name of compressor, you got -EINVAL, please write
> > correct value. This would be very useful when scripting.
> > 
> 
> I can't see how printing error 0.0012 seconds earlier helps. really.
> if one sets a compression algorithm the very next thing to do is to
> set device's disksize. even if he/she usually watch a baseball game in
> between, then the error message appears right when it's needed anyway:
> during `setup my device and make it usable' stage.
> 
> 
> >I'm not for exposing more internals, but getting -EINVAL would be nice I
> 
> you are.
> 
> find_backend() returns back to its caller a raw and completely initialized
> zcomp_backend pointer. this is very dangerous zcomp internals, which should
> never be exposed. from zcomp layer we return either ERR_PTR() or a usable
> zcomp_backend pointer. that's the rule.
> 
> 
> if you guys still insist that this is critical and very important change,
> then there should be a small helper function instead with a clear name
> (starting with zcomp_ to indicate its place) which will simply return bool
> TRUE for `algorithm is known' case and FALSE otherwise (aka `algorithm is
> unknown').
> 
> 
> something like below:
> 
> 
>   # echo LZ5 > /sys/block/zram0/comp_algorithm
>   -bash: echo: write error: Invalid argument
> 
>   dmesg
>   [ 7440.544852] Error: unknown compression algorithm: LZ5
> 
> 
> p.s. but, I still see a very little value.
> p.p.s notice a necessary and inevitable `dmesg'. (back to 'no one reads
> syslog' argument).
> 
> ---
> 
>  drivers/block/zram/zcomp.c    | 10 ++++++++++
>  drivers/block/zram/zcomp.h    |  1 +
>  drivers/block/zram/zram_drv.c |  3 +++
>  3 files changed, 14 insertions(+)
> 
> diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
> index a1a8b8e..b68b16f 100644
> --- a/drivers/block/zram/zcomp.c
> +++ b/drivers/block/zram/zcomp.c
> @@ -54,11 +54,16 @@ static struct zcomp_backend *backends[] = {
>  static struct zcomp_backend *find_backend(const char *compress)
>  {
>  	int i = 0;
> +
>  	while (backends[i]) {
>  		if (sysfs_streq(compress, backends[i]->name))
>  			break;
>  		i++;
>  	}
> +
> +	if (!backends[i])
> +		pr_err("Error: unknown compression algorithm: %s\n",
> +				compress);

find_backend is just utility function to get zcomp_backend.
IOW, it might be used for several cases in future so I want
make error report as caller's work.

>  	return backends[i];
>  }
>  
> @@ -320,6 +325,11 @@ void zcomp_destroy(struct zcomp *comp)
>  	kfree(comp);
>  }
>  
> +bool zcomp_known_algorithm(const char *comp)
> +{
> +	return find_backend(comp) != NULL;
> +}
> +
>  /*
>   * search available compressors for requested algorithm.
>   * allocate new zcomp and initialize it. return compressing
> diff --git a/drivers/block/zram/zcomp.h b/drivers/block/zram/zcomp.h
> index c59d1fc..773bdf1 100644
> --- a/drivers/block/zram/zcomp.h
> +++ b/drivers/block/zram/zcomp.h
> @@ -51,6 +51,7 @@ struct zcomp {
>  };
>  
>  ssize_t zcomp_available_show(const char *comp, char *buf);
> +bool zcomp_known_algorithm(const char *comp);
>  
>  struct zcomp *zcomp_create(const char *comp, int max_strm);
>  void zcomp_destroy(struct zcomp *comp);
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index f750e34..2197a81 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -378,6 +378,9 @@ static ssize_t comp_algorithm_store(struct device *dev,
>  	if (sz > 0 && zram->compressor[sz - 1] == '\n')
>  		zram->compressor[sz - 1] = 0x00;
>  
> +	if (!zcomp_known_algorithm(zram->compressor))

In here, we could report back to the user.

> +		len = -EINVAL;
> +
>  	up_write(&zram->init_lock);
>  	return len;
>  }
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@...ck.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@...ck.org"> email@...ck.org </a>

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

Powered by blists - more mailing lists