Re: [RFC v3 4/8] Add kernel module support for linux-kernel target

[Philipp Rudo <prudo at linux dot vnet dot ibm dot com>]

Hi,

On Tue, 02 May 2017 14:15:43 +0100
Yao Qi <qiyaoltc@gmail.com> wrote:

> Philipp Rudo <prudo@linux.vnet.ibm.com> writes:
> 
> > +/* Translate a kernel virtual address ADDR to a physical address.  */
> > +
> > +CORE_ADDR
> > +lk_kvtop (CORE_ADDR addr)  
> 
> How about lk_kernel_vir_to_phy_addr?

I prefer kvtop.  It's much shorter and (for my taste) is just as readable.  But
I don't insist on keeping the name.  Are there other opinions?

> 
> > +{
> > +  CORE_ADDR pgd = lk_read_addr (LK_ADDR (init_mm)
> > +				+ LK_OFFSET (mm_struct, pgd));
> > +  return LK_HOOK->vtop (pgd, addr);
> > +}
> > +
> > +/* Restore current_target to TARGET.  */
> > +static void
> > +restore_current_target (void *target)
> > +{
> > +  current_target.beneath = (struct target_ops *) target;
> > +}
> > +
> > +/* Function for targets to_xfer_partial hook.  */
> > +
> > +enum target_xfer_status
> > +lk_xfer_partial (struct target_ops *ops, enum target_object object,
> > +		 const char *annex, gdb_byte *readbuf,
> > +		 const gdb_byte *writebuf, ULONGEST offset, ULONGEST len,
> > +		 ULONGEST *xfered_len)
> > +{
> > +  enum target_xfer_status ret_val;
> > +  struct cleanup *old_chain = make_cleanup (restore_current_target,
> > ops);  
> 
> Use make_scoped_restore instead of make_cleanup?

Using a scoped_restore probably makes sense.  Although I don't see the
advantage over old style cleanups other than having marginally shorter code ...

> > +
> > +  current_target.beneath = ops->beneath;
> > +  
> 
> Any reasons you switch current_target.beneath temporarily?

Yes.  lk_kvtop (at least for s390) reads memory if the address is not
physical.  Thus reading a virtual address calls xfer_partial twice.  Once for
the actual address and a second time for the data lk_kvtop needs.  This can
lead to an endless recursion if there is a bug or memory corruption.  Switching
to the target beneath prevents this.

> > +  if (LK_HOOK->is_kvaddr (offset))
> > +    offset = lk_kvtop (offset);
> > +
> > +  ret_val =  ops->beneath->to_xfer_partial (ops->beneath, object, annex,
> > +					    readbuf, writebuf, offset, len,
> > +					    xfered_len);  
> 
> Two spaces after "=".

Fixed, thanks.

> > diff --git a/gdb/lk-modules.c b/gdb/lk-modules.c
> > new file mode 100644
> > index 0000000..f3c559d
> > --- /dev/null
> > +++ b/gdb/lk-modules.c
> > @@ -0,0 +1,412 @@
> > +/* Handle Linux kernel modules as shared libraries.
> > +
> > +   Copyright (C) 2016 Free Software Foundation, Inc.
> > +
> > +   This file is part of GDB.
> > +
> > +   This program is free software; you can redistribute it and/or modify
> > +   it under the terms of the GNU General Public License as published by
> > +   the Free Software Foundation; either version 3 of the License, or
> > +   (at your option) any later version.
> > +
> > +   This program is distributed in the hope that it will be useful,
> > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +   GNU General Public License for more details.
> > +
> > +   You should have received a copy of the GNU General Public License
> > +   along with this program.  If not, see <http://www.gnu.org/licenses/>.
> > */ +
> > +#include "defs.h"
> > +
> > +#include "common/filestuff.h"
> > +#include "filenames.h"
> > +#include "gdbcmd.h"
> > +#include "gdbcore.h"
> > +#include "gdb_regex.h"
> > +#include "lk-lists.h"
> > +#include "lk-low.h"
> > +#include "lk-modules.h"
> > +#include "objfiles.h"
> > +#include "observer.h"
> > +#include "readline/readline.h"
> > +#include "solib.h"
> > +#include "solist.h"
> > +#include "utils.h"
> > +
> > +#include <unordered_map>
> > +#include <string>
> > +
> > +struct target_so_ops *lk_modules_so_ops = NULL;
> > +
> > +/* Info for single section type.  */
> > +
> > +struct lm_info_sec
> > +{
> > +  CORE_ADDR start;
> > +  CORE_ADDR offset;
> > +  unsigned int size;
> > +};
> > +
> > +/* Link map info to include in an allocated so_list entry.  */
> > +
> > +struct lm_info
> > +{
> > +  CORE_ADDR base;
> > +  unsigned int size;
> > +
> > +  struct lm_info_sec text;
> > +  struct lm_info_sec init_text;
> > +  struct lm_info_sec ro_data;
> > +  struct lm_info_sec rw_data;
> > +  struct lm_info_sec percpu;
> > +};  
> 
> Comments to these fields are needed.

Yes, you are right. I will fix it when I adjust to the classification of
lm_info.
 
> > +
> > +/* Build map between module name and path to binary file by reading file
> > +   modules.order.  Returns unordered_map with module name as key and its
> > +   path as value.  */
> > +
> > +std::unordered_map<std::string, std::string>
> > +lk_modules_build_path_map ()
> > +{
> > +  std::unordered_map<std::string, std::string> umap;
> > +  FILE *mod_order;
> > +  struct cleanup *old_chain;
> > +  char line[SO_NAME_MAX_PATH_SIZE + 1];
> > +
> > +  mod_order = lk_modules_open_mod_order ();
> > +  old_chain = make_cleanup_fclose (mod_order);
> > +
> > +  line[SO_NAME_MAX_PATH_SIZE] = '\0';
> > +  std::string search_path = lk_modules_expand_search_path ();
> > +  while (fgets (line, SO_NAME_MAX_PATH_SIZE, mod_order))
> > +    {
> > +      /* Remove trailing newline.  */
> > +      line[strlen (line) - 1] = '\0';
> > +
> > +      std::string name = lbasename (line);
> > +
> > +      /* 3 = strlen (".ko").  */
> > +      if (!endswith (name.c_str (), ".ko")
> > +	  || name.length () >= LK_MODULE_NAME_LEN + 3)
> > +	continue;
> > +
> > +      name = name.substr (0, name.length () - 3);
> > +
> > +      /* Kernel modules are named after the files they are stored in with
> > +	 all minus '-' replaced by underscore '_'.  Do the same to enable
> > +	 mapping.  */
> > +      for (size_t p = name.find('-'); p != std::string::npos;
> > +	   p = name.find ('-', p + 1))
> > +	name[p] = '_';
> > +
> > +      umap[name] = concat_path(search_path, line);
> > +    }
> > +
> > +  do_cleanups (old_chain);
> > +  return umap;
> > +}
> > +
> > +/* Allocate and fill a copy of struct lm_info for module at address MOD.
> > */ +
> > +struct lm_info *
> > +lk_modules_read_lm_info (CORE_ADDR mod)
> > +{
> > +  struct lm_info *lmi = XNEW (struct lm_info);
> > +  struct cleanup *old_chain = make_cleanup (xfree, lmi);
> > +  
> 
> use std::unique_ptr to avoid cleanup.
> 
> sdt::unique_ptr<lm_info> lmi (new lm_info ());

Yes, a unique_ptr is better.  There are probably many cleanups I can get rid
off with the latest C++-yfication.

The only problem I currently have is that the C++-yfication rate is too high I
can barely follow and add new features to have a reliable kernel debugger.
Thanks for the hint anyway.

> > +  if (LK_FIELD (module, module_core)) /* linux -4.4 */
> > +    {
> > +      lmi->base = lk_read_addr (mod + LK_OFFSET (module, module_core));
> > +      lmi->size = lk_read_addr (mod + LK_OFFSET (module, core_size));
> > +
> > +      lmi->text.start = lmi->base;
> > +      lmi->text.offset = LK_HOOK->get_module_text_offset (mod);
> > +      lmi->text.size = lk_read_uint (mod + LK_OFFSET (module,
> > core_text_size)); +
> > +      lmi->ro_data.start = lmi->base + lmi->text.size;
> > +      lmi->ro_data.offset = 0;
> > +      lmi->ro_data.size =  lk_read_uint (mod + LK_OFFSET (module,
> > +							  core_ro_size));
> > +    }
> > +  else /* linux 4.5+ */
> > +    {
> > +      CORE_ADDR mod_core = mod + LK_OFFSET (module, core_layout);
> > +
> > +      lmi->base = lk_read_addr (mod_core
> > +				+ LK_OFFSET (module_layout, base));
> > +      lmi->size = lk_read_uint (mod_core
> > +				+ LK_OFFSET (module_layout, size));
> > +
> > +      lmi->text.start = lmi->base;
> > +      lmi->text.offset = LK_HOOK->get_module_text_offset (mod);
> > +      lmi->text.size = lk_read_uint (mod_core
> > +				     + LK_OFFSET (module_layout,
> > text_size)); +
> > +      lmi->ro_data.start = lmi->base + lmi->text.size;
> > +      lmi->ro_data.offset = 0;
> > +      lmi->ro_data.size =  lk_read_uint (mod_core
> > +					 + LK_OFFSET (module_layout,
> > ro_size));
> > +    }
> > +
> > +  lmi->rw_data.start = lmi->base + lmi->ro_data.size;
> > +  lmi->rw_data.offset = 0;
> > +  lmi->rw_data.size = lmi->size - lmi->ro_data.size;
> > +
> > +  lmi->init_text.start = lk_read_addr (mod + LK_OFFSET (module, init));
> > +  lmi->init_text.offset = 0;
> > +
> > +  lmi->percpu.start = lk_read_addr (mod + LK_OFFSET (module, percpu));
> > +  lmi->percpu.size = lk_read_uint (mod + LK_OFFSET (module, percpu_size));
> > +  lmi->percpu.offset = 0;
> > +
> > +  discard_cleanups (old_chain);
> > +  return lmi;
> > +}
> > +  
>