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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160601064709.GM19976@bbox>
Date:	Wed, 1 Jun 2016 15:47:09 +0900
From:	Minchan Kim <minchan@...nel.org>
To:	Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
CC:	Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Joonsoo Kim <iamjoonsoo.kim@....com>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 4/8] zram: use crypto api to check alg availability

On Wed, Jun 01, 2016 at 12:17:35PM +0900, Sergey Senozhatsky wrote:
> On (06/01/16 11:27), Minchan Kim wrote:
> [..]
> > > > 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?
> > > 
> > > yes, I think so.
> > 
> > It scares me. Common case, except one we choosed, every loaded modules
> > will be not used. I think it's really not good. Although the wastage
> > might be not big now, it will be heavy as crypto comp modules are
> > increased.
> 
> well... if you have those modules enabled then you somehow expect
> them to be loaded at some point, if not by zram, then by something
> else (networking, etc.). /* not speaking of the systems that have
> those modules built-in */ I'm not saying that what we have is
> optimal, of course, but it's not so senseless at the same time.

In my local system, there are a lot of modules I am not using but
distro installed for some point but I believe that some point will
not come. IMO, they shouldn't be loaded by the reason they will be
used some point, potentially.

> 
> 
> > What do you think about it?
> 
> I can do something like this:
> 
> diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
> index 1a4bd20..9b704cc 100644
> --- a/drivers/block/zram/zcomp.c
> +++ b/drivers/block/zram/zcomp.c
> @@ -20,10 +20,18 @@
>  
>  static const char * const backends[] = {
>         "lzo",
> +#if IS_ENABLED(CONFIG_CRYPTO_LZ4)
>         "lz4",
> +#endif
> +#if IS_ENABLED(CONFIG_CRYPTO_DEFLATE)
>         "deflate",
> +#endif
> +#if IS_ENABLED(CONFIG_CRYPTO_LZ4HC)
>         "lz4hc",
> +#endif
> +#if IS_ENABLED(CONFIG_CRYPTO_842)
>         "842",
> +#endif
>         NULL
>  };
> 
> 
> so both BUILTIN and BUILT-AS-A-MODULE cases are handled at compile
> time now and we can avoid crypto_has_comp() checks for most of the
> comp_algorithm calls, except for the case when someone requests an
> out-of-tree module.

Hmm, isn't it problem, either?

That module was built but not installed. In that case, setting the
algorithm will be failed. IOW, we are lying to user.
For solving the problem, if we check it with crypto_has_comp, again,
it will load module into memory. :(

1) Use IS_BUILTIN, not IS_ENABLED for backend

For module-crypto, user should set directly without supporting from
backend.

2) Deprecated /sys/block/zram0/comp_alrogithm totally

Admin should set algorithm by himself like zswap and other cyrpto users.

3) Supporting from zramctl.

Maybe, we can teach zramctl so zramctl can find /lib/modules/`uname-r`/
into not-yet-known-modules and use lsmod to find loaded-known-module for
zram to use compression algorithm.

So, 1,3 combination can be or 2,3 combination can be.

Welcome other suggestion.

> 
> 	-ss