From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S941189AbcIHUEx (ORCPT ); Thu, 8 Sep 2016 16:04:53 -0400 Received: from mail-wm0-f51.google.com ([74.125.82.51]:38847 "EHLO mail-wm0-f51.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753572AbcIHUEt (ORCPT ); Thu, 8 Sep 2016 16:04:49 -0400 MIME-Version: 1.0 In-Reply-To: <1472742828-24267-1-git-send-email-salyzyn@android.com> References: <1472742828-24267-1-git-send-email-salyzyn@android.com> From: Kees Cook Date: Thu, 8 Sep 2016 13:04:36 -0700 X-Google-Sender-Auth: bhNBouzxe_DR6BB5O67A2MKu-34 Message-ID: Subject: Re: [PATCH] pstore: drop pmsg bounce buffer To: Mark Salyzyn Cc: LKML , Anton Vorontsov , Colin Cross , Tony Luck Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Sep 1, 2016 at 8:13 AM, Mark Salyzyn 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 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 > #include > #include > -#include > #include > #include > +#include > #include > #include > +#include > #include > #include > +#include > #include > -#include > #include > > 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