Re: [PATCH 00/11] Add compression support to pstore

[Aruna Balakrishnaiah <aruna@linux.vnet.ibm.com>]

On Wednesday 07 August 2013 05:06 AM, Tony Luck wrote:
> On Mon, Aug 5, 2013 at 2:20 PM, Tony Luck <tony.luck@gmail.com> wrote:
>> Still have problems booting if there are any compressed images in ERST
>> to be inflated.
> So I took another look at this part of the code ... and saw a couple of issues:
>
>          while ((size = psi->read(&id, &type, &count, &time, &buf, &compressed,
>                                  psi)) > 0) {
>                  if (compressed && (type == PSTORE_TYPE_DMESG)) {
>                          big_buf_sz = (psinfo->bufsize * 100) / 45;
>                          big_buf = allocate_buf_for_decompression(big_buf_sz);
>
>                          if (big_buf || stream.workspace)
>>>> Did you mean "&&" here rather that "||"?

Yes right, it should be &&.

>                                  unzipped_len = pstore_decompress(buf, big_buf,
>                                                          size, big_buf_sz);
>>>> Need an "else" here to set unzipped_len to -1 (or set it to -1 down
>>>> at the bottom of the loop ready for next time around.
>                          if (unzipped_len > 0) {
>                                  buf = big_buf;
>>>> This sets us up for problems.  First, you just overwrote the address
>>>> of the buffer that psi->read allocated - so we have a memory leak. But
>>>> worse than that we now double free the same buffer below when we
>>>> kfree(buf) and then kfree(big_buf)
>                                  size = unzipped_len;
>                                  compressed = false;
>                          } else {
>                                  pr_err("pstore: decompression failed;"
>                                          "returned %d\n", unzipped_len);
>                                  compressed = true;
>                          }
>                  }
>                  rc = pstore_mkfile(type, psi->name, id, count, buf,
>                                    compressed, (size_t)size, time, psi);
>                  kfree(buf);
>                  kfree(stream.workspace);
>                  kfree(big_buf);
>                  buf = NULL;
>                  stream.workspace = NULL;
>                  big_buf = NULL;
>                  if (rc && (rc != -EEXIST || !quiet))
>                          failed++;
>          }
>
>
> See attached patch that fixes these - but the code still looks like it
> could be cleaned up a bit more.

The patch looks right. I will clean it up. Does the issue still persist after this?

> -Tony