[PATCHv4 0/6] printk/ia64/ppc64/parisc64: let's deprecate %pF/%pf printk specifiers

[PATCHv4 0/6] printk/ia64/ppc64/parisc64: let's deprecate %pF/%pf printk specifiers

[Sergey Senozhatsky]

	Hello,

	A reworked version. There is a new dereference_symbol_descriptor()
function now, where "the magic happens", so I don't touch kallsyms_lookup()
and module_address_lookup() anymore.

	All Ack-s/Tested-by-s were dropped, since the patch set has been
reworked. I'm kindly asking arch-s maintainers and developers to test it
once again. Sorry for any inconveniences and thanks for your help in
advance.

===========================================================================

	On some arches C function pointers are indirect and point to
a function descriptor, which contains the actual pointer to the code.
This mostly doesn't matter, except for cases when people want to print
out function pointers in symbolic format, because the usual '%pS/%ps'
does not work on those arches as expected. That's the reason why we
have '%pF/%pf', but since it's here because of a subtle ABI detail
specific to some arches (ppc64/ia64/parisc64) it's easy to misuse
'%pF/%pf' and '%pS/%ps' (see [1], for example).

	This patch set attempts to move ia64/ppc64/parisc64 C function
pointer ABI details out of printk() to arch code. Function dereference
code now checks if a pointer belongs to a .opd ELF section and dereferences
that pointer only if it does. The kernel and modules have their own .opd
sections that's why I use two different ARCH functions: for kernel and
for module pointer dereference.

	I planned to remove dereference_function_descriptor() entirely,
but then I discovered a bunch other uses cases (kgdbts, init/main.c,
extable, etc.), so I decided to keep dereference_function_descriptor()
around because the main point of this patch set is to deprecate %pF/%pf.
But at the same time, I think I can go further and handle both kernel
and module descriptor dereference in dereference_function_descriptor().
We need a module pointer for module .opd check, so that will come at an
extra cost of module lookup (may be there will some other issues along
the way, haven't checked it).

Right now we've got:

- dereference_function_descriptor(addr)
        a generic (old) function. it simply attempts to dereference
        whatever pointer we give it.

- dereference_kernel_function_descriptor(addr)
        dereferences a kernel pointer if it's within the kernel's .opd
        section.

- dereference_module_function_descriptor(module, addr)
        dereference a module pointer if it's within the module's .opd
        section.

v4:
-- don't switch to ulong in derefence functions, keep using void pointer
-- introduce dereference_symbol_descriptor() function
-- avoid any dereference in kallsyms lookup/module address lookup
-- improved documentation
-- since this is a new version, I dropped all the the Ack-s and Tested-by-s

v3:
-- picked up ACKs and Tested-by
-- tweaked checkpatch warning (Joe)
-- updated Documentation

v2:
-- convert dereference_function_descriptor() to unsigned long
-- fix kernel descriptor range checks (Helge)
-- fix parisc module descriptor range check (Helge)
-- fix ppc64 module range check
-- add checkpatch patch

Sergey Senozhatsky (6):
  sections: split dereference_function_descriptor()
  ia64: Add .opd based function descriptor dereference
  powerpc64: Add .opd based function descriptor dereference
  parisc64: Add .opd based function descriptor dereference
  symbol lookup: introduce dereference_symbol_descriptor()
  checkpatch: add pF/pf deprecation warning

 Documentation/printk-formats.txt          | 49 ++++++++++++-------------------
 arch/ia64/include/asm/sections.h          | 10 ++++++-
 arch/ia64/kernel/module.c                 | 12 ++++++++
 arch/ia64/kernel/vmlinux.lds.S            |  2 ++
 arch/parisc/boot/compressed/vmlinux.lds.S |  2 ++
 arch/parisc/include/asm/sections.h        |  6 ++++
 arch/parisc/kernel/module.c               | 16 ++++++++++
 arch/parisc/kernel/process.c              |  9 ++++++
 arch/parisc/kernel/vmlinux.lds.S          |  2 ++
 arch/powerpc/include/asm/module.h         |  3 ++
 arch/powerpc/include/asm/sections.h       | 12 ++++++++
 arch/powerpc/kernel/module_64.c           | 14 +++++++++
 arch/powerpc/kernel/vmlinux.lds.S         |  2 ++
 include/asm-generic/sections.h            |  8 +++--
 include/linux/kallsyms.h                  |  2 ++
 include/linux/module.h                    |  3 ++
 kernel/kallsyms.c                         | 19 ++++++++++++
 kernel/module.c                           |  6 ++++
 lib/vsprintf.c                            |  5 ++--
 scripts/checkpatch.pl                     | 11 +++++--
 20 files changed, 155 insertions(+), 38 deletions(-)

-- 
2.15.0

[PATCHv4 1/6] sections: split dereference_function_descriptor()

[Sergey Senozhatsky]

There are two format specifiers to print out a pointer in symbolic
format: '%pS/%ps' and '%pF/%pf'. On most architectures, the two
mean exactly the same thing, but some architectures (ia64, ppc64,
parisc64) use an indirect pointer for C function pointers, where
the function pointer points to a function descriptor (which in
turn contains the actual pointer to the code). The '%pF/%pf, when
used appropriately, automatically does the appropriate function
descriptor dereference on such architectures.

The "when used appropriately" part is tricky. Basically this is
a subtle ABI detail, specific to some platforms, that made it to
the API level and people can be unaware of it and miss the whole
"we need to dereference the function" business out. [1] proves
that point (note that it fixes only '%pF' and '%pS', there might
be '%pf' and '%ps' cases as well).

It appears that we can handle everything within the affected
arches and make '%pS/%ps' smart enough to retire '%pF/%pf'.
Function descriptors live in .opd elf section and all affected
arches (ia64, ppc64, parisc64) handle it properly for kernel
and modules. So we, technically, can decide if the dereference
is needed by simply looking at the pointer: if it belongs to
.opd section then we need to dereference it.

The kernel and modules have their own .opd sections, obviously,
that's why we need to split dereference_function_descriptor()
and use separate kernel and module dereference arch callbacks.

This patch does the first step, it
a) adds dereference_kernel_function_descriptor() function.
b) adds a weak alias to dereference_module_function_descriptor()
   function.

So, for the time being, we will have:
1) dereference_function_descriptor()
   A generic function, that simply dereferences the pointer. There is
   bunch of places that call it: kgdbts, init/main.c, extable, etc.

2) dereference_kernel_function_descriptor()
   A function to call on kernel symbols that does kernel .opd section
   address range test.

3) dereference_module_function_descriptor()
   A function to call on modules' symbols that does modules' .opd
   section address range test.

[1] https://marc.info/?l=linux-kernel&m=150472969730573

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
 include/asm-generic/sections.h | 8 ++++++--
 include/linux/module.h         | 3 +++
 kernel/module.c                | 6 ++++++
 3 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
index 03cc5f9bba71..849cd8eb5ca0 100644
--- a/include/asm-generic/sections.h
+++ b/include/asm-generic/sections.h
@@ -30,6 +30,7 @@
  *	__ctors_start, __ctors_end
  *	__irqentry_text_start, __irqentry_text_end
  *	__softirqentry_text_start, __softirqentry_text_end
+ *	__start_opd, __end_opd
  */
 extern char _text[], _stext[], _etext[];
 extern char _data[], _sdata[], _edata[];
@@ -49,12 +50,15 @@ extern char __start_once[], __end_once[];
 /* Start and end of .ctors section - used for constructor calls. */
 extern char __ctors_start[], __ctors_end[];
 
+/* Start and end of .opd section - used for function descriptors. */
+extern char __start_opd[], __end_opd[];
+
 extern __visible const void __nosave_begin, __nosave_end;
 
-/* function descriptor handling (if any).  Override
- * in asm/sections.h */
+/* Function descriptor handling (if any).  Override in asm/sections.h */
 #ifndef dereference_function_descriptor
 #define dereference_function_descriptor(p) (p)
+#define dereference_kernel_function_descriptor(p) (p)
 #endif
 
 /* random extra sections (if any).  Override
diff --git a/include/linux/module.h b/include/linux/module.h
index c69b49abe877..9dac6973b001 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -606,6 +606,9 @@ int ref_module(struct module *a, struct module *b);
 	__mod ? __mod->name : "kernel";		\
 })
 
+/* Dereference module function descriptor */
+void *dereference_module_function_descriptor(struct module *mod, void *ptr);
+
 /* For kallsyms to ask for address resolution.  namebuf should be at
  * least KSYM_NAME_LEN long: a pointer to namebuf is returned if
  * found, otherwise NULL. */
diff --git a/kernel/module.c b/kernel/module.c
index ab2978e4239c..1d6e996caa13 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3938,6 +3938,12 @@ static const char *get_ksymbol(struct module *mod,
 	return symname(kallsyms, best);
 }
 
+void * __weak dereference_module_function_descriptor(struct module *mod,
+						     void *ptr)
+{
+	return ptr;
+}
+
 /* For kallsyms to ask for address resolution.  NULL means not found.  Careful
  * not to lock to avoid deadlock on oopses, simply disable preemption. */
 const char *module_address_lookup(unsigned long addr,
-- 
2.15.0

[PATCHv4 2/6] ia64: Add .opd based function descriptor dereference

[Sergey Senozhatsky]

We are moving towards separate kernel and module function descriptor
dereference callbacks. This patch enables it for IA64.

For pointers that belong to the kernel
-  Added __start_opd and __end_opd pointers, to track the kernel
   .opd section address range;

-  Added dereference_kernel_function_descriptor(). Now we
   will dereference only function pointers that are within
   [__start_opd, __end_opd);

For pointers that belong to a module
-  Added dereference_module_function_descriptor() to handle module
   function descriptor dereference. Now we will dereference only
   pointers that are within [module->opd.start, module->opd.end).

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
 arch/ia64/include/asm/sections.h | 10 +++++++++-
 arch/ia64/kernel/module.c        | 12 ++++++++++++
 arch/ia64/kernel/vmlinux.lds.S   |  2 ++
 3 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/arch/ia64/include/asm/sections.h b/arch/ia64/include/asm/sections.h
index f3481408594e..cea15f2dd38d 100644
--- a/arch/ia64/include/asm/sections.h
+++ b/arch/ia64/include/asm/sections.h
@@ -27,6 +27,8 @@ extern char __start_gate_brl_fsys_bubble_down_patchlist[], __end_gate_brl_fsys_b
 extern char __start_unwind[], __end_unwind[];
 extern char __start_ivt_text[], __end_ivt_text[];
 
+#define HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR 1
+
 #undef dereference_function_descriptor
 static inline void *dereference_function_descriptor(void *ptr)
 {
@@ -38,6 +40,12 @@ static inline void *dereference_function_descriptor(void *ptr)
 	return ptr;
 }
 
+#undef dereference_kernel_function_descriptor
+static inline void *dereference_kernel_function_descriptor(void *ptr)
+{
+	if (ptr < (void *)__start_opd || ptr >= (void *)__end_opd)
+		return ptr;
+	return dereference_function_descriptor(ptr);
+}
 
 #endif /* _ASM_IA64_SECTIONS_H */
-
diff --git a/arch/ia64/kernel/module.c b/arch/ia64/kernel/module.c
index 853b5611a894..326448f9df16 100644
--- a/arch/ia64/kernel/module.c
+++ b/arch/ia64/kernel/module.c
@@ -36,6 +36,7 @@
 
 #include <asm/patch.h>
 #include <asm/unaligned.h>
+#include <asm/sections.h>
 
 #define ARCH_MODULE_DEBUG 0
 
@@ -918,3 +919,14 @@ module_arch_cleanup (struct module *mod)
 	if (mod->arch.core_unw_table)
 		unw_remove_unwind_table(mod->arch.core_unw_table);
 }
+
+void *dereference_module_function_descriptor(struct module *mod, void *ptr)
+{
+	Elf64_Shdr *opd = mod->arch.opd;
+
+	if (ptr < (void *)opd->sh_addr ||
+			ptr >= (void *)(opd->sh_addr + opd->sh_size))
+		return ptr;
+
+	return dereference_function_descriptor(ptr);
+}
diff --git a/arch/ia64/kernel/vmlinux.lds.S b/arch/ia64/kernel/vmlinux.lds.S
index 58db59da0bd8..31e688981b4b 100644
--- a/arch/ia64/kernel/vmlinux.lds.S
+++ b/arch/ia64/kernel/vmlinux.lds.S
@@ -108,7 +108,9 @@ SECTIONS {
 	RODATA
 
 	.opd : AT(ADDR(.opd) - LOAD_OFFSET) {
+		__start_opd = .;
 		*(.opd)
+		__end_opd = .;
 	}
 
 	/*
-- 
2.15.0

[PATCHv4 3/6] powerpc64: Add .opd based function descriptor dereference

[Sergey Senozhatsky]

We are moving towards separate kernel and module function descriptor
dereference callbacks. This patch enables it for powerpc64.

For pointers that belong to the kernel
-  Added __start_opd and __end_opd pointers, to track the kernel
   .opd section address range;

-  Added dereference_kernel_function_descriptor(). Now we
   will dereference only function pointers that are within
   [__start_opd, __end_opd);

For pointers that belong to a module
-  Added dereference_module_function_descriptor() to handle module
   function descriptor dereference. Now we will dereference only
   pointers that are within [module->opd.start, module->opd.end).

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
 arch/powerpc/include/asm/module.h   |  3 +++
 arch/powerpc/include/asm/sections.h | 12 ++++++++++++
 arch/powerpc/kernel/module_64.c     | 14 ++++++++++++++
 arch/powerpc/kernel/vmlinux.lds.S   |  2 ++
 4 files changed, 31 insertions(+)

diff --git a/arch/powerpc/include/asm/module.h b/arch/powerpc/include/asm/module.h
index 6c0132c7212f..7e28442827f1 100644
--- a/arch/powerpc/include/asm/module.h
+++ b/arch/powerpc/include/asm/module.h
@@ -45,6 +45,9 @@ struct mod_arch_specific {
 	unsigned long tramp;
 #endif
 
+	/* For module function descriptor dereference */
+	unsigned long start_opd;
+	unsigned long end_opd;
 #else /* powerpc64 */
 	/* Indices of PLT sections within module. */
 	unsigned int core_plt_section;
diff --git a/arch/powerpc/include/asm/sections.h b/arch/powerpc/include/asm/sections.h
index 82bec63bbd4f..e335a8f846af 100644
--- a/arch/powerpc/include/asm/sections.h
+++ b/arch/powerpc/include/asm/sections.h
@@ -66,6 +66,9 @@ static inline int overlaps_kvm_tmp(unsigned long start, unsigned long end)
 }
 
 #ifdef PPC64_ELF_ABI_v1
+
+#define HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR 1
+
 #undef dereference_function_descriptor
 static inline void *dereference_function_descriptor(void *ptr)
 {
@@ -76,6 +79,15 @@ static inline void *dereference_function_descriptor(void *ptr)
 		ptr = p;
 	return ptr;
 }
+
+#undef dereference_kernel_function_descriptor
+static inline void *dereference_kernel_function_descriptor(void *ptr)
+{
+	if (ptr < (void *)__start_opd || ptr >= (void *)__end_opd)
+		return ptr;
+
+	return dereference_function_descriptor(ptr);
+}
 #endif /* PPC64_ELF_ABI_v1 */
 
 #endif
diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index 759104b99f9f..218971ac7e04 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c
@@ -93,6 +93,15 @@ static unsigned int local_entry_offset(const Elf64_Sym *sym)
 {
 	return 0;
 }
+
+void *dereference_module_function_descriptor(struct module *mod, void *ptr)
+{
+	if (ptr < (void *)mod->arch.start_opd ||
+			ptr >= (void *)mod->arch.end_opd)
+		return ptr;
+
+	return dereference_function_descriptor(ptr);
+}
 #endif
 
 #define STUB_MAGIC 0x73747562 /* stub */
@@ -344,6 +353,11 @@ int module_frob_arch_sections(Elf64_Ehdr *hdr,
 		else if (strcmp(secstrings+sechdrs[i].sh_name,"__versions")==0)
 			dedotify_versions((void *)hdr + sechdrs[i].sh_offset,
 					  sechdrs[i].sh_size);
+		else if (!strcmp(secstrings + sechdrs[i].sh_name, ".opd")) {
+			me->arch.start_opd = sechdrs[i].sh_addr;
+			me->arch.end_opd = sechdrs[i].sh_addr +
+					   sechdrs[i].sh_size;
+		}
 
 		/* We don't handle .init for the moment: rename to _init */
 		while ((p = strstr(secstrings + sechdrs[i].sh_name, ".init")))
diff --git a/arch/powerpc/kernel/vmlinux.lds.S b/arch/powerpc/kernel/vmlinux.lds.S
index 0494e1566ee2..5dac5ab22fa2 100644
--- a/arch/powerpc/kernel/vmlinux.lds.S
+++ b/arch/powerpc/kernel/vmlinux.lds.S
@@ -278,7 +278,9 @@ SECTIONS
 	}
 
 	.opd : AT(ADDR(.opd) - LOAD_OFFSET) {
+		__start_opd = .;
 		*(.opd)
+		__end_opd = .;
 	}
 
 	. = ALIGN(256);
-- 
2.15.0

[PATCHv4 4/6] parisc64: Add .opd based function descriptor dereference

[Sergey Senozhatsky]

We are moving towards separate kernel and module function descriptor
dereference callbacks. This patch enables it for parisc64.

For pointers that belong to the kernel
-  Added __start_opd and __end_opd pointers, to track the kernel
   .opd section address range;

-  Added dereference_kernel_function_descriptor(). Now we
   will dereference only function pointers that are within
   [__start_opd, __end_opd);

For pointers that belong to a module
-  Added dereference_module_function_descriptor() to handle module
   function descriptor dereference. Now we will dereference only
   pointers that are within [module->opd.start, module->opd.end).

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Signed-off-by: Helge Deller <deller@gmx.de>
---
 arch/parisc/boot/compressed/vmlinux.lds.S |  2 ++
 arch/parisc/include/asm/sections.h        |  6 ++++++
 arch/parisc/kernel/module.c               | 16 ++++++++++++++++
 arch/parisc/kernel/process.c              |  9 +++++++++
 arch/parisc/kernel/vmlinux.lds.S          |  2 ++
 5 files changed, 35 insertions(+)

diff --git a/arch/parisc/boot/compressed/vmlinux.lds.S b/arch/parisc/boot/compressed/vmlinux.lds.S
index a4ce3314e78e..4ebd4e65524c 100644
--- a/arch/parisc/boot/compressed/vmlinux.lds.S
+++ b/arch/parisc/boot/compressed/vmlinux.lds.S
@@ -29,7 +29,9 @@ SECTIONS
 	. = ALIGN(16);
 	/* Linkage tables */
 	.opd : {
+		__start_opd = .;
 		*(.opd)
+		__end_opd = .;
 	} PROVIDE (__gp = .);
 	.plt : {
 		*(.plt)
diff --git a/arch/parisc/include/asm/sections.h b/arch/parisc/include/asm/sections.h
index accdf40aa5b7..5a40b51df80c 100644
--- a/arch/parisc/include/asm/sections.h
+++ b/arch/parisc/include/asm/sections.h
@@ -6,8 +6,14 @@
 #include <asm-generic/sections.h>
 
 #ifdef CONFIG_64BIT
+
+#define HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR 1
+
 #undef dereference_function_descriptor
 void *dereference_function_descriptor(void *);
+
+#undef dereference_kernel_function_descriptor
+void *dereference_kernel_function_descriptor(void *);
 #endif
 
 #endif
diff --git a/arch/parisc/kernel/module.c b/arch/parisc/kernel/module.c
index f1a76935a314..b5b3cb00f1fb 100644
--- a/arch/parisc/kernel/module.c
+++ b/arch/parisc/kernel/module.c
@@ -66,6 +66,7 @@
 
 #include <asm/pgtable.h>
 #include <asm/unwind.h>
+#include <asm/sections.h>
 
 #if 0
 #define DEBUGP printk
@@ -954,3 +955,18 @@ void module_arch_cleanup(struct module *mod)
 {
 	deregister_unwind_table(mod);
 }
+
+#ifdef CONFIG_64BIT
+void *dereference_module_function_descriptor(struct module *mod, void *ptr)
+{
+	unsigned long start_opd = (Elf64_Addr)mod->core_layout.base +
+				   mod->arch.fdesc_offset;
+	unsigned long end_opd = start_opd +
+				mod->arch.fdesc_count * sizeof(Elf64_Fdesc);
+
+	if (ptr < (void *)start_opd || ptr >= (void *)end_opd)
+		return ptr;
+
+	return dereference_function_descriptor(ptr);
+}
+#endif
diff --git a/arch/parisc/kernel/process.c b/arch/parisc/kernel/process.c
index 30f92391a93e..6c4585103a91 100644
--- a/arch/parisc/kernel/process.c
+++ b/arch/parisc/kernel/process.c
@@ -276,6 +276,15 @@ void *dereference_function_descriptor(void *ptr)
 		ptr = p;
 	return ptr;
 }
+
+void *dereference_kernel_function_descriptor(void *ptr)
+{
+	if (ptr < (void *)__start_opd ||
+			ptr >= (void *)__end_opd)
+		return ptr;
+
+	return dereference_function_descriptor(ptr);
+}
 #endif
 
 static inline unsigned long brk_rnd(void)
diff --git a/arch/parisc/kernel/vmlinux.lds.S b/arch/parisc/kernel/vmlinux.lds.S
index 159a2ec0b4e0..da2e31190efa 100644
--- a/arch/parisc/kernel/vmlinux.lds.S
+++ b/arch/parisc/kernel/vmlinux.lds.S
@@ -100,7 +100,9 @@ SECTIONS
 	. = ALIGN(16);
 	/* Linkage tables */
 	.opd : {
+		__start_opd = .;
 		*(.opd)
+		__end_opd = .;
 	} PROVIDE (__gp = .);
 	.plt : {
 		*(.plt)
-- 
2.15.0

[PATCHv4 5/6] symbol lookup: introduce dereference_symbol_descriptor()

[Sergey Senozhatsky]

dereference_symbol_descriptor() invokes appropriate ARCH specific
function descriptor dereference callbacks:
- dereference_kernel_function_descriptor() if the pointer is a
  kernel symbol;

- dereference_module_function_descriptor() if the pointer is a
  module symbol.

This is the last step needed to make '%pS/%ps' smart enough to
handle function descriptor dereference on affected ARCHs and
to retire '%pF/%pf'.

To refresh it:
  Some architectures (ia64, ppc64, parisc64) use an indirect pointer
  for C function pointers - the function pointer points to a function
  descriptor and we need to dereference it to get the actual function
  pointer.

  Function descriptors live in .opd elf section and all affected
  ARCHs (ia64, ppc64, parisc64) handle it properly for kernel and
  modules. So we, technically, can decide if the dereference is
  needed by simply looking at the pointer: if it belongs to .opd
  section then we need to dereference it.

  The kernel and modules have their own .opd sections, obviously,
  that's why we need to split dereference_function_descriptor()
  and use separate kernel and module dereference arch callbacks.

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
 Documentation/printk-formats.txt | 49 ++++++++++++++++------------------------
 include/linux/kallsyms.h         |  2 ++
 kernel/kallsyms.c                | 19 ++++++++++++++++
 lib/vsprintf.c                   |  5 ++--
 4 files changed, 42 insertions(+), 33 deletions(-)

diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
index 361789df51ec..2f17e684b72e 100644
--- a/Documentation/printk-formats.txt
+++ b/Documentation/printk-formats.txt
@@ -50,42 +50,31 @@ Symbols/Function Pointers
 
 ::
 
-	%pF	versatile_init+0x0/0x110
-	%pf	versatile_init
-	%pS	versatile_init+0x0/0x110
-	%pSR	versatile_init+0x9/0x110
-		(with __builtin_extract_return_addr() translation)
-	%ps	versatile_init
-	%pB	prev_fn_of_versatile_init+0x88/0x88
-
-The ``F`` and ``f`` specifiers are for printing function pointers,
-for example, f->func, &gettimeofday. They have the same result as
-``S`` and ``s`` specifiers. But they do an extra conversion on
-ia64, ppc64 and parisc64 architectures where the function pointers
-are actually function descriptors.
-
-The ``S`` and ``s`` specifiers can be used for printing symbols
-from direct addresses, for example, __builtin_return_address(0),
-(void *)regs->ip. They result in the symbol name with (``S``) or
-without (``s``) offsets. If KALLSYMS are disabled then the symbol
-address is printed instead.
+        %pS     versatile_init+0x0/0x110
+        %ps     versatile_init
+        %pF     versatile_init+0x0/0x110
+        %pf     versatile_init
+        %pSR    versatile_init+0x9/0x110
+                (with __builtin_extract_return_addr() translation)
+        %pB     prev_fn_of_versatile_init+0x88/0x88
+
+The ``S`` and ``s`` specifiers are used for printing a pointer in symbolic
+format. They result in the symbol name with (``S``) or without (``s``)
+offsets. If KALLSYMS are disabled then the symbol address is printed instead.
+
+Note, that the ``F`` and ``f`` specifiers are identical to ``S`` (``s``)
+and thus deprecated. We have ``F`` and ``f`` because on ia64, ppc64 and
+parisc64 function pointers are indirect and, in fact, are function
+descriptors, which require additional dereferencing before we can lookup
+the symbol. As of now, ``S`` and ``s`` perform dereferencing on those
+platforms (when needed), so ``F`` and ``f`` exist for compatibility
+reasons only.
 
 The ``B`` specifier results in the symbol name with offsets and should be
 used when printing stack backtraces. The specifier takes into
 consideration the effect of compiler optimisations which may occur
 when tail-call``s are used and marked with the noreturn GCC attribute.
 
-Examples::
-
-	printk("Going to call: %pF\n", gettimeofday);
-	printk("Going to call: %pF\n", p->func);
-	printk("%s: called from %pS\n", __func__, (void *)_RET_IP_);
-	printk("%s: called from %pS\n", __func__,
-				(void *)__builtin_return_address(0));
-	printk("Faulted at %pS\n", (void *)regs->ip);
-	printk(" %s%pB\n", (reliable ? "" : "? "), (void *)*stack);
-
-
 Kernel Pointers
 ===============
 
diff --git a/include/linux/kallsyms.h b/include/linux/kallsyms.h
index 11dd93e42580..73f7e874c3e1 100644
--- a/include/linux/kallsyms.h
+++ b/include/linux/kallsyms.h
@@ -16,6 +16,8 @@
 
 struct module;
 
+void *dereference_symbol_descriptor(void *ptr);
+
 #ifdef CONFIG_KALLSYMS
 /* Lookup the address for a symbol. Returns 0 if not found. */
 unsigned long kallsyms_lookup_name(const char *name);
diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index 1966fe1c2b57..18b3dffc4b84 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c
@@ -411,6 +411,25 @@ static int __sprint_symbol(char *buffer, unsigned long address,
 	return len;
 }
 
+void *dereference_symbol_descriptor(void *ptr)
+{
+#ifdef HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR
+	struct module *mod;
+
+	ptr = dereference_kernel_function_descriptor(ptr);
+	if (is_ksym_addr((unsigned long)ptr))
+		return ptr;
+
+	preempt_disable();
+	mod = __module_address((unsigned long)ptr);
+	preempt_enable();
+
+	if (mod)
+		ptr = dereference_module_function_descriptor(mod, ptr);
+#endif
+	return ptr;
+}
+
 /**
  * sprint_symbol - Look up a kernel symbol and return it in a text buffer
  * @buffer: buffer to be stored
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 1746bae94d41..16e2eefb0f79 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -40,7 +40,6 @@
 #include "../mm/internal.h"	/* For the trace_print_flags arrays */
 
 #include <asm/page.h>		/* for PAGE_SIZE */
-#include <asm/sections.h>	/* for dereference_function_descriptor() */
 #include <asm/byteorder.h>	/* cpu_to_le16 */
 
 #include <linux/string_helpers.h>
@@ -1723,10 +1722,10 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 	switch (*fmt) {
 	case 'F':
 	case 'f':
-		ptr = dereference_function_descriptor(ptr);
-		/* Fallthrough */
 	case 'S':
 	case 's':
+		ptr = dereference_symbol_descriptor(ptr);
+		/* Fallthrough */
 	case 'B':
 		return symbol_string(buf, end, ptr, spec, fmt);
 	case 'R':
-- 
2.15.0

[PATCHv4 6/6] checkpatch: add pF/pf deprecation warning

[Sergey Senozhatsky]

We deprecated '%pF/%pf' printk specifiers, since '%pS/%ps' is now smart
enough to handle function pointer dereference on platforms where such
dereference is required.

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Signed-off-by: Joe Perches <joe@perches.com>
Cc: Andy Whitcroft <apw@canonical.com>
Reviewed-by: Petr Mladek <pmladek@suse.com>
---
 scripts/checkpatch.pl | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 3453df9f90ab..d081a2b7166e 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5752,18 +5752,25 @@ sub process {
 		        for (my $count = $linenr; $count <= $lc; $count++) {
 				my $fmt = get_quoted_string($lines[$count - 1], raw_line($count, 0));
 				$fmt =~ s/%%//g;
-				if ($fmt =~ /(\%[\*\d\.]*p(?![\WFfSsBKRraEhMmIiUDdgVCbGNO]).)/) {
+				if ($fmt =~ /(\%[\*\d\.]*p(?![\WSsBKRraEhMmIiUDdgVCbGNO]).)/) {
 					$bad_extension = $1;
 					last;
 				}
 			}
 			if ($bad_extension ne "") {
 				my $stat_real = raw_line($linenr, 0);
+				my $ext_type = "Invalid";
+				my $use = "";
 				for (my $count = $linenr + 1; $count <= $lc; $count++) {
 					$stat_real = $stat_real . "\n" . raw_line($count, 0);
 				}
+				if ($bad_extension =~ /p[Ff]/) {
+					$ext_type = "Deprecated";
+					$use = " - use %pS instead";
+					$use =~ s/pS/ps/ if ($bad_extension =~ /pf/);
+				}
 				WARN("VSPRINTF_POINTER_EXTENSION",
-				     "Invalid vsprintf pointer extension '$bad_extension'\n" . "$here\n$stat_real\n");
+				     "$ext_type vsprintf pointer extension '$bad_extension'$use\n" . "$here\n$stat_real\n");
 			}
 		}
 
-- 
2.15.0

Re: [PATCHv4 5/6] symbol lookup: introduce dereference_symbol_descriptor()

[Luck, Tony]

On Fri, Nov 10, 2017 at 08:48:29AM +0900, Sergey Senozhatsky wrote:
> -Examples::
> -
> -	printk("Going to call: %pF\n", gettimeofday);
> -	printk("Going to call: %pF\n", p->func);
> -	printk("%s: called from %pS\n", __func__, (void *)_RET_IP_);
> -	printk("%s: called from %pS\n", __func__,
> -				(void *)__builtin_return_address(0));
> -	printk("Faulted at %pS\n", (void *)regs->ip);
> -	printk(" %s%pB\n", (reliable ? "" : "? "), (void *)*stack);

Did you mean to delete the Examples completely?  Wouldn't it
be better to just update (s/%pF/%pS/g)?

-Tony

Re: [PATCHv4 0/6] printk/ia64/ppc64/parisc64: let's deprecate %pF/%pf printk specifiers

[Luck, Tony]

On Fri, Nov 10, 2017 at 08:48:24AM +0900, Sergey Senozhatsky wrote:
> 	All Ack-s/Tested-by-s were dropped, since the patch set has been
> reworked. I'm kindly asking arch-s maintainers and developers to test it
> once again. Sorry for any inconveniences and thanks for your help in
> advance.

You can add back the:

Tested-by: Tony Luck <tony.luck@intel.com> #ia64

Apart from my comment about dropping the Examples from the
Documentation the series looks OK to me.

-Tony

Re: [PATCHv4 0/6] printk/ia64/ppc64/parisc64: let's deprecate %pF/%pf printk specifiers

[Sergey Senozhatsky]

On (11/10/17 10:11), Luck, Tony wrote:
> On Fri, Nov 10, 2017 at 08:48:24AM +0900, Sergey Senozhatsky wrote:
> > 	All Ack-s/Tested-by-s were dropped, since the patch set has been
> > reworked. I'm kindly asking arch-s maintainers and developers to test it
> > once again. Sorry for any inconveniences and thanks for your help in
> > advance.
> 
> You can add back the:
> 
> Tested-by: Tony Luck <tony.luck@intel.com> #ia64

Thanks a ton, Tony!

	-ss

Re: [PATCHv4 5/6] symbol lookup: introduce dereference_symbol_descriptor()

[Sergey Senozhatsky]

On (11/10/17 10:09), Luck, Tony wrote:
> On Fri, Nov 10, 2017 at 08:48:29AM +0900, Sergey Senozhatsky wrote:
> > -Examples::
> > -
> > -	printk("Going to call: %pF\n", gettimeofday);
> > -	printk("Going to call: %pF\n", p->func);
> > -	printk("%s: called from %pS\n", __func__, (void *)_RET_IP_);
> > -	printk("%s: called from %pS\n", __func__,
> > -				(void *)__builtin_return_address(0));
> > -	printk("Faulted at %pS\n", (void *)regs->ip);
> > -	printk(" %s%pB\n", (reliable ? "" : "? "), (void *)*stack);
> 
> Did you mean to delete the Examples completely?  Wouldn't it
> be better to just update (s/%pF/%pS/g)?

good question. yes, I think I did it deliberately :) we still
kinda have some sort of "examples", right at the beginning of
section "Symbols/Function Pointers"


>  Symbols/Function Pointers
>  =========================
>
>  ::
>
>         %pS     versatile_init+0x0/0x110
>          %ps     versatile_init
>          %pF     versatile_init+0x0/0x110
>          %pf     versatile_init
>          %pSR    versatile_init+0x9/0x110
>                 (with __builtin_extract_return_addr() translation)
>          %pB     prev_fn_of_versatile_init+0x88/0x88
>
>  The ``S`` and ``s`` specifiers are used for printing a pointer in symbolic
>  format. They result in the symbol name with (``S``) or without (``s``)
>  offsets. If KALLSYMS are disabled then the symbol address is printed instead.
>
>  Note, that the ``F`` and ``f`` specifiers are identical to ``S`` (``s``)
>  and thus deprecated. We have ``F`` and ``f`` because on ia64, ppc64 and
>  parisc64 function pointers are indirect and, in fact, are function
>  descriptors, which require additional dereferencing before we can lookup
>  the symbol. As of now, ``S`` and ``s`` perform dereferencing on those
>  platforms (when needed), so ``F`` and ``f`` exist for compatibility
>  reasons only.
>
>  The ``B`` specifier results in the symbol name with offsets and should be
>  used when printing stack backtraces. The specifier takes into
>  consideration the effect of compiler optimisations which may occur
>  when tail-call``s are used and marked with the noreturn GCC attribute.

I can return Examples back. don't really have a strong opinion
on this. let me know.

	-ss

Re: [PATCHv4 3/6] powerpc64: Add .opd based function descriptor dereference

[Santosh Sivaraj]

* Sergey Senozhatsky <sergey.senozhatsky@gmail.com> wrote (on 2017-11-10 08:48:27 +0900):

> We are moving towards separate kernel and module function descriptor
> dereference callbacks. This patch enables it for powerpc64.
> 
> For pointers that belong to the kernel
> -  Added __start_opd and __end_opd pointers, to track the kernel
>    .opd section address range;
> 
> -  Added dereference_kernel_function_descriptor(). Now we
>    will dereference only function pointers that are within
>    [__start_opd, __end_opd);
> 
> For pointers that belong to a module
> -  Added dereference_module_function_descriptor() to handle module
>    function descriptor dereference. Now we will dereference only
>    pointers that are within [module->opd.start, module->opd.end).
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> ---
>  arch/powerpc/include/asm/module.h   |  3 +++
>  arch/powerpc/include/asm/sections.h | 12 ++++++++++++
>  arch/powerpc/kernel/module_64.c     | 14 ++++++++++++++
>  arch/powerpc/kernel/vmlinux.lds.S   |  2 ++
>  4 files changed, 31 insertions(+)
>

Looks good on powerpc. If you wish:

Tested-by: Santosh Sivaraj <santosh@fossix.org> # for powerpc

Thanks,
Santosh

> diff --git a/arch/powerpc/include/asm/module.h b/arch/powerpc/include/asm/module.h
> index 6c0132c7212f..7e28442827f1 100644
> --- a/arch/powerpc/include/asm/module.h
> +++ b/arch/powerpc/include/asm/module.h
> @@ -45,6 +45,9 @@ struct mod_arch_specific {
>  	unsigned long tramp;
>  #endif
>  
> +	/* For module function descriptor dereference */
> +	unsigned long start_opd;
> +	unsigned long end_opd;
>  #else /* powerpc64 */
>  	/* Indices of PLT sections within module. */
>  	unsigned int core_plt_section;
> diff --git a/arch/powerpc/include/asm/sections.h b/arch/powerpc/include/asm/sections.h
> index 82bec63bbd4f..e335a8f846af 100644
> --- a/arch/powerpc/include/asm/sections.h
> +++ b/arch/powerpc/include/asm/sections.h
> @@ -66,6 +66,9 @@ static inline int overlaps_kvm_tmp(unsigned long start, unsigned long end)
>  }
>  
>  #ifdef PPC64_ELF_ABI_v1
> +
> +#define HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR 1
> +
>  #undef dereference_function_descriptor
>  static inline void *dereference_function_descriptor(void *ptr)
>  {
> @@ -76,6 +79,15 @@ static inline void *dereference_function_descriptor(void *ptr)
>  		ptr = p;
>  	return ptr;
>  }
> +
> +#undef dereference_kernel_function_descriptor
> +static inline void *dereference_kernel_function_descriptor(void *ptr)
> +{
> +	if (ptr < (void *)__start_opd || ptr >= (void *)__end_opd)
> +		return ptr;
> +
> +	return dereference_function_descriptor(ptr);
> +}
>  #endif /* PPC64_ELF_ABI_v1 */
>  
>  #endif
> diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
> index 759104b99f9f..218971ac7e04 100644
> --- a/arch/powerpc/kernel/module_64.c
> +++ b/arch/powerpc/kernel/module_64.c
> @@ -93,6 +93,15 @@ static unsigned int local_entry_offset(const Elf64_Sym *sym)
>  {
>  	return 0;
>  }
> +
> +void *dereference_module_function_descriptor(struct module *mod, void *ptr)
> +{
> +	if (ptr < (void *)mod->arch.start_opd ||
> +			ptr >= (void *)mod->arch.end_opd)
> +		return ptr;
> +
> +	return dereference_function_descriptor(ptr);
> +}
>  #endif
>  
>  #define STUB_MAGIC 0x73747562 /* stub */
> @@ -344,6 +353,11 @@ int module_frob_arch_sections(Elf64_Ehdr *hdr,
>  		else if (strcmp(secstrings+sechdrs[i].sh_name,"__versions")==0)
>  			dedotify_versions((void *)hdr + sechdrs[i].sh_offset,
>  					  sechdrs[i].sh_size);
> +		else if (!strcmp(secstrings + sechdrs[i].sh_name, ".opd")) {
> +			me->arch.start_opd = sechdrs[i].sh_addr;
> +			me->arch.end_opd = sechdrs[i].sh_addr +
> +					   sechdrs[i].sh_size;
> +		}
>  
>  		/* We don't handle .init for the moment: rename to _init */
>  		while ((p = strstr(secstrings + sechdrs[i].sh_name, ".init")))
> diff --git a/arch/powerpc/kernel/vmlinux.lds.S b/arch/powerpc/kernel/vmlinux.lds.S
> index 0494e1566ee2..5dac5ab22fa2 100644
> --- a/arch/powerpc/kernel/vmlinux.lds.S
> +++ b/arch/powerpc/kernel/vmlinux.lds.S
> @@ -278,7 +278,9 @@ SECTIONS
>  	}
>  
>  	.opd : AT(ADDR(.opd) - LOAD_OFFSET) {
> +		__start_opd = .;
>  		*(.opd)
> +		__end_opd = .;
>  	}
>  
>  	. = ALIGN(256);

-- 

Re: [PATCHv4 3/6] powerpc64: Add .opd based function descriptor dereference

[Sergey Senozhatsky]

On (11/13/17 12:41), Santosh Sivaraj wrote:
> * Sergey Senozhatsky <sergey.senozhatsky@gmail.com> wrote (on 2017-11-10 08:48:27 +0900):
> 
> > We are moving towards separate kernel and module function descriptor
> > dereference callbacks. This patch enables it for powerpc64.
> > 
> > For pointers that belong to the kernel
> > -  Added __start_opd and __end_opd pointers, to track the kernel
> >    .opd section address range;
> > 
> > -  Added dereference_kernel_function_descriptor(). Now we
> >    will dereference only function pointers that are within
> >    [__start_opd, __end_opd);
> > 
> > For pointers that belong to a module
> > -  Added dereference_module_function_descriptor() to handle module
> >    function descriptor dereference. Now we will dereference only
> >    pointers that are within [module->opd.start, module->opd.end).
> > 
> > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> > ---
> >  arch/powerpc/include/asm/module.h   |  3 +++
> >  arch/powerpc/include/asm/sections.h | 12 ++++++++++++
> >  arch/powerpc/kernel/module_64.c     | 14 ++++++++++++++
> >  arch/powerpc/kernel/vmlinux.lds.S   |  2 ++
> >  4 files changed, 31 insertions(+)
> >
> 
> Looks good on powerpc. If you wish:
> 
> Tested-by: Santosh Sivaraj <santosh@fossix.org> # for powerpc

thanks!

	-ss

Re: [PATCHv4 0/6] printk/ia64/ppc64/parisc64: let's deprecate %pF/%pf printk specifiers

[Helge Deller]

On 10.11.2017 00:48, Sergey Senozhatsky wrote:
> 	All Ack-s/Tested-by-s were dropped, since the patch set has been
> reworked. I'm kindly asking arch-s maintainers and developers to test it
> once again. Sorry for any inconveniences and thanks for your help in
> advance.

I tested it successfully on parisc64.
You can add back the
Tested-by: Helge Deller <deller@gmx.de> #parisc64

Thanks!
Helge

Re: [PATCHv4 0/6] printk/ia64/ppc64/parisc64: let's deprecate %pF/%pf printk specifiers

[Sergey Senozhatsky]

On (11/13/17 18:17), Helge Deller wrote:
> On 10.11.2017 00:48, Sergey Senozhatsky wrote:
> > 	All Ack-s/Tested-by-s were dropped, since the patch set has been
> > reworked. I'm kindly asking arch-s maintainers and developers to test it
> > once again. Sorry for any inconveniences and thanks for your help in
> > advance.
> 
> I tested it successfully on parisc64.
> You can add back the
> Tested-by: Helge Deller <deller@gmx.de> #parisc64

thanks!

	-ss

Re: [PATCHv4 5/6] symbol lookup: introduce dereference_symbol_descriptor()

[Petr Mladek]

On Sat 2017-11-11 13:49:32, Sergey Senozhatsky wrote:
> On (11/10/17 10:09), Luck, Tony wrote:
> > On Fri, Nov 10, 2017 at 08:48:29AM +0900, Sergey Senozhatsky wrote:
> > > -Examples::
> > > -
> > > -	printk("Going to call: %pF\n", gettimeofday);
> > > -	printk("Going to call: %pF\n", p->func);
> > > -	printk("%s: called from %pS\n", __func__, (void *)_RET_IP_);
> > > -	printk("%s: called from %pS\n", __func__,
> > > -				(void *)__builtin_return_address(0));
> > > -	printk("Faulted at %pS\n", (void *)regs->ip);
> > > -	printk(" %s%pB\n", (reliable ? "" : "? "), (void *)*stack);
> > 
> > Did you mean to delete the Examples completely?  Wouldn't it
> > be better to just update (s/%pF/%pS/g)?
> 
> good question. yes, I think I did it deliberately :) we still
> kinda have some sort of "examples", right at the beginning of
> section "Symbols/Function Pointers"

These extra examples were added just recently (v4.14-rc1)
by the commit fd46cd55fbc5a8e8c ("printk-formats.txt: Add examples
for %pF and %pS usage"). They were supposed to help using
%pF and %pS correctly according to the situation. But we
have a better solution now. %pF is obsoleted by this
patchset.

IMHO, it is perfectly fine to remove the extra examples.

Best Regards,
Petr

Re: [PATCHv4 0/6] printk/ia64/ppc64/parisc64: let's deprecate %pF/%pf printk specifiers

[Petr Mladek]

On Fri 2017-11-10 08:48:24, Sergey Senozhatsky wrote:
> 	Hello,
> 
> 	A reworked version. There is a new dereference_symbol_descriptor()
> function now, where "the magic happens", so I don't touch kallsyms_lookup()
> and module_address_lookup() anymore.

The new version looks good to me. Thanks a lot for reworking it.
I feel much better now. For the whole series:

Reviewed-by: Petr Mladek <pmladek@suse.com>

> 	All Ack-s/Tested-by-s were dropped, since the patch set has been
> reworked. I'm kindly asking arch-s maintainers and developers to test it
> once again. Sorry for any inconveniences and thanks for your help in
> advance.

I see that it was tested on all affected architectures. Thanks a lot
all testers.

It seems that we are ready to go. I am going to push this into
for-4.16 branch in printk.git.

Best Regards,
Petr

Re: [PATCHv4 0/6] printk/ia64/ppc64/parisc64: let's deprecate %pF/%pf printk specifiers

[Sergey Senozhatsky]

On (11/28/17 16:47), Petr Mladek wrote:
> On Fri 2017-11-10 08:48:24, Sergey Senozhatsky wrote:
> > 	Hello,
> > 
> > 	A reworked version. There is a new dereference_symbol_descriptor()
> > function now, where "the magic happens", so I don't touch kallsyms_lookup()
> > and module_address_lookup() anymore.
> 
> The new version looks good to me. Thanks a lot for reworking it.
> I feel much better now. For the whole series:
> 
> Reviewed-by: Petr Mladek <pmladek@suse.com>
> 
> > 	All Ack-s/Tested-by-s were dropped, since the patch set has been
> > reworked. I'm kindly asking arch-s maintainers and developers to test it
> > once again. Sorry for any inconveniences and thanks for your help in
> > advance.
> 
> I see that it was tested on all affected architectures. Thanks a lot
> all testers.
> 
> It seems that we are ready to go. I am going to push this into
> for-4.16 branch in printk.git.

thanks.

	-ss

Re: [PATCHv4 5/6] symbol lookup: introduce dereference_symbol_descriptor()

[Sergey Senozhatsky]

Hello,

	so we got a number of build-error reports [somehow I
thought 0day has compile tested the patches already; well, I
was wrong] basically on congifs that have no KALLSYMS.


Petr, can we replace 0006 with the following patch?

8<--- --- ---

From: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Subject: [PATCH] symbol lookup: introduce dereference_symbol_descriptor()

dereference_symbol_descriptor() invokes appropriate ARCH specific
function descriptor dereference callbacks:
- dereference_kernel_function_descriptor() if the pointer is a
  kernel symbol;

- dereference_module_function_descriptor() if the pointer is a
  module symbol.

This is the last step needed to make '%pS/%ps' smart enough to
handle function descriptor dereference on affected ARCHs and
to retire '%pF/%pf'.

To refresh it:
  Some architectures (ia64, ppc64, parisc64) use an indirect pointer
  for C function pointers - the function pointer points to a function
  descriptor and we need to dereference it to get the actual function
  pointer.

  Function descriptors live in .opd elf section and all affected
  ARCHs (ia64, ppc64, parisc64) handle it properly for kernel and
  modules. So we, technically, can decide if the dereference is
  needed by simply looking at the pointer: if it belongs to .opd
  section then we need to dereference it.

  The kernel and modules have their own .opd sections, obviously,
  that's why we need to split dereference_function_descriptor()
  and use separate kernel and module dereference arch callbacks.

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
 Documentation/printk-formats.txt | 42 ++++++++++++-------------------
 include/linux/kallsyms.h         | 53 ++++++++++++++++++++++++++++++++++++++++
 kernel/kallsyms.c                | 33 -------------------------
 lib/vsprintf.c                   |  5 ++--
 4 files changed, 71 insertions(+), 62 deletions(-)

diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
index aa0a776c817a..02745028e909 100644
--- a/Documentation/printk-formats.txt
+++ b/Documentation/printk-formats.txt
@@ -61,41 +61,31 @@ Symbols/Function Pointers
 
 ::
 
-	%pF	versatile_init+0x0/0x110
-	%pf	versatile_init
-	%pS	versatile_init+0x0/0x110
-	%pSR	versatile_init+0x9/0x110
+	%pS     versatile_init+0x0/0x110
+	%ps     versatile_init
+	%pF     versatile_init+0x0/0x110
+	%pf     versatile_init
+	%pSR    versatile_init+0x9/0x110
 		(with __builtin_extract_return_addr() translation)
-	%ps	versatile_init
-	%pB	prev_fn_of_versatile_init+0x88/0x88
+	%pB     prev_fn_of_versatile_init+0x88/0x88
 
-The ``F`` and ``f`` specifiers are for printing function pointers,
-for example, f->func, &gettimeofday. They have the same result as
-``S`` and ``s`` specifiers. But they do an extra conversion on
-ia64, ppc64 and parisc64 architectures where the function pointers
-are actually function descriptors.
+The ``S`` and ``s`` specifiers are used for printing a pointer in symbolic
+format. They result in the symbol name with (``S``) or without (``s``)
+offsets. If KALLSYMS are disabled then the symbol address is printed instead.
 
-The ``S`` and ``s`` specifiers can be used for printing symbols
-from direct addresses, for example, __builtin_return_address(0),
-(void *)regs->ip. They result in the symbol name with (``S``) or
-without (``s``) offsets. If KALLSYMS are disabled then the symbol
-address is printed instead.
+Note, that the ``F`` and ``f`` specifiers are identical to ``S`` (``s``)
+and thus deprecated. We have ``F`` and ``f`` because on ia64, ppc64 and
+parisc64 function pointers are indirect and, in fact, are function
+descriptors, which require additional dereferencing before we can lookup
+the symbol. As of now, ``S`` and ``s`` perform dereferencing on those
+platforms (when needed), so ``F`` and ``f`` exist for compatibility
+reasons only.
 
 The ``B`` specifier results in the symbol name with offsets and should be
 used when printing stack backtraces. The specifier takes into
 consideration the effect of compiler optimisations which may occur
 when tail-call``s are used and marked with the noreturn GCC attribute.
 
-Examples::
-
-	printk("Going to call: %pF\n", gettimeofday);
-	printk("Going to call: %pF\n", p->func);
-	printk("%s: called from %pS\n", __func__, (void *)_RET_IP_);
-	printk("%s: called from %pS\n", __func__,
-				(void *)__builtin_return_address(0));
-	printk("Faulted at %pS\n", (void *)regs->ip);
-	printk(" %s%pB\n", (reliable ? "" : "? "), (void *)*stack);
-
 Kernel Pointers
 ===============
 
diff --git a/include/linux/kallsyms.h b/include/linux/kallsyms.h
index bd118a6c60cb..1bcfe221e62c 100644
--- a/include/linux/kallsyms.h
+++ b/include/linux/kallsyms.h
@@ -9,6 +9,9 @@
 #include <linux/errno.h>
 #include <linux/kernel.h>
 #include <linux/stddef.h>
+#include <linux/mm.h>
+
+#include <asm/sections.h>
 
 #define KSYM_NAME_LEN 128
 #define KSYM_SYMBOL_LEN (sizeof("%s+%#lx/%#lx [%s]") + (KSYM_NAME_LEN - 1) + \
@@ -16,6 +19,56 @@
 
 struct module;
 
+static inline int is_kernel_inittext(unsigned long addr)
+{
+	if (addr >= (unsigned long)_sinittext
+	    && addr <= (unsigned long)_einittext)
+		return 1;
+	return 0;
+}
+
+static inline int is_kernel_text(unsigned long addr)
+{
+	if ((addr >= (unsigned long)_stext && addr <= (unsigned long)_etext) ||
+	    arch_is_kernel_text(addr))
+		return 1;
+	return in_gate_area_no_mm(addr);
+}
+
+static inline int is_kernel(unsigned long addr)
+{
+	if (addr >= (unsigned long)_stext && addr <= (unsigned long)_end)
+		return 1;
+	return in_gate_area_no_mm(addr);
+}
+
+static inline int is_ksym_addr(unsigned long addr)
+{
+	if (IS_ENABLED(CONFIG_KALLSYMS_ALL))
+		return is_kernel(addr);
+
+	return is_kernel_text(addr) || is_kernel_inittext(addr);
+}
+
+static inline void *dereference_symbol_descriptor(void *ptr)
+{
+#ifdef HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR
+	struct module *mod;
+
+	ptr = dereference_kernel_function_descriptor(ptr);
+	if (is_ksym_addr((unsigned long)ptr))
+		return ptr;
+
+	preempt_disable();
+	mod = __module_address((unsigned long)ptr);
+	preempt_enable();
+
+	if (mod)
+		ptr = dereference_module_function_descriptor(mod, ptr);
+#endif
+	return ptr;
+}
+
 #ifdef CONFIG_KALLSYMS
 /* Lookup the address for a symbol. Returns 0 if not found. */
 unsigned long kallsyms_lookup_name(const char *name);
diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index d5fa4116688a..4a79598e92c7 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c
@@ -27,8 +27,6 @@
 #include <linux/ftrace.h>
 #include <linux/compiler.h>
 
-#include <asm/sections.h>
-
 /*
  * These will be re-linked against their real values
  * during the second link stage.
@@ -52,37 +50,6 @@ extern const u16 kallsyms_token_index[] __weak;
 
 extern const unsigned long kallsyms_markers[] __weak;
 
-static inline int is_kernel_inittext(unsigned long addr)
-{
-	if (addr >= (unsigned long)_sinittext
-	    && addr <= (unsigned long)_einittext)
-		return 1;
-	return 0;
-}
-
-static inline int is_kernel_text(unsigned long addr)
-{
-	if ((addr >= (unsigned long)_stext && addr <= (unsigned long)_etext) ||
-	    arch_is_kernel_text(addr))
-		return 1;
-	return in_gate_area_no_mm(addr);
-}
-
-static inline int is_kernel(unsigned long addr)
-{
-	if (addr >= (unsigned long)_stext && addr <= (unsigned long)_end)
-		return 1;
-	return in_gate_area_no_mm(addr);
-}
-
-static int is_ksym_addr(unsigned long addr)
-{
-	if (IS_ENABLED(CONFIG_KALLSYMS_ALL))
-		return is_kernel(addr);
-
-	return is_kernel_text(addr) || is_kernel_inittext(addr);
-}
-
 /*
  * Expand a compressed symbol data into the resulting uncompressed string,
  * if uncompressed string is too long (>= maxlen), it will be truncated,
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 01c3957b2de6..03950269f35d 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -42,7 +42,6 @@
 #include "../mm/internal.h"	/* For the trace_print_flags arrays */
 
 #include <asm/page.h>		/* for PAGE_SIZE */
-#include <asm/sections.h>	/* for dereference_function_descriptor() */
 #include <asm/byteorder.h>	/* cpu_to_le16 */
 
 #include <linux/string_helpers.h>
@@ -1862,10 +1861,10 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 	switch (*fmt) {
 	case 'F':
 	case 'f':
-		ptr = dereference_function_descriptor(ptr);
-		/* Fallthrough */
 	case 'S':
 	case 's':
+		ptr = dereference_symbol_descriptor(ptr);
+		/* Fallthrough */
 	case 'B':
 		return symbol_string(buf, end, ptr, spec, fmt);
 	case 'R':
-- 
2.15.1

Re: [PATCHv4 5/6] symbol lookup: introduce dereference_symbol_descriptor()

[Petr Mladek]

On Wed 2017-12-06 13:36:49, Sergey Senozhatsky wrote:
> Hello,
> 
> 	so we got a number of build-error reports [somehow I
> thought 0day has compile tested the patches already; well, I
> was wrong] basically on congifs that have no KALLSYMS.
> 
> 
> Petr, can we replace 0006 with the following patch?

Done. See comments below.

> From: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> Subject: [PATCH] symbol lookup: introduce dereference_symbol_descriptor()
> 
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

The new patch only shuffled the code to fix a compilation problem
with CONFIG_CALLSYMS undefined. It did not change the functionality.
Therefore I put back:

Tested-by: Tony Luck <tony.luck@intel.com> #ia64
Tested-by: Santosh Sivaraj <santosh@fossix.org> #powerpc
Tested-by: Helge Deller <deller@gmx.de> #parisc64

> ---
>  Documentation/printk-formats.txt | 42 ++++++++++++-------------------
>  include/linux/kallsyms.h         | 53 ++++++++++++++++++++++++++++++++++++++++
>  kernel/kallsyms.c                | 33 -------------------------
>  lib/vsprintf.c                   |  5 ++--
>  4 files changed, 71 insertions(+), 62 deletions(-)
> 
> diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
> index aa0a776c817a..02745028e909 100644
> --- a/Documentation/printk-formats.txt
> +++ b/Documentation/printk-formats.txt
> @@ -61,41 +61,31 @@ Symbols/Function Pointers
>  
>  ::
>  
> -	%pF	versatile_init+0x0/0x110
> -	%pf	versatile_init
> -	%pS	versatile_init+0x0/0x110
> -	%pSR	versatile_init+0x9/0x110
> +	%pS     versatile_init+0x0/0x110
> +	%ps     versatile_init
> +	%pF     versatile_init+0x0/0x110
> +	%pf     versatile_init
> +	%pSR    versatile_init+0x9/0x110
>  		(with __builtin_extract_return_addr() translation)
> -	%ps	versatile_init
> -	%pB	prev_fn_of_versatile_init+0x88/0x88
> +	%pB     prev_fn_of_versatile_init+0x88/0x88

I was curious why so many lines were changed here. You converted
the 2nd tab to spaces. I put back the tab. The result is:

--- a/Documentation/printk-formats.txt
+++ b/Documentation/printk-formats.txt
@@ -50,42 +50,31 @@ Symbols/Function Pointers
 
 ::
 
+	%pS	versatile_init+0x0/0x110
+	%ps	versatile_init
 	%pF	versatile_init+0x0/0x110
 	%pf	versatile_init
-	%pS	versatile_init+0x0/0x110
 	%pSR	versatile_init+0x9/0x110
 		(with __builtin_extract_return_addr() translation)
-	%ps	versatile_init
 	%pB	prev_fn_of_versatile_init+0x88/0x88
 
https://git.kernel.org/pub/scm/linux/kernel/git/pmladek/printk.git/commit/?h=for-4.16-deprecate-printk-pf&id=78675fe41d57c2bf9cb671f0a85b369a5a156f0a

>  
> diff --git a/include/linux/kallsyms.h b/include/linux/kallsyms.h
> index bd118a6c60cb..1bcfe221e62c 100644
> --- a/include/linux/kallsyms.h
> +++ b/include/linux/kallsyms.h
> +static inline void *dereference_symbol_descriptor(void *ptr)
> +{
> +#ifdef HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR
> +	struct module *mod;
> +
> +	ptr = dereference_kernel_function_descriptor(ptr);
> +	if (is_ksym_addr((unsigned long)ptr))
> +		return ptr;
> +
> +	preempt_disable();
> +	mod = __module_address((unsigned long)ptr);
> +	preempt_enable();
> +
> +	if (mod)
> +		ptr = dereference_module_function_descriptor(mod, ptr);
> +#endif
> +	return ptr;
> +}

It is a bit too long for an inline function but I did not find a
better solution. It should always be defined and all suitable
.c files are compiled only under certain configuration. Well,
it is a nop on most architectures.

Best Regards,
Petr

Re: [PATCHv4 5/6] symbol lookup: introduce dereference_symbol_descriptor()

[Sergey Senozhatsky]

On (12/06/17 11:32), Petr Mladek wrote:
[..]
> > diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
> > index aa0a776c817a..02745028e909 100644
> > --- a/Documentation/printk-formats.txt
> > +++ b/Documentation/printk-formats.txt
> > @@ -61,41 +61,31 @@ Symbols/Function Pointers
> >  
> >  ::
> >  
> > -	%pF	versatile_init+0x0/0x110
> > -	%pf	versatile_init
> > -	%pS	versatile_init+0x0/0x110
> > -	%pSR	versatile_init+0x9/0x110
> > +	%pS     versatile_init+0x0/0x110
> > +	%ps     versatile_init
> > +	%pF     versatile_init+0x0/0x110
> > +	%pf     versatile_init
> > +	%pSR    versatile_init+0x9/0x110
> >  		(with __builtin_extract_return_addr() translation)
> > -	%ps	versatile_init
> > -	%pB	prev_fn_of_versatile_init+0x88/0x88
> > +	%pB     prev_fn_of_versatile_init+0x88/0x88
> 
> I was curious why so many lines were changed here. You converted
> the 2nd tab to spaces. I put back the tab. The result is:

ew... how did that happen. thanks for fixing up.

> > +static inline void *dereference_symbol_descriptor(void *ptr)
> > +{
> > +#ifdef HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR
> > +	struct module *mod;
> > +
> > +	ptr = dereference_kernel_function_descriptor(ptr);
> > +	if (is_ksym_addr((unsigned long)ptr))
> > +		return ptr;
> > +
> > +	preempt_disable();
> > +	mod = __module_address((unsigned long)ptr);
> > +	preempt_enable();
> > +
> > +	if (mod)
> > +		ptr = dereference_module_function_descriptor(mod, ptr);
> > +#endif
> > +	return ptr;
> > +}
> 
> It is a bit too long for an inline function but I did not find a
> better solution. It should always be defined and all suitable
> .c files are compiled only under certain configuration. Well,
> it is a nop on most architectures.

or we can move dereference_symbol_descriptor() to vsprintf.c,
since all the functions it depends on are now available either
as exported symbols or via kallsyms header file. not that it
annoys me, so we can keep it as it is.

	-ss