Re: [PATCH 2/2] Save current timestamp part of dmesg while writing oops message to pstore

From: Ankit Kumar
Date: Tue May 23 2017 - 04:50:54 EST


Hi Kees,



On Tuesday 23 May 2017 05:21 AM, Kees Cook wrote:
On Mon, May 22, 2017 at 3:20 AM, Ankit Kumar <ankit@xxxxxxxxxxxxxxxxxx> wrote:
Currently on panic or Oops, kernel saves the last few bytes from dmesg
buffer to nvram. Usually kdump does capture kernel memory and provide
dmesg logs as well. But in some cases where kdump fails to capture
vmcore, the dmesg buffer stored in nvram/pstore turns out to be very
helpful in analyzing root cause.

Present code creates pstore dump file(/sys/fs/pstore/dmesg-***) based on
timestamp(retrieved from header). Current pstore code creates dump file
(/sys/fs/pstore/dmesg-***) with that timestamp. Dump file can be analyzed
based on file creation time and we can make out whether dump file has latest
data or not.

But when we transfer pstore dump file(/sys/fs/pstore/dmesg-***) to other
machine or collect file using some utilities(sosreport/supportconfig) then file
timestamp gets changed and hence by looking at device file (dmesg-***) we won't
be able to identify whether dump has latest data or not.

Above issue can be fixed if we also have timestamp(dump creation time) as
initial few bytes while capturing dmesg buffer to pstore dump file
(/sys/fs/pstore/dmesg-***).


This patch enhances pstore write code to also write timestamp as part of data.

Here is sample log of dump file:(/sys/fs/pstore/dmesg-***)
Oops#1 Part1 [timestamp:1494939359.590463]
While I understand your rationale about possibly losing file timestamp
information in userspace, I think this is a solvable problem on the
collection side. If an additional header is needed, perhaps copy the
dmesg files like this:

for i in dmesg-*; do
(stat --format=%y /sys/fs/pstore/$i; \
cat /sys/fs/pstore/$i) > $collect_dir/$i
done

Yes. We can handle this in userspace. But we wanted to see if we can add this as part of pstore
log itself.


One of the primary concerns for pstore is the stored dump size,

I understand. How about adding timestamp to file name itself? Something like below

index 792a4e5..0837365 100644
--- a/fs/pstore/inode.c
+++ b/fs/pstore/inode.c
@@ -349,9 +349,10 @@ int pstore_mkfile(struct dentry *root, struct pstore_record *record)

switch (record->type) {
case PSTORE_TYPE_DMESG:
- scnprintf(name, sizeof(name), "dmesg-%s-%lld%s",
+ scnprintf(name, sizeof(name), "dmesg-%s-%lld%s-%lu.%lu",
record->psi->name, record->id,
- record->compressed ? ".enc.z" : "");
+ record->compressed ? ".enc.z" : "",
+ record->time.tv_sec, record->time.tv_nsec / 1000);
break;
case PSTORE_TYPE_CONSOLE:


~Ankit