Re: [PATCH] pstore: add lz4hc and 842 compression support

[Kees Cook]

Sorry for the delay on this review!

On Mon, Aug 28, 2017 at 6:56 AM, Geliang Tang <geliangtang@xxxxxxxxx> wrote:
> Currently, pstore has supported three compression algorithms: zlib,
> lzo and lz4. This patch added two more compression algorithms: lz4hc
> and 842.
>
> Signed-off-by: Geliang Tang <geliangtang@xxxxxxxxx>
> ---
>  fs/pstore/Kconfig    |  14 ++++++
>  fs/pstore/platform.c | 128 +++++++++++++++++++++++++++++++++++++++++++++------
>  2 files changed, 127 insertions(+), 15 deletions(-)
>
> diff --git a/fs/pstore/Kconfig b/fs/pstore/Kconfig
> index b42e5bd..e288596 100644
> --- a/fs/pstore/Kconfig
> +++ b/fs/pstore/Kconfig
> @@ -39,6 +39,20 @@ config PSTORE_LZ4_COMPRESS
>          select LZ4_DECOMPRESS
>          help
>            This option enables LZ4 compression algorithm support.
> +
> +config PSTORE_LZ4HC_COMPRESS
> +       bool "LZ4HC"
> +       select LZ4HC_COMPRESS
> +       select LZ4_DECOMPRESS
> +       help
> +         This option enables LZ4 high compression mode algorithm.
> +
> +config PSTORE_842_COMPRESS
> +       bool "842"
> +       select 842_COMPRESS
> +       select 842_DECOMPRESS
> +       help
> +         This option enables 842 compression algorithm support.
>  endchoice

I'm fine with these short descriptions. checkpatch's warning for
"choice" options doesn't always make sense. :)

>
>  config PSTORE_CONSOLE
> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
> index 2b21d18..252c320 100644
> --- a/fs/pstore/platform.c
> +++ b/fs/pstore/platform.c
> @@ -34,9 +34,12 @@
>  #ifdef CONFIG_PSTORE_LZO_COMPRESS
>  #include <linux/lzo.h>
>  #endif
> -#ifdef CONFIG_PSTORE_LZ4_COMPRESS
> +#if defined(CONFIG_PSTORE_LZ4_COMPRESS) || defined(CONFIG_PSTORE_LZ4HC_COMPRESS)
>  #include <linux/lz4.h>
>  #endif
> +#ifdef CONFIG_PSTORE_842_COMPRESS
> +#include <linux/sw842.h>
> +#endif
>  #include <linux/string.h>
>  #include <linux/timer.h>
>  #include <linux/slab.h>
> @@ -337,20 +340,7 @@ static const struct pstore_zbackend backend_lzo = {
>  };
>  #endif
>
> -#ifdef CONFIG_PSTORE_LZ4_COMPRESS
> -static int compress_lz4(const void *in, void *out, size_t inlen, size_t outlen)
> -{
> -       int ret;
> -
> -       ret = LZ4_compress_default(in, out, inlen, outlen, workspace);
> -       if (!ret) {
> -               pr_err("LZ4_compress_default error; compression failed!\n");
> -               return -EIO;
> -       }
> -
> -       return ret;
> -}
> -
> +#if defined(CONFIG_PSTORE_LZ4_COMPRESS) || defined(CONFIG_PSTORE_LZ4HC_COMPRESS)
>  static int decompress_lz4(void *in, void *out, size_t inlen, size_t outlen)
>  {
>         int ret;
> @@ -392,6 +382,21 @@ static void free_lz4(void)
>         big_oops_buf = NULL;
>         big_oops_buf_sz = 0;
>  }
> +#endif
> +
> +#ifdef CONFIG_PSTORE_LZ4_COMPRESS
> +static int compress_lz4(const void *in, void *out, size_t inlen, size_t outlen)
> +{
> +       int ret;
> +
> +       ret = LZ4_compress_default(in, out, inlen, outlen, workspace);
> +       if (!ret) {
> +               pr_err("LZ4_compress_default error; compression failed!\n");
> +               return -EIO;
> +       }
> +
> +       return ret;

Other compress/decompress return outlen, rather than ret. Is "ret" the
outlen here?

> +}
>
>  static const struct pstore_zbackend backend_lz4 = {
>         .compress       = compress_lz4,
> @@ -402,6 +407,95 @@ static const struct pstore_zbackend backend_lz4 = {
>  };
>  #endif
>
> +#ifdef CONFIG_PSTORE_LZ4HC_COMPRESS
> +static int compress_lz4hc(const void *in, void *out,
> +                         size_t inlen, size_t outlen)
> +{
> +       int ret;
> +
> +       ret = LZ4_compress_HC(in, out, inlen, outlen,
> +                             LZ4HC_DEFAULT_CLEVEL, workspace);
> +       if (!ret) {
> +               pr_err("LZ4_compress_HC error; compression failed!\n");
> +               return -EIO;
> +       }
> +
> +       return ret;

And here?

> +}
> +
> +static const struct pstore_zbackend backend_lz4hc = {
> +       .compress       = compress_lz4hc,
> +       .decompress     = decompress_lz4,
> +       .allocate       = allocate_lz4,
> +       .free           = free_lz4,
> +       .name           = "lz4hc",
> +};
> +#endif
> +
> +#ifdef CONFIG_PSTORE_842_COMPRESS
> +static int compress_842(const void *in, void *out, size_t inlen, size_t outlen)
> +{
> +       int ret;
> +       unsigned int len;
> +
> +       ret = sw842_compress(in, inlen, out, &len, workspace);
> +       if (!ret) {
> +               pr_err("sw842_compress error; compression failed!\n");
> +               return -EIO;
> +       }
> +       outlen = len;

This has no effect. What was intended?

> +
> +       return ret;
> +}
> +
> +static int decompress_842(void *in, void *out, size_t inlen, size_t outlen)
> +{
> +       int ret;
> +       unsigned int len;
> +
> +       ret = sw842_decompress(in, inlen, out, &len);
> +       if (ret < 0) {
> +               pr_err("sw842_decompress error, ret = %d!\n", ret);
> +               return -EIO;
> +       }
> +       outlen = len;

Same.

> +
> +       return ret;
> +}
> +
> +static void allocate_842(void)
> +{
> +       big_oops_buf_sz = psinfo->bufsize * 2;
> +       big_oops_buf = kmalloc(big_oops_buf_sz, GFP_KERNEL);
> +       if (big_oops_buf) {
> +               workspace = kmalloc(SW842_MEM_COMPRESS, GFP_KERNEL);
> +               if (!workspace) {
> +                       kfree(big_oops_buf);
> +                       big_oops_buf = NULL;
> +               }
> +       } else {
> +               pr_err("No memory for uncompressed data; skipping compression\n");
> +               workspace = NULL;
> +       }
> +}
> +
> +static void free_842(void)
> +{
> +       kfree(workspace);
> +       kfree(big_oops_buf);
> +       big_oops_buf = NULL;
> +       big_oops_buf_sz = 0;
> +}
> +
> +static const struct pstore_zbackend backend_842 = {
> +       .compress       = compress_842,
> +       .decompress     = decompress_842,
> +       .allocate       = allocate_842,
> +       .free           = free_842,
> +       .name           = "842",
> +};
> +#endif
> +
>  static const struct pstore_zbackend *zbackend =
>  #if defined(CONFIG_PSTORE_ZLIB_COMPRESS)
>         &backend_zlib;
> @@ -409,6 +503,10 @@ static const struct pstore_zbackend *zbackend =
>         &backend_lzo;
>  #elif defined(CONFIG_PSTORE_LZ4_COMPRESS)
>         &backend_lz4;
> +#elif defined(CONFIG_PSTORE_LZ4HC_COMPRESS)
> +       &backend_lz4hc;
> +#elif defined(CONFIG_PSTORE_842_COMPRESS)
> +       &backend_842;
>  #else
>         NULL;
>  #endif
> --
> 2.9.3
>

Otherwise this all looks good. Thanks!

-Kees

-- 
Kees Cook
Pixel Security