Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S936140AbcKKSRg (ORCPT ); Fri, 11 Nov 2016 13:17:36 -0500 Received: from mail-wm0-f48.google.com ([74.125.82.48]:35505 "EHLO mail-wm0-f48.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934797AbcKKSR3 (ORCPT ); Fri, 11 Nov 2016 13:17:29 -0500 MIME-Version: 1.0 In-Reply-To: References: <1476948846-15006-1-git-send-email-joelaf@google.com> <1476948846-15006-3-git-send-email-joelaf@google.com> From: Kees Cook Date: Fri, 11 Nov 2016 10:16:48 -0800 X-Google-Sender-Auth: wYREGHZXAyWO-k9GZbtVSzpL0PY 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: 5172 Lines: 135 On Thu, Nov 10, 2016 at 4:10 PM, Kees Cook wrote: > 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. :) To answer this: yes, sparse is fine with this. :) -Kees -- Kees Cook Nexus Security