2016-10-23 21:22:07

by Kevin Cernekee

[permalink] [raw]
Subject: [PATCH] printk: Make %pS print offsets into modules

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]

Signed-off-by: Kevin Cernekee <[email protected]>
---
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).


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;
+ *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


2016-10-24 14:17:03

by Ard Biesheuvel

[permalink] [raw]
Subject: Re: [PATCH] printk: Make %pS print offsets into modules

Hi Kevin,

On 23 October 2016 at 22:21, Kevin Cernekee <[email protected]> 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 <[email protected]>
> ---
> 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
>