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

[Kees Cook]

On Thu, Nov 10, 2016 at 4:10 PM, Kees Cook <keescook@xxxxxxxxxxxx> wrote:
> On Thu, Oct 20, 2016 at 12:34 AM, Joel Fernandes <joelaf@xxxxxxxxxx> 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@xxxxxxxxxx>
>> ---
>>  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