mbox series

[0/5] pstore: add tty frontend and multi-backend

Message ID 20230928024244.257687-1-xiangzao@linux.alibaba.com (mailing list archive)
Headers show
Series pstore: add tty frontend and multi-backend | expand

Message

Yuanhe Shu Sept. 28, 2023, 2:42 a.m. UTC
In public cloud scenario, if kdump service works abnormally,
users cannot get vmcore. Without vmcore, user has no idea why the
kernel crashed. Meanwhile, there is no additional information
to find the reason why the kdump service is abnormal.

One way is to obtain console messages through VNC. The drawback 
is that VNC is real-time, if user missed the timing to get the VNC
output, the crash needs to be retriggered.

Another way is to enable the console frontend of pstore and record the
console messages to the pstore backend. On the one hand, the console
logs only contain kernel printk logs and does not cover
user-mode print logs. Although we can redirect user-mode logs to the
pmsg frontend provided by pstore, user-mode information related to
booting and kdump service vary from systemd, kdump.sh, and so on which
makes redirection troublesome. So we added a tty frontend and save all
logs of tty driver to the pstore backend.

Another problem is that currently pstore only supports a single backend.
For debugging kdump problems, we hope to save the console logs and tty
logs to the ramoops backend of pstore, as it will not be lost after
rebooting. If the user has enabled another backend, the ramoops backend
will not be registered. To this end, we add the multi-backend function
to support simultaneous registration of multiple backends.

Based on the above changes, we can enable pstore in the crashdump kernel
and save the console logs and tty logs to the ramoops backend of pstore.
After rebooting, we can view the relevant logs by mounting the pstore
file system.

Furthermore, we also modified kexec-tools referring to crash-utils for
reading memory, so that pstore ramoops information can be read without
enabling pstore in first kernel. As we set the address and size of ramoops,
as well as the sizes of console and tty, we can infer the physical address
of console logs and tty logs in memory. Referring to the read method of
crash-utils, the console logs and tty logs are read from the memory,
user can get pstore debug information without affecting the first kernel
at all.

kexec-tools modification can be seen at
https://github.com/shuyuanmen/kexec-tools/blob/main/Add-pstore-segment.patch

Yuanhe Shu (5):
  pstore: add tty frontend
  pstore: add multi-backends support
  pstore: add subdirs for multi-backends
  pstore: remove the module parameter "backend"
  tools/pstore: update pstore selftests

 drivers/tty/n_tty.c                         |   1 +
 fs/pstore/Kconfig                           |  23 ++
 fs/pstore/Makefile                          |   2 +
 fs/pstore/blk.c                             |  10 +
 fs/pstore/ftrace.c                          |  22 +-
 fs/pstore/inode.c                           |  86 ++++++-
 fs/pstore/internal.h                        |  16 +-
 fs/pstore/platform.c                        | 238 ++++++++++++--------
 fs/pstore/pmsg.c                            |  23 +-
 fs/pstore/ram.c                             |  40 +++-
 fs/pstore/tty.c                             |  56 +++++
 fs/pstore/zone.c                            |  42 +++-
 include/linux/pstore.h                      |  33 +++
 include/linux/pstore_blk.h                  |   3 +
 include/linux/pstore_ram.h                  |   1 +
 include/linux/pstore_zone.h                 |   2 +
 include/linux/tty.h                         |  14 ++
 tools/testing/selftests/pstore/common_tests |   4 -
 18 files changed, 500 insertions(+), 116 deletions(-)
 create mode 100644 fs/pstore/tty.c

Comments

Kees Cook Sept. 29, 2023, 3:49 a.m. UTC | #1
On Thu, Sep 28, 2023 at 10:42:39AM +0800, Yuanhe Shu wrote:
> In public cloud scenario, if kdump service works abnormally,
> users cannot get vmcore. Without vmcore, user has no idea why the
> kernel crashed. Meanwhile, there is no additional information
> to find the reason why the kdump service is abnormal.
> 
> One way is to obtain console messages through VNC. The drawback 
> is that VNC is real-time, if user missed the timing to get the VNC
> output, the crash needs to be retriggered.
> 
> Another way is to enable the console frontend of pstore and record the
> console messages to the pstore backend. On the one hand, the console
> logs only contain kernel printk logs and does not cover
> user-mode print logs. Although we can redirect user-mode logs to the
> pmsg frontend provided by pstore, user-mode information related to
> booting and kdump service vary from systemd, kdump.sh, and so on which
> makes redirection troublesome. So we added a tty frontend and save all
> logs of tty driver to the pstore backend.

This is a clever solution!

> Another problem is that currently pstore only supports a single backend.
> For debugging kdump problems, we hope to save the console logs and tty
> logs to the ramoops backend of pstore, as it will not be lost after
> rebooting. If the user has enabled another backend, the ramoops backend
> will not be registered. To this end, we add the multi-backend function
> to support simultaneous registration of multiple backends.

Ah very cool; I really like this idea. I'd wanted to do it for a while
just to make testing easier, but I hadn't had time to attempt it.

> Based on the above changes, we can enable pstore in the crashdump kernel
> and save the console logs and tty logs to the ramoops backend of pstore.
> After rebooting, we can view the relevant logs by mounting the pstore
> file system.

So, before I do a line-at-a-time review of this code, I'd like to
address some design issues first.

I really don't want to make behavioral differences when we don't have
to:

- The multi-backend will enable _all possible_ backends, and that's a
  big change that will do weird things for some pstore users. I would
  prefer a pstore option to opt-in to enabling all backends. Perhaps
  have "pstore.backend=" be parsed with commas, so a list of backends
  can be provided, or "all" for the "all backends" behavior.

- Moving the pstorefs files into a subdirectory will break userspace
  immediately (e.g. systemd-pstore expects very specifically named
  files). Using subdirectories seems like a good idea, but perhaps
  we need hardlinks into the root pstorefs for the "first" backend,
  or some other creative solution here.

Then some technical thoughts about the TTY frontend's behavior:

- That 2 pstore records are created for every line of TTY output
  feels kind of inefficient, though I don't have a better idea.
  This is really only doable as you have it because the ramoops
  and zone backends treat the single prz as a circular buffer.
  I wonder about supporting this on other backends like EFI, but
  perhaps it's just not going to happen.

- I'd like to check with the TTY folks to see if this is the "right"
  place to hook to get a copy of what's being written.

Thanks and let me know what you think!

-Kees
Greg KH Sept. 29, 2023, 5:47 a.m. UTC | #2
On Thu, Sep 28, 2023 at 08:49:02PM -0700, Kees Cook wrote:
> On Thu, Sep 28, 2023 at 10:42:39AM +0800, Yuanhe Shu wrote:
> - I'd like to check with the TTY folks to see if this is the "right"
>   place to hook to get a copy of what's being written.

It depends on what you want to get.  What exact data are you looking
for here?  I couldn't figure it out and I think I already asked it in my
review of the "hook" location.

thanks,

greg k-h
Guilherme G. Piccoli Nov. 24, 2023, 9:43 p.m. UTC | #3
Hi Yuanhe / Kees.

My apologies (and embarrassment) for responding almost 2mo later...


On 29/09/2023 00:49, Kees Cook wrote:
> [...]
>> Another problem is that currently pstore only supports a single backend.
>> For debugging kdump problems, we hope to save the console logs and tty
>> logs to the ramoops backend of pstore, as it will not be lost after
>> rebooting. If the user has enabled another backend, the ramoops backend
>> will not be registered. To this end, we add the multi-backend function
>> to support simultaneous registration of multiple backends.
> 
> Ah very cool; I really like this idea. I'd wanted to do it for a while
> just to make testing easier, but I hadn't had time to attempt it.

I found the idea of multi-backend quite interesting, thanks for that!!!
And to add on what's Kees mentioned, not sure others' opinions but seems
to me this is a bit more straightforward / path-of-less-resistance than
the the tty frontend, so I'd suggest split the series and focus first on
this and once accepted, hook the tty thingy.

Not that the series can't be sent altogether, reviews could work in
parallel...I just see them as a bit tangential one to the other, personally.

> [...]
> - The multi-backend will enable _all possible_ backends, and that's a
>   big change that will do weird things for some pstore users. I would
>   prefer a pstore option to opt-in to enabling all backends. Perhaps
>   have "pstore.backend=" be parsed with commas, so a list of backends
>   can be provided, or "all" for the "all backends" behavior.
> 
> - Moving the pstorefs files into a subdirectory will break userspace
>   immediately (e.g. systemd-pstore expects very specifically named
>   files). Using subdirectories seems like a good idea, but perhaps
>   we need hardlinks into the root pstorefs for the "first" backend,
>   or some other creative solution here.
>

Big +1 in these two, commas are a very nice idea and changing the sysfs
current way of exposing pstore logs would break at least kdumpst (the
Steam Deck/Arch pstore / kdump tool), besides systemd-pstore that was
already mentioned (and who knows what more tools / scripts out in the
field).

Overall, thanks a bunch for this work Yuanhe!
Cheers,


Guilherme