LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Kees Cook <keescook@chromium.org>
To: Petr Mladek <pmladek@suse.com>
Cc: Pavel Tatashin <pasha.tatashin@soleen.com>,
	Anton Vorontsov <anton@enomsg.org>,
	Colin Cross <ccross@android.com>, Tony Luck <tony.luck@intel.com>,
	Jonathan Corbet <corbet@lwn.net>,
	Rob Herring <robh+dt@kernel.org>,
	Benson Leung <bleung@chromium.org>,
	Enric Balletbo i Serra <enric.balletbo@collabora.com>,
	Sergey Senozhatsky <sergey.senozhatsky@gmail.com>,
	Steven Rostedt <rostedt@goodmis.org>,
	James Morris <jmorris@namei.org>, Sasha Levin <sashal@kernel.org>,
	Linux Doc Mailing List <linux-doc@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	devicetree@vger.kernel.org
Subject: Re: [PATCH v3 0/6] allow ramoops to collect all kmesg_dump events
Date: Tue, 12 May 2020 11:45:54 -0700
Message-ID: <202005121111.6BECC45@keescook> (raw)
In-Reply-To: <20200512155207.GF17734@linux-b0ei>

On Tue, May 12, 2020 at 05:52:07PM +0200, Petr Mladek wrote:
> On Tue 2020-05-12 10:03:44, Pavel Tatashin wrote:
> > > OK, I personally see this as two separate problems:
> > >
> > >    1. Missing support to set loglevel per console.
> > >    2. Missing support to dump messages for other reasons.
> > >
> > > I would remove the paragraph about console log levels completely.
> > 
> > OK, I see your point, this paragraph can be removed, however, I think
> > it makes it clear to understand the rationale for this change. As I
> > understand, the per console loglevel has been proposed but were never
> > accepted.

I understood Pavel's rationale as an output from my questions in the v1
series, that went like this, paraphrased:

Pavel: "I need to have other kmsg dump reasons available to pstore."
Kees:  "Why can't you just use the pstore console dumper?"
Pavel: "It's too much for the slow device; we only need to know about
        specific events that are already provided by kmsg dump."
Kees:  "Ah! Sounds good, max_reasons it is."

So, AIUI, loglevel remains orthogonal to this, and it's my fault for
even causing to be be brought up. Please disregard! :)

> > printk.always_kmsg_dump is not working for me because ramoops has its
> > own filtering based on dump_oops boolean, and ignores everything but
> > panics and conditionally oops.
> > max_reason makes the ramoops internal logic cleaner compared to using dump_oops.
> 
> I see. Just to be sure. Is the main reason to add max_reason parameter
> to keep complatibility of the deprecated dump_oops parameter? Or is
> there any real use case for this granularity?

In my mind it seemed like a nice mapping, so it was an easy port.

> I wonder if anyone is actually using the ramoops.dump_oops parameter
> in reality. I would personally make it deprecated and change the
> default behavior to work according to printk.always_kmsg_dump parameter.

Yes. For things I'm aware of: ARM devices with very tiny persistent RAM
were using ramoops and setting dump_oops to 0 (specifically, setting
the DT "no-dump-oops" to 1), and larger Android and Chrome OS devices
using ramoops were setting to dump_oops to 1[1].

The logic built into pstore recognizes a difference between panic and
non-panic dumps as well, as the expectation is that there is little to
no kernel infrastructure available for use during a panic kmsg.

> IMHO, ramoops.dump_oops just increases complexity and should not have
> been introduced at all. I would try hard to avoid introducing even bigger
> complecity and mess.

I think dump_oops was the wrong implementation, but granularity control
is still needed. It is an old parameter, and is baked into many device
trees on systems, so I can't just drop it. (In fact, I've had to support
some other DT compat issues[2] as well.)

> I know that there is the "do not break existing userspace" rule. The
> question is if there is any user and if it is worth it.

For dump_oops, yes, there is unfortunately.

> > I agree, the reasons in kmsg_dump_reason do not order well  (I
> > actually want to add another reason for kexec type reboots, and where
> > do I put it?), so how about if we change the ordering list to
> > bitfield/flags, and instead of max_reason provide: "reasons" bitset?
> 
> It looks too complicated. I would really try hard to avoid the
> parameter at all.

Here are the problems I see being solved by this:

- lifting kmsg dump reason filtering out of the individual pstore
  backends and making it part of the "infrastructure", so that
  there is a central place to set expectations. Right now there
  is a mix of explicit and implicit kmsg dump handling:

  - arch/powerpc/kernel/nvram_64.c has a hard-coded list
  - drivers/firmware/efi/efi-pstore.c doesn't expect anything but
    OOPS and PANIC.
  - drivers/mtd/mtdoops.c tries to filter using its own dump_oops
    and doesn't expect anything but OOPS and PANIC.
  - fs/pstore/ram.c: has a hard-coded list and uses its own
    dump_oops.
  - drivers/mtd/mtdpstore.c (under development[3]) expected only
    OOPS and PANIC and had its own dump_oops.

- providing a way for backends that can deal with all kmsg dump reasons
  to do so without breaking existing default behavior (i.e. getting
  Pavel what he's interested in).

So, that said, I'm totally fine with a bit field. I just need a way to
map the kmsg dump reasons onto the existing backend expectations and to
have Pavel's needs addressed.

-Kees

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/platform/chrome/chromeos_pstore.c?h=v5.6#n60
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/pstore/ram.c?h=v5.6#n708
[3] https://lore.kernel.org/lkml/20200511233229.27745-11-keescook@chromium.org/

-- 
Kees Cook

  parent reply index

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-06 21:15 Kees Cook
2020-05-06 21:15 ` [PATCH v3 1/6] printk: honor the max_reason field in kmsg_dumper Kees Cook
2020-05-06 21:15 ` [PATCH v3 2/6] pstore/platform: Pass max_reason to kmesg dump Kees Cook
2020-05-06 21:25   ` Joe Perches
2020-05-06 22:40     ` Kees Cook
2020-05-06 21:15 ` [PATCH v3 3/6] pstore/ram: Refactor DT size parsing Kees Cook
2020-05-07 12:57   ` Pavel Tatashin
2020-05-07 18:04     ` Kees Cook
2020-05-06 21:15 ` [PATCH v3 4/6] pstore/ram: Introduce max_reason and convert dump_oops Kees Cook
2020-05-06 21:17   ` Kees Cook
2020-05-12 23:35   ` Tyler Hicks
2020-05-12 23:57     ` Kees Cook
2020-05-16  2:43     ` Kees Cook
2020-05-06 21:15 ` [PATCH v3 5/6] ramoops: Add max_reason optional field to ramoops DT node Kees Cook
2020-05-06 21:15 ` [PATCH v3 6/6] pstore/ram: Adjust module param permissions to reflect reality Kees Cook
2020-05-12 13:16 ` [PATCH v3 0/6] allow ramoops to collect all kmesg_dump events Petr Mladek
2020-05-12 14:03   ` Pavel Tatashin
2020-05-12 15:52     ` Petr Mladek
2020-05-12 16:03       ` Steven Rostedt
2020-05-12 16:49       ` Pavel Tatashin
2020-05-12 18:53         ` Kees Cook
2020-05-12 18:45       ` Kees Cook [this message]
2020-05-13  7:34         ` Petr Mladek
2020-05-13  7:47           ` Kees Cook
2020-05-13 14:35             ` Pavel Tatashin
2020-05-13 20:15               ` Kees Cook

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=202005121111.6BECC45@keescook \
    --to=keescook@chromium.org \
    --cc=anton@enomsg.org \
    --cc=bleung@chromium.org \
    --cc=ccross@android.com \
    --cc=corbet@lwn.net \
    --cc=devicetree@vger.kernel.org \
    --cc=enric.balletbo@collabora.com \
    --cc=jmorris@namei.org \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=pasha.tatashin@soleen.com \
    --cc=pmladek@suse.com \
    --cc=robh+dt@kernel.org \
    --cc=rostedt@goodmis.org \
    --cc=sashal@kernel.org \
    --cc=sergey.senozhatsky@gmail.com \
    --cc=tony.luck@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git
	git clone --mirror https://lore.kernel.org/lkml/9 lkml/git/9.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git