Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1519472 > unrolled thread

Re: [PATCH v2 2/7] pstore: locking: dont lock unless caller asks to

Started byKees Cook <keescook@chromium.org>
First post2016-11-11 01:20 +0100
Last post2016-11-11 19:20 +0100
Articles 2 — 1 participant

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 2/7] pstore: locking: dont lock unless caller asks to Kees Cook <keescook@chromium.org> - 2016-11-11 01:20 +0100
    Re: [PATCH v2 2/7] pstore: locking: dont lock unless caller asks to Kees Cook <keescook@chromium.org> - 2016-11-11 19:20 +0100

#1519472 — Re: [PATCH v2 2/7] pstore: locking: dont lock unless caller asks to

FromKees Cook <keescook@chromium.org>
Date2016-11-11 01:20 +0100
SubjectRe: [PATCH v2 2/7] pstore: locking: dont lock unless caller asks to
Message-ID<sC7Jv-5hq-9@gated-at.bofh.it>
On Thu, Oct 20, 2016 at 12:34 AM, Joel Fernandes <joelaf@google.com> 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 <joelaf@google.com>
> ---
>  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

[toc] | [next] | [standalone]

#1519947

FromKees Cook <keescook@chromium.org>
Date2016-11-11 19:20 +0100
Message-ID<sCoAF-7P3-11@gated-at.bofh.it>
In reply to#1519472
On Thu, Nov 10, 2016 at 4:10 PM, Kees Cook <keescook@chromium.org> wrote:
> On Thu, Oct 20, 2016 at 12:34 AM, Joel Fernandes <joelaf@google.com> 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 <joelaf@google.com>
>> ---
>>  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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web