Re: [PATCH v2 2/5] pstore/platform: pass max_reason to kmesg dump

[Pavel Tatashin]

On Tue, May 5, 2020 at 5:59 PM Kees Cook <keescook@xxxxxxxxxxxx> wrote:
>
> On Tue, May 05, 2020 at 11:45:07AM -0400, Pavel Tatashin wrote:
> > Add a new field to pstore_info that passes information about kmesg dump
> > maximum reason.
> >
> > This allows a finer control of what kmesg dumps are stored on pstore
> > device.
> >
> > Those clients that do not explicitly set this field (keep it equal to 0),
> > get the default behavior: dump only Oops and Panics, and dump everything
> > if printk.always_kmsg_dump is provided.
> >
> > Signed-off-by: Pavel Tatashin <pasha.tatashin@xxxxxxxxxx>
> > ---
> >  fs/pstore/platform.c   | 4 +++-
> >  include/linux/pstore.h | 3 +++
> >  2 files changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
> > index 408277ee3cdb..75bf8a43f92a 100644
> > --- a/fs/pstore/platform.c
> > +++ b/fs/pstore/platform.c
> > @@ -602,8 +602,10 @@ int pstore_register(struct pstore_info *psi)
> >       if (pstore_is_mounted())
> >               pstore_get_records(0);
> >
> > -     if (psi->flags & PSTORE_FLAGS_DMESG)
> > +     if (psi->flags & PSTORE_FLAGS_DMESG) {
> > +             pstore_dumper.max_reason = psinfo->max_reason;
> >               pstore_register_kmsg();
> > +     }
>
> I haven't finished reading the whole series carefully, but I think
> something we can do here to make things a bit more user-friendly is to
> do the KMSG_DUMP_UNDEF value here to get us the "all" instead of INT_MAX:
>
>         if (psi->flags & PSTORE_FLAGS_DMESG) {
>                 pstore_dumper.max_reason = psinfo->max_reason;
>                 if (pstore_dumper.max_reason == KMSG_DUMP_UNDEF)
>                         pstore_dumper.max_reason = KMSG_DUMP_MAX;
>                 pstore_register_kmsg();
>         }
>
> That way setting max_reason to 0 without setting dump_oops at all will
> get "all". I think it'll need some tweaks to the next patch.

Hm, but if we change it this way, it will change the behavior for
other backends. With the current version of this patchset,
when psinfo->max_reason is left undefined (0, KMSG_DUMP_UNDEF) -> the
existing behaviour is honored, which means: printk chooses the kmesg
dump level, and users can set dump for all reasons via printk
always_kmsg_dump. This is what efi-pstore, erst, and nvram_64 are
currently doing, and I am not sure we should change that.
However, with the proposed change if the backend specifically sets
max_reason: printk will use it and ignore always_kmsg_dump.

Thank you,
Pasha