Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S966011AbcKKAKL (ORCPT ); Thu, 10 Nov 2016 19:10:11 -0500 Received: from mail-wm0-f54.google.com ([74.125.82.54]:35131 "EHLO mail-wm0-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S965547AbcKKAKK (ORCPT ); Thu, 10 Nov 2016 19:10:10 -0500 MIME-Version: 1.0 In-Reply-To: <1476948846-15006-3-git-send-email-joelaf@google.com> References: <1476948846-15006-1-git-send-email-joelaf@google.com> <1476948846-15006-3-git-send-email-joelaf@google.com> From: Kees Cook Date: Thu, 10 Nov 2016 16:10:08 -0800 X-Google-Sender-Auth: v2hrlQvTAV7LbLKlAYfs-utDxvU Message-ID: Subject: Re: [PATCH v2 2/7] pstore: locking: dont lock unless caller asks to To: Joel Fernandes 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 Content-Length: 4912 Lines: 132 On Thu, Oct 20, 2016 at 12:34 AM, 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. > > Signed-off-by: Joel Fernandes > --- > fs/pstore/ram.c | 10 +++++----- > fs/pstore/ram_core.c | 27 ++++++++++++++++----------- > include/linux/pstore_ram.h | 10 +++++++++- > 3 files changed, 30 insertions(+), 17 deletions(-) > > diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c > index f1219af..cb07ef6 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, PSTORE_RAM_LOCK); > 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, PSTORE_RAM_LOCK); > 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, PSTORE_RAM_LOCK); > 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, PSTORE_RAM_LOCK); > 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, PSTORE_RAM_LOCK); > > 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..69c7b96 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 == PSTORE_RAM_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 == PSTORE_RAM_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 == PSTORE_RAM_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 == PSTORE_RAM_LOCK) > + raw_spin_unlock_irqrestore(&prz->buffer_lock, flags); > } > > static void notrace persistent_ram_encode_rs8(struct persistent_ram_zone *prz, Does this pass a sparse test? Depending on how dumb sparse is, you may need to add "else" statements to the lock == RAM_LOCK tests: else __acquire(&prz->buffer_lock); ... else __release(&prz->buffer_lock); Otherwise, this looks fine. :) -Kees -- Kees Cook Nexus Security