linux-kernel - Re: [PATCH v3] zram: break the strict dependency from lzo

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: <CALjTZvaTiAS1vMyrTNvrfy7KOjemy+tW5-hbTy5SHYfsbRE=_Q@mail.gmail.com>
Date:   Wed, 28 Oct 2020 11:25:32 +0000
From:   Rui Salvaterra <rsalvaterra@...il.com>
To:     Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
Cc:     minchan@...nel.org, ngupta@...are.org,
        linux-kernel@...r.kernel.org, linux-block@...r.kernel.org
Subject: Re: [PATCH v3] zram: break the strict dependency from lzo

Hi, Sergey,

On Wed, 28 Oct 2020 at 10:19, Sergey Senozhatsky
<sergey.senozhatsky.work@...il.com> wrote:
>
> Can the following work then?

Almost! See below. :)

> Completely untested.
>
> ===8<===
>
> diff --git a/drivers/block/zram/Kconfig b/drivers/block/zram/Kconfig
> index fe7a4b7d30cf..f93eed40e155 100644
> --- a/drivers/block/zram/Kconfig
> +++ b/drivers/block/zram/Kconfig
> @@ -2,7 +2,7 @@
>  config ZRAM
>         tristate "Compressed RAM block device support"
>         depends on BLOCK && SYSFS && ZSMALLOC && CRYPTO
> -       select CRYPTO_LZO
> +       depends on (CRYPTO_LZO || CRYPTO_LZ4 || CRYPTO_LZ4HC || CRYPTO_842 || CRYPTO_ZSTD)

This reverses the dependency order, as now we have to select a
supported compression algorithm in order for zram to be visible in the
block device drivers list. This is why I wrote the ZRAM_AUTOSEL_ALGO
kconfig symbol, which automatically selects lzo as a fallback. If the
user chooses to select another supported algorithm, he will then be
allowed to deselect lzo. We thus follow the principle of least
surprise, IMHO.

>         help
>           Creates virtual block devices called /dev/zramX (X = 0, 1, ...).
>           Pages written to these disks are compressed and stored in memory
> diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
> index 33e3b76c4fa9..98c7c46c9c3a 100644
> --- a/drivers/block/zram/zcomp.c
> +++ b/drivers/block/zram/zcomp.c
> @@ -15,8 +15,10 @@
>  #include "zcomp.h"
>
>  static const char * const backends[] = {
> +#if IS_ENABLED(CONFIG_CRYPTO_LZO)
>         "lzo",
>         "lzo-rle",
> +#endif
>  #if IS_ENABLED(CONFIG_CRYPTO_LZ4)
>         "lz4",
>  #endif
> @@ -202,6 +204,21 @@ void zcomp_destroy(struct zcomp *comp)
>         kfree(comp);
>  }
>
> +const char *default_compressor(void)
> +{
> +       /*
> +        * Pick the first available one (there should be at least one).
> +        *
> +        * In theory, we can drop all the ifdefs from backends[] and
> +        * just iterate backends array doing crypto_has_comp(comp, 0, 0)
> +        * for each entry and return the first one which is recognized by
> +        * crypto. But crypto_has_comp() modprobes compression drivers,
> +        * so we may endup with extra loaded drivers, when the 'default'
> +        * compressor is not what zram is configured to use.
> +        */
> +       return backends[0];
> +}

This is much more elegant, indeed, and I completely agree with just
returning the first one. I'll incorporate your feedback and send a v4
soon.

Thanks a lot,
Rui