From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id DFB3CC433DB for ; Tue, 16 Mar 2021 13:40:20 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id A679B6506A for ; Tue, 16 Mar 2021 13:40:20 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235345AbhCPNjx (ORCPT ); Tue, 16 Mar 2021 09:39:53 -0400 Received: from gecko.sbs.de ([194.138.37.40]:49317 "EHLO gecko.sbs.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230373AbhCPNjZ (ORCPT ); Tue, 16 Mar 2021 09:39:25 -0400 Received: from mail2.sbs.de (mail2.sbs.de [192.129.41.66]) by gecko.sbs.de (8.15.2/8.15.2) with ESMTPS id 12GDctAr018700 (version=TLSv1.2 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Tue, 16 Mar 2021 14:38:55 +0100 Received: from [167.87.27.98] ([167.87.27.98]) by mail2.sbs.de (8.15.2/8.15.2) with ESMTP id 12GDcrkA017486; Tue, 16 Mar 2021 14:38:54 +0100 Subject: Re: [PATCH 1/3] scripts/gdb: rework lx-symbols gdb script To: Maxim Levitsky , kvm@vger.kernel.org Cc: Vitaly Kuznetsov , linux-kernel@vger.kernel.org, Thomas Gleixner , Wanpeng Li , Kieran Bingham , Jessica Yu , Andrew Morton , "maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)" , Joerg Roedel , Sean Christopherson , Jim Mattson , Borislav Petkov , Stefano Garzarella , "H. Peter Anvin" , Paolo Bonzini , Ingo Molnar References: <20210315221020.661693-1-mlevitsk@redhat.com> <20210315221020.661693-2-mlevitsk@redhat.com> From: Jan Kiszka Message-ID: <9dcacf3e-bba5-a062-610e-dcf1b0a261a3@siemens.com> Date: Tue, 16 Mar 2021 14:38:53 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.0 MIME-Version: 1.0 In-Reply-To: <20210315221020.661693-2-mlevitsk@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 15.03.21 23:10, Maxim Levitsky wrote: > Fix several issues that are present in lx-symbols script: > > * Track module unloads by placing another software breakpoint at 'free_module' > (force uninline this symbol just in case), and use remove-symbol-file > gdb command to unload the symobls of the module that is unloading. > > That gives the gdb a chance to mark all software breakpoints from > this module as pending again. > Also remove the module from the 'known' module list once it is unloaded. > > * Since we now track module unload, we don't need to reload all > symbols anymore when 'known' module loaded again (that can't happen anymore). > This allows reloading a module in the debugged kernel to finish much faster, > while lx-symbols tracks module loads and unloads. > > * Disable/enable all gdb breakpoints on both module load and unload breakpoint > hits, and not only in 'load_all_symbols' as was done before. > (load_all_symbols is no longer called on breakpoint hit) > That allows gdb to avoid getting confused about the state of the (now two) > internal breakpoints we place. > > Otherwise it will leave them in the kernel code segment, when continuing > which triggers a guest kernel panic as soon as it skips over the 'int3' > instruction and executes the garbage tail of the optcode on which > the breakpoint was placed. > > Signed-off-by: Maxim Levitsky > --- > kernel/module.c | 8 ++- > scripts/gdb/linux/symbols.py | 106 +++++++++++++++++++++++++---------- > 2 files changed, 83 insertions(+), 31 deletions(-) > > diff --git a/kernel/module.c b/kernel/module.c > index 30479355ab850..ea81fc06ea1f5 100644 > --- a/kernel/module.c > +++ b/kernel/module.c > @@ -901,8 +901,12 @@ int module_refcount(struct module *mod) > } > EXPORT_SYMBOL(module_refcount); > > -/* This exists whether we can unload or not */ > -static void free_module(struct module *mod); > +/* This exists whether we can unload or not > + * Keep it uninlined to provide a reliable breakpoint target, > + * e.g. for the gdb helper command 'lx-symbols'. > + */ > + > +static noinline void free_module(struct module *mod); > > SYSCALL_DEFINE2(delete_module, const char __user *, name_user, > unsigned int, flags) > diff --git a/scripts/gdb/linux/symbols.py b/scripts/gdb/linux/symbols.py > index 1be9763cf8bb2..4ce879548a1ae 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(): Naming is a bit too generic. And it's not only saving the state, it's also disabling things. > + 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): Maybe rather something with "restore", to make naming balanced. Or is there a use case where "state" is not coming from the function above? > + 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 Possibly at all, now that we track unloading? Can our state tracking become out-of-sync? > + > # 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 > + > + def stop(self): > + module = gdb.parse_and_eval("mod") > + module_name = module['name'].string() > + cmd = self.gdb_command > > - # restore pagination state > - gdb.execute("set pagination %s" % ("on" if pagination else "off")) > + if not module_name in cmd.loaded_modules: > + return False > Same question as above. For robustness, checking is not bad. But maybe it's worth reporting as well. > + 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 = [] > - breakpoint = None > + loaded_modules = {} > + module_load_breakpoint = None > + module_unload_breakpoint = 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()] > @@ -174,11 +215,18 @@ lx-symbols command.""" > self.load_all_symbols() > > if hasattr(gdb, 'Breakpoint'): > - if self.breakpoint is not None: > - self.breakpoint.delete() > - self.breakpoint = None > - self.breakpoint = LoadModuleBreakpoint( > - "kernel/module.c:do_init_module", self) > + if self.module_load_breakpoint is not None: > + self.module_load_breakpoint.delete() > + self.module_load_breakpoint = None > + self.module_load_breakpoint = \ > + LoadModuleBreakpoint("kernel/module.c:do_init_module", self) > + > + if self.module_unload_breakpoint is not None: > + self.module_unload_breakpoint.delete() > + self.module_unload_breakpoint = None > + self.module_unload_breakpoint = \ > + UnLoadModuleBreakpoint("kernel/module.c:free_module", self) > + > else: > gdb.write("Note: symbol update on module loading not supported " > "with this gdb version\n") > Good improvement! Jan -- Siemens AG, T RDA IOT Corporate Competence Center Embedded Linux