Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
In-Reply-To: <9ebe40c0-5acc-70e1-587d-ba3f0edf2e7b@codeaurora.org>
References: <20181017214124.GA8239@beast> <20181018013001.GA99677@joelaf.mtv.corp.google.com>
 <9ebe40c0-5acc-70e1-587d-ba3f0edf2e7b@codeaurora.org>
From:   Kees Cook <keescook@chromium.org>
Date:   Wed, 17 Oct 2018 21:44:32 -0700
Message-ID: <CAGXu5jJOBqpGG_cdtDN0cTjEqWDHx-=u4sc=5mMD7_fxZnH7fQ@mail.gmail.com>
Subject: Re: [PATCH] pstore: Refactor compression initialization
To:     Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
Cc:     Joel Fernandes <joel@joelfernandes.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Oct 17, 2018 at 9:42 PM, Sai Prakash Ranjan
<saiprakash.ranjan@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 <keescook@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) <joel@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.ranjan@codeaurora.org>

Awesome! Thanks for testing it. :)

-Kees

-- 
Kees Cook
Pixel Security