Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752473AbcD3C3S (ORCPT ); Fri, 29 Apr 2016 22:29:18 -0400 Received: from m50-132.163.com ([123.125.50.132]:52463 "EHLO m50-132.163.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751679AbcD3C3Q (ORCPT ); Fri, 29 Apr 2016 22:29:16 -0400 Date: Sat, 30 Apr 2016 10:26:38 +0800 From: Geliang Tang To: Kees Cook , Tony Luck , Matt Fleming , Anton Vorontsov , Colin Cross Cc: linux-efi@vger.kernel.org, linux-kernel@vger.kernel.org, Geliang Tang Subject: Re: [PATCH v2] pstore: add lzo/lz4 compression support Message-ID: <20160430022637.GA8702@localhost.localdomain> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) X-CM-TRANSID: DNGowAD3___eFyRXVruiCA--.4017S3 X-Coremail-Antispam: 1Uf129KBjvAXoW3Cw1kurW3Kr45XryfWw18uFg_yoW8GrW8Ko Wftrs3Wr1xG342gr4kAryIqayIvw1Fgw18JFZ5KwnruasIqa1j934UJw43tayfCF4jgryv yayktFyUZFW0q3Wrn29KB7ZKAUJUUUUU529EdanIXcx71UUUUU7v73VFW2AGmfu7bjvjm3 AaLaJ3UbIYCTnIWIevJa73UjIFyTuYvjxUsaZXUUUUU X-Originating-IP: [221.204.170.5] X-CM-SenderInfo: 5jhoxtpqjwt0rj6rljoofrz/1tbiJR5bmVUL-Z+XegAAso Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 16775 Lines: 468 On Thu, Feb 18, 2016 at 09:37:26AM -0800, Kees Cook wrote: > On Thu, Feb 18, 2016 at 6:04 AM, Geliang Tang wrote: > > Like zlib compression in pstore, this patch added lzo and lz4 > > compression support so that users can have more options and better > > compression ratio. > > > > The original code treats the compressed data together with the > > uncompressed ECC correction notice by using zlib decompress. The > > ECC correction notice is missing in the decompression process. The > > treatment also makes lzo and lz4 not working. So I treat them > > separately by using pstore_decompress() to treat the compressed > > data, and memcpy() to treat the uncompressed ECC correction notice. > > > > Signed-off-by: Geliang Tang > > Thanks for the updates! > > Reviewed-by: Kees Cook > > -Kees Hi there, It's been three months since this patch has been reviewed. But it hasn't been replied yet by far. Can I please know if there's anything wrong with it? -Geliang Tang > > --- > > Changes in v2: > > revise the patch as Kees suggested: > > - drop 'depends on PSTORE' in config choice PSTORE_ZLIB/LZO/LZ4_COMPRESS; > > - set big_oops_buf_sz to 0 in free_zlib/lzo/lz4; > > - change zbackend[] to zbackend. > > --- > > drivers/firmware/efi/efi-pstore.c | 1 + > > fs/pstore/Kconfig | 31 +++++- > > fs/pstore/platform.c | 218 ++++++++++++++++++++++++++++++++++++-- > > fs/pstore/ram.c | 10 +- > > include/linux/pstore.h | 3 +- > > 5 files changed, 248 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/firmware/efi/efi-pstore.c b/drivers/firmware/efi/efi-pstore.c > > index eac76a7..cd8c35f 100644 > > --- a/drivers/firmware/efi/efi-pstore.c > > +++ b/drivers/firmware/efi/efi-pstore.c > > @@ -210,6 +210,7 @@ static int efi_pstore_sysfs_entry_iter(void *data, struct efivar_entry **pos) > > static ssize_t efi_pstore_read(u64 *id, enum pstore_type_id *type, > > int *count, struct timespec *timespec, > > char **buf, bool *compressed, > > + ssize_t *ecc_notice_size, > > struct pstore_info *psi) > > { > > struct pstore_read_data data; > > diff --git a/fs/pstore/Kconfig b/fs/pstore/Kconfig > > index 360ae43..be40813 100644 > > --- a/fs/pstore/Kconfig > > +++ b/fs/pstore/Kconfig > > @@ -1,8 +1,6 @@ > > config PSTORE > > tristate "Persistent store support" > > default n > > - select ZLIB_DEFLATE > > - select ZLIB_INFLATE > > help > > This option enables generic access to platform level > > persistent storage via "pstore" filesystem that can > > @@ -14,6 +12,35 @@ config PSTORE > > If you don't have a platform persistent store driver, > > say N. > > > > +choice > > + prompt "Choose compression algorithm" > > + depends on PSTORE > > + default PSTORE_ZLIB_COMPRESS > > + help > > + This option chooses compression algorithm. > > + > > +config PSTORE_ZLIB_COMPRESS > > + bool "ZLIB" > > + select ZLIB_DEFLATE > > + select ZLIB_INFLATE > > + help > > + This option enables ZLIB compression algorithm support. > > + > > +config PSTORE_LZO_COMPRESS > > + bool "LZO" > > + select LZO_COMPRESS > > + select LZO_DECOMPRESS > > + help > > + This option enables LZO compression algorithm support. > > + > > +config PSTORE_LZ4_COMPRESS > > + bool "LZ4" > > + select LZ4_COMPRESS > > + select LZ4_DECOMPRESS > > + help > > + This option enables LZ4 compression algorithm support. > > +endchoice > > + > > config PSTORE_CONSOLE > > bool "Log kernel console messages" > > depends on PSTORE > > diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c > > index 588461b..7246d50 100644 > > --- a/fs/pstore/platform.c > > +++ b/fs/pstore/platform.c > > @@ -28,7 +28,15 @@ > > #include > > #include > > #include > > +#ifdef CONFIG_PSTORE_ZLIB_COMPRESS > > #include > > +#endif > > +#ifdef CONFIG_PSTORE_LZO_COMPRESS > > +#include > > +#endif > > +#ifdef CONFIG_PSTORE_LZ4_COMPRESS > > +#include > > +#endif > > #include > > #include > > #include > > @@ -69,10 +77,23 @@ struct pstore_info *psinfo; > > static char *backend; > > > > /* Compression parameters */ > > +#ifdef CONFIG_PSTORE_ZLIB_COMPRESS > > #define COMPR_LEVEL 6 > > #define WINDOW_BITS 12 > > #define MEM_LEVEL 4 > > static struct z_stream_s stream; > > +#else > > +static unsigned char *workspace; > > +#endif > > + > > +struct pstore_zbackend { > > + int (*compress)(const void *in, void *out, size_t inlen, size_t outlen); > > + int (*decompress)(void *in, void *out, size_t inlen, size_t outlen); > > + void (*allocate)(void); > > + void (*free)(void); > > + > > + const char *name; > > +}; > > > > static char *big_oops_buf; > > static size_t big_oops_buf_sz; > > @@ -129,9 +150,9 @@ bool pstore_cannot_block_path(enum kmsg_dump_reason reason) > > } > > EXPORT_SYMBOL_GPL(pstore_cannot_block_path); > > > > +#ifdef CONFIG_PSTORE_ZLIB_COMPRESS > > /* Derived from logfs_compress() */ > > -static int pstore_compress(const void *in, void *out, size_t inlen, > > - size_t outlen) > > +static int compress_zlib(const void *in, void *out, size_t inlen, size_t outlen) > > { > > int err, ret; > > > > @@ -165,7 +186,7 @@ error: > > } > > > > /* Derived from logfs_uncompress */ > > -static int pstore_decompress(void *in, void *out, size_t inlen, size_t outlen) > > +static int decompress_zlib(void *in, void *out, size_t inlen, size_t outlen) > > { > > int err, ret; > > > > @@ -194,7 +215,7 @@ error: > > return ret; > > } > > > > -static void allocate_buf_for_compression(void) > > +static void allocate_zlib(void) > > { > > size_t size; > > size_t cmpr; > > @@ -237,12 +258,190 @@ static void allocate_buf_for_compression(void) > > > > } > > > > -static void free_buf_for_compression(void) > > +static void free_zlib(void) > > { > > kfree(stream.workspace); > > stream.workspace = NULL; > > kfree(big_oops_buf); > > big_oops_buf = NULL; > > + big_oops_buf_sz = 0; > > +} > > + > > +static struct pstore_zbackend backend_zlib = { > > + .compress = compress_zlib, > > + .decompress = decompress_zlib, > > + .allocate = allocate_zlib, > > + .free = free_zlib, > > + .name = "zlib", > > +}; > > +#endif > > + > > +#ifdef CONFIG_PSTORE_LZO_COMPRESS > > +static int compress_lzo(const void *in, void *out, size_t inlen, size_t outlen) > > +{ > > + int ret; > > + > > + ret = lzo1x_1_compress(in, inlen, out, &outlen, workspace); > > + if (ret != LZO_E_OK) { > > + pr_err("lzo_compress error, ret = %d!\n", ret); > > + return -EIO; > > + } > > + > > + return outlen; > > +} > > + > > +static int decompress_lzo(void *in, void *out, size_t inlen, size_t outlen) > > +{ > > + int ret; > > + > > + ret = lzo1x_decompress_safe(in, inlen, out, &outlen); > > + if (ret != LZO_E_OK) { > > + pr_err("lzo_decompress error, ret = %d!\n", ret); > > + return -EIO; > > + } > > + > > + return outlen; > > +} > > + > > +static void allocate_lzo(void) > > +{ > > + big_oops_buf_sz = lzo1x_worst_compress(psinfo->bufsize); > > + big_oops_buf = kmalloc(big_oops_buf_sz, GFP_KERNEL); > > + if (big_oops_buf) { > > + workspace = kmalloc(LZO1X_MEM_COMPRESS, GFP_KERNEL); > > + if (!workspace) { > > + pr_err("No memory for compression workspace; skipping compression\n"); > > + kfree(big_oops_buf); > > + big_oops_buf = NULL; > > + } > > + } else { > > + pr_err("No memory for uncompressed data; skipping compression\n"); > > + workspace = NULL; > > + } > > +} > > + > > +static void free_lzo(void) > > +{ > > + kfree(workspace); > > + kfree(big_oops_buf); > > + big_oops_buf = NULL; > > + big_oops_buf_sz = 0; > > +} > > + > > +static struct pstore_zbackend backend_lzo = { > > + .compress = compress_lzo, > > + .decompress = decompress_lzo, > > + .allocate = allocate_lzo, > > + .free = free_lzo, > > + .name = "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(in, inlen, out, &outlen, workspace); > > + if (ret) { > > + pr_err("lz4_compress error, ret = %d!\n", ret); > > + return -EIO; > > + } > > + > > + return outlen; > > +} > > + > > +static int decompress_lz4(void *in, void *out, size_t inlen, size_t outlen) > > +{ > > + int ret; > > + > > + ret = lz4_decompress_unknownoutputsize(in, inlen, out, &outlen); > > + if (ret) { > > + pr_err("lz4_decompress error, ret = %d!\n", ret); > > + return -EIO; > > + } > > + > > + return outlen; > > +} > > + > > +static void allocate_lz4(void) > > +{ > > + big_oops_buf_sz = lz4_compressbound(psinfo->bufsize); > > + big_oops_buf = kmalloc(big_oops_buf_sz, GFP_KERNEL); > > + if (big_oops_buf) { > > + workspace = kmalloc(LZ4_MEM_COMPRESS, GFP_KERNEL); > > + if (!workspace) { > > + pr_err("No memory for compression workspace; skipping compression\n"); > > + kfree(big_oops_buf); > > + big_oops_buf = NULL; > > + } > > + } else { > > + pr_err("No memory for uncompressed data; skipping compression\n"); > > + workspace = NULL; > > + } > > +} > > + > > +static void free_lz4(void) > > +{ > > + kfree(workspace); > > + kfree(big_oops_buf); > > + big_oops_buf = NULL; > > + big_oops_buf_sz = 0; > > +} > > + > > +static struct pstore_zbackend backend_lz4 = { > > + .compress = compress_lz4, > > + .decompress = decompress_lz4, > > + .allocate = allocate_lz4, > > + .free = free_lz4, > > + .name = "lz4", > > +}; > > +#endif > > + > > +static struct pstore_zbackend *zbackend = > > +#if defined(CONFIG_PSTORE_ZLIB_COMPRESS) > > + &backend_zlib; > > +#elif defined(CONFIG_PSTORE_LZO_COMPRESS) > > + &backend_lzo; > > +#elif defined(CONFIG_PSTORE_LZ4_COMPRESS) > > + &backend_lz4; > > +#else > > + NULL; > > +#endif > > + > > +static int pstore_compress(const void *in, void *out, > > + size_t inlen, size_t outlen) > > +{ > > + if (zbackend) > > + return zbackend->compress(in, out, inlen, outlen); > > + else > > + return -EIO; > > +} > > + > > +static int pstore_decompress(void *in, void *out, size_t inlen, size_t outlen) > > +{ > > + if (zbackend) > > + return zbackend->decompress(in, out, inlen, outlen); > > + else > > + return -EIO; > > +} > > + > > +static void allocate_buf_for_compression(void) > > +{ > > + if (zbackend) { > > + pr_info("using %s compression\n", zbackend->name); > > + zbackend->allocate(); > > + } else { > > + pr_err("allocate compression buffer error!\n"); > > + } > > +} > > + > > +static void free_buf_for_compression(void) > > +{ > > + if (zbackend) > > + zbackend->free(); > > + else > > + pr_err("free compression buffer error!\n"); > > } > > > > /* > > @@ -527,6 +726,7 @@ void pstore_get_records(int quiet) > > int failed = 0, rc; > > bool compressed; > > int unzipped_len = -1; > > + ssize_t ecc_notice_size = 0; > > > > if (!psi) > > return; > > @@ -536,7 +736,7 @@ void pstore_get_records(int quiet) > > goto out; > > > > while ((size = psi->read(&id, &type, &count, &time, &buf, &compressed, > > - psi)) > 0) { > > + &ecc_notice_size, psi)) > 0) { > > if (compressed && (type == PSTORE_TYPE_DMESG)) { > > if (big_oops_buf) > > unzipped_len = pstore_decompress(buf, > > @@ -544,6 +744,9 @@ void pstore_get_records(int quiet) > > big_oops_buf_sz); > > > > if (unzipped_len > 0) { > > + if (ecc_notice_size) > > + memcpy(big_oops_buf + unzipped_len, > > + buf + size, ecc_notice_size); > > kfree(buf); > > buf = big_oops_buf; > > size = unzipped_len; > > @@ -555,7 +758,8 @@ void pstore_get_records(int quiet) > > } > > } > > rc = pstore_mkfile(type, psi->name, id, count, buf, > > - compressed, (size_t)size, time, psi); > > + compressed, size + ecc_notice_size, > > + time, psi); > > if (unzipped_len < 0) { > > /* Free buffer other than big oops */ > > kfree(buf); > > diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c > > index 319c3a6..11ec024 100644 > > --- a/fs/pstore/ram.c > > +++ b/fs/pstore/ram.c > > @@ -181,10 +181,10 @@ static bool prz_ok(struct persistent_ram_zone *prz) > > static ssize_t ramoops_pstore_read(u64 *id, enum pstore_type_id *type, > > int *count, struct timespec *time, > > char **buf, bool *compressed, > > + ssize_t *ecc_notice_size, > > struct pstore_info *psi) > > { > > ssize_t size; > > - ssize_t ecc_notice_size; > > struct ramoops_context *cxt = psi->data; > > struct persistent_ram_zone *prz = NULL; > > int header_length = 0; > > @@ -229,16 +229,16 @@ static ssize_t ramoops_pstore_read(u64 *id, enum pstore_type_id *type, > > size = persistent_ram_old_size(prz) - header_length; > > > > /* ECC correction notice */ > > - ecc_notice_size = persistent_ram_ecc_string(prz, NULL, 0); > > + *ecc_notice_size = persistent_ram_ecc_string(prz, NULL, 0); > > > > - *buf = kmalloc(size + ecc_notice_size + 1, GFP_KERNEL); > > + *buf = kmalloc(size + *ecc_notice_size + 1, GFP_KERNEL); > > if (*buf == NULL) > > return -ENOMEM; > > > > memcpy(*buf, (char *)persistent_ram_old(prz) + header_length, size); > > - persistent_ram_ecc_string(prz, *buf + size, ecc_notice_size + 1); > > + persistent_ram_ecc_string(prz, *buf + size, *ecc_notice_size + 1); > > > > - return size + ecc_notice_size; > > + return size; > > } > > > > static size_t ramoops_write_kmsg_hdr(struct persistent_ram_zone *prz, > > diff --git a/include/linux/pstore.h b/include/linux/pstore.h > > index 831479f..899e95e 100644 > > --- a/include/linux/pstore.h > > +++ b/include/linux/pstore.h > > @@ -58,7 +58,8 @@ struct pstore_info { > > int (*close)(struct pstore_info *psi); > > ssize_t (*read)(u64 *id, enum pstore_type_id *type, > > int *count, struct timespec *time, char **buf, > > - bool *compressed, struct pstore_info *psi); > > + bool *compressed, ssize_t *ecc_notice_size, > > + struct pstore_info *psi); > > int (*write)(enum pstore_type_id type, > > enum kmsg_dump_reason reason, u64 *id, > > unsigned int part, int count, bool compressed, > > -- > > 2.5.0 > > > > > > > > -- > Kees Cook > Chrome OS & Brillo Security