crash-reporter: Update crash-reporter to work with ramoops
The old crash reporter used preserved to get the memory dump. Updated it
to use the upstream ramoops module. Also updated the unit tests.
Depends on other patches related to this bug report:
The backport series starting with:
http://gerrit.chromium.org/gerrit/#change,5342 until 5347 (included)
The series starting with: http://gerrit.chromium.org/gerrit/#change,5426
until 5429 (included)
http://gerrit.chromium.org/gerrit/#change,5159
BUG=chromium-os:12059
TEST=Ran module unit tests, ran logging_KernelCrashServer and
platform_KernelErrorPaths, checked manually that the
crash file existed.
Change-Id: I37bdb30513acfab79997e478af5de35ca33557fd
Signed-off-by: Sergiu Iordache <sergiu@chromium.org>
Reviewed-on: http://gerrit.chromium.org/gerrit/5432
Reviewed-by: Ken Mixter <kmixter@chromium.org>
Reviewed-by: Mandeep Singh Baines <msb@chromium.org>
Reviewed-by: Michael Krebs <mkrebs@chromium.org>
diff --git a/crash_reporter/kernel_collector.cc b/crash_reporter/kernel_collector.cc
index 529e35f..0c69dd6 100644
--- a/crash_reporter/kernel_collector.cc
+++ b/crash_reporter/kernel_collector.cc
@@ -8,15 +8,19 @@
#include "base/logging.h"
#include "base/string_util.h"
-const char KernelCollector::kClearingSequence[] = " ";
static const char kDefaultKernelStackSignature[] =
"kernel-UnspecifiedStackSignature";
+static const char kDumpMemSize[] = "/sys/module/ramoops/parameters/mem_size";
+static const char kDumpMemStart[] =
+ "/sys/module/ramoops/parameters/mem_address";
+static const char kDumpPath[] = "/dev/mem";
+static const char kDumpRecordSize[] =
+ "/sys/module/ramoops/parameters/record_size";
static const char kKernelExecName[] = "kernel";
const pid_t kKernelPid = 0;
static const char kKernelSignatureKey[] = "sig";
// Byte length of maximum human readable portion of a kernel crash signature.
static const int kMaxHumanStringLength = 40;
-static const char kPreservedDumpPath[] = "/sys/kernel/debug/preserved/kcrash";
const uid_t kRootUid = 0;
// Time in seconds from the final kernel log message for a call stack
// to count towards the signature of the kcrash.
@@ -46,7 +50,10 @@
KernelCollector::KernelCollector()
: is_enabled_(false),
- preserved_dump_path_(kPreservedDumpPath) {
+ ramoops_dump_path_(kDumpPath),
+ ramoops_record_size_path_(kDumpRecordSize),
+ ramoops_dump_start_path_(kDumpMemStart),
+ ramoops_dump_size_path_(kDumpMemSize) {
// We expect crash dumps in the format of the architecture we are built for.
arch_ = GetCompilerArch();
}
@@ -55,16 +62,130 @@
}
void KernelCollector::OverridePreservedDumpPath(const FilePath &file_path) {
- preserved_dump_path_ = file_path;
+ ramoops_dump_path_ = file_path;
+}
+
+bool KernelCollector::ReadRecordToString(std::string *contents,
+ unsigned int current_record,
+ bool *record_found) {
+ FILE *file = fopen(ramoops_dump_path_.value().c_str(), "rb");
+
+ // A record is a ramoops dump. It has an associated size of "record_size".
+ std::string record;
+ std::string captured;
+ static const unsigned int s_total_size_to_read = 4096;
+ size_t len = s_total_size_to_read;
+
+ // Ramoops appends a header to a crash which contains ==== followed by a
+ // timestamp. Ignore the header.
+ pcrecpp::RE record_re("====\\d+\\.\\d+\n(.*)",
+ pcrecpp::RE_Options().set_multiline(true).set_dotall(true));
+
+ if (!file) {
+ LOG(ERROR) << "Unable to open " << kDumpPath;
+ return false;
+ }
+ // We're reading from /dev/mem, so we have to fseek to the desired area.
+ fseek(file, mem_start_ + current_record * record_size_, SEEK_SET);
+ size_t size_to_read = record_size_;
+
+ while (size_to_read > 0 && !feof(file) && len == s_total_size_to_read) {
+ char buf[s_total_size_to_read + 1];
+ len = fread(buf, 1, s_total_size_to_read, file);
+ // In the rare case that we read less than the minimum size null terminate
+ // the string properly
+ if (len < s_total_size_to_read) {
+ buf[len] = 0;
+ } else {
+ buf[s_total_size_to_read] = 0;
+ }
+ // Add the data.. if the string is null terminated and smaller then the
+ // buffer than add just that part. Otherwise add the whole buffer.
+ record.append(buf);
+ size_to_read -= len;
+ }
+
+ fclose(file);
+
+ if (record_re.FullMatch(record, &captured)){
+ // Found a match, append it to the content.
+ contents->append(captured);
+ *record_found = true;
+ } else {
+ *record_found = false;
+ }
+
+ return true;
+}
+
+bool LoadValue(FilePath path, unsigned int *element){
+ std::string buf;
+ char *end;
+ if (!file_util::ReadFileToString(path, &buf)) {
+ LOG(ERROR) << "Unable to read " << path.value();
+ return false;
+ }
+ *element = strtol(buf.c_str(), &end, 10);
+ if (end != NULL && *end =='\0') {
+ LOG(ERROR) << "Invalid number in " << path.value();
+ return false;
+ }
+
+ return true;
+}
+
+bool KernelCollector::LoadParameters() {
+ // Read the parameters from the /sys/module/ramoops/parameters/* files
+ // and convert them to numbers.
+
+ if (!LoadValue(ramoops_record_size_path_, &record_size_))
+ return false;
+ if (record_size_ <= 0){
+ LOG(ERROR) << "Record size is <= 0" ;
+ }
+ if (!LoadValue(ramoops_dump_start_path_, &mem_start_))
+ return false;
+ if (!LoadValue(ramoops_dump_size_path_, &mem_size_))
+ return false;
+ if (mem_size_ <= 0){
+ LOG(ERROR) << "Memory size is <= 0" ;
+ }
+
+ return true;
+}
+
+void KernelCollector::SetParameters(size_t record_size, size_t mem_start,
+ size_t mem_size) {
+ // Used for unit testing.
+ record_size_ = record_size;
+ mem_start_ = mem_start;
+ mem_size_ = mem_size;
}
bool KernelCollector::LoadPreservedDump(std::string *contents) {
+ // Load dumps from the preserved memory and save them in contents.
+ // Since the system is set to restart on oops we won't actually ever have
+ // multiple records (only 0 or 1), but check in case we don't restart on
+ // oops in the future.
+ bool any_records_found = false;
+ bool record_found = false;
// clear contents since ReadFileToString actually appends to the string.
contents->clear();
- if (!file_util::ReadFileToString(preserved_dump_path_, contents)) {
- LOG(ERROR) << "Unable to read " << preserved_dump_path_.value();
+
+ for (size_t i = 0; i < mem_size_ / record_size_; ++i) {
+ if (!ReadRecordToString(contents, i, &record_found)) {
+ break;
+ }
+ if (record_found) {
+ any_records_found = true;
+ }
+ }
+
+ if (!any_records_found) {
+ LOG(ERROR) << "No valid records found in " << ramoops_dump_path_.value();
return false;
}
+
return true;
}
@@ -149,7 +270,7 @@
LOG(WARNING) << "KernelCollector does not understand this architecture";
return false;
}
- else if (!file_util::PathExists(preserved_dump_path_)) {
+ else if (!file_util::PathExists(ramoops_dump_path_)) {
LOG(WARNING) << "Kernel does not support crash dumping";
return false;
}
@@ -161,20 +282,6 @@
return true;
}
-bool KernelCollector::ClearPreservedDump() {
- // It is necessary to write at least one byte to the kcrash file for
- // the log to actually be cleared.
- if (file_util::WriteFile(
- preserved_dump_path_,
- kClearingSequence,
- strlen(kClearingSequence)) != strlen(kClearingSequence)) {
- LOG(ERROR) << "Failed to clear kernel crash dump";
- return false;
- }
- LOG(INFO) << "Cleared kernel crash diagnostics";
- return true;
-}
-
// Hash a string to a number. We define our own hash function to not
// be dependent on a C++ library that might change. This function
// uses basically the same approach as tr1/functional_hash.h but with
@@ -372,6 +479,10 @@
bool KernelCollector::Collect() {
std::string kernel_dump;
FilePath root_crash_directory;
+
+ if (!LoadParameters()) {
+ return false;
+ }
if (!LoadPreservedDump(&kernel_dump)) {
return false;
}
@@ -427,9 +538,6 @@
LOG(INFO) << "Stored kcrash to " << kernel_crash_path.value();
}
- if (!ClearPreservedDump()) {
- return false;
- }
return true;
}
diff --git a/crash_reporter/kernel_collector.h b/crash_reporter/kernel_collector.h
index 1c86f40..4dfb1d7 100644
--- a/crash_reporter/kernel_collector.h
+++ b/crash_reporter/kernel_collector.h
@@ -54,9 +54,11 @@
void SetArch(enum ArchKind arch);
enum ArchKind GetArch() { return arch_; }
+ // Set the parameters for reading the crash dump.
+ void SetParameters(size_t record_size, size_t mem_start, size_t mem_size);
+
private:
friend class KernelCollectorTest;
- FRIEND_TEST(KernelCollectorTest, ClearPreservedDump);
FRIEND_TEST(KernelCollectorTest, LoadPreservedDump);
FRIEND_TEST(KernelCollectorTest, StripSensitiveDataBasic);
FRIEND_TEST(KernelCollectorTest, StripSensitiveDataBulk);
@@ -64,9 +66,22 @@
FRIEND_TEST(KernelCollectorTest, CollectOK);
bool LoadPreservedDump(std::string *contents);
- bool ClearPreservedDump();
void StripSensitiveData(std::string *kernel_dump);
+ virtual bool LoadParameters();
+ bool HasMoreRecords();
+
+ // Read a record to string, modified from file_utils since that didn't
+ // provide a way to restrict the read length.
+ // Return value indicates (only) error state:
+ // * false when we get an error (can't read from dump location).
+ // * true if no error occured.
+ // Not finding a valid record is not an error state and is signaled by the
+ // record_found output parameter.
+ bool ReadRecordToString(std::string *contents,
+ unsigned int current_record,
+ bool *record_found);
+
void ProcessStackTrace(pcrecpp::StringPiece kernel_dump,
bool print_diagnostics,
unsigned *hash,
@@ -83,8 +98,13 @@
enum ArchKind GetCompilerArch(void);
bool is_enabled_;
- FilePath preserved_dump_path_;
- static const char kClearingSequence[];
+ size_t mem_start_;
+ size_t mem_size_;
+ FilePath ramoops_dump_path_;
+ FilePath ramoops_record_size_path_;
+ FilePath ramoops_dump_start_path_;
+ FilePath ramoops_dump_size_path_;
+ size_t record_size_;
// The architecture of kernel dump strings we are working with.
enum ArchKind arch_;
diff --git a/crash_reporter/kernel_collector_test.cc b/crash_reporter/kernel_collector_test.cc
index 431bfca..35bb0ec 100644
--- a/crash_reporter/kernel_collector_test.cc
+++ b/crash_reporter/kernel_collector_test.cc
@@ -28,6 +28,14 @@
return s_metrics;
}
+class TKernelCollector : public KernelCollector {
+ bool LoadParameters() {
+ // Since we don't have /sys/module/ramoops/parameters on our system just
+ // return true instead of getting the parameters from the files.
+ return true;
+ }
+};
+
class KernelCollectorTest : public ::testing::Test {
void SetUp() {
s_crashes = 0;
@@ -49,26 +57,28 @@
}
void SetUpSuccessfulCollect();
- void CheckPreservedDumpClear();
void ComputeKernelStackSignatureCommon();
- KernelCollector collector_;
+ TKernelCollector collector_;
FilePath test_kcrash_;
};
TEST_F(KernelCollectorTest, ComputeKernelStackSignatureBase) {
// Make sure the normal build architecture is detected
- EXPECT_TRUE(collector_.GetArch() != KernelCollector::archUnknown);
+ EXPECT_TRUE(collector_.GetArch() != TKernelCollector::archUnknown);
}
TEST_F(KernelCollectorTest, LoadPreservedDump) {
ASSERT_FALSE(file_util::PathExists(test_kcrash_));
std::string dump;
+ dump.clear();
+
+ collector_.SetParameters(9, 0, 9);
+ WriteStringToFile(test_kcrash_, "emptydata");
ASSERT_FALSE(collector_.LoadPreservedDump(&dump));
- WriteStringToFile(test_kcrash_, "");
- ASSERT_TRUE(collector_.LoadPreservedDump(&dump));
ASSERT_EQ("", dump);
- WriteStringToFile(test_kcrash_, "something");
+ collector_.SetParameters(17, 0, 17);
+ WriteStringToFile(test_kcrash_, "====1.1\nsomething");
ASSERT_TRUE(collector_.LoadPreservedDump(&dump));
ASSERT_EQ("something", dump);
}
@@ -89,17 +99,6 @@
ASSERT_EQ(s_crashes, 0);
}
-TEST_F(KernelCollectorTest, ClearPreservedDump) {
- std::string dump;
- ASSERT_FALSE(file_util::PathExists(test_kcrash_));
- WriteStringToFile(test_kcrash_, "something");
- ASSERT_TRUE(collector_.LoadPreservedDump(&dump));
- ASSERT_EQ("something", dump);
- ASSERT_TRUE(collector_.ClearPreservedDump());
- ASSERT_TRUE(collector_.LoadPreservedDump(&dump));
- ASSERT_EQ(KernelCollector::kClearingSequence, dump);
-}
-
TEST_F(KernelCollectorTest, StripSensitiveDataBasic) {
// Basic tests of StripSensitiveData...
@@ -219,7 +218,8 @@
TEST_F(KernelCollectorTest, CollectPreservedFileMissing) {
ASSERT_FALSE(collector_.Collect());
- ASSERT_TRUE(FindLog("Unable to read test/kcrash"));
+ ASSERT_TRUE(FindLog("Unable to open"));
+ ASSERT_TRUE(FindLog("No valid records found"));
ASSERT_EQ(0, s_crashes);
}
@@ -231,35 +231,28 @@
}
// Disabled for crosbug.com/18622
-// TEST_F(KernelCollectorTest, CollectBadDirectory) {
-// WriteStringToFile(test_kcrash_, "something");
+//TEST_F(KernelCollectorTest, CollectBadDirectory) {
+// collector_.SetParameters(17, 0, 17);
+// WriteStringToFile(test_kcrash_, "====1.1\nsomething");
// ASSERT_TRUE(collector_.Collect());
// ASSERT_TRUE(FindLog(
// "Unable to create appropriate crash directory"));
// ASSERT_EQ(1, s_crashes);
-// }
+//}
void KernelCollectorTest::SetUpSuccessfulCollect() {
collector_.ForceCrashDirectory(kTestCrashDirectory);
- WriteStringToFile(test_kcrash_, "something");
+ collector_.SetParameters(17, 0, 17);
+ WriteStringToFile(test_kcrash_, "====1.1\nsomething");
ASSERT_EQ(0, s_crashes);
}
-void KernelCollectorTest::CheckPreservedDumpClear() {
- // Make sure the preserved dump is now clear.
- std::string dump;
- ASSERT_TRUE(collector_.LoadPreservedDump(&dump));
- ASSERT_EQ(KernelCollector::kClearingSequence, dump);
-}
-
TEST_F(KernelCollectorTest, CollectOptedOut) {
SetUpSuccessfulCollect();
s_metrics = false;
ASSERT_TRUE(collector_.Collect());
ASSERT_TRUE(FindLog("(ignoring - no consent)"));
ASSERT_EQ(0, s_crashes);
-
- CheckPreservedDumpClear();
}
TEST_F(KernelCollectorTest, CollectOK) {
@@ -283,7 +276,6 @@
ASSERT_TRUE(file_util::ReadFileToString(FilePath(filename), &contents));
ASSERT_EQ("something", contents);
- CheckPreservedDumpClear();
}
// Perform tests which are common across architectures
@@ -362,7 +354,7 @@
"[<c017bfe0>] (proc_reg_write+0x88/0x9c)\n";
std::string signature;
- collector_.SetArch(KernelCollector::archArm);
+ collector_.SetArch(TKernelCollector::archArm);
EXPECT_TRUE(
collector_.ComputeKernelStackSignature(kBugToPanic, &signature, false));
EXPECT_EQ("kernel-write_breakme-97D3E92F", signature);