Re: [PATCH] pstore: drop pmsg bounce buffer

[Kees Cook <keescook@chromium.org>]

On Thu, Sep 1, 2016 at 8:13 AM, Mark Salyzyn <salyzyn@android.com> wrote:
> Removing a bounce buffer copy operation in the pmsg driver path is
> always better. We also gain in overall performance by not requesting
> a vmalloc on every write as this can cause precious RT tasks, such
> as user facing media operation, to stall while memory is being
> reclaimed. Added a write_buf_user to the pstore functions, a backup
> platform write_buf_user that uses the small buffer that is part of
> the instance, and implemented a ramoops write_buf_user that only
> supports PSTORE_TYPE_PMSG.
>
> Signed-off-by: Mark Salyzyn <salyzyn@android.com>

I've applied this, thanks!

> ---
>  fs/pstore/platform.c       | 36 +++++++++++++++++++++++++++++++++++
>  fs/pstore/pmsg.c           | 35 ++++++----------------------------
>  fs/pstore/ram.c            | 19 +++++++++++++++++++
>  fs/pstore/ram_core.c       | 47 ++++++++++++++++++++++++++++++++++++++++++++--
>  include/linux/pstore.h     | 11 ++++++++---
>  include/linux/pstore_ram.h |  7 +++++--
>  6 files changed, 119 insertions(+), 36 deletions(-)
>
> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
> index 16ecca5..b9840a6 100644
> --- a/fs/pstore/platform.c
> +++ b/fs/pstore/platform.c
> @@ -623,6 +623,40 @@ static int pstore_write_compat(enum pstore_type_id type,
>                              size, psi);
>  }
>
> +static int pstore_write_buf_user_compat(enum pstore_type_id type,
> +                              enum kmsg_dump_reason reason,
> +                              u64 *id, unsigned int part,
> +                              const char __user *buf,
> +                              bool compressed, size_t size,
> +                              struct pstore_info *psi)
> +{
> +       unsigned long flags = 0;
> +       size_t i, bufsize = size;
> +       long ret = 0;
> +
> +       if (unlikely(!access_ok(VERIFY_READ, buf, size)))
> +               return -EFAULT;
> +       if (bufsize > psinfo->bufsize)
> +               bufsize = psinfo->bufsize;
> +       spin_lock_irqsave(&psinfo->buf_lock, flags);
> +       for (i = 0; i < size; ) {
> +               size_t c = min(size - i, bufsize);
> +
> +               ret = __copy_from_user(psinfo->buf, buf + i, c);
> +               if (unlikely(ret != 0)) {
> +                       ret = -EFAULT;
> +                       break;
> +               }
> +               ret = psi->write_buf(type, reason, id, part, psinfo->buf,
> +                                    compressed, c, psi);
> +               if (unlikely(ret < 0))
> +                       break;
> +               i += c;
> +       }
> +       spin_unlock_irqrestore(&psinfo->buf_lock, flags);
> +       return unlikely(ret < 0) ? ret : size;
> +}
> +
>  /*
>   * platform specific persistent storage driver registers with
>   * us here. If pstore is already mounted, call the platform
> @@ -645,6 +679,8 @@ int pstore_register(struct pstore_info *psi)
>
>         if (!psi->write)
>                 psi->write = pstore_write_compat;
> +       if (!psi->write_buf_user)
> +               psi->write_buf_user = pstore_write_buf_user_compat;
>         psinfo = psi;
>         mutex_init(&psinfo->read_mutex);
>         spin_unlock(&pstore_lock);
> diff --git a/fs/pstore/pmsg.c b/fs/pstore/pmsg.c
> index 7de20cd..78f6176 100644
> --- a/fs/pstore/pmsg.c
> +++ b/fs/pstore/pmsg.c
> @@ -19,48 +19,25 @@
>  #include "internal.h"
>
>  static DEFINE_MUTEX(pmsg_lock);
> -#define PMSG_MAX_BOUNCE_BUFFER_SIZE (2*PAGE_SIZE)
>
>  static ssize_t write_pmsg(struct file *file, const char __user *buf,
>                           size_t count, loff_t *ppos)
>  {
> -       size_t i, buffer_size;
> -       char *buffer;
> +       u64 id;
> +       int ret;
>
>         if (!count)
>                 return 0;
>
> +       /* check outside lock, page in any data. write_buf_user also checks */
>         if (!access_ok(VERIFY_READ, buf, count))
>                 return -EFAULT;
>
> -       buffer_size = count;
> -       if (buffer_size > PMSG_MAX_BOUNCE_BUFFER_SIZE)
> -               buffer_size = PMSG_MAX_BOUNCE_BUFFER_SIZE;
> -       buffer = vmalloc(buffer_size);
> -       if (!buffer)
> -               return -ENOMEM;
> -
>         mutex_lock(&pmsg_lock);
> -       for (i = 0; i < count; ) {
> -               size_t c = min(count - i, buffer_size);
> -               u64 id;
> -               long ret;
> -
> -               ret = __copy_from_user(buffer, buf + i, c);
> -               if (unlikely(ret != 0)) {
> -                       mutex_unlock(&pmsg_lock);
> -                       vfree(buffer);
> -                       return -EFAULT;
> -               }
> -               psinfo->write_buf(PSTORE_TYPE_PMSG, 0, &id, 0, buffer, 0, c,
> -                                 psinfo);
> -
> -               i += c;
> -       }
> -
> +       ret = psinfo->write_buf_user(PSTORE_TYPE_PMSG, 0, &id, 0, buf, 0, count,
> +                                    psinfo);
>         mutex_unlock(&pmsg_lock);
> -       vfree(buffer);
> -       return count;
> +       return ret ? ret : count;
>  }
>
>  static const struct file_operations pmsg_fops = {
> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> index 7a034d6..893e040c 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -331,6 +331,24 @@ static int notrace ramoops_pstore_write_buf(enum pstore_type_id type,
>         return 0;
>  }
>
> +static int notrace ramoops_pstore_write_buf_user(enum pstore_type_id type,
> +                                                enum kmsg_dump_reason reason,
> +                                                u64 *id, unsigned int part,
> +                                                const char __user *buf,
> +                                                bool compressed, size_t size,
> +                                                struct pstore_info *psi)
> +{
> +       if (type == PSTORE_TYPE_PMSG) {
> +               struct ramoops_context *cxt = psi->data;
> +
> +               if (!cxt->mprz)
> +                       return -ENOMEM;
> +               return persistent_ram_write_user(cxt->mprz, buf, size);
> +       }
> +
> +       return -EINVAL;
> +}
> +
>  static int ramoops_pstore_erase(enum pstore_type_id type, u64 id, int count,
>                                 struct timespec time, struct pstore_info *psi)
>  {
> @@ -369,6 +387,7 @@ static struct ramoops_context oops_cxt = {
>                 .open   = ramoops_pstore_open,
>                 .read   = ramoops_pstore_read,
>                 .write_buf      = ramoops_pstore_write_buf,
> +               .write_buf_user = ramoops_pstore_write_buf_user,
>                 .erase  = ramoops_pstore_erase,
>         },
>  };
> diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
> index 76c3f80..aa9afe5 100644
> --- a/fs/pstore/ram_core.c
> +++ b/fs/pstore/ram_core.c
> @@ -17,15 +17,16 @@
>  #include <linux/device.h>
>  #include <linux/err.h>
>  #include <linux/errno.h>
> -#include <linux/kernel.h>
>  #include <linux/init.h>
>  #include <linux/io.h>
> +#include <linux/kernel.h>
>  #include <linux/list.h>
>  #include <linux/memblock.h>
> +#include <linux/pstore_ram.h>
>  #include <linux/rslib.h>
>  #include <linux/slab.h>
> +#include <linux/uaccess.h>
>  #include <linux/vmalloc.h>
> -#include <linux/pstore_ram.h>
>  #include <asm/page.h>
>
>  struct persistent_ram_buffer {
> @@ -303,6 +304,16 @@ static void notrace persistent_ram_update(struct persistent_ram_zone *prz,
>         persistent_ram_update_ecc(prz, start, count);
>  }
>
> +static int notrace persistent_ram_update_user(struct persistent_ram_zone *prz,
> +       const void __user *s, unsigned int start, unsigned int count)
> +{
> +       struct persistent_ram_buffer *buffer = prz->buffer;
> +       int ret = unlikely(__copy_from_user(buffer->data + start, s, count)) ?
> +               -EFAULT : 0;
> +       persistent_ram_update_ecc(prz, start, count);
> +       return ret;
> +}
> +
>  void persistent_ram_save_old(struct persistent_ram_zone *prz)
>  {
>         struct persistent_ram_buffer *buffer = prz->buffer;
> @@ -356,6 +367,38 @@ int notrace persistent_ram_write(struct persistent_ram_zone *prz,
>         return count;
>  }
>
> +int notrace persistent_ram_write_user(struct persistent_ram_zone *prz,
> +       const void __user *s, unsigned int count)
> +{
> +       int rem, ret = 0, c = count;
> +       size_t start;
> +
> +       if (unlikely(!access_ok(VERIFY_READ, s, count)))
> +               return -EFAULT;
> +       if (unlikely(c > prz->buffer_size)) {
> +               s += c - prz->buffer_size;
> +               c = prz->buffer_size;
> +       }
> +
> +       buffer_size_add(prz, c);
> +
> +       start = buffer_start_add(prz, c);
> +
> +       rem = prz->buffer_size - start;
> +       if (unlikely(rem < c)) {
> +               ret = persistent_ram_update_user(prz, s, start, rem);
> +               s += rem;
> +               c -= rem;
> +               start = 0;
> +       }
> +       if (likely(!ret))
> +               ret = persistent_ram_update_user(prz, s, start, c);
> +
> +       persistent_ram_update_header_ecc(prz);
> +
> +       return unlikely(ret) ? ret : count;
> +}
> +

As a follow-up, I wonder if ram_write and ram_write_user could be
consolidated in some way since they're identical except for the update
function, and contain a lot of non-obvious work that should stay the
same between both functions (and could probably use more comments).

-Kees

-- 
Kees Cook
Nexus Security