Switch away from /dev/mem to new ramoops pstore interface
Use the new ramoops pstore interface instead of reading /dev/mem or
needing to know anything about the ramoops memory configurations.
BUG=chromium-os:12059
TEST=x86-alex build and boot, logging_KernelCrashServer passes,
local "make tests" passes.
Change-Id: Ie713d4f93df7f666537c7181b1b438d7b85d4861
Signed-off-by: Kees Cook <keescook@chromium.org>
Reviewed-on: https://gerrit.chromium.org/gerrit/11245
Reviewed-by: Will Drewry <wad@chromium.org>
diff --git a/crash_reporter/kernel_collector.cc b/crash_reporter/kernel_collector.cc
index 8652d35..de4f92d 100644
--- a/crash_reporter/kernel_collector.cc
+++ b/crash_reporter/kernel_collector.cc
@@ -10,13 +10,11 @@
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 kDumpPath[] = "/dev/pstore";
+static const char kDumpFormat[] = "dmesg-ramoops-%d";
static const char kKernelExecName[] = "kernel";
+// Maximum number of records to examine in the kDumpPath.
+static const size_t kMaxDumpRecords = 100;
const pid_t kKernelPid = 0;
static const char kKernelSignatureKey[] = "sig";
// Byte length of maximum human readable portion of a kernel crash signature.
@@ -50,10 +48,7 @@
KernelCollector::KernelCollector()
: is_enabled_(false),
- ramoops_dump_path_(kDumpPath),
- ramoops_record_size_path_(kDumpRecordSize),
- ramoops_dump_start_path_(kDumpMemStart),
- ramoops_dump_size_path_(kDumpMemSize) {
+ ramoops_dump_path_(kDumpPath) {
// We expect crash dumps in the format of the architecture we are built for.
arch_ = GetCompilerArch();
}
@@ -66,46 +61,23 @@
}
bool KernelCollector::ReadRecordToString(std::string *contents,
- unsigned int current_record,
+ size_t 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;
+ FilePath ramoops_record;
+ GetRamoopsRecordPath(&ramoops_record, current_record);
+ if (!file_util::ReadFileToString(ramoops_record, &record)) {
+ LOG(ERROR) << "Unable to open " << ramoops_record.value();
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.
@@ -118,48 +90,25 @@
return true;
}
-bool LoadValue(FilePath path, size_t *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;
+void KernelCollector::GetRamoopsRecordPath(FilePath *path,
+ size_t record) {
+ *path = ramoops_dump_path_.Append(StringPrintf(kDumpFormat, record));
}
bool KernelCollector::LoadParameters() {
- // Read the parameters from the /sys/module/ramoops/parameters/* files
- // and convert them to numbers.
+ // Discover how many ramoops records are being exported by the driver.
+ size_t count;
- 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" ;
+ for (count = 0; count < kMaxDumpRecords; ++count) {
+ FilePath ramoops_record;
+ GetRamoopsRecordPath(&ramoops_record, count);
+
+ if (!file_util::PathExists(ramoops_record))
+ break;
}
- 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;
+ records_ = count;
+ return (records_ > 0);
}
bool KernelCollector::LoadPreservedDump(std::string *contents) {
@@ -172,7 +121,7 @@
// clear contents since ReadFileToString actually appends to the string.
contents->clear();
- for (size_t i = 0; i < mem_size_ / record_size_; ++i) {
+ for (size_t i = 0; i < records_; ++i) {
if (!ReadRecordToString(contents, i, &record_found)) {
break;
}
@@ -270,9 +219,13 @@
LOG(WARNING) << "KernelCollector does not understand this architecture";
return false;
}
- else if (!file_util::PathExists(ramoops_dump_path_)) {
- LOG(WARNING) << "Kernel does not support crash dumping";
- return false;
+ else {
+ FilePath ramoops_record;
+ GetRamoopsRecordPath(&ramoops_record, 0);
+ if (!file_util::PathExists(ramoops_record)) {
+ LOG(WARNING) << "Kernel does not support crash dumping";
+ return false;
+ }
}
// To enable crashes, we will eventually need to set
diff --git a/crash_reporter/kernel_collector.h b/crash_reporter/kernel_collector.h
index 4dfb1d7..e8698a7 100644
--- a/crash_reporter/kernel_collector.h
+++ b/crash_reporter/kernel_collector.h
@@ -54,9 +54,6 @@
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, LoadPreservedDump);
@@ -68,6 +65,7 @@
bool LoadPreservedDump(std::string *contents);
void StripSensitiveData(std::string *kernel_dump);
+ void GetRamoopsRecordPath(FilePath *path, size_t record);
virtual bool LoadParameters();
bool HasMoreRecords();
@@ -79,7 +77,7 @@
// 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,
+ size_t current_record,
bool *record_found);
void ProcessStackTrace(pcrecpp::StringPiece kernel_dump,
@@ -98,13 +96,8 @@
enum ArchKind GetCompilerArch(void);
bool is_enabled_;
- 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_;
+ size_t records_;
// 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 35bb0ec..10e9e36 100644
--- a/crash_reporter/kernel_collector_test.cc
+++ b/crash_reporter/kernel_collector_test.cc
@@ -43,9 +43,11 @@
collector_.Initialize(CountCrash,
IsMetrics);
mkdir("test", 0777);
+ mkdir(kTestKCrash, 0777);
test_kcrash_ = FilePath(kTestKCrash);
collector_.OverridePreservedDumpPath(test_kcrash_);
- unlink(kTestKCrash);
+ test_kcrash_ = test_kcrash_.Append("dmesg-ramoops-0");
+ unlink(test_kcrash_.value().c_str());
mkdir(kTestCrashDirectory, 0777);
chromeos::ClearLog();
}
@@ -73,11 +75,10 @@
std::string dump;
dump.clear();
- collector_.SetParameters(9, 0, 9);
WriteStringToFile(test_kcrash_, "emptydata");
ASSERT_FALSE(collector_.LoadPreservedDump(&dump));
ASSERT_EQ("", dump);
- collector_.SetParameters(17, 0, 17);
+
WriteStringToFile(test_kcrash_, "====1.1\nsomething");
ASSERT_TRUE(collector_.LoadPreservedDump(&dump));
ASSERT_EQ("something", dump);
@@ -232,7 +233,6 @@
// Disabled for crosbug.com/18622
//TEST_F(KernelCollectorTest, CollectBadDirectory) {
-// collector_.SetParameters(17, 0, 17);
// WriteStringToFile(test_kcrash_, "====1.1\nsomething");
// ASSERT_TRUE(collector_.Collect());
// ASSERT_TRUE(FindLog(
@@ -242,7 +242,6 @@
void KernelCollectorTest::SetUpSuccessfulCollect() {
collector_.ForceCrashDirectory(kTestCrashDirectory);
- collector_.SetParameters(17, 0, 17);
WriteStringToFile(test_kcrash_, "====1.1\nsomething");
ASSERT_EQ(0, s_crashes);
}