Re: [PATCH v2 4/8] zram: use crypto api to check alg availability

[Minchan Kim]

On Tue, May 31, 2016 at 09:20:13PM +0900, Sergey Senozhatsky wrote:
> There is no way to get a string with all the crypto comp
> algorithms supported by the crypto comp engine, so we need
> to maintain our own backends list. At the same time we
> additionally need to use crypto_has_comp() to make sure
> that the user has requested a compression algorithm that is
> recognized by the crypto comp engine. Relying on /proc/crypto
> is not an options here, because it does not show not-yet-inserted
> compression modules.
> 
> Example:
> 
>  modprobe zram
>  cat /proc/crypto | grep -i lz4
>  modprobe lz4
>  cat /proc/crypto | grep -i lz4
> name         : lz4
> driver       : lz4-generic
> module       : lz4
> 
> So the user can't tell exactly if the lz4 is really supported
> from /proc/crypto output, unless someone or something has loaded
> it.
> 
> This patch also adds crypto_has_comp() to zcomp_available_show().
> We store all the compression algorithms names in zcomp's `backends'
> array, regardless the CONFIG_CRYPTO_FOO configuration, but show
> only those that are also supported by crypto engine. This helps
> user to know the exact list of compression algorithms that can be
> used.

So, if we do 'cat /sys/block/zram0/comp_algorithm", every crypto modules
in the backend array are loaded in memory and not unloaded until admin
executes rmmod? Right?

> 
> Example:
>   module lz4 is not loaded yet, but is supported by the crypto
>   engine. /proc/crypto has no information on this module, while
>   zram's `comp_algorithm' lists it:
> 
>  cat /proc/crypto | grep -i lz4
> 
>  cat /sys/block/zram0/comp_algorithm
> [lzo] lz4 deflate lz4hc 842
> 
> We also now fully rely on crypto_has_comp() when configure a new
> device. The existing `backends' array is kept for user's convenience
> only -- there is no way to list all of the compression algorithms
> supported by crypto -- and is not guaranteed to contain every
> compression module name supported by the kernel. Switch to
> crypto_has_comp() has an advantage of permitting the usage of
> out-of-tree crypto compression modules (implementing S/W or H/W
> compression).

If user load out-of-tree crypto compression module, what's status of
comp_algorithm?

#> insmod foo_crypto.ko
#> echo foo > /sys/block/zram0/comp_algorithm
#> cat /sys/block/zram0/comp_algorithm
lzo lz4 [foo]
?

> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@xxxxxxxxx>
> Cc: Minchan Kim <minchan@xxxxxxxxxx>
> Cc: Joonsoo Kim <iamjoonsoo.kim@xxxxxxx>
> ---
>  Documentation/blockdev/zram.txt | 11 ++++++++
>  drivers/block/zram/zcomp.c      | 58 ++++++++++++++++++++++++-----------------
>  drivers/block/zram/zram_drv.c   | 16 +++++++-----
>  drivers/block/zram/zram_drv.h   |  5 ++--
>  4 files changed, 57 insertions(+), 33 deletions(-)
> 
> diff --git a/Documentation/blockdev/zram.txt b/Documentation/blockdev/zram.txt
> index 13100fb..7c05357 100644
> --- a/Documentation/blockdev/zram.txt
> +++ b/Documentation/blockdev/zram.txt
> @@ -83,6 +83,17 @@ pre-created. Default: 1.
>  	#select lzo compression algorithm
>  	echo lzo > /sys/block/zram0/comp_algorithm
>  
> +	For the time being, the `comp_algorithm' content does not necessarily
> +	show every compression algorithm supported by the kernel. We keep this
> +	list primarily to simplify device configuration and one can configure
> +	a new device with a compression algorithm that is not listed in
> +	`comp_algorithm'. The thing is that, internally, ZRAM uses Crypto API
> +	and, if some of the algorithms were built as modules, it's impossible
> +	to list all of them using, for instance, /proc/crypto or any other
> +	method. This, however, has an advantage of permitting the usage of
> +	custom crypto compression modules (implementing S/W or H/W
> +	compression).
> +
>  4) Set Disksize
>          Set disk size by writing the value to sysfs node 'disksize'.
>          The value can be either in bytes or you can use mem suffixes.
> diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
> index f357268..2381ca9 100644
> --- a/drivers/block/zram/zcomp.c
> +++ b/drivers/block/zram/zcomp.c
> @@ -26,17 +26,6 @@ static const char * const backends[] = {
>  	NULL
>  };
>  
> -static const char *find_backend(const char *compress)
> -{
> -	int i = 0;
> -	while (backends[i]) {
> -		if (sysfs_streq(compress, backends[i]))
> -			break;
> -		i++;
> -	}
> -	return backends[i];
> -}
> -
>  static void zcomp_strm_free(struct zcomp_strm *zstrm)
>  {
>  	if (!IS_ERR_OR_NULL(zstrm->tfm))
> @@ -68,30 +57,53 @@ static struct zcomp_strm *zcomp_strm_alloc(struct zcomp *comp, gfp_t flags)
>  	return zstrm;
>  }
>  
> +bool zcomp_available_algorithm(const char *comp)
> +{
> +	/*
> +	 * Crypto does not ignore a trailing new line symbol,
> +	 * so make sure you don't supply a string containing
> +	 * one.
> +	 * This also means that we keep `backends' array for
> +	 * zcomp_available_show() only and will init a new zram
> +	 * device with any compressing algorithm known to crypto
> +	 * api.
> +	 */
> +	return crypto_has_comp(comp, 0, 0) == 1;
> +}
> +
>  /* show available compressors */
>  ssize_t zcomp_available_show(const char *comp, char *buf)
>  {
> +	bool known_algorithm = false;
>  	ssize_t sz = 0;
>  	int i = 0;
>  
> -	while (backends[i]) {
> -		if (!strcmp(comp, backends[i]))
> +	for (; backends[i]; i++) {
> +		if (!zcomp_available_algorithm(backends[i]))
> +			continue;
> +
> +		if (!strcmp(comp, backends[i])) {
> +			known_algorithm = true;
>  			sz += scnprintf(buf + sz, PAGE_SIZE - sz - 2,
>  					"[%s] ", backends[i]);
> -		else
> +		} else {
>  			sz += scnprintf(buf + sz, PAGE_SIZE - sz - 2,
>  					"%s ", backends[i]);
> -		i++;
> +		}
>  	}
> +
> +	/*
> +	 * Out-of-tree module known to crypto api or a missing
> +	 * entry in `backends'.
> +	 */
> +	if (!known_algorithm && zcomp_available_algorithm(comp))
> +		sz += scnprintf(buf + sz, PAGE_SIZE - sz - 2,
> +				"[%s] ", comp);
> +
>  	sz += scnprintf(buf + sz, PAGE_SIZE - sz, "\n");
>  	return sz;
>  }
>  
> -bool zcomp_available_algorithm(const char *comp)
> -{
> -	return find_backend(comp) != NULL;
> -}
> -
>  struct zcomp_strm *zcomp_stream_get(struct zcomp *comp)
>  {
>  	return *get_cpu_ptr(comp->stream);
> @@ -227,18 +239,16 @@ void zcomp_destroy(struct zcomp *comp)
>  struct zcomp *zcomp_create(const char *compress)
>  {
>  	struct zcomp *comp;
> -	const char *backend;
>  	int error;
>  
> -	backend = find_backend(compress);
> -	if (!backend)
> +	if (!zcomp_available_algorithm(compress))
>  		return ERR_PTR(-EINVAL);
>  
>  	comp = kzalloc(sizeof(struct zcomp), GFP_KERNEL);
>  	if (!comp)
>  		return ERR_PTR(-ENOMEM);
>  
> -	comp->name = backend;
> +	comp->name = compress;
>  	error = zcomp_init(comp);
>  	if (error) {
>  		kfree(comp);
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index 65d1403..c2a1d7d 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -342,9 +342,16 @@ static ssize_t comp_algorithm_store(struct device *dev,
>  		struct device_attribute *attr, const char *buf, size_t len)
>  {
>  	struct zram *zram = dev_to_zram(dev);
> +	char compressor[CRYPTO_MAX_ALG_NAME];
>  	size_t sz;
>  
> -	if (!zcomp_available_algorithm(buf))
> +	strlcpy(compressor, buf, sizeof(compressor));
> +	/* ignore trailing newline */
> +	sz = strlen(compressor);
> +	if (sz > 0 && compressor[sz - 1] == '\n')
> +		compressor[sz - 1] = 0x00;
> +
> +	if (!zcomp_available_algorithm(compressor))
>  		return -EINVAL;
>  
>  	down_write(&zram->init_lock);
> @@ -353,13 +360,8 @@ static ssize_t comp_algorithm_store(struct device *dev,
>  		pr_info("Can't change algorithm for initialized device\n");
>  		return -EBUSY;
>  	}
> -	strlcpy(zram->compressor, buf, sizeof(zram->compressor));
> -
> -	/* ignore trailing newline */
> -	sz = strlen(zram->compressor);
> -	if (sz > 0 && zram->compressor[sz - 1] == '\n')
> -		zram->compressor[sz - 1] = 0x00;
>  
> +	strlcpy(zram->compressor, compressor, sizeof(compressor));
>  	up_write(&zram->init_lock);
>  	return len;
>  }
> diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
> index 3f5bf66..74fcf10 100644
> --- a/drivers/block/zram/zram_drv.h
> +++ b/drivers/block/zram/zram_drv.h
> @@ -15,8 +15,9 @@
>  #ifndef _ZRAM_DRV_H_
>  #define _ZRAM_DRV_H_
>  
> -#include <linux/spinlock.h>
> +#include <linux/rwsem.h>
>  #include <linux/zsmalloc.h>
> +#include <linux/crypto.h>
>  
>  #include "zcomp.h"
>  
> @@ -113,7 +114,7 @@ struct zram {
>  	 * we can store in a disk.
>  	 */
>  	u64 disksize;	/* bytes */
> -	char compressor[10];
> +	char compressor[CRYPTO_MAX_ALG_NAME];
>  	/*
>  	 * zram is claimed so open request will be failed
>  	 */
> -- 
> 2.8.3.394.g3916adf
>