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>
thanks,
- Joel
