On Wed, Oct 17, 2018 at 9:42 PM, Sai Prakash Ranjan <saiprakash.ran...@codeaurora.org> wrote: > On 10/18/2018 7:00 AM, Joel Fernandes wrote: > On Wed, Oct 17, 2018 at 02:41:24PM -0700, Kees Cook wrote: >>> >>> With compression initialization now separated from pstore_register(), >>> there is no longer a good reason to do compression method selection >>> during fs init. Instead, merge everything together into the late init. >>> Additionally cleans up the reporting to be more clear. >>> >>> Signed-off-by: Kees Cook <keesc...@chromium.org> >>> --- >>> This patch goes on top of Joel's. I'll have both in linux-next shortly... >>> --- >>> fs/pstore/inode.c | 2 -- >>> fs/pstore/internal.h | 3 -- >>> fs/pstore/platform.c | 65 ++++++++++++++++++++++++++++---------------- >>> 3 files changed, 41 insertions(+), 29 deletions(-) >>> >>> diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c >>> index 5fcb845b9fec..d814723fb27d 100644 >>> --- a/fs/pstore/inode.c >>> +++ b/fs/pstore/inode.c >>> @@ -486,8 +486,6 @@ static int __init init_pstore_fs(void) >>> { >>> int err; >>> - pstore_choose_compression(); >>> - >>> /* Create a convenient mount point for people to access pstore */ >>> err = sysfs_create_mount_point(fs_kobj, "pstore"); >>> if (err) >>> diff --git a/fs/pstore/internal.h b/fs/pstore/internal.h >>> index fb767e28aeb2..c029314478fa 100644 >>> --- a/fs/pstore/internal.h >>> +++ b/fs/pstore/internal.h >>> @@ -37,7 +37,4 @@ extern bool pstore_is_mounted(void); >>> extern void pstore_record_init(struct pstore_record *record, >>> struct pstore_info *psi); >>> -/* Called during module_init() */ >>> -extern void __init pstore_choose_compression(void); >>> - >>> #endif >>> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c >>> index f09066db2d4d..e4737a5fa68c 100644 >>> --- a/fs/pstore/platform.c >>> +++ b/fs/pstore/platform.c >>> @@ -274,36 +274,56 @@ static int pstore_decompress(void *in, void *out, >>> static void allocate_buf_for_compression(void) >>> { >>> + struct crypto_comp *ctx; >>> + int size; >>> + char *buf; >>> + >>> + /* Skip if not built-in or compression backend not selected yet. >>> */ >>> if (!IS_ENABLED(CONFIG_PSTORE_COMPRESS) || !zbackend) >>> return; >>> + /* Skip if no pstore backend yet or compression init already >>> done. */ >>> + if (!psinfo || tfm) >>> + return; >>> + >>> if (!crypto_has_comp(zbackend->name, 0, 0)) { >>> - pr_err("No %s compression\n", zbackend->name); >>> + pr_err("Unknown compression: %s\n", zbackend->name); >>> return; >>> } >>> - big_oops_buf_sz = zbackend->zbufsize(psinfo->bufsize); >>> - if (big_oops_buf_sz <= 0) >>> + size = zbackend->zbufsize(psinfo->bufsize); >>> + if (size <= 0) { >>> + pr_err("Invalid compression size for %s: %d\n", >>> + zbackend->name, size); >>> return; >>> + } >>> - big_oops_buf = kmalloc(big_oops_buf_sz, GFP_KERNEL); >>> - if (!big_oops_buf) { >>> - pr_err("allocate compression buffer error!\n"); >>> + buf = kmalloc(size, GFP_KERNEL); >>> + if (!buf) { >>> + pr_err("Failed %d byte compression buffer allocation for: >>> %s\n", >>> + size, zbackend->name); >>> return; >>> } >>> - tfm = crypto_alloc_comp(zbackend->name, 0, 0); >>> - if (IS_ERR_OR_NULL(tfm)) { >>> - kfree(big_oops_buf); >>> - big_oops_buf = NULL; >>> - pr_err("crypto_alloc_comp() failed!\n"); >>> + ctx = crypto_alloc_comp(zbackend->name, 0, 0); >>> + if (IS_ERR_OR_NULL(ctx)) { >>> + kfree(buf); >>> + pr_err("crypto_alloc_comp('%s') failed: %ld\n", >>> zbackend->name, >>> + PTR_ERR(ctx)); >>> return; >>> } >>> + >>> + /* A non-NULL big_oops_buf indicates compression is available. */ >>> + tfm = ctx; >>> + big_oops_buf_sz = size; >>> + big_oops_buf = buf; >>> + >>> + pr_info("Using compression: %s\n", zbackend->name); >>> } >>> static void free_buf_for_compression(void) >>> { >>> - if (IS_ENABLED(CONFIG_PSTORE_COMPRESS) && !IS_ERR_OR_NULL(tfm)) >>> + if (IS_ENABLED(CONFIG_PSTORE_COMPRESS) && tfm) >>> crypto_free_comp(tfm); >>> kfree(big_oops_buf); >>> big_oops_buf = NULL; >>> @@ -764,31 +784,28 @@ static void pstore_timefunc(struct timer_list >>> *unused) >>> jiffies + msecs_to_jiffies(pstore_update_ms)); >>> } >>> -void __init pstore_choose_compression(void) >>> +static int __init pstore_compression_late_init(void) >>> { >>> const struct pstore_zbackend *step; >>> if (!compress) >>> - return; >>> + return 0; >>> + /* Locate desired compression method. */ >>> for (step = zbackends; step->name; step++) { >>> if (!strcmp(compress, step->name)) { >>> zbackend = step; >>> - pr_info("using %s compression\n", >>> zbackend->name); >>> - return; >>> + break; >>> } >>> } >>> -} >>> -static int __init pstore_compression_late_init(void) >>> -{ >>> /* >>> - * Check if any pstore backends registered earlier but did not >>> allocate >>> - * for compression because crypto was not ready, if so then >>> initialize >>> - * compression. >>> + * Check if any pstore backends registered earlier but did not >>> + * initialize compression because crypto was not ready. If so, >>> + * then initialize compression now. >>> */ >>> - if (psinfo && !tfm) >>> - allocate_buf_for_compression(); >>> + allocate_buf_for_compression(); >> >> >> We can also get rid of the 'zbackend' global variable since choosing the >> compression backend and allocating the buffers are done at the same time? >> >> Otherwise looks good to me, >> >> Reviewed-by: Joel Fernandes (Google) <j...@joelfernandes.org> >> > > Tested this on top of Joel's patch, works fine on getting early crash pstore > dmesg logs, thanks. > > Tested-by: Sai Prakash Ranjan <saiprakash.ran...@codeaurora.org>
Awesome! Thanks for testing it. :) -Kees -- Kees Cook Pixel Security