[lldb][Process/FreeBSDKernelCore] Implement DoWriteMemory()#183553
[lldb][Process/FreeBSDKernelCore] Implement DoWriteMemory()#183553
Conversation
Signed-off-by: Minsoo Choo <minsoochoo0122@proton.me>
|
@llvm/pr-subscribers-lldb Author: Minsoo Choo (mchoo7) ChangesImplement Full diff: https://github.com/llvm/llvm-project/pull/183553.diff 5 Files Affected:
diff --git a/lldb/source/Plugins/Process/FreeBSD-Kernel-Core/CMakeLists.txt b/lldb/source/Plugins/Process/FreeBSD-Kernel-Core/CMakeLists.txt
index 8aafee3e43314..399baf432dcf8 100644
--- a/lldb/source/Plugins/Process/FreeBSD-Kernel-Core/CMakeLists.txt
+++ b/lldb/source/Plugins/Process/FreeBSD-Kernel-Core/CMakeLists.txt
@@ -1,3 +1,11 @@
+lldb_tablegen(ProcessFreeBSDKernelCoreProperties.inc -gen-lldb-property-defs
+ SOURCE ProcessFreeBSDKernelCoreProperties.td
+ TARGET LLDBPluginProcessFreeBSDKernelCorePropertiesGen)
+
+lldb_tablegen(ProcessFreeBSDKernelCorePropertiesEnum.inc -gen-lldb-property-enum-defs
+ SOURCE ProcessFreeBSDKernelCoreProperties.td
+ TARGET LLDBPluginProcessFreeBSDKernelCorePropertiesEnumGen)
+
add_lldb_library(lldbPluginProcessFreeBSDKernelCore PLUGIN
ProcessFreeBSDKernelCore.cpp
RegisterContextFreeBSDKernelCore_arm64.cpp
@@ -12,3 +20,7 @@ add_lldb_library(lldbPluginProcessFreeBSDKernelCore PLUGIN
lldbTarget
kvm
)
+
+add_dependencies(lldbPluginProcessFreeBSDKernelCore
+ LLDBPluginProcessFreeBSDKernelCorePropertiesGen
+ LLDBPluginProcessFreeBSDKernelCorePropertiesEnumGen)
diff --git a/lldb/source/Plugins/Process/FreeBSD-Kernel-Core/ProcessFreeBSDKernelCore.cpp b/lldb/source/Plugins/Process/FreeBSD-Kernel-Core/ProcessFreeBSDKernelCore.cpp
index ebbf89669cc40..577d8e0d50cf1 100644
--- a/lldb/source/Plugins/Process/FreeBSD-Kernel-Core/ProcessFreeBSDKernelCore.cpp
+++ b/lldb/source/Plugins/Process/FreeBSD-Kernel-Core/ProcessFreeBSDKernelCore.cpp
@@ -6,7 +6,6 @@
//
//===----------------------------------------------------------------------===//
-#include "lldb/Core/Debugger.h"
#include "lldb/Core/Module.h"
#include "lldb/Core/PluginManager.h"
#include "lldb/Interpreter/CommandInterpreter.h"
@@ -25,6 +24,42 @@ using namespace lldb_private;
LLDB_PLUGIN_DEFINE(ProcessFreeBSDKernelCore)
+namespace {
+
+#define LLDB_PROPERTIES_processfreebsdkernelcore
+#include "ProcessFreeBSDKernelCoreProperties.inc"
+
+enum {
+#define LLDB_PROPERTIES_processfreebsdkernelcore
+#include "ProcessFreeBSDKernelCorePropertiesEnum.inc"
+};
+
+class PluginProperties : public Properties {
+public:
+ static llvm::StringRef GetSettingName() {
+ return ProcessFreeBSDKernelCore::GetPluginNameStatic();
+ }
+
+ PluginProperties() : Properties() {
+ m_collection_sp = std::make_shared<OptionValueProperties>(GetSettingName());
+ m_collection_sp->Initialize(g_processfreebsdkernelcore_properties_def);
+ }
+
+ ~PluginProperties() override = default;
+
+ bool GetReadOnly() const {
+ const uint32_t idx = ePropertyReadOnly;
+ return GetPropertyAtIndexAs<bool>(idx, true);
+ }
+};
+
+} // namespace
+
+static PluginProperties &GetGlobalPluginProperties() {
+ static PluginProperties g_settings;
+ return g_settings;
+}
+
ProcessFreeBSDKernelCore::ProcessFreeBSDKernelCore(lldb::TargetSP target_sp,
ListenerSP listener_sp,
kvm_t *kvm,
@@ -56,10 +91,22 @@ void ProcessFreeBSDKernelCore::Initialize() {
llvm::call_once(g_once_flag, []() {
PluginManager::RegisterPlugin(GetPluginNameStatic(),
- GetPluginDescriptionStatic(), CreateInstance);
+ GetPluginDescriptionStatic(), CreateInstance,
+ DebuggerInitialize);
});
}
+void ProcessFreeBSDKernelCore::DebuggerInitialize(Debugger &debugger) {
+ if (!PluginManager::GetSettingForProcessPlugin(
+ debugger, PluginProperties::GetSettingName())) {
+ const bool is_global_setting = true;
+ PluginManager::CreateSettingForProcessPlugin(
+ debugger, GetGlobalPluginProperties().GetValueProperties(),
+ "Properties for the freebsd-kernel process plug-in.",
+ is_global_setting);
+ }
+}
+
void ProcessFreeBSDKernelCore::Terminate() {
PluginManager::UnregisterPlugin(ProcessFreeBSDKernelCore::CreateInstance);
}
@@ -90,6 +137,26 @@ void ProcessFreeBSDKernelCore::RefreshStateAfterStop() {
}
}
+size_t ProcessFreeBSDKernelCore::DoWriteMemory(lldb::addr_t addr,
+ const void *buf, size_t size,
+ Status &error) {
+ if (GetGlobalPluginProperties().GetReadOnly()) {
+ error = Status::FromErrorString(
+ "Memory writes are currently disabled. You can enable them with "
+ "`settings set plugin.process.freebsd-kernel-core.read-only false`.");
+ return 0;
+ }
+
+ ssize_t rd = 0;
+ rd = kvm_write(m_kvm, addr, buf, size);
+ if (rd < 0 || static_cast<size_t>(rd) != size) {
+ error = Status::FromErrorStringWithFormat("Writing memory failed: %s",
+ GetError());
+ return rd > 0 ? rd : 0;
+ }
+ return rd;
+}
+
bool ProcessFreeBSDKernelCore::DoUpdateThreadList(ThreadList &old_thread_list,
ThreadList &new_thread_list) {
if (old_thread_list.GetSize(false) == 0) {
diff --git a/lldb/source/Plugins/Process/FreeBSD-Kernel-Core/ProcessFreeBSDKernelCore.h b/lldb/source/Plugins/Process/FreeBSD-Kernel-Core/ProcessFreeBSDKernelCore.h
index c677c236ddc21..67cfae13d2a4d 100644
--- a/lldb/source/Plugins/Process/FreeBSD-Kernel-Core/ProcessFreeBSDKernelCore.h
+++ b/lldb/source/Plugins/Process/FreeBSD-Kernel-Core/ProcessFreeBSDKernelCore.h
@@ -9,6 +9,7 @@
#ifndef LLDB_SOURCE_PLUGINS_PROCESS_FREEBSDKERNEL_PROCESSFREEBSDKERNELCORE_H
#define LLDB_SOURCE_PLUGINS_PROCESS_FREEBSDKERNEL_PROCESSFREEBSDKERNELCORE_H
+#include "lldb/Core/Debugger.h"
#include "lldb/Target/PostMortemProcess.h"
#include <kvm.h>
@@ -27,6 +28,8 @@ class ProcessFreeBSDKernelCore : public lldb_private::PostMortemProcess {
static void Initialize();
+ static void DebuggerInitialize(lldb_private::Debugger &debugger);
+
static void Terminate();
static llvm::StringRef GetPluginNameStatic() { return "freebsd-kernel-core"; }
@@ -48,6 +51,9 @@ class ProcessFreeBSDKernelCore : public lldb_private::PostMortemProcess {
void RefreshStateAfterStop() override;
+ size_t DoWriteMemory(lldb::addr_t addr, const void *buf, size_t size,
+ lldb_private::Status &error) override;
+
protected:
bool DoUpdateThreadList(lldb_private::ThreadList &old_thread_list,
lldb_private::ThreadList &new_thread_list) override;
diff --git a/lldb/source/Plugins/Process/FreeBSD-Kernel-Core/ProcessFreeBSDKernelCoreProperties.td b/lldb/source/Plugins/Process/FreeBSD-Kernel-Core/ProcessFreeBSDKernelCoreProperties.td
new file mode 100644
index 0000000000000..22bf3c8bfefca
--- /dev/null
+++ b/lldb/source/Plugins/Process/FreeBSD-Kernel-Core/ProcessFreeBSDKernelCoreProperties.td
@@ -0,0 +1,8 @@
+include "../../../../include/lldb/Core/PropertiesBase.td"
+
+let Definition = "processfreebsdkernelcore", Path = "plugin.process.freebsd-kernel-core" in {
+ def ReadOnly: Property<"read-only", "Boolean">,
+ Global,
+ DefaultTrue,
+ Desc<"Disable memory writes to the core. This is enabled by default for safety reasons.">;
+}
diff --git a/llvm/docs/ReleaseNotes.md b/llvm/docs/ReleaseNotes.md
index d9fb4ddc36c9d..e69605b9d9cce 100644
--- a/llvm/docs/ReleaseNotes.md
+++ b/llvm/docs/ReleaseNotes.md
@@ -220,6 +220,8 @@ Changes to LLDB
* Threads are listed in incrmental order by pid then by tid.
* Unread kernel messages saved in msgbufp are now printed when lldb starts. This information is printed only
when lldb is in the interactive mode (i.e. not in batch mode).
+* Writing to core is now supported. For safety reasons, this feature is off by default. To enable it,
+ `plugin.process.freebsd-kernel-core.read-only` must be set to `false`.
### Linux
|
|
@DavidSpickett I recreated this PR with changes addressing comments from #183237. Sorry for inconvenience. |
Unless you have a good argument for saying "security" rather than "safety" I think "safety" is a better description of your intent here. If there's official FreeBSD documentation that uses the term "security" around kernel debugging, I'd also accept that as a justification. As I said before, once you're debugging, feels like "security" is pretty low on the concerns. Breaking the system so I can't debug anymore, yeah, that's a big concern, as you've stated. But feels more like "safety" than "security". In the "memory safety" sense. Pretty safe if you can't write to it :) |
|
So all you gotta do is change that one word in the PR description, I typed all that before reading the code again. |
|
I'm ok with the code, and the settings part we already had reviewed. I just want to know about:
Can you define what "kernel dump" is here? Because I'm thinking of it like an elf core, and writing to "memory" in an elf core I suppose could be done but is generally not allowed. Can you explain that a bit more? I agree with this setting's importance for /dev/mem, which I understand to be a window into the live kernel. |
| @@ -0,0 +1,8 @@ | |||
| include "../../../../include/lldb/Core/PropertiesBase.td" | |||
|
|
|||
| let Definition = "processfreebsdkernelcore", Path = "plugin.process.freebsd-kernel-core" in { | |||
There was a problem hiding this comment.
Actually didn't you rename this plugin? Should it just be freebsd-kernel?
There was a problem hiding this comment.
Maybe that change is in flight or I'm mixing things up, anyway please check.
There was a problem hiding this comment.
Sorry, you renamed it to freebsd-kernel-core didn't you. You added the core part.
Right? 😆 It's been a long day here.
Your right. After re-reading what I wrote, safety makes more sense. E.g.: $ lldb /boot/kernel/kernel -c /dev/mem
(lldb) target create "/boot/kernel/kernel" --core "/dev/mem"
error: Cannot open '/dev/mem': Permission denied.
By kernel dump, I mean kernel crash dump described here. I'll reword the PR description. Kernel crash dump is/isn't kernel crash dump depending on its format. Full kernel crash dump is a elf core. The whole system memory (including unused pages) are dumped onto physical storage. The downside of this is that there are systems using hundreds of GB or a few TB of ram, and sometimes it's difficult to secure a block storage with that capacity. So FreeBSD got another kernel crash dump format called minidump (itis different from lldb's minidump plugin which is windows crash dump only). Minidump is not elf core so it needs FreeBSD's |
|
Writing memory on crash dump is possible but meaningless. FreeBSD devs only writes |
I didn't think of that actually, I was just going on vibes to be honest. That's a great justification for "safety". 👍
Thanks, this helped me clear up a big point of confusion for me (and those manual pages are great btw). Is there anywhere that documents LLDB commands for similar things? If the feature is not mature enough to put in the FreeBSD manual then I think it would be fine to have a page on the LLDB site instead. For example I have one that collects all the important trivia for AArch64 Linux (https://lldb.llvm.org/use/aarch64-linux.html). Not a requirement, but if you are going to write up a summary of your work anyway, consider doing that too. It's always nice to have a single place to send people too to show off what you did (aside from the release notes ofc :) ).
I figured out why I'm so confused. This full elf core fits my usual expectations for However, for the FreeBSDKernelCore plugin, there is also the option to use this /dev/mem which is the running kernel. Which didn't make sense to me because that's not a "core file". But that's just me fixating on the naming. If I think about core file plugins more as "get memory from this place", then /dev/mem is just another place. So: Is basically saying: I'm so used to So with that cleared up...
I wonder if this setting should only apply to /dev/mem, and writes should never be allowed for offline crash dump debugging. The average user of userspace debugging would not expect to be able to write to memory read from a core. Since we read the file from disk, I'm not sure it could work without significant effort. Perhaps there is a use case for doing so, but I'd need to see that either it's allowed by the other kernel debuggers, or that you're going to implement it, and how you will do that. There is a blocker for what I'm suggesting though. If the FreeBSDKernelCore cannot tell whether it's reading from /dev/mem, or from a crash dump (or the code to do so would be unwieldy) then we cannot do this. I suspect writes in a crash dump session would fail anyway, so we could rely on users to quickly realise why that's the case. Can you show me what kgdb allows, if anything, and what happens in lldb if you write during a crash dump session? |
I'm planning to update it once my LLDB works are completed. But before that I need to write LLDB version of debugging script for kernel debugging (e.g. acttrace).
Memory write is done through However this will be problematic once I start working on making this plugin cross-platform. But it's a future problem and the current implementation is closer to KGDB which has been working for decades. |
KGDB: (kgdb) set {void *} 0xffffffff81e02460 = 0xffffffff81e02460
Cannot access memory at address 0xffffffff81e02460LLDB: (lldb) memory write msgbufp 0x0080
error: Memory write to 0xfffff8185de7ffb8 failed: Writing memory failed: kvm_write not implemented for dead kernels.So it doesn't work unlike what the manpage says.. But at least LLDB gives better error description. |
DavidSpickett
left a comment
DavidSpickett
left a comment
There was a problem hiding this comment.
Yeah lldb's message makes it pretty clear. That's given me a lot of confidence in this change.
On the grounds that:
- FreeBSDKernelCore uses kvm to write to whatever it's been given.
- If someone wants to enable writing to offline files then they'd have to extend kvm, not lldb.
- Therefore it's fine for FreeBSDKernelCore to have this read-only setting that applies to all kvm powered access. LLDB shouldn't be checking "is this /dev/mem or not".
(at least for as long as kvm is the only method of access)
So this LGTM to merge as long as the release note is adjusted.
Yep, "somebody else's problem" (https://en.wikipedia.org/wiki/Somebody_else%27s_problem) :) |
Signed-off-by: Minsoo Choo <minsoochoo0122@proton.me>
) Implement `ProcessFreeBSDKernelCore::DoWriteMemory()` to write data on kernel crash dump or `/dev/mem`. Due to safety reasons (e.g. writing wrong value on `/dev/mem` can trigger kernel panic), this feature is only enabled when `plugin.process.freebsd-kernel-core.read-only` is set to false (true by default). Since 85a1fe6 (llvm#183237) was reverted as it was prematurely merged, I'm committing changes again with corrections here. --------- Signed-off-by: Minsoo Choo <minsoochoo0122@proton.me>
3 participants
Implement
ProcessFreeBSDKernelCore::DoWriteMemory()to write data on kernel crash dump or/dev/mem. Due to safety reasons (e.g. writing wrong value on/dev/memcan trigger kernel panic), this feature is only enabled whenplugin.process.freebsd-kernel-core.read-onlyis set to false (true by default).Since 85a1fe6 (#183237) was reverted as it was prematurely merged, I'm committing changes again with corrections here.