Re: [PATCH] KVM: x86: do not fail if software breakpoint has already been removed

[Maxim Levitsky <mlevitsk@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2071 bytes --]

On Mon, 2021-03-01 at 12:17 +0100, Paolo Bonzini wrote:
> If kvm_arch_remove_sw_breakpoint finds that a software breakpoint does not
> have an INT3 instruction, it fails.  This can happen if one sets a
> software breakpoint in a kernel module and then reloads it.  gdb then
> thinks the breakpoint cannot be deleted and there is no way to add it
> back.
> 
> Suggested-by: Maxim Levitsky <mlevitsk@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  target/i386/kvm/kvm.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index 0b5755e42b..c8d61daf68 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -4352,8 +4352,13 @@ int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
>  {
>      uint8_t int3;
>  
> -    if (cpu_memory_rw_debug(cs, bp->pc, &int3, 1, 0) || int3 != 0xcc ||
> -        cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 1, 1)) {
> +    if (cpu_memory_rw_debug(cs, bp->pc, &int3, 1, 0)) {
> +        return -EINVAL;
> +    }
> +    if (int3 != 0xcc) {
> +        return 0;
> +    }
> +    if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 1, 1)) {
>          return -EINVAL;
>      }
>      return 0;

There still remains a philosopical question if kvm_arch_remove_sw_breakpoint
should always return 0, since for the usual case of kernel debugging where
a breakpoint is in unloaded module, the above will probably still fail
as paging for this module is removed as well by the kernel.
It is still better though so:

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>

Note that I managed to make lx-symbols to work in a very stable way
with attached WIP patch I hacked on this Sunday.
I will send a cleaned up version of it to upstream when I have time.

Since I make gdb unload the symbols, it works even without this patch. 

Added Stefano Garzarella to CC as I know that he tried to make this work as well.
https://lkml.org/lkml/2020/10/5/514


Best regards,
	Maxim Levitsky


[-- Attachment #2: fix-lx-symbols.diff --]
[-- Type: text/x-patch, Size: 6732 bytes --]

commit 86f0d5a08a40528350589ed54126f06d4ac293a8
Author: Maxim Levitsky <mlevitsk@redhat.com>
Date:   Sun Feb 28 23:52:08 2021 +0200

    gdb: rework gdb debug script
    
    Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>

diff --git a/scripts/gdb/linux/symbols.py b/scripts/gdb/linux/symbols.py
index 1be9763cf8bb..13f21afc2059 100644
--- a/scripts/gdb/linux/symbols.py
+++ b/scripts/gdb/linux/symbols.py
@@ -17,6 +17,24 @@ import re
 
 from linux import modules, utils
 
+def save_state():
+        breakpoints = []
+        if hasattr(gdb, 'breakpoints') and not gdb.breakpoints() is None:
+            for bp in gdb.breakpoints():
+                breakpoints.append({'breakpoint': bp, 'enabled': bp.enabled})
+                bp.enabled = False
+
+        show_pagination = gdb.execute("show pagination", to_string=True)
+        pagination = show_pagination.endswith("on.\n")
+        gdb.execute("set pagination off")
+
+        return {"breakpoints":breakpoints, "show_pagination": show_pagination}
+
+def load_state(state):
+    for breakpoint in state["breakpoints"]:
+        breakpoint['breakpoint'].enabled = breakpoint['enabled']
+    gdb.execute("set pagination %s" % ("on" if state["show_pagination"] else "off"))
+
 
 if hasattr(gdb, 'Breakpoint'):
     class LoadModuleBreakpoint(gdb.Breakpoint):
@@ -30,26 +48,38 @@ if hasattr(gdb, 'Breakpoint'):
             module_name = module['name'].string()
             cmd = self.gdb_command
 
+            # module already loaded, false alarm
+            if module_name in cmd.loaded_modules:
+                return False
+
             # enforce update if object file is not found
             cmd.module_files_updated = False
 
             # Disable pagination while reporting symbol (re-)loading.
             # The console input is blocked in this context so that we would
             # get stuck waiting for the user to acknowledge paged output.
-            show_pagination = gdb.execute("show pagination", to_string=True)
-            pagination = show_pagination.endswith("on.\n")
-            gdb.execute("set pagination off")
+            state = save_state()
+            cmd.load_module_symbols(module)
+            load_state(state)
+            return False
 
-            if module_name in cmd.loaded_modules:
-                gdb.write("refreshing all symbols to reload module "
-                          "'{0}'\n".format(module_name))
-                cmd.load_all_symbols()
-            else:
-                cmd.load_module_symbols(module)
+    class UnLoadModuleBreakpoint(gdb.Breakpoint):
+        def __init__(self, spec, gdb_command):
+            super(UnLoadModuleBreakpoint, self).__init__(spec, internal=True)
+            self.silent = True
+            self.gdb_command = gdb_command
 
-            # restore pagination state
-            gdb.execute("set pagination %s" % ("on" if pagination else "off"))
+        def stop(self):
+            module = gdb.parse_and_eval("mod")
+            module_name = module['name'].string()
+            cmd = self.gdb_command
 
+            if not module_name in cmd.loaded_modules:
+                return False
+
+            state = save_state()
+            cmd.unload_module_symbols(module)
+            load_state(state)
             return False
 
 
@@ -64,8 +94,9 @@ lx-symbols command."""
     module_paths = []
     module_files = []
     module_files_updated = False
-    loaded_modules = []
+    loaded_modules = {}
     breakpoint = None
+    ubreakpoint = None
 
     def __init__(self):
         super(LxSymbols, self).__init__("lx-symbols", gdb.COMMAND_FILES,
@@ -129,21 +160,32 @@ lx-symbols command."""
                 filename=module_file,
                 addr=module_addr,
                 sections=self._section_arguments(module))
+
             gdb.execute(cmdline, to_string=True)
-            if module_name not in self.loaded_modules:
-                self.loaded_modules.append(module_name)
+            self.loaded_modules[module_name] = {"module_file": module_file,
+                                                "module_addr": module_addr}
         else:
             gdb.write("no module object found for '{0}'\n".format(module_name))
 
+    def unload_module_symbols(self, module):
+        module_name = module['name'].string()
+
+        module_file = self.loaded_modules[module_name]["module_file"]
+        module_addr = self.loaded_modules[module_name]["module_addr"]
+
+        gdb.write("unloading @{addr}: {filename}\n".format(
+            addr=module_addr, filename=module_file))
+        cmdline = "remove-symbol-file {filename}".format(
+            filename=module_file)
+
+        gdb.execute(cmdline, to_string=True)
+        del self.loaded_modules[module_name]
+
+
     def load_all_symbols(self):
         gdb.write("loading vmlinux\n")
 
-        # Dropping symbols will disable all breakpoints. So save their states
-        # and restore them afterward.
-        saved_states = []
-        if hasattr(gdb, 'breakpoints') and not gdb.breakpoints() is None:
-            for bp in gdb.breakpoints():
-                saved_states.append({'breakpoint': bp, 'enabled': bp.enabled})
+        state = save_state()
 
         # drop all current symbols and reload vmlinux
         orig_vmlinux = 'vmlinux'
@@ -153,15 +195,14 @@ lx-symbols command."""
         gdb.execute("symbol-file", to_string=True)
         gdb.execute("symbol-file {0}".format(orig_vmlinux))
 
-        self.loaded_modules = []
+        self.loaded_modules = {}
         module_list = modules.module_list()
         if not module_list:
             gdb.write("no modules found\n")
         else:
             [self.load_module_symbols(module) for module in module_list]
 
-        for saved_state in saved_states:
-            saved_state['breakpoint'].enabled = saved_state['enabled']
+        load_state(state)
 
     def invoke(self, arg, from_tty):
         self.module_paths = [os.path.expanduser(p) for p in arg.split()]
@@ -177,8 +218,13 @@ lx-symbols command."""
             if self.breakpoint is not None:
                 self.breakpoint.delete()
                 self.breakpoint = None
-            self.breakpoint = LoadModuleBreakpoint(
-                "kernel/module.c:do_init_module", self)
+            self.breakpoint = LoadModuleBreakpoint("kernel/module.c:do_init_module", self)
+
+            if self.ubreakpoint is not None:
+                self.ubreakpoint.delete()
+                self.ubreakpoint = None
+            self.ubreakpoint = UnLoadModuleBreakpoint("kernel/module.c:free_module", self)
+
         else:
             gdb.write("Note: symbol update on module loading not supported "
                       "with this gdb version\n")