Gdb server #2168

baciumar · 2020-10-08T09:15:57Z

Reason for This PR

Initial implementation of a GDB server for guest debugging.

Description of Changes

Created a new package implementing the gdbstub interface for client-server communication
Implemented the following functionalities: breakpoint, single-step, reading of registers and memory
Implemented virtual-to-physical address translation, used by any guest memory access performed as part of the above-mentioned functionalities
Created a new thread for handling client requests and two channels for vCPU - GDB server thread communication
Added a "--debugger" command-line option for the firecracker executable
This functionality can be added in rust-vmm.

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license.

PR Checklist

[Author TODO: Meet these criteria.]
[Reviewer TODO: Verify that these criteria are met. Request changes if not]

All commits in this PR are signed (git commit -s).
The reason for this PR is clearly provided (issue no. or explanation).
The description of changes is clear and encompassing.
Any required documentation changes (code and docs) are included in this PR.
Any newly added unsafe code is properly documented.
Any API changes are reflected in firecracker/swagger.yaml.
Any user-facing changes are mentioned in CHANGELOG.md.
All added/changed functionality is tested.

daniel5151 · 2020-10-08T19:54:30Z

Hey there, I'm the author of gdbstub, and I couldn't help but notice you're using version 0.3.0 for your implementation. I wanted to let you know that I'm planning on releasing version 0.4.0 this weekend, which comes with a whole slew of API improvements, new features, and bug fixes (including a fix for the incorrect thread id issue you're experiencing). The code should be identical to the current daniel5151/gdbstub:master branch, modulo some documentation improvements.

Oh, and if you're wondering how some random guy stumbled across this PR: I'll occasionally search GitHub to see if anyone's using gdbstub, just to make sure it's working as intended. Sorry for barging in unannounced 😅

P.S: I was a former intern on the crosvm team back at google, so it's pretty neat seeing my code used in it's sister-project 😄

baciumar · 2020-10-09T14:59:05Z

Hey there, I'm the author of gdbstub, and I couldn't help but notice you're using version 0.3.0 for your implementation. I wanted to let you know that I'm planning on releasing version 0.4.0 this weekend, which comes with a whole slew of API improvements, new features, and bug fixes (including a fix for the incorrect thread id issue you're experiencing). The code should be identical to the current daniel5151/gdbstub:master branch, modulo some documentation improvements.

Oh, and if you're wondering how some random guy stumbled across this PR: I'll occasionally search GitHub to see if anyone's using gdbstub, just to make sure it's working as intended. Sorry for barging in unannounced 😅

P.S: I was a former intern on the crosvm team back at google, so it's pretty neat seeing my code used in it's sister-project 😄

Hey, thanks for the heads up; I'll be sure to use the new release. Also, great work on gdbstub 😁

Signed-off-by: Marius-Cristian Baciu <baciumar@amazon.com>

This provides support for breakpoints Signed-off-by: Marius-Cristian Baciu <baciumar@amazon.com>

The KVM_SET_GUEST_DEBUG ioctl enables the handling of a trap signal, generated when execution reaches a breakpoint or single-stepping is enabled. An alternative to this seccomp rule would be requiring the execution of the guest under GDB to be performed with seccomp disabled Signed-off-by: Marius-Cristian Baciu <baciumar@amazon.com>

Improve readability, define proper error types and add support for early-boot virtual address access Signed-off-by: Marius-Cristian Baciu <baciumar@amazon.com>

Cases in which PDPTE's PS flag would be 1 or PDE's PS flag would be 0 were not properly treated. This problem only manifested itself later in the boot process (e.g., register_virtio_device for a stack address) Signed-off-by: Marius-Cristian Baciu <baciumar@amazon.com>

Removed the option of setting breakpoints at linear addresses in early boot. The support for it contained a work-around and not all inconsistencies at the client could have been solved on the server side (e.g.: "info breakpoints" would not display such a breakpoint as "hit" when it was the case). Signed-off-by: Marius-Cristian Baciu <baciumar@amazon.com>

Specifying the --debugger argument will enable GDB guest debugging for the current session; not specifying it leads to normal execution Signed-off-by: Marius-Cristian Baciu <baciumar@amazon.com>

Signed-off-by: Marius-Cristian Baciu <baciumar@amazon.com>

Adding a "debugger_enabled" argument to some public functions impacted some unit and integration tests Signed-off-by: Marius-Cristian Baciu <baciumar@amazon.com>

Change virt_to_phys() function signature so that it only receives the data it actually needs Signed-off-by: Marius-Cristian Baciu <baciumar@amazon.com>

This unittest verifies the virtual-to-physical address translation is properly performed in the early boot process of a kernel Signed-off-by: Marius-Cristian Baciu <baciumar@amazon.com>

gbionescu · 2020-10-23T12:00:06Z

I've started looking over the PR, but I'm occasionally having a hard time understanding some of the magic numbers and why some operations are done.

I suggest going over the PR and trying to either move the magic numbers to constants and adding more comments so that it's easier to follow the logic.

src/gdb_server/src/util.rs

gbionescu · 2020-10-23T11:51:15Z

src/gdb_server/src/util.rs

+impl Debugger {
+    pub fn enable_kvm_debug(vcpu: &VcpuFd, step: bool) -> Result<(), DebuggerError> {
+        let mut control: __u32 = 0;
+        control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP;


Could control be declared as KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP directly instead of 0?

gbionescu · 2020-10-23T12:00:51Z

src/gdb_server/src/util.rs

+            mask = 0x0000ff8000000000u64;
+
+            paddr = context.cr3 & 0x000ffffffffff000u64;
+            movem = 36;


What is 36?

gbionescu · 2020-10-23T12:01:41Z

src/gdb_server/src/util.rs

+    /// obtaining the physical address corresponding to the one received from a GDB client
+    pub fn virt_to_phys(
+        addr: u64,
+        gm: &GuestMemoryMmap,


gm could be named guest_memory so that it's easier to follow. It would also be consistent with the guest_state parameter declared below.

src/gdb_server/src/util.rs

gbionescu · 2020-10-23T12:05:18Z

src/gdb_server/src/util.rs

+        }
+
+        if Debugger::check_entry(table_entry, TABLE_ENTRY_RSVD_BITS[0]).is_err() {
+            return Debugger::fixup_pointer(addr, e_phdrs);


Needs a comment on why we are exiting through here.

src/gdb_server/src/util.rs

Before, a target remote command would start the guest VM and would let it wait at the entry point. This commit offers the user the responsibility of issuing a continue command in the beginning so that the VM starts and executes until reaching the entry point. This is also aligned with the practice used in IDEs of starting the debugging process with a default continue. Signed-off-by: Marius-Cristian Baciu <baciumar@amazon.com>

Add comments, remove magic numbers, etc. Also, add support for setting multiple registers Signed-off-by: Marius-Cristian Baciu <baciumar@amazon.com>

This update allows for connection on any host interface and on a port that is easily accessible for a EC2 host Signed-off-by: Marius-Cristian Baciu <baciumar@amazon.com>

gbionescu · 2020-11-08T18:00:12Z

src/gdb_server/src/lib.rs

+        entry_addr,
+    )?;
+
+    if target.insert_bp(entry_addr.0, false).is_err() {


Please add a comment on why we are setting this breakpoint.

gbionescu · 2020-11-08T18:02:08Z

src/gdb_server/src/lib.rs

+        return Err("GDB server error".into());
+    }
+
+    let connection: Box<dyn Connection<Error = std::io::Error>> = { Box::new(wait_for_tcp(8443)?) };


8443 should be a constant defined at the beginning of the file.

gbionescu · 2020-11-08T18:03:12Z

src/kernel/src/loader/mod.rs

@@ -421,3 +423,30 @@ mod tests {
        assert_eq!(val, b'\0');
    }
 }
+
+#[cfg(any(target_arch = "x86", target_arch = "x86_64"))]
+pub fn extract_phdrs<F>(kernel_image: &mut F) -> Result<Vec<elf::Elf64_Phdr>>


Please add a comment on the purpose of the function.

Can it be unit tested?

gbionescu · 2020-11-08T18:05:44Z

src/vmm/src/builder.rs

@@ -453,7 +472,9 @@ fn load_kernel(
        kernel::loader::load_kernel(guest_memory, &mut kernel_file, arch::get_kernel_start())
            .map_err(StartMicrovmError::KernelLoader)?;

-    Ok(entry_addr)
+    let phdrs = kernel::loader::extract_phdrs(&mut kernel_file).unwrap();


Please add a comment on why we are doing the header extraction.

Signed-off-by: Marius-Cristian Baciu <baciumar@amazon.com>

Add a few code comments and fix some minor code style issues Signed-off-by: Marius-Cristian Baciu <baciumar@amazon.com>

Signed-off-by: Marius-Cristian Baciu <baciumar@amazon.com>

kumargu · 2020-11-29T07:10:28Z

The build here is failing with following errors.

error[E0599]: no method named `set_guest_debug` found for reference `&kvm_ioctls::ioctls::vcpu::VcpuFd` in the current scope
   --> src/gdb_server/src/util.rs:144:34
    |
144 |         if let Err(errno) = vcpu.set_guest_debug(&debug_struct) {
    |                                  ^^^^^^^^^^^^^^^ method not found in `&kvm_ioctls::ioctls::vcpu::VcpuFd`

This is because changes in pull request firecracker-microvm/kvm-ioctls#11 is not yet merged. It would be great if someone can review those changes and then we have a working build.

kumargu · 2020-12-02T11:52:01Z

The build here is failing with following errors.
error[E0599]: no method named `set_guest_debug` found for reference `&kvm_ioctls::ioctls::vcpu::VcpuFd` in the current scope
   --> src/gdb_server/src/util.rs:144:34
    |
144 |         if let Err(errno) = vcpu.set_guest_debug(&debug_struct) {
    |                                  ^^^^^^^^^^^^^^^ method not found in `&kvm_ioctls::ioctls::vcpu::VcpuFd`
This is because changes in pull request firecracker-microvm/kvm-ioctls#11 is not yet merged. It would be great if someone can review those changes and then we have a working build.

The changes are already merged in upstream: rust-vmm/kvm-ioctls@f87ee79, just need a sync from upstream to https://github.com/firecracker-microvm/kvm-ioctls

Mic92 · 2020-12-10T13:12:19Z

Do you think it would be feasible to start the debugger stub on demand via REST api? This would open up some interesting use cases for me.

Mic92 · 2020-12-10T13:23:32Z

src/vmm/src/builder.rs

-    Ok(entry_addr)
+    // The program headers of the kernel image are necessary in the address translation
+    // mechanism in the GDB Server thread
+    let phdrs = kernel::loader::extract_phdrs(&mut kernel_file).unwrap();


To me it seems this information could be also extracted later at runtime, i.e. in the RPC interface no? I am just brainstorming here how to make the debug interface attachable on demand

gbionescu · 2020-12-15T14:26:55Z

@Mic92 the GDB server could be started up at runtime, yes. We will probably add this as an enhancement as we move towards integrating it in Firecracker.

alindima · 2021-05-26T08:34:24Z

closing in favour of: #2333

dianpopa assigned baciumar Oct 8, 2020

acatangiu self-requested a review October 9, 2020 08:55

dianpopa added the Status: Author label Oct 12, 2020

baciumar added 12 commits October 12, 2020 09:25

GDB server: Add gdb server skeleton

4a19575

Signed-off-by: Marius-Cristian Baciu <baciumar@amazon.com>

GDB server: Add PoC for the address translation

62e1705

Signed-off-by: Marius-Cristian Baciu <baciumar@amazon.com>

GDB server: Add initial implementation

0af7df8

This provides support for breakpoints Signed-off-by: Marius-Cristian Baciu <baciumar@amazon.com>

GDB server: Refactor initial implementation

4699a39

Improve readability, define proper error types and add support for early-boot virtual address access Signed-off-by: Marius-Cristian Baciu <baciumar@amazon.com>

GDB server: Add command-line option for GDB

4a969fb

Specifying the --debugger argument will enable GDB guest debugging for the current session; not specifying it leads to normal execution Signed-off-by: Marius-Cristian Baciu <baciumar@amazon.com>

GDB server: Apply cargo fmt

25b5cf3

Signed-off-by: Marius-Cristian Baciu <baciumar@amazon.com>

GDB server: Update tests impacted by changes

02dbe1f

Adding a "debugger_enabled" argument to some public functions impacted some unit and integration tests Signed-off-by: Marius-Cristian Baciu <baciumar@amazon.com>

GDB server: Small update

21407a5

Change virt_to_phys() function signature so that it only receives the data it actually needs Signed-off-by: Marius-Cristian Baciu <baciumar@amazon.com>

GDB server: Add unittest for virt_to_phys()

d0ff2a0

This unittest verifies the virtual-to-physical address translation is properly performed in the early boot process of a kernel Signed-off-by: Marius-Cristian Baciu <baciumar@amazon.com>

baciumar force-pushed the gdb_server branch from 901b947 to d0ff2a0 Compare October 12, 2020 10:15

gbionescu reviewed Oct 23, 2020

View reviewed changes

baciumar added 3 commits November 5, 2020 08:46

Improve code readability

5d6dc1a

Add comments, remove magic numbers, etc. Also, add support for setting multiple registers Signed-off-by: Marius-Cristian Baciu <baciumar@amazon.com>

Update listening address:port

1ba1d31

This update allows for connection on any host interface and on a port that is easily accessible for a EC2 host Signed-off-by: Marius-Cristian Baciu <baciumar@amazon.com>

gbionescu reviewed Nov 8, 2020

View reviewed changes

baciumar added 3 commits November 10, 2020 07:36

Update code according to style tests checks

074f646

Signed-off-by: Marius-Cristian Baciu <baciumar@amazon.com>

Address small review comments and run cargo fmt

654c700

Add a few code comments and fix some minor code style issues Signed-off-by: Marius-Cristian Baciu <baciumar@amazon.com>

Add documentation

022f24c

Signed-off-by: Marius-Cristian Baciu <baciumar@amazon.com>

dianpopa self-requested a review November 24, 2020 09:47

Mic92 reviewed Dec 10, 2020

View reviewed changes

georgepisaltu marked this pull request as draft January 6, 2021 10:00

kumargu mentioned this pull request Feb 17, 2021

Gdb server #2168 #2333

Closed

9 tasks

alindima closed this May 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gdb server #2168

Gdb server #2168

baciumar commented Oct 8, 2020

daniel5151 commented Oct 8, 2020

baciumar commented Oct 9, 2020

gbionescu commented Oct 23, 2020

gbionescu Oct 23, 2020

gbionescu Oct 23, 2020

gbionescu Oct 23, 2020

gbionescu Oct 23, 2020

gbionescu Nov 8, 2020

gbionescu Nov 8, 2020

gbionescu Nov 8, 2020 •

edited

Loading

gbionescu Nov 8, 2020

kumargu commented Nov 29, 2020

kumargu commented Dec 2, 2020

Mic92 commented Dec 10, 2020 •

edited

Loading

Mic92 Dec 10, 2020 •

edited

Loading

gbionescu commented Dec 15, 2020

alindima commented May 26, 2021

Gdb server #2168

Gdb server #2168

Conversation

baciumar commented Oct 8, 2020

Reason for This PR

Description of Changes

License Acceptance

PR Checklist

daniel5151 commented Oct 8, 2020

baciumar commented Oct 9, 2020

gbionescu commented Oct 23, 2020

gbionescu Oct 23, 2020

Choose a reason for hiding this comment

gbionescu Oct 23, 2020

Choose a reason for hiding this comment

gbionescu Oct 23, 2020

Choose a reason for hiding this comment

gbionescu Oct 23, 2020

Choose a reason for hiding this comment

gbionescu Nov 8, 2020

Choose a reason for hiding this comment

gbionescu Nov 8, 2020

Choose a reason for hiding this comment

gbionescu Nov 8, 2020 • edited Loading

Choose a reason for hiding this comment

gbionescu Nov 8, 2020

Choose a reason for hiding this comment

kumargu commented Nov 29, 2020

kumargu commented Dec 2, 2020

Mic92 commented Dec 10, 2020 • edited Loading

Mic92 Dec 10, 2020 • edited Loading

Choose a reason for hiding this comment

gbionescu commented Dec 15, 2020

alindima commented May 26, 2021

gbionescu Nov 8, 2020 •

edited

Loading

Mic92 commented Dec 10, 2020 •

edited

Loading

Mic92 Dec 10, 2020 •

edited

Loading