From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S941064AbcJXORD (ORCPT <rfc822;w@1wt.eu>);
        Mon, 24 Oct 2016 10:17:03 -0400
Received: from mail-oi0-f46.google.com ([209.85.218.46]:33162 "EHLO
        mail-oi0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S936517AbcJXOQ6 (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Mon, 24 Oct 2016 10:16:58 -0400
MIME-Version: 1.0
In-Reply-To: <1477257713-22206-1-git-send-email-cernekee@chromium.org>
References: <1477257713-22206-1-git-send-email-cernekee@chromium.org>
From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Date: Mon, 24 Oct 2016 15:16:54 +0100
Message-ID: <CAKv+Gu99k6-NH+W25P9D8T07oEwJj9MHZHyT+DOsw5Fazrpxjg@mail.gmail.com>
Subject: Re: [PATCH] printk: Make %pS print offsets into modules
To: Kevin Cernekee <cernekee@chromium.org>
Cc: Rusty Russell <rusty@rustcorp.com.au>,
        Andrew Morton <akpm@linux-foundation.org>,
        Kees Cook <keescook@chromium.org>,
        Brian Norris <computersforpeace@gmail.com>,
        Doug Anderson <dianders@chromium.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset=UTF-8
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Kevin,

On 23 October 2016 at 22:21, Kevin Cernekee <cernekee@chromium.org> wrote:
> If kallsyms cannot find a symbol for an address, entries like this will
> appear in backtraces:
>
>     Call trace:
>     [<ffffffbffc1ecd7c>] 0xffffffbffc1ecd7c
>     [<ffffffbffc1ef7f0>] 0xffffffbffc1ef7f0
>     [<ffffffbffc1f0094>] 0xffffffbffc1f0094
>
> This isn't particularly useful for debugging because modules are not
> loaded at fixed addresses.  Instead, print the offset from the module's
> base, so that the offending location can be easily located in a
> disassembly of the .ko file:
>
>     Call trace:
>     [<ffffffbffc1d57f4>] [mwifiex_pcie+0x37f4/0x9000]
>     [<ffffffbffc1d60ac>] [mwifiex_pcie+0x40ac/0x9000]
>     [<ffffffbffc188fe0>] mwifiex_main_process+0xdc/0x6fc [mwifiex]
>

This looks like a useful feature, although this example is a bit
misleading, since it suggests that only some symbols are unresolveable
in some cases, whereas the primary use case for this facility imo is a
kernel with no kallsyms to begin with.

> Signed-off-by: Kevin Cernekee <cernekee@chromium.org>
> ---
>  include/linux/module.h | 16 ++++++++++++++++
>  kernel/kallsyms.c      | 19 ++++++++++++++++++-
>  kernel/module.c        | 23 +++++++++++++++++++++++
>  3 files changed, 57 insertions(+), 1 deletion(-)
>
>
> Tested on 4.4 only (so the core_layout / init_layout stuff is untested).
>

Please don't send untested stuff. v4.4 is ancient history for many of
us, so this needs to be tested and working against mainline or -next

> diff --git a/include/linux/module.h b/include/linux/module.h
> index 0c3207d26ac0..611d1e71b7c8 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -616,6 +616,14 @@ const char *module_address_lookup(unsigned long addr,
>                             unsigned long *offset,
>                             char **modname,
>                             char *namebuf);
> +
> +/* For kallsyms to ask which module, if any, contains addr.  On success,
> + * returns true and populates module_size, module_offset, and modname. */
> +bool module_base_lookup(unsigned long addr,
> +                       unsigned long *module_size,
> +                       unsigned long *module_offset,
> +                       char **modname);
> +
>  int lookup_module_symbol_name(unsigned long addr, char *symname);
>  int lookup_module_symbol_attrs(unsigned long addr, unsigned long *size, unsigned long *offset, char *modname, char *name);
>
> @@ -698,6 +706,14 @@ static inline const char *module_address_lookup(unsigned long addr,
>         return NULL;
>  }
>
> +static inline bool module_base_lookup(unsigned long addr,
> +                                     unsigned long *module_size,
> +                                     unsigned long *module_offset,
> +                                     char **modname)
> +{
> +       return false;
> +}
> +
>  static inline int lookup_module_symbol_name(unsigned long addr, char *symname)
>  {
>         return -ERANGE;
> diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> index fafd1a3ef0da..43b114e6861f 100644
> --- a/kernel/kallsyms.c
> +++ b/kernel/kallsyms.c
> @@ -387,8 +387,25 @@ static int __sprint_symbol(char *buffer, unsigned long address,
>
>         address += symbol_offset;
>         name = kallsyms_lookup(address, &size, &offset, &modname, buffer);
> -       if (!name)
> +       if (!name) {
> +               /*
> +                * Fall back to [module_base+offset/size] if the actual
> +                * symbol name is unavailable.
> +                */
> +               if (module_base_lookup(address, &size, &offset, &modname)) {
> +                       if (add_offset) {
> +                               return snprintf(buffer, KSYM_SYMBOL_LEN,
> +                                               "[%s+%#lx/%#lx]", modname,
> +                                               address - offset -
> +                                                       symbol_offset,
> +                                               size);
> +                       } else {
> +                               return snprintf(buffer, KSYM_SYMBOL_LEN,
> +                                               "[%s]", modname);
> +                       }
> +               }
>                 return sprintf(buffer, "0x%lx", address - symbol_offset);
> +       }
>
>         if (name != buffer)
>                 strcpy(buffer, name);
> diff --git a/kernel/module.c b/kernel/module.c
> index f57dd63186e6..5e09f568c601 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3899,6 +3899,29 @@ const char *module_address_lookup(unsigned long addr,
>         return ret;
>  }
>
> +bool module_base_lookup(unsigned long addr,
> +                       unsigned long *module_size,
> +                       unsigned long *module_offset,
> +                       char **modname)
> +{
> +       struct module *mod;
> +
> +       preempt_disable();
> +
> +       mod = __module_address(addr);
> +       if (!mod) {
> +               preempt_enable();
> +               return false;
> +       }
> +
> +       *modname = mod->name;

I'm aware that this pattern may exist elsewhere, but I don't see how
it is safe to dereference modname after reenabling preemption. If mod
gets freed inadvertently, so will mod->name.


> +       *module_offset = (unsigned long)mod->core_layout.base;
> +       *module_size = mod->init_layout.size + mod->core_layout.size;
> +
> +       preempt_enable();
> +       return true;
> +}
> +
>  int lookup_module_symbol_name(unsigned long addr, char *symname)
>  {
>         struct module *mod;
> --
> 2.8.0.rc3.226.g39d4020
>