[RFC][PATCH v2 0/7] printk/ia64/ppc64/parisc64: let's deprecate %pF/%pf printk specifiers

        Hello

        RFC

        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.


*** A BIG NOTE ***
        I don't own ia64/ppc64/parisc64 hardware, so the patches are not
        tested. Sorry about that!

Another note:
        I need to check what is BPF symbol lookup and do we need to
        do any dereference there.

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 (7):
  switch dereference_function_descriptor() to `unsigned long'
  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: use new kernel and module dereference functions
  checkpatch: add pF/pf deprecation warning

 Documentation/printk-formats.txt          | 15 +++++----------
 arch/ia64/include/asm/sections.h          | 16 ++++++++++++----
 arch/ia64/kernel/module.c                 | 13 +++++++++++++
 arch/ia64/kernel/vmlinux.lds.S            |  2 ++
 arch/parisc/boot/compressed/vmlinux.lds.S |  2 ++
 arch/parisc/include/asm/sections.h        |  4 +++-
 arch/parisc/kernel/module.c               | 17 +++++++++++++++++
 arch/parisc/kernel/process.c              | 15 ++++++++++++---
 arch/parisc/kernel/vmlinux.lds.S          |  2 ++
 arch/parisc/mm/init.c                     |  4 ++--
 arch/powerpc/include/asm/module.h         |  3 +++
 arch/powerpc/include/asm/sections.h       | 17 ++++++++++++++---
 arch/powerpc/kernel/module_64.c           | 16 ++++++++++++++++
 arch/powerpc/kernel/vmlinux.lds.S         |  2 ++
 drivers/misc/kgdbts.c                     |  2 +-
 include/asm-generic/sections.h            |  8 ++++++--
 include/linux/moduleloader.h              |  4 ++++
 init/main.c                               |  2 +-
 kernel/extable.c                          |  2 +-
 kernel/kallsyms.c                         |  1 +
 kernel/module.c                           |  9 ++++++++-
 lib/vsprintf.c                            |  5 +----
 scripts/checkpatch.pl                     |  6 ++++--
 23 files changed, 132 insertions(+), 35 deletions(-)

-- 
2.14.1

Convert dereference_function_descriptor() to accept and return
`unsigned long'. There will be two new ARCH function for kernel
and module function pointer dereference, which will work with
`unsigned long', so the patch unifies interfaces.

Besides, dereference_function_descriptor() mostly work with
`unsigned long':

drivers/misc/kgdbts.c:
addr = (unsigned long) dereference_function_descriptor((void *)addr);

init/main.c:
addr = (unsigned long) dereference_function_descriptor(fn);

kernel/extable.c:
addr = (unsigned long) dereference_function_descriptor(ptr);

kernel/module.c:
unsigned long a = (unsigned long)dereference_function_descriptor(addr);

Convert dereference_function_descriptor() users tree-wide.

Signed-off-by: Sergey Senozhatsky <redacted>
---
 arch/ia64/include/asm/sections.h    | 6 +++---
 arch/parisc/include/asm/sections.h  | 2 +-
 arch/parisc/kernel/process.c        | 6 +++---
 arch/parisc/mm/init.c               | 4 ++--
 arch/powerpc/include/asm/sections.h | 6 +++---
 drivers/misc/kgdbts.c               | 2 +-
 init/main.c                         | 2 +-
 kernel/extable.c                    | 2 +-
 kernel/module.c                     | 2 +-
 lib/vsprintf.c                      | 2 +-
 10 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/arch/ia64/include/asm/sections.h b/arch/ia64/include/asm/sections.h
index 2ab2003698ef..de6bfa1ef8fb 100644
--- a/arch/ia64/include/asm/sections.h
+++ b/arch/ia64/include/asm/sections.h

@@ -27,13 +27,13 @@ extern char __start_unwind[], __end_unwind[];
 extern char __start_ivt_text[], __end_ivt_text[];
 
 #undef dereference_function_descriptor
-static inline void *dereference_function_descriptor(void *ptr)
+static inline unsigned long dereference_function_descriptor(unsigned long ptr)
 {
-	struct fdesc *desc = ptr;
+	struct fdesc *desc = (struct fdesc *)ptr;
 	void *p;
 
 	if (!probe_kernel_address(&desc->ip, p))
-		ptr = p;
+		ptr = (unsigned long)p;
 	return ptr;
 }
 

diff --git a/arch/parisc/include/asm/sections.h b/arch/parisc/include/asm/sections.h
index 9d13c3507ad6..59fbe0067112 100644
--- a/arch/parisc/include/asm/sections.h
+++ b/arch/parisc/include/asm/sections.h

@@ -6,7 +6,7 @@
 
 #ifdef CONFIG_64BIT
 #undef dereference_function_descriptor
-void *dereference_function_descriptor(void *);
+unsigned long dereference_function_descriptor(unsigned long);
 #endif
 
 #endif

diff --git a/arch/parisc/kernel/process.c b/arch/parisc/kernel/process.c
index a45a67d526f8..f00a5f93492a 100644
--- a/arch/parisc/kernel/process.c
+++ b/arch/parisc/kernel/process.c

@@ -267,13 +267,13 @@ get_wchan(struct task_struct *p)
 }
 
 #ifdef CONFIG_64BIT
-void *dereference_function_descriptor(void *ptr)
+unsigned long dereference_function_descriptor(unsigned long ptr)
 {
-	Elf64_Fdesc *desc = ptr;
+	Elf64_Fdesc *desc = (Elf64_Fdesc *)ptr;
 	void *p;
 
 	if (!probe_kernel_address(&desc->addr, p))
-		ptr = p;
+		ptr = (unsigned long)p;
 	return ptr;
 }
 #endif

diff --git a/arch/parisc/mm/init.c b/arch/parisc/mm/init.c
index 1ca9a2b4239f..06e1b79e2946 100644
--- a/arch/parisc/mm/init.c
+++ b/arch/parisc/mm/init.c

@@ -389,10 +389,10 @@ static void __init setup_bootmem(void)
 static int __init parisc_text_address(unsigned long vaddr)
 {
 	static unsigned long head_ptr __initdata;
+	unsigned long addr = (unsigned long)&parisc_kernel_start;
 
 	if (!head_ptr)
-		head_ptr = PAGE_MASK & (unsigned long)
-			dereference_function_descriptor(&parisc_kernel_start);
+		head_ptr = PAGE_MASK & dereference_function_descriptor(addr);
 
 	return core_kernel_text(vaddr) || vaddr == head_ptr;
 }

diff --git a/arch/powerpc/include/asm/sections.h b/arch/powerpc/include/asm/sections.h
index 7902d6358854..67379b8945e8 100644
--- a/arch/powerpc/include/asm/sections.h
+++ b/arch/powerpc/include/asm/sections.h

@@ -66,13 +66,13 @@ static inline int overlaps_kvm_tmp(unsigned long start, unsigned long end)
 
 #ifdef PPC64_ELF_ABI_v1
 #undef dereference_function_descriptor
-static inline void *dereference_function_descriptor(void *ptr)
+static inline unsigned long dereference_function_descriptor(unsigned long ptr)
 {
-	struct ppc64_opd_entry *desc = ptr;
+	struct ppc64_opd_entry *desc = (struct ppc64_opd_entry *)ptr;
 	void *p;
 
 	if (!probe_kernel_address(&desc->funcaddr, p))
-		ptr = p;
+		ptr = (unsigned long)p;
 	return ptr;
 }
 #endif /* PPC64_ELF_ABI_v1 */

diff --git a/drivers/misc/kgdbts.c b/drivers/misc/kgdbts.c
index fc7efedbc4be..6a5a159dfb75 100644
--- a/drivers/misc/kgdbts.c
+++ b/drivers/misc/kgdbts.c

@@ -225,7 +225,7 @@ static unsigned long lookup_addr(char *arg)
 		addr = (unsigned long)_do_fork;
 	else if (!strcmp(arg, "hw_break_val"))
 		addr = (unsigned long)&hw_break_val;
-	addr = (unsigned long) dereference_function_descriptor((void *)addr);
+	addr = dereference_function_descriptor(addr);
 	return addr;
 }
 

diff --git a/init/main.c b/init/main.c
index 0ee9c6866ada..396b0bda696e 100644
--- a/init/main.c
+++ b/init/main.c

@@ -761,7 +761,7 @@ static bool __init_or_module initcall_blacklisted(initcall_t fn)
 	if (list_empty(&blacklisted_initcalls))
 		return false;
 
-	addr = (unsigned long) dereference_function_descriptor(fn);
+	addr = dereference_function_descriptor((unsigned long)fn);
 	sprint_symbol_no_offset(fn_name, addr);
 
 	/*

diff --git a/kernel/extable.c b/kernel/extable.c
index 38c2412401a1..ca4c34d1d5a4 100644
--- a/kernel/extable.c
+++ b/kernel/extable.c

@@ -150,7 +150,7 @@ int kernel_text_address(unsigned long addr)
 int func_ptr_is_kernel_text(void *ptr)
 {
 	unsigned long addr;
-	addr = (unsigned long) dereference_function_descriptor(ptr);
+	addr = dereference_function_descriptor((unsigned long)ptr);
 	if (core_kernel_text(addr))
 		return 1;
 	return is_module_text_address(addr);

diff --git a/kernel/module.c b/kernel/module.c
index de66ec825992..ea77ab13bead 100644
--- a/kernel/module.c
+++ b/kernel/module.c

@@ -1067,7 +1067,7 @@ EXPORT_SYMBOL(__symbol_put);
 void symbol_put_addr(void *addr)
 {
 	struct module *modaddr;
-	unsigned long a = (unsigned long)dereference_function_descriptor(addr);
+	unsigned long a = dereference_function_descriptor((unsigned long)addr);
 
 	if (core_kernel_text(a))
 		return;

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 86c3385b9eb3..bcd906a39010 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c

@@ -1723,7 +1723,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 	switch (*fmt) {
 	case 'F':
 	case 'f':
-		ptr = dereference_function_descriptor(ptr);
+		ptr = (void *)dereference_function_descriptor((unsigned long)ptr);
 		/* Fallthrough */
 	case 'S':
 	case 's':

-- 
2.14.1

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 <redacted>
---
 include/asm-generic/sections.h | 8 ++++++--
 include/linux/moduleloader.h   | 4 ++++
 kernel/module.c                | 6 ++++++
 3 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
index e5da44eddd2f..387f22c41e0d 100644
--- a/include/asm-generic/sections.h
+++ b/include/asm-generic/sections.h

@@ -29,6 +29,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[];

@@ -47,12 +48,15 @@ extern char __softirqentry_text_start[], __softirqentry_text_end[];
 /* 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/moduleloader.h b/include/linux/moduleloader.h
index 4d0cb9bba93e..172904e9cded 100644
--- a/include/linux/moduleloader.h
+++ b/include/linux/moduleloader.h

@@ -85,6 +85,10 @@ void module_arch_cleanup(struct module *mod);
 /* Any cleanup before freeing mod->module_init */
 void module_arch_freeing_init(struct module *mod);
 
+/* Dereference module function descriptor */
+unsigned long dereference_module_function_descriptor(struct module *mod,
+						     unsigned long addr);
+
 #ifdef CONFIG_KASAN
 #include <linux/kasan.h>
 #define MODULE_ALIGN (PAGE_SIZE << KASAN_SHADOW_SCALE_SHIFT)

diff --git a/kernel/module.c b/kernel/module.c
index ea77ab13bead..b792e814150a 100644
--- a/kernel/module.c
+++ b/kernel/module.c

@@ -2121,6 +2121,12 @@ void __weak module_arch_freeing_init(struct module *mod)
 {
 }
 
+unsigned long __weak dereference_module_function_descriptor(struct module *mod,
+							    unsigned long addr)
+{
+	return addr;
+}
+
 /* Free a module, remove from lists, etc. */
 static void free_module(struct module *mod)
 {

-- 
2.14.1

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 <redacted>
---
 arch/ia64/include/asm/sections.h | 10 +++++++++-
 arch/ia64/kernel/module.c        | 13 +++++++++++++
 arch/ia64/kernel/vmlinux.lds.S   |  2 ++
 3 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/arch/ia64/include/asm/sections.h b/arch/ia64/include/asm/sections.h
index de6bfa1ef8fb..3ba7ce9d8bc8 100644
--- a/arch/ia64/include/asm/sections.h
+++ b/arch/ia64/include/asm/sections.h

@@ -37,6 +37,14 @@ static inline unsigned long dereference_function_descriptor(unsigned long ptr)
 	return ptr;
 }
 
+#undef dereference_kernel_function_descriptor
+static inline unsigned long
+dereference_kernel_function_descriptor(unsigned long addr)
+{
+	if (addr < (unsigned long)__start_opd ||
+			addr >= (unsigned long)__end_opd)
+		return addr;
+	return dereference_function_descriptor(addr);
+}
 
 #endif /* _ASM_IA64_SECTIONS_H */
-

diff --git a/arch/ia64/kernel/module.c b/arch/ia64/kernel/module.c
index d1d945c6bd05..0741ae6fa957 100644
--- a/arch/ia64/kernel/module.c
+++ b/arch/ia64/kernel/module.c

@@ -35,6 +35,7 @@
 
 #include <asm/patch.h>
 #include <asm/unaligned.h>
+#include <asm/sections.h>
 
 #define ARCH_MODULE_DEBUG 0
 

@@ -917,3 +918,15 @@ module_arch_cleanup (struct module *mod)
 	if (mod->arch.core_unw_table)
 		unw_remove_unwind_table(mod->arch.core_unw_table);
 }
+
+unsigned long
+dereference_module_function_descriptor(struct module *mod, unsigned long addr)
+{
+	Elf64_Shdr *opd = mod->arch.opd;
+
+	if (addr < opd->sh_addr ||
+			addr >= (opd->sh_addr + opd->sh_size))
+		return addr;
+
+	return dereference_function_descriptor(addr);
+}

diff --git a/arch/ia64/kernel/vmlinux.lds.S b/arch/ia64/kernel/vmlinux.lds.S
index 798026dde52e..f872ba5ff82a 100644
--- a/arch/ia64/kernel/vmlinux.lds.S
+++ b/arch/ia64/kernel/vmlinux.lds.S

@@ -107,7 +107,9 @@ SECTIONS {
 	RODATA
 
 	.opd : AT(ADDR(.opd) - LOAD_OFFSET) {
+		__start_opd = .;
 		*(.opd)
+		__end_opd = .;
 	}
 
 	/*

-- 
2.14.1

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 <redacted>
Signed-off-by: Helge Deller <deller@gmx.de>
---
 arch/parisc/boot/compressed/vmlinux.lds.S |  2 ++
 arch/parisc/include/asm/sections.h        |  2 ++
 arch/parisc/kernel/module.c               | 17 +++++++++++++++++
 arch/parisc/kernel/process.c              |  9 +++++++++
 arch/parisc/kernel/vmlinux.lds.S          |  2 ++
 5 files changed, 32 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 59fbe0067112..845ddc9a3421 100644
--- a/arch/parisc/include/asm/sections.h
+++ b/arch/parisc/include/asm/sections.h

@@ -7,6 +7,8 @@
 #ifdef CONFIG_64BIT
 #undef dereference_function_descriptor
 unsigned long dereference_function_descriptor(unsigned long);
+#undef dereference_kernel_function_descriptor
+unsigned long dereference_kernel_function_descriptor(unsigned long);
 #endif
 
 #endif

diff --git a/arch/parisc/kernel/module.c b/arch/parisc/kernel/module.c
index f1a76935a314..28f89b3dcc11 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,19 @@ void module_arch_cleanup(struct module *mod)
 {
 	deregister_unwind_table(mod);
 }
+
+#ifdef CONFIG_64BIT
+unsigned long dereference_module_function_descriptor(struct module *mod,
+						     unsigned long addr)
+{
+	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 (addr < start_opd || addr >= end_opd)
+		return addr;
+
+	return dereference_function_descriptor(addr);
+}
+#endif

diff --git a/arch/parisc/kernel/process.c b/arch/parisc/kernel/process.c
index f00a5f93492a..ff13726b2d2d 100644
--- a/arch/parisc/kernel/process.c
+++ b/arch/parisc/kernel/process.c

@@ -276,6 +276,15 @@ unsigned long dereference_function_descriptor(unsigned long ptr)
 		ptr = (unsigned long)p;
 	return ptr;
 }
+
+unsigned long dereference_kernel_function_descriptor(unsigned long addr)
+{
+	if (addr < (unsigned long)__start_opd ||
+			addr >= (unsigned long)__end_opd)
+		return addr;
+
+	return dereference_function_descriptor(addr);
+}
 #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 ffe2cbf52d1a..ab030895dd1e 100644
--- a/arch/parisc/kernel/vmlinux.lds.S
+++ b/arch/parisc/kernel/vmlinux.lds.S

@@ -99,7 +99,9 @@ SECTIONS
 	. = ALIGN(16);
 	/* Linkage tables */
 	.opd : {
+		__start_opd = .;
 		*(.opd)
+		__end_opd = .;
 	} PROVIDE (__gp = .);
 	.plt : {
 		*(.plt)

-- 
2.14.1

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

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

This patch also removes dereference_function_descriptor() from
'%pF/%pf' vsprintf handler, because it has the same behavior with
'%pS/%ps' now.

Signed-off-by: Sergey Senozhatsky <redacted>
---
 Documentation/printk-formats.txt | 15 +++++----------
 kernel/kallsyms.c                |  1 +
 kernel/module.c                  |  1 +
 lib/vsprintf.c                   |  5 +----
 4 files changed, 8 insertions(+), 14 deletions(-)

diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
index 361789df51ec..b2afafc84638 100644
--- a/Documentation/printk-formats.txt
+++ b/Documentation/printk-formats.txt

@@ -50,26 +50,23 @@ 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
 
-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.
 
+Note, that the ``F`` and ``f`` specifiers are identical to ``S`` (``s``)
+and thus deprecated.
+
 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

@@ -77,8 +74,6 @@ 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));

diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index 127e7cfafa55..e2fc09ea9509 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c

@@ -322,6 +322,7 @@ const char *kallsyms_lookup(unsigned long addr,
 	if (is_ksym_addr(addr)) {
 		unsigned long pos;
 
+		addr = dereference_kernel_function_descriptor(addr);
 		pos = get_symbol_pos(addr, symbolsize, offset);
 		/* Grab name */
 		kallsyms_expand_symbol(get_symbol_offset(pos),

diff --git a/kernel/module.c b/kernel/module.c
index b792e814150a..63361de377ad 100644
--- a/kernel/module.c
+++ b/kernel/module.c

@@ -3948,6 +3948,7 @@ const char *module_address_lookup(unsigned long addr,
 	preempt_disable();
 	mod = __module_address(addr);
 	if (mod) {
+		addr = dereference_module_function_descriptor(mod, addr);
 		if (modname)
 			*modname = mod->name;
 		ret = get_ksymbol(mod, addr, size, offset);

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index bcd906a39010..bf04b4f5d8e7 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>

@@ -1721,10 +1720,8 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 	}
 
 	switch (*fmt) {
-	case 'F':
+	case 'F': /* %pF and %pf are kept for compatibility reasons only */
 	case 'f':
-		ptr = (void *)dereference_function_descriptor((unsigned long)ptr);
-		/* Fallthrough */
 	case 'S':
 	case 's':
 	case 'B':

-- 
2.14.1

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.

checkpatch warning example:

WARNING: Use '%pS/%ps' instead. This pointer extension was deprecated: '%pF'

Signed-off-by: Sergey Senozhatsky <redacted>
Cc: Andy Whitcroft <apw@canonical.com>
Cc: Joe Perches <joe@perches.com>
---
 scripts/checkpatch.pl | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index dd2c262aebbf..5945e4843466 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl

@@ -5762,18 +5762,20 @@ 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 $error_msg = "Invalid vsprintf pointer extension ";
 				for (my $count = $linenr + 1; $count <= $lc; $count++) {
 					$stat_real = $stat_real . "\n" . raw_line($count, 0);
 				}
+				$error_msg = "Use '%pS/%ps' instead. This pointer extension was deprecated:" if ($bad_extension =~ /pF|pf/);
 				WARN("VSPRINTF_POINTER_EXTENSION",
-				     "Invalid vsprintf pointer extension '$bad_extension'\n" . "$here\n$stat_real\n");
+				     "$error_msg '$bad_extension'\n" . "$here\n$stat_real\n");
 			}
 		}
 

-- 
2.14.1

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.

checkpatch warning example:

WARNING: Use '%pS/%ps' instead. This pointer extension was deprecated: '%pF'

Signed-off-by: Sergey Senozhatsky <redacted>
Cc: Andy Whitcroft <apw@canonical.com>
Cc: Joe Perches <joe@perches.com>
---
 scripts/checkpatch.pl | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index dd2c262aebbf..5945e4843466 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl

@@ -5762,18 +5762,20 @@ 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 $error_msg = "Invalid vsprintf pointer extension ";
 				for (my $count = $linenr + 1; $count <= $lc; $count++) {
 					$stat_real = $stat_real . "\n" . raw_line($count, 0);
 				}
+				$error_msg = "Use '%pS/%ps' instead. This pointer extension was deprecated:" if ($bad_extension =~ /pF|pf/);
 				WARN("VSPRINTF_POINTER_EXTENSION",
-				     "Invalid vsprintf pointer extension '$bad_extension'\n" . "$here\n$stat_real\n");
+				     "$error_msg '$bad_extension'\n" . "$here\n$stat_real\n");
 			}
 		}
 

-- 
2.14.1

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 <redacted>
---
 arch/powerpc/include/asm/module.h   |  3 +++
 arch/powerpc/include/asm/sections.h | 11 +++++++++++
 arch/powerpc/kernel/module_64.c     | 16 ++++++++++++++++
 arch/powerpc/kernel/vmlinux.lds.S   |  2 ++
 4 files changed, 32 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 67379b8945e8..6b4ee0d1645f 100644
--- a/arch/powerpc/include/asm/sections.h
+++ b/arch/powerpc/include/asm/sections.h

@@ -75,6 +75,17 @@ static inline unsigned long dereference_function_descriptor(unsigned long ptr)
 		ptr = (unsigned long)p;
 	return ptr;
 }
+
+#undef dereference_kernel_function_descriptor
+static inline unsigned long
+dereference_kernel_function_descriptor(unsigned long addr)
+{
+	if (addr < (unsigned long)__start_opd ||
+			addr >= (unsigned long)__end_opd)
+		return addr;
+
+	return dereference_function_descriptor(addr);
+}
 #endif /* PPC64_ELF_ABI_v1 */
 
 #endif

diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index 0b0f89685b67..94caec045a90 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c

@@ -344,6 +344,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")))

@@ -712,6 +717,17 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
 	return 0;
 }
 
+#ifdef PPC64_ELF_ABI_v1
+unsigned long dereference_module_function_descriptor(struct module *mod,
+						     unsigned long addr)
+{
+	if (addr < mod->arch.start_opd || addr >= mod->arch.end_opd)
+		return addr;
+
+	return dereference_function_descriptor(addr);
+}
+#endif /* PPC64_ELF_ABI_v1 */
+
 #ifdef CONFIG_DYNAMIC_FTRACE
 
 #ifdef CC_USING_MPROFILE_KERNEL

diff --git a/arch/powerpc/kernel/vmlinux.lds.S b/arch/powerpc/kernel/vmlinux.lds.S
index 882628fa6987..70e10251e083 100644
--- a/arch/powerpc/kernel/vmlinux.lds.S
+++ b/arch/powerpc/kernel/vmlinux.lds.S

@@ -277,7 +277,9 @@ SECTIONS
 	}
 
 	.opd : AT(ADDR(.opd) - LOAD_OFFSET) {
+		__start_opd = .;
 		*(.opd)
+		__end_opd = .;
 	}
 
 	. = ALIGN(256);

-- 
2.14.1

On (09/21/17 01:29), Sergey Senozhatsky wrote:

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.

checkpatch warning example:

WARNING: Use '%pS/%ps' instead. This pointer extension was deprecated: '%pF'

weird... somehow I sent this patch twice. please ignore this one.

	-ss

On Thu, 2017-09-21 at 01:29 +0900, Sergey Senozhatsky wrote:

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.

checkpatch warning example:

WARNING: Use '%pS/%ps' instead. This pointer extension was deprecated: '%pF'

If this series is accepted,  I think this message
is unclear and would prefer something like:
---
scripts/checkpatch.pl | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index dd2c262aebbf..71f3273d5913 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl

@@ -5762,18 +5762,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]/i) {
+					$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");
 			}
 		}
 

On 20.09.2017 19:38, Joe Perches wrote:

On Thu, 2017-09-21 at 01:29 +0900, Sergey Senozhatsky wrote:

quoted

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.

checkpatch warning example:

WARNING: Use '%pS/%ps' instead. This pointer extension was deprecated: '%pF'

If this series is accepted,  I think this message
is unclear and would prefer something like:

Is it worth to mention, that it's still needed in older kernels?
Just in case some patch get's backported.

Helge

On Wed, 2017-09-20 at 19:53 +0200, Helge Deller wrote:

On 20.09.2017 19:38, Joe Perches wrote:

quoted

On Thu, 2017-09-21 at 01:29 +0900, Sergey Senozhatsky wrote:

quoted

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.

checkpatch warning example:

WARNING: Use '%pS/%ps' instead. This pointer extension was deprecated: '%pF'

If this series is accepted,  I think this message
is unclear and would prefer something like:

Is it worth to mention, that it's still needed in older kernels?
Just in case some patch get's backported.

I think probably not.

There are relatively few references and
modifications are unlikely to be backported.

On 20.09.2017 18:29, Sergey Senozhatsky wrote:

         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.
...> *** A BIG NOTE ***
         I don't own ia64/ppc64/parisc64 hardware, so the patches are not
         tested. Sorry about that!

I just now tested your patch series successfully on parisc64.

You may add to the whole series:
Tested-by: Helge Deller <deller@gmx.de> # parisc64

  

Another note:
         I need to check what is BPF symbol lookup and do we need to
         do any dereference there.

Not relevant for parisc, since we don't support it yet.

Helge

On (09/20/17 10:38), Joe Perches wrote:

On Thu, 2017-09-21 at 01:29 +0900, Sergey Senozhatsky wrote:

quoted

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.

checkpatch warning example:

WARNING: Use '%pS/%ps' instead. This pointer extension was deprecated: '%pF'

If this series is accepted,  I think this message
is unclear and would prefer something like:

sure, can tweak the patch.

[..]

 			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]/i) {

						I think /i is not necessary here

+					$ext_type = "Deprecated";
+					$use = " - use %pS instead";
+					$use =~ s/pS/ps/ if ($bad_extension =~ /pf/);

							ok, handy :)

	-ss

On (09/20/17 22:14), Helge Deller wrote:

On 20.09.2017 18:29, Sergey Senozhatsky wrote:

quoted

         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.
...> *** A BIG NOTE ***
         I don't own ia64/ppc64/parisc64 hardware, so the patches are not
         tested. Sorry about that!

I just now tested your patch series successfully on parisc64.

You may add to the whole series:
Tested-by: Helge Deller <deller@gmx.de> # parisc64

thanks, Helge!

quoted

Another note:
         I need to check what is BPF symbol lookup and do we need to
         do any dereference there.

Not relevant for parisc, since we don't support it yet.

so that was my suspicion as well. at glance it didn't look like
bpf symbol resolution would work on platforms that do description
dereference.

	-ss

On Thu, 2017-09-21 at 09:27 +0900, Sergey Senozhatsky wrote:

On (09/20/17 10:38), Joe Perches wrote:

quoted

On Thu, 2017-09-21 at 01:29 +0900, Sergey Senozhatsky wrote:

quoted

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.

checkpatch warning example:

WARNING: Use '%pS/%ps' instead. This pointer extension was deprecated: '%pF'

If this series is accepted,  I think this message
is unclear and would prefer something like:

sure, can tweak the patch.

[..]

quoted

 			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]/i) {

						I think /i is not necessary here

You are right, it's not.

On (09/20/17 11:24), Joe Perches wrote:

On Wed, 2017-09-20 at 19:53 +0200, Helge Deller wrote:

[..]

quoted

Is it worth to mention, that it's still needed in older kernels?
Just in case some patch get's backported.

good question.

I think probably not.

There are relatively few references and
modifications are unlikely to be backported.

I tend to agree.

unlikely anyone backports printk message updates. I have quickly glanced
through stable-4.9 and haven't seen such backports. well, may be there
are some.

can tweak the warning a bit, probably. e.g. "if you are planning to
backport your change to kernels older than 4.14 then ignore this
warning". but not sure if it's worth it.

	-ss

On (09/21/17 01:29), Sergey Senozhatsky wrote:
[..]

+	%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
 
-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.
 
+Note, that the ``F`` and ``f`` specifiers are identical to ``S`` (``s``)
+and thus deprecated.

JFI,

I have updated this part. it's probably too early to completely
wipe out pF/pf info.

the updated Doc goes like this:

+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.

	-ss

* Sergey Senozhatsky [off-list ref] wrote (on 2017-09-20 16:29:02 +0000):

        Hello

        RFC

        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.


*** A BIG NOTE ***
        I don't own ia64/ppc64/parisc64 hardware, so the patches are not
        tested. Sorry about that!

Tested patch series on ppc64 sucessfully.

You may add tested by to the series.

Tested-by: Santosh Sivaraj <redacted>

Thanks,
Santosh

Another note:
        I need to check what is BPF symbol lookup and do we need to
        do any dereference there.

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 (7):
  switch dereference_function_descriptor() to `unsigned long'
  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: use new kernel and module dereference functions
  checkpatch: add pF/pf deprecation warning

 Documentation/printk-formats.txt          | 15 +++++----------
 arch/ia64/include/asm/sections.h          | 16 ++++++++++++----
 arch/ia64/kernel/module.c                 | 13 +++++++++++++
 arch/ia64/kernel/vmlinux.lds.S            |  2 ++
 arch/parisc/boot/compressed/vmlinux.lds.S |  2 ++
 arch/parisc/include/asm/sections.h        |  4 +++-
 arch/parisc/kernel/module.c               | 17 +++++++++++++++++
 arch/parisc/kernel/process.c              | 15 ++++++++++++---
 arch/parisc/kernel/vmlinux.lds.S          |  2 ++
 arch/parisc/mm/init.c                     |  4 ++--
 arch/powerpc/include/asm/module.h         |  3 +++
 arch/powerpc/include/asm/sections.h       | 17 ++++++++++++++---
 arch/powerpc/kernel/module_64.c           | 16 ++++++++++++++++
 arch/powerpc/kernel/vmlinux.lds.S         |  2 ++
 drivers/misc/kgdbts.c                     |  2 +-
 include/asm-generic/sections.h            |  8 ++++++--
 include/linux/moduleloader.h              |  4 ++++
 init/main.c                               |  2 +-
 kernel/extable.c                          |  2 +-
 kernel/kallsyms.c                         |  1 +
 kernel/module.c                           |  9 ++++++++-
 lib/vsprintf.c                            |  5 +----
 scripts/checkpatch.pl                     |  6 ++++--
 23 files changed, 132 insertions(+), 35 deletions(-)

-- 

On (09/22/17 11:04), Santosh Sivaraj wrote:
[..]

quoted

*** A BIG NOTE ***
        I don't own ia64/ppc64/parisc64 hardware, so the patches are not
        tested. Sorry about that!

Tested patch series on ppc64 sucessfully.

You may add tested by to the series.

Tested-by: Santosh Sivaraj <redacted>

thanks!

	-ss

Tested patch series on ia64 successfully.

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

After this goes upstream, you should submit a patch to get rid of
all uses of %pF (70 instances in 35 files) and %pf (63 in 34)

Perhaps break the patch by top-level directory (e.g. get all the %pF
and %pF in the 17 files under drivers/ in one patch).

-Tony

On (09/22/17 16:48), Luck, Tony wrote:
[..]

Tested patch series on ia64 successfully.

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

thanks!

After this goes upstream, you should submit a patch to get rid of
all uses of %pF (70 instances in 35 files) and %pf (63 in 34)

Perhaps break the patch by top-level directory (e.g. get all the %pF
and %pF in the 17 files under drivers/ in one patch).

frankly, I was going to have some sort of a lazy deprecation process:
didn't plan to send out a patch set that would hunt down all pf/pF-s.
hm...


speaking of upstream, any objections if this patch set will go through
the printk tree, in one piece?

I'll wait for several more days and then resend v3 with updated
Documentation and tweaked checkpatch warning message.

	-ss

speaking of upstream, any objections if this patch set will go through
the printk tree, in one piece?

Seems to be a better idea than trying to coordinate pulls from three
separate "arch/"  trees.  Fine with me.

-Tony

On 25.09.2017 18:29, Luck, Tony wrote:

quoted

speaking of upstream, any objections if this patch set will go through
the printk tree, in one piece?

Fine with me too.

Helge

Sergey Senozhatsky [off-list ref] writes:

On (09/22/17 16:48), Luck, Tony wrote:
[..]

quoted

Tested patch series on ia64 successfully.

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

thanks!

quoted

After this goes upstream, you should submit a patch to get rid of
all uses of %pF (70 instances in 35 files) and %pf (63 in 34)

Perhaps break the patch by top-level directory (e.g. get all the %pF
and %pF in the 17 files under drivers/ in one patch).

frankly, I was going to have some sort of a lazy deprecation process:
didn't plan to send out a patch set that would hunt down all pf/pF-s.
hm...

That never works though, we have lots of cruft left over from times when
that's happened and the conversion never quite got finished.

At least if you send out the patches to do the removal they might
eventually get merged.

speaking of upstream, any objections if this patch set will go through
the printk tree, in one piece?

Do you mind putting it in a topic branch (based on rc2) and then merge
that into the printk tree? That way I can merge the topic branch iff
there are conflicts later down the line towards 4.15.

cheers

Santosh Sivaraj [off-list ref] writes:

* Sergey Senozhatsky [off-list ref] wrote (on 2017-09-20 16:29:02 +0000):

quoted

        Hello

        RFC

        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.


*** A BIG NOTE ***
        I don't own ia64/ppc64/parisc64 hardware, so the patches are not
        tested. Sorry about that!

Tested patch series on ppc64 sucessfully.

You may add tested by to the series.

Tested-by: Santosh Sivaraj <redacted>

Thanks Santosh.

I also gave it a quick spin. I'll give you an ack for the powerpc changes.

Acked-by: Michael Ellerman <mpe@ellerman.id.au> (powerpc)


Thanks for cleaning this up Sergey.

cheers

On (09/27/17 16:26), Michael Ellerman wrote:
[..]

quoted

Tested-by: Santosh Sivaraj <redacted>

Thanks Santosh.

I also gave it a quick spin. I'll give you an ack for the powerpc changes.

Acked-by: Michael Ellerman <mpe@ellerman.id.au> (powerpc)


Thanks for cleaning this up Sergey.

thanks!

	-ss

Michael,

On (09/27/17 15:01), Michael Ellerman wrote:

Sergey Senozhatsky [off-list ref] writes:

quoted

On (09/22/17 16:48), Luck, Tony wrote:
[..]

quoted

Tested patch series on ia64 successfully.

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

thanks!

quoted

After this goes upstream, you should submit a patch to get rid of
all uses of %pF (70 instances in 35 files) and %pf (63 in 34)

Perhaps break the patch by top-level directory (e.g. get all the %pF
and %pF in the 17 files under drivers/ in one patch).

frankly, I was going to have some sort of a lazy deprecation process:
didn't plan to send out a patch set that would hunt down all pf/pF-s.
hm...

That never works though, we have lots of cruft left over from times when
that's happened and the conversion never quite got finished.

this time around it's different, I promise! :)


well...
I guess I can send out a tree wide pf/pF removal patch set. later.
when we will see that .opd based dereference does not make anyone
unhappy.

and I think we can't remove pf/pF from the kernel completely. it
will stay in vscnprintf() for some time. old habits die hard, I suppose,
there might be people using it for debugging/etc.

At least if you send out the patches to do the removal they might
eventually get merged.

quoted

speaking of upstream, any objections if this patch set will go through
the printk tree, in one piece?

Do you mind putting it in a topic branch (based on rc2) and then merge
that into the printk tree? That way I can merge the topic branch iff
there are conflicts later down the line towards 4.15.

ok, let me re-spin the series. there are some changes here
and there, so I'll drop Tested-by/Reviewed-by tags and will
ask platforms' maintainers to re-test the patch set :(

if everything goes OK, then we can ask Petr to do the topic
branch (I don't have a kernel.org account).

	-ss