Re: [PATCH v4] ramoops: use pstore interface

[Kees Cook]

On Tue, Jan 17, 2012 at 1:59 PM, Andrew Morton
<akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Sat, 7 Jan 2012 09:15:16 -0800
> Kees Cook <keescook@xxxxxxxxxxxx> wrote:
>
>> Instead of using /dev/mem directly, use the common pstore infrastructure
>> to handle Oops gathering and extraction.
>
> um, why?  This changelog provides no reason for anyone to apply the
> patch!

Good point; this was lost as the patch cycled. I will expand this.

>> +     struct ramoops_context *cxt = (struct ramoops_context *)psi->data;
>
> Unneeded and undesirable cast of void*.  Multiple instances of this.

Ah yes; I will fix this.

>> +     /* TODO(kees): Bogus time for the moment. */
>
> Is this hard to fix now?

I felt it was out of scope for the moment. There was enough change
happening for bolting it to pstore that adding a header with magic
values, etc, seemed like a logically separate task. As such, I left
this as TODO.

> Note that pstore_get_records() will treat the -ve errno returns from
> ->read() in the same manner as EOF.  IOW, your error codes will be
> dropped on the floor.  This appears to be a bug in pstore_get_records().

Well, IIUC, it just means the file doesn't get populated at all; there
is no userspace interface to finding out why a file didn't appear in
the pstore fliesystem. But yes, the specifics of the error are ignored
by pstore_get_records(). It didn't seem wrong to produce meaningful
codes in ramoops, though.

>> +     /* Only store dmesg dumps. */
>> +     if (type != PSTORE_TYPE_DMESG)
>> +             return -EINVAL;
>> +
>> +     /* Only store crash dumps. */
>>       if (reason != KMSG_DUMP_OOPS &&
>> -         reason != KMSG_DUMP_PANIC &&
>> -         reason != KMSG_DUMP_KEXEC)
>> -             return;
>> +         reason != KMSG_DUMP_PANIC)
>> +             return -EINVAL;
>>
>>       /* Only dump oopses if dump_oops is set */
>>       if (reason == KMSG_DUMP_OOPS && !cxt->dump_oops)
>
> The above three comments describe what the code does, which was
> obvious.  They failed to describe why it does this, which was
> unobvious.  Sigh.

They were terse; I will attempt to expand on them.

Thanks for the review! I will send v5 shortly...

-Kees

-- 
Kees Cook
ChromeOS Security
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/