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: <20181018013001.GA99677@joelaf.mtv.corp.google.com>
References: <20181017214124.GA8239@beast> <20181018013001.GA99677@joelaf.mtv.corp.google.com>
From:   Kees Cook <keescook@chromium.org>
Date:   Wed, 17 Oct 2018 21:44:01 -0700
Message-ID: <CAGXu5jKqjMrrGi17EQ5-oCq3_pHVtoehajSzSfhaxOyJw-ZNKA@mail.gmail.com>
Subject: Re: [PATCH] pstore: Refactor compression initialization
To:     Joel Fernandes <joel@joelfernandes.org>
Cc:     Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.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 6:30 PM, Joel Fernandes <joel@joelfernandes.org> 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?

That's my intention once I push the zbufsize functions up into the
crypto subsystem (there's a separate series for this that I need to
refresh).

> Otherwise looks good to me,
>
> Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>

Thanks!

-Kees

-- 
Kees Cook
Pixel Security