Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752774AbcJJXsh (ORCPT ); Mon, 10 Oct 2016 19:48:37 -0400 Received: from mail-lf0-f42.google.com ([209.85.215.42]:36465 "EHLO mail-lf0-f42.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752421AbcJJXse (ORCPT ); Mon, 10 Oct 2016 19:48:34 -0400 MIME-Version: 1.0 In-Reply-To: <1475904515-24970-3-git-send-email-joelaf@google.com> References: <1475904515-24970-1-git-send-email-joelaf@google.com> <1475904515-24970-3-git-send-email-joelaf@google.com> From: Kees Cook Date: Mon, 10 Oct 2016 16:48:31 -0700 X-Google-Sender-Auth: sRqO8dHOvw9Pge6ptGKCPpLMuZo Message-ID: Subject: Re: [PATCH 2/7] pstore: locking: dont lock unless caller asks to To: Joel Fernandes Cc: LKML , Steven Rostedt , 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 Content-Length: 6710 Lines: 183 On Fri, Oct 7, 2016 at 10:28 PM, Joel Fernandes wrote: > In preparation of not locking at all for certain buffers depending on if > there's contention, make locking optional depending if caller requested it. This should be a bool argument (or enum), with named values so it's more readable. For example, these don't immediately tell me what the argument does: persistent_ram_write(cxt->cprz, buf, size, 1); persistent_ram_write(cxt->cprz, buf, size, true); But this does: persistent_ram_write(cxt->cprz, buf, size, PSTORE_REQUIRE_LOCKING); As part of this, can you document in the pstore structure which types of front-ends require locking and if they don't, why? -Kees > > Signed-off-by: Joel Fernandes > --- > fs/pstore/ram.c | 10 +++++----- > fs/pstore/ram_core.c | 27 ++++++++++++++++----------- > include/linux/pstore_ram.h | 2 +- > 3 files changed, 22 insertions(+), 17 deletions(-) > > diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c > index f1219af..751197d 100644 > --- a/fs/pstore/ram.c > +++ b/fs/pstore/ram.c > @@ -260,7 +260,7 @@ static size_t ramoops_write_kmsg_hdr(struct persistent_ram_zone *prz, > compressed ? 'C' : 'D'); > WARN_ON_ONCE(!hdr); > len = hdr ? strlen(hdr) : 0; > - persistent_ram_write(prz, hdr, len); > + persistent_ram_write(prz, hdr, len, 1); > kfree(hdr); > > return len; > @@ -280,17 +280,17 @@ static int notrace ramoops_pstore_write_buf(enum pstore_type_id type, > if (type == PSTORE_TYPE_CONSOLE) { > if (!cxt->cprz) > return -ENOMEM; > - persistent_ram_write(cxt->cprz, buf, size); > + persistent_ram_write(cxt->cprz, buf, size, 1); > return 0; > } else if (type == PSTORE_TYPE_FTRACE) { > if (!cxt->fprz) > return -ENOMEM; > - persistent_ram_write(cxt->fprz, buf, size); > + persistent_ram_write(cxt->fprz, buf, size, 1); > return 0; > } else if (type == PSTORE_TYPE_PMSG) { > if (!cxt->mprz) > return -ENOMEM; > - persistent_ram_write(cxt->mprz, buf, size); > + persistent_ram_write(cxt->mprz, buf, size, 1); > return 0; > } > > @@ -324,7 +324,7 @@ static int notrace ramoops_pstore_write_buf(enum pstore_type_id type, > hlen = ramoops_write_kmsg_hdr(prz, compressed); > if (size + hlen > prz->buffer_size) > size = prz->buffer_size - hlen; > - persistent_ram_write(prz, buf, size); > + persistent_ram_write(prz, buf, size, 1); > > cxt->dump_write_cnt = (cxt->dump_write_cnt + 1) % cxt->max_dump_cnt; > > diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c > index cb92055..c4722f0 100644 > --- a/fs/pstore/ram_core.c > +++ b/fs/pstore/ram_core.c > @@ -49,13 +49,15 @@ static inline size_t buffer_start(struct persistent_ram_zone *prz) > } > > /* increase and wrap the start pointer, returning the old value */ > -static size_t buffer_start_add(struct persistent_ram_zone *prz, size_t a) > +static size_t buffer_start_add(struct persistent_ram_zone *prz, size_t a, > + int lock) > { > int old; > int new; > unsigned long flags; > > - raw_spin_lock_irqsave(&prz->buffer_lock, flags); > + if (lock) > + raw_spin_lock_irqsave(&prz->buffer_lock, flags); > > old = atomic_read(&prz->buffer->start); > new = old + a; > @@ -63,19 +65,21 @@ static size_t buffer_start_add(struct persistent_ram_zone *prz, size_t a) > new -= prz->buffer_size; > atomic_set(&prz->buffer->start, new); > > - raw_spin_unlock_irqrestore(&prz->buffer_lock, flags); > + if (lock) > + raw_spin_unlock_irqrestore(&prz->buffer_lock, flags); > > return old; > } > > /* increase the size counter until it hits the max size */ > -static void buffer_size_add(struct persistent_ram_zone *prz, size_t a) > +static void buffer_size_add(struct persistent_ram_zone *prz, size_t a, int lock) > { > size_t old; > size_t new; > unsigned long flags; > > - raw_spin_lock_irqsave(&prz->buffer_lock, flags); > + if (lock) > + raw_spin_lock_irqsave(&prz->buffer_lock, flags); > > old = atomic_read(&prz->buffer->size); > if (old == prz->buffer_size) > @@ -87,7 +91,8 @@ static void buffer_size_add(struct persistent_ram_zone *prz, size_t a) > atomic_set(&prz->buffer->size, new); > > exit: > - raw_spin_unlock_irqrestore(&prz->buffer_lock, flags); > + if (lock) > + raw_spin_unlock_irqrestore(&prz->buffer_lock, flags); > } > > static void notrace persistent_ram_encode_rs8(struct persistent_ram_zone *prz, > @@ -300,7 +305,7 @@ void persistent_ram_save_old(struct persistent_ram_zone *prz) > } > > int notrace persistent_ram_write(struct persistent_ram_zone *prz, > - const void *s, unsigned int count) > + const void *s, unsigned int count, int lock) > { > int rem; > int c = count; > @@ -311,9 +316,9 @@ int notrace persistent_ram_write(struct persistent_ram_zone *prz, > c = prz->buffer_size; > } > > - buffer_size_add(prz, c); > + buffer_size_add(prz, c, lock); > > - start = buffer_start_add(prz, c); > + start = buffer_start_add(prz, c, lock); > > rem = prz->buffer_size - start; > if (unlikely(rem < c)) { > @@ -342,9 +347,9 @@ int notrace persistent_ram_write_user(struct persistent_ram_zone *prz, > c = prz->buffer_size; > } > > - buffer_size_add(prz, c); > + buffer_size_add(prz, c, 1); > > - start = buffer_start_add(prz, c); > + start = buffer_start_add(prz, c, 1); > > rem = prz->buffer_size - start; > if (unlikely(rem < c)) { > diff --git a/include/linux/pstore_ram.h b/include/linux/pstore_ram.h > index 244d242..782af68 100644 > --- a/include/linux/pstore_ram.h > +++ b/include/linux/pstore_ram.h > @@ -61,7 +61,7 @@ void persistent_ram_free(struct persistent_ram_zone *prz); > void persistent_ram_zap(struct persistent_ram_zone *prz); > > int persistent_ram_write(struct persistent_ram_zone *prz, const void *s, > - unsigned int count); > + unsigned int count, int lock); > int persistent_ram_write_user(struct persistent_ram_zone *prz, > const void __user *s, unsigned int count); > > -- > 2.7.4 > -- Kees Cook Nexus Security