| Message ID | 20200515184434.8470-1-keescook@chromium.org |
|---|---|
| Headers | show |
| Series | allow ramoops to collect all kmesg_dump events | expand |
On Fri, May 15, 2020 at 2:44 PM Kees Cook <keescook@chromium.org> wrote: > > Hello! > > I wanted to get the pstore tree nailed down, so here's the v4 of > Pavel's series, tweaked for the feedback during v3 review. Hi Kees, Thank you, I was planning to send a new version of this series later today. Let me quickly review it. Pasha > > -Kees > > v4: > - rebase on pstore tree > - collapse shutdown types into a single dump reason > https://lore.kernel.org/lkml/CA+CK2bAPv5u1ih5y9t5FUnTyximtFCtDYXJCpuyjOyHNOkRdqw@mail.gmail.com/ > - fix dump_oops vs max_reason module params > https://lore.kernel.org/lkml/20200512233504.GA118720@sequoia/ > - typos > https://lore.kernel.org/lkml/4cdeaa2af2fe0d6cc2ca8ce3a37608340799df8a.camel@perches.com/ > - rename DT parsing routines ..._size -> ..._u32 > https://lore.kernel.org/lkml/CA+CK2bCu8eFomiU+NeBjVn-o2dbuECxwRfssNjB3ys3caCbXeA@mail.gmail.com/ > v3: https://lore.kernel.org/lkml/20200506211523.15077-1-keescook@chromium.org/ > v2: https://lore.kernel.org/lkml/20200505154510.93506-1-pasha.tatashin@soleen.com > v1: https://lore.kernel.org/lkml/20200502143555.543636-1-pasha.tatashin@soleen.com > > Kees Cook (3): > printk: Collapse shutdown types into a single dump reason > printk: Introduce kmsg_dump_reason_str() > pstore/ram: Introduce max_reason and convert dump_oops > > Pavel Tatashin (3): > printk: honor the max_reason field in kmsg_dumper > pstore/platform: Pass max_reason to kmesg dump > ramoops: Add max_reason optional field to ramoops DT node > > Documentation/admin-guide/ramoops.rst | 14 +++-- > .../bindings/reserved-memory/ramoops.txt | 13 ++++- > arch/powerpc/kernel/nvram_64.c | 4 +- > drivers/platform/chrome/chromeos_pstore.c | 2 +- > fs/pstore/platform.c | 26 ++------- > fs/pstore/ram.c | 58 +++++++++++++------ > include/linux/kmsg_dump.h | 12 +++- > include/linux/pstore.h | 7 +++ > include/linux/pstore_ram.h | 2 +- > kernel/printk/printk.c | 32 ++++++++-- > kernel/reboot.c | 6 +- > 11 files changed, 114 insertions(+), 62 deletions(-) > > -- > 2.20.1 >
On Fri, May 15, 2020 at 2:44 PM Kees Cook <keescook@chromium.org> wrote: > > To turn the KMSG_DUMP_* reasons into a more ordered list, collapse > the redundant KMSG_DUMP_(RESTART|HALT|POWEROFF) reasons into > KMSG_DUMP_SHUTDOWN. The current users already don't meaningfully > distinguish between them, so there's no need to, as discussed here: > https://lore.kernel.org/lkml/CA+CK2bAPv5u1ih5y9t5FUnTyximtFCtDYXJCpuyjOyHNOkRdqw@mail.gmail.com/ > > Signed-off-by: Kees Cook <keescook@chromium.org> Maybe it makes sense to mention in the commit log that for all three merged cases there is a pr_emerg() message logged right before the kmsg_dump(), thus the reason is distinguishable from the dmesg log itself. Reviewed-by: Pavel Tatashin <pasha.tatashin@soleen.com>
On Fri, May 15, 2020 at 2:44 PM Kees Cook <keescook@chromium.org> wrote: > > The pstore subsystem already had a private version of this function. > With the coming addition of the pstore/zone driver, this needs to be > shared. As it really should live with printk, move it there instead. > > Link: https://lore.kernel.org/lkml/20200510202436.63222-8-keescook@chromium.org/ > Acked-by: Petr Mladek <pmladek@suse.com> > Acked-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com> > Signed-off-by: Kees Cook <keescook@chromium.org> Reviewed-by: Pavel Tatashin <pasha.tatashin@soleen.com>
> #define parse_u32(name, field, default_value) { \ > ret = ramoops_parse_dt_u32(pdev, name, default_value, \ The series seems to be missing the patch where ramoops_parse_dt_size -> ramoops_parse_dt_u32 get renamed, and updated to handle default value.
pdata.dump_oops = dump_oops; > + /* If "max_reason" is set, its value has priority over "dump_oops". */ > + if (ramoops_max_reason != -1) > + pdata.max_reason = ramoops_max_reason; (ramoops_max_reason >= 0) might make more sense here, we do not want negative max_reason even if it was provided by the user. Otherwise the series looks good. Thank you, Pasha
On Fri, May 15, 2020 at 03:30:27PM -0400, Pavel Tatashin wrote: > > #define parse_u32(name, field, default_value) { \ > > ret = ramoops_parse_dt_u32(pdev, name, default_value, \ > > The series seems to be missing the patch where ramoops_parse_dt_size > -> ramoops_parse_dt_u32 get renamed, and updated to handle default > value. Oops! Sorry, I cut the line in the wrong place for sending out the delta on top of the pstore tree. :) It's unchanged from: https://lore.kernel.org/lkml/20200506211523.15077-4-keescook@chromium.org/
On Fri, May 15, 2020 at 03:40:14PM -0400, Pavel Tatashin wrote: > pdata.dump_oops = dump_oops; > > + /* If "max_reason" is set, its value has priority over "dump_oops". */ > > + if (ramoops_max_reason != -1) > > + pdata.max_reason = ramoops_max_reason; > > (ramoops_max_reason >= 0) might make more sense here, we do not want > negative max_reason even if it was provided by the user. Yeah, that's a good point. I'll tweak that. Thanks!
On Fri 2020-05-15 11:44:29, Kees Cook wrote: > To turn the KMSG_DUMP_* reasons into a more ordered list, collapse > the redundant KMSG_DUMP_(RESTART|HALT|POWEROFF) reasons into > KMSG_DUMP_SHUTDOWN. The current users already don't meaningfully > distinguish between them, so there's no need to, as discussed here: > https://lore.kernel.org/lkml/CA+CK2bAPv5u1ih5y9t5FUnTyximtFCtDYXJCpuyjOyHNOkRdqw@mail.gmail.com/ > > Signed-off-by: Kees Cook <keescook@chromium.org> Looks good to me: Reviewed-by: Petr Mladek <pmladek@suse.com> Best Regards, Petr
On Fri 2020-05-15 11:44:30, Kees Cook wrote: > From: Pavel Tatashin <pasha.tatashin@soleen.com> > > kmsg_dump() allows to dump kmesg buffer for various system events: oops, > panic, reboot, etc. It provides an interface to register a callback call > for clients, and in that callback interface there is a field "max_reason" > which gets ignored unless always_kmsg_dump is passed as kernel parameter. Strictly speaking, this is not fully true. "max_reason" field is not ignored when set to KMSG_DUMP_PANIC even when always_kmsg_dump was not set. It should be something like: "which gets ignored for reason higher than KMSG_DUMP_OOPS unless always_kmsg_dump is passed as kernel parameter". Heh, I wonder if anyone will be able to parse this ;-) Otherwise, it looks good to me. With the updated commit message: Reviewed-by: Petr Mladek <pmladek@suse.com> Best Regards, Petr
On Fri, May 22, 2020 at 06:51:20PM +0200, Petr Mladek wrote: > On Fri 2020-05-15 11:44:30, Kees Cook wrote: > > From: Pavel Tatashin <pasha.tatashin@soleen.com> > > > > kmsg_dump() allows to dump kmesg buffer for various system events: oops, > > panic, reboot, etc. It provides an interface to register a callback call > > for clients, and in that callback interface there is a field "max_reason" > > which gets ignored unless always_kmsg_dump is passed as kernel parameter. > > Strictly speaking, this is not fully true. "max_reason" field is not > ignored when set to KMSG_DUMP_PANIC even when always_kmsg_dump was not set. > > It should be something like: > > "which gets ignored for reason higher than KMSG_DUMP_OOPS unless > always_kmsg_dump is passed as kernel parameter". > > Heh, I wonder if anyone will be able to parse this ;-) Ah yeah, good point. I've reworded things like this: kmsg_dump() allows to dump kmesg buffer for various system events: oops, panic, reboot, etc. It provides an interface to register a callback call for clients, and in that callback interface there is a field "max_reason", but it was getting ignored when set to any "reason" higher than KMSG_DUMP_OOPS unless "always_kmsg_dump" was passed as kernel parameter. Allow clients to actually control their "max_reason", and keep the current behavior when "max_reason" is not set. > Otherwise, it looks good to me. With the updated commit message: > > Reviewed-by: Petr Mladek <pmladek@suse.com> Thanks!
Kees Cook <keescook@chromium.org> writes: > To turn the KMSG_DUMP_* reasons into a more ordered list, collapse > the redundant KMSG_DUMP_(RESTART|HALT|POWEROFF) reasons into > KMSG_DUMP_SHUTDOWN. The current users already don't meaningfully > distinguish between them, so there's no need to, as discussed here: > https://lore.kernel.org/lkml/CA+CK2bAPv5u1ih5y9t5FUnTyximtFCtDYXJCpuyjOyHNOkRdqw@mail.gmail.com/ > > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > arch/powerpc/kernel/nvram_64.c | 4 +--- > fs/pstore/platform.c | 8 ++------ > include/linux/kmsg_dump.h | 4 +--- > kernel/reboot.c | 6 +++--- > 4 files changed, 7 insertions(+), 15 deletions(-) > > diff --git a/arch/powerpc/kernel/nvram_64.c b/arch/powerpc/kernel/nvram_64.c > index fb4f61096613..0cd1c88bfc8b 100644 > --- a/arch/powerpc/kernel/nvram_64.c > +++ b/arch/powerpc/kernel/nvram_64.c > @@ -655,9 +655,7 @@ static void oops_to_nvram(struct kmsg_dumper *dumper, > int rc = -1; > > switch (reason) { > - case KMSG_DUMP_RESTART: > - case KMSG_DUMP_HALT: > - case KMSG_DUMP_POWEROFF: > + case KMSG_DUMP_SHUTDOWN: > /* These are almost always orderly shutdowns. */ > return; > case KMSG_DUMP_OOPS: Acked-by: Michael Ellerman <mpe@ellerman.id.au> (powerpc) cheers