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-doc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
